diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a7c0857..8d3cc87 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -150,9 +150,13 @@ automatic** (D-021). Anthropic and Ollama endpoints, called over plain HTTP — no vendor SDKs. - **Skill harness** (`ai/harness`): prompt layering, validation of the structured output, and **one repair retry**; if the repair also fails the - run fails — no silent fallback. Every run that reaches the provider writes - one `ai_runs` row when it ends, covering the repair turn too, with status - `ok`, `repaired`, `bad_output` or `provider_error`. Request and response + run fails — no silent fallback. Every run writes one `ai_runs` row when it + ends, covering the repair turn too, with status `ok`, `repaired`, + `bad_output` or `provider_error`; a prompt that cannot be rendered is + `bad_output` and a provider that cannot be built is `provider_error`, both + with no turns. The harness builds the provider itself from the settings + and the decrypted API key (`presets.FromSettings`), so the key it redacts + is always the key the provider sends. Request and response bodies are redacted before they are stored: the decrypted API key, Authorization and API-key header values, Google Chat webhook URLs and URLs with key-like query parameters are removed, inside JSON strings too. diff --git a/internal/ai/harness/harness.go b/internal/ai/harness/harness.go index fa52e9d..5ad82d1 100644 --- a/internal/ai/harness/harness.go +++ b/internal/ai/harness/harness.go @@ -4,8 +4,7 @@ // for output matching the skill's schema, decodes and validates the reply, // and on failure sends exactly one repair turn quoting the problems. If the // repair fails too the run fails with llm.ErrBadOutput; there is no silent -// fallback (D-007). Every run that reaches the provider is recorded in -// ai_runs (see Run and Purge). +// fallback (D-007). Every run is recorded in ai_runs (see Run and Purge). package harness import ( @@ -42,23 +41,26 @@ type Skill[In, Out any] interface { PostProcess(out Out) (Out, error) } +// NewProvider builds the provider a run sends its requests to from the +// household's settings and decrypted API key. In production it is +// presets.FromSettings. +type NewProvider func(s llm.Settings, key string) (llm.Provider, error) + // Config is what a run needs from the household's AI settings. type Config struct { - // Provider answers the requests. - Provider llm.Provider + // Settings is the household's AI configuration. Settings.Provider and + // Settings.Model are recorded in ai_runs and may not be empty; + // Settings.Temperature is sent with every request. + Settings llm.Settings + // APIKey is the decrypted API key, or "" for a provider that takes + // none. It reaches the provider only through NewProvider, and every + // occurrence of it is removed from the ai_runs row. + APIKey string + // NewProvider builds the run's provider from Settings and APIKey. + NewProvider NewProvider // Persona is ai_settings.system_prompt, the first layer of every // system prompt; empty means none. Persona string - // Temperature is the household's sampling temperature, sent with - // every request. - Temperature float64 - // ProviderName is ai_settings.provider and Model is ai_settings.model, - // both recorded in ai_runs. Neither may be empty. - ProviderName string - Model string - // APIKey is the decrypted API key Provider sends, or "" when it sends - // none. It is used only to remove the key from the ai_runs row. - APIKey string // Store receives the run's ai_runs row. It must not be nil. Store *store.Store } @@ -66,11 +68,11 @@ type Config struct { // validate reports a Config missing what every run needs. func (c Config) validate() error { switch { - case c.Provider == nil: - return errors.New("config has no provider") - case c.ProviderName == "": + case c.NewProvider == nil: + return errors.New("config has no provider constructor") + case c.Settings.Provider == "": return errors.New("config has no provider name") - case c.Model == "": + case c.Settings.Model == "": return errors.New("config has no model") case c.Store == nil: return errors.New("config has no store") @@ -101,31 +103,21 @@ func (e *BadOutputError) Unwrap() error { return llm.ErrBadOutput } // output. A provider error other than llm.ErrBadOutput is returned // unchanged and never repaired. // -// Every run that sends a request writes one ai_runs row when it ends, +// Every run with a valid Config writes one ai_runs row when it ends, // covering both turns, with its RunStatus and the request and response -// bodies redacted (see redact). A Config or render error fails the run -// before any request, and writes no row. When the row cannot be written -// the run fails with that error too, even if its output was usable. +// bodies redacted (see redact). A render failure is recorded as +// StatusBadOutput and a provider that cannot be built as +// StatusProviderError, both with no turns. A Config error fails the run +// before anything else, and writes no row since the row cannot be +// written without one. When the row cannot be written the run fails with +// that error too, even if its output was usable. func Run[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], in In) (Out, error) { var zero Out if err := cfg.validate(); err != nil { return zero, fmt.Errorf("harness: skill %s v%d: %w", s.Name(), s.Version(), err) } - prompt, err := Render(cfg.Persona, s, in) - if err != nil { - return zero, fmt.Errorf("harness: skill %s v%d: render: %w", s.Name(), s.Version(), err) - } - schema := s.Schema() - temperature := cfg.Temperature - req := llm.Request{ - System: prompt.System, - Messages: []llm.Message{{Role: llm.RoleUser, Content: prompt.User}}, - Temperature: &temperature, - Schema: &schema, - } - log := &runLog{start: store.Now()} - out, status, err := turns(ctx, cfg, s, req, log) + out, status, err := execute(ctx, cfg, s, in, log) if saveErr := log.save(ctx, cfg, s.Name(), s.Version(), status, err); saveErr != nil { return zero, errors.Join(err, fmt.Errorf("harness: skill %s v%d: record ai_runs row: %w", s.Name(), s.Version(), saveErr)) } @@ -135,11 +127,34 @@ func Run[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], in In) return out, nil } +// execute renders the prompt, builds the provider and sends the turns, +// recording them in log. It returns the run's status alongside its result. +func execute[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], in In, log *runLog) (Out, RunStatus, error) { + var zero Out + prompt, err := Render(cfg.Persona, s, in) + if err != nil { + return zero, StatusBadOutput, fmt.Errorf("harness: skill %s v%d: render: %w", s.Name(), s.Version(), err) + } + provider, err := cfg.NewProvider(cfg.Settings, cfg.APIKey) + if err != nil { + return zero, StatusProviderError, fmt.Errorf("harness: skill %s v%d: provider: %w", s.Name(), s.Version(), err) + } + schema := s.Schema() + temperature := cfg.Settings.Temperature + req := llm.Request{ + System: prompt.System, + Messages: []llm.Message{{Role: llm.RoleUser, Content: prompt.User}}, + Temperature: &temperature, + Schema: &schema, + } + return turns(ctx, cfg.APIKey, provider, s, req, log) +} + // turns sends req and, when its reply is unusable, the repair turn, // recording both in log. It returns the run's status alongside its result. -func turns[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], req llm.Request, log *runLog) (Out, RunStatus, error) { +func turns[In, Out any](ctx context.Context, apiKey string, provider llm.Provider, s Skill[In, Out], req llm.Request, log *runLog) (Out, RunStatus, error) { var zero Out - out, reply, first, status, err := attempt(ctx, cfg, s, req, log) + out, reply, first, status, err := attempt(ctx, apiKey, provider, s, req, log) if err != nil { return zero, status, err } @@ -151,7 +166,7 @@ func turns[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], req l req.Messages = append(req.Messages, llm.Message{Role: llm.RoleAssistant, Content: reply}) } req.Messages = append(req.Messages, llm.Message{Role: llm.RoleUser, Content: repairMessage(req.Schema.Name, first)}) - out, _, second, status, err := attempt(ctx, cfg, s, req, log) + out, _, second, status, err := attempt(ctx, apiKey, provider, s, req, log) if err != nil { return zero, status, err } @@ -161,14 +176,14 @@ func turns[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], req l return out, StatusRepaired, nil } -// attempt sends req once, records the turn in log, then decodes, +// attempt sends req to provider once, records the turn in log, then decodes, // validates, post-processes and validates the reply again. It returns the // reply for the repair turn and the problems found; err is set only for a // provider or post-process failure, neither of which is repairable, and // status then says which. -func attempt[In, Out any](ctx context.Context, cfg Config, s Skill[In, Out], req llm.Request, log *runLog) (out Out, reply string, problems []string, status RunStatus, err error) { - resp, err := cfg.Provider.Generate(ctx, req) - log.add(cfg.APIKey, req, resp, err) +func attempt[In, Out any](ctx context.Context, apiKey string, provider llm.Provider, s Skill[In, Out], req llm.Request, log *runLog) (out Out, reply string, problems []string, status RunStatus, err error) { + resp, err := provider.Generate(ctx, req) + log.add(apiKey, req, resp, err) // The repair turn must quote what failed: the structured output when // there is any, otherwise the text. reply = string(resp.JSON) diff --git a/internal/ai/harness/harness_test.go b/internal/ai/harness/harness_test.go index 1c0ad9f..9e0470b 100644 --- a/internal/ai/harness/harness_test.go +++ b/internal/ai/harness/harness_test.go @@ -15,10 +15,15 @@ func reply(js string) llm.GenerateResult { return llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}} } -// testConfig returns a Config for f recording into a fresh migrated store. +// testConfig returns a Config whose provider is f, recording into a fresh +// migrated store. func testConfig(t *testing.T, f *llm.Fake) Config { t.Helper() - return Config{Provider: f, Temperature: 0.7, ProviderName: "anthropic", Model: "claude-test", Store: emptyHousehold(t)} + return Config{ + Settings: llm.Settings{Provider: "anthropic", Model: "claude-test", Temperature: 0.7}, + NewProvider: func(llm.Settings, string) (llm.Provider, error) { return f, nil }, + Store: emptyHousehold(t), + } } func run(t *testing.T, f *llm.Fake) (echoOutput, error) { @@ -215,8 +220,29 @@ func TestRunRenderError(t *testing.T) { t.Fatalf("err = %v, want a render error", err) } assertCalls(t, &f, 0) - if rows := runRows(t, cfg.Store); len(rows) != 0 { - t.Fatalf("render error wrote %d ai_runs rows, want none", len(rows)) + rows := runRows(t, cfg.Store) + if len(rows) != 1 { + t.Fatalf("render error wrote %d ai_runs rows, want 1", len(rows)) + } + r := rows[0] + if r.Status != string(StatusBadOutput) || r.Error.String != err.Error() || + r.Request != "[]" || r.Response.String != "[]" || r.TokensIn.Valid || r.TokensOut.Valid { + t.Fatalf("row = %+v, want bad_output with the render error and no turns", r) + } +} + +func TestRunProviderBuildError(t *testing.T) { + var f llm.Fake + cfg := testConfig(t, &f) + cfg.NewProvider = func(llm.Settings, string) (llm.Provider, error) { return nil, llm.ErrMissingKey } + _, err := Run(context.Background(), cfg, echoSkill{}, echoInput{Word: "taco"}) + if !errors.Is(err, llm.ErrMissingKey) { + t.Fatalf("err = %v, want ErrMissingKey", err) + } + rows := runRows(t, cfg.Store) + if len(rows) != 1 || rows[0].Status != string(StatusProviderError) || + rows[0].Error.String != err.Error() || rows[0].Request != "[]" { + t.Fatalf("rows = %+v, want one provider_error row with no turns", rows) } } @@ -224,10 +250,10 @@ func TestRunConfigError(t *testing.T) { var f llm.Fake full := testConfig(t, &f) for name, edit := range map[string]func(*Config){ - "provider": func(c *Config) { c.Provider = nil }, - "provider name": func(c *Config) { c.ProviderName = "" }, - "model": func(c *Config) { c.Model = "" }, - "store": func(c *Config) { c.Store = nil }, + "provider constructor": func(c *Config) { c.NewProvider = nil }, + "provider name": func(c *Config) { c.Settings.Provider = "" }, + "model": func(c *Config) { c.Settings.Model = "" }, + "store": func(c *Config) { c.Store = nil }, } { t.Run(name, func(t *testing.T) { cfg := full diff --git a/internal/ai/harness/runs.go b/internal/ai/harness/runs.go index 9b13bd4..fb12141 100644 --- a/internal/ai/harness/runs.go +++ b/internal/ai/harness/runs.go @@ -26,10 +26,11 @@ const ( // StatusRepaired means the first reply was not usable and the reply to // the repair turn was. StatusRepaired RunStatus = "repaired" - // StatusBadOutput means neither reply was usable, or post-processing - // a valid reply failed. + // StatusBadOutput means neither reply was usable, post-processing a + // valid reply failed, or the skill's prompt could not be rendered. StatusBadOutput RunStatus = "bad_output" - // StatusProviderError means the provider failed on either turn. + // StatusProviderError means the provider could not be built or failed + // on either turn. StatusProviderError RunStatus = "provider_error" ) @@ -140,11 +141,12 @@ func (l *runLog) add(apiKey string, req llm.Request, resp llm.Response, err erro // save writes the run's ai_runs row. It is written even when ctx is done, // so a cancelled run is recorded too. func (l *runLog) save(ctx context.Context, cfg Config, skill string, version int, status RunStatus, runErr error) error { - request, err := marshalRedacted(l.requests, cfg.APIKey) + // A run that failed before its first turn stores an empty list. + request, err := marshalRedacted(nonNil(l.requests), cfg.APIKey) if err != nil { return err } - response, err := marshalRedacted(l.responses, cfg.APIKey) + response, err := marshalRedacted(nonNil(l.responses), cfg.APIKey) if err != nil { return err } @@ -160,9 +162,9 @@ func (l *runLog) save(ctx context.Context, cfg Config, skill string, version int ID: store.NewID(), Skill: skill, SkillVersion: strconv.Itoa(version), - Provider: cfg.ProviderName, - Model: cfg.Model, - Temperature: cfg.Temperature, + Provider: cfg.Settings.Provider, + Model: cfg.Settings.Model, + Temperature: cfg.Settings.Temperature, Status: string(status), LatencyMs: sql.NullInt64{Int64: now.Sub(l.start.Time).Milliseconds(), Valid: true}, TokensIn: reported(l.tokensIn), @@ -175,6 +177,14 @@ func (l *runLog) save(ctx context.Context, cfg Config, skill string, version int }) } +// nonNil is s, or an empty slice when s is nil, so it encodes as []. +func nonNil[T any](s []T) []T { + if s == nil { + return []T{} + } + return s +} + // reported is n as a column value, NULL when no turn reported a count. func reported(n int64) sql.NullInt64 { return sql.NullInt64{Int64: n, Valid: n > 0} diff --git a/internal/ai/harness/runs_test.go b/internal/ai/harness/runs_test.go index 182cc42..339f40d 100644 --- a/internal/ai/harness/runs_test.go +++ b/internal/ai/harness/runs_test.go @@ -6,11 +6,14 @@ import ( "encoding/json" "errors" "fmt" + "net/http" + "net/http/httptest" "strings" "testing" "time" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm/presets" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" ) @@ -212,7 +215,7 @@ func TestRunRecordsCancelledRun(t *testing.T) { func TestRunFailsWhenRowCannotBeWritten(t *testing.T) { var f llm.Fake cfg := testConfig(t, &f) - cfg.ProviderName = "gemini" // refused by the provider CHECK + cfg.Settings.Provider = "gemini" // refused by the provider CHECK f.QueueGenerate(reply(`{"echo":"taco"}`)) _, err := Run(context.Background(), cfg, echoSkill{}, echoInput{Word: "taco"}) if err == nil || !strings.Contains(err.Error(), "record ai_runs row") { @@ -266,6 +269,12 @@ func TestRunRowsHoldNoSecrets(t *testing.T) { f.QueueGenerate(script...) cfg := testConfig(t, &f) cfg.APIKey = seededKey + cfg.NewProvider = func(_ llm.Settings, key string) (llm.Provider, error) { + if key != seededKey { + t.Errorf("provider built with key %q, want the seeded key", key) + } + return &f, nil + } cfg.Persona = "Our kitchen. " + leaky _, _ = Run(context.Background(), cfg, echoSkill{}, echoInput{Word: "tacos " + seededWebhook}) assertCalls(t, &f, len(script)) @@ -289,6 +298,63 @@ func TestRunRowsHoldNoSecrets(t *testing.T) { } } +// TestRunRowsHoldNoProviderKey builds the provider the production way, +// presets.FromSettings, against a server that quotes the bearer key it +// receives with no Authorization label: in the reply text, the reply JSON +// and the model name. The repair turn then sends the quoted key back. +func TestRunRowsHoldNoProviderKey(t *testing.T) { + turn := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + key, ok := strings.CutPrefix(r.Header.Get("Authorization"), "Bearer ") + if !ok || key == "" { + t.Errorf("Authorization = %q, want a bearer key", r.Header.Get("Authorization")) + } + turn++ + echo := key // lower case, so usable + if turn == 1 { + echo = "Your key is " + key // upper case, so repaired + } + content, err := json.Marshal(map[string]string{"echo": echo}) + if err != nil { + t.Error(err) + } + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(map[string]any{ + "model": "model-" + key, + "choices": []any{map[string]any{"finish_reason": "stop", "message": map[string]any{"role": "assistant", "content": string(content)}}}, + "usage": map[string]int{"prompt_tokens": 10, "completion_tokens": 5}, + }); err != nil { + t.Error(err) + } + })) + defer srv.Close() + + cfg := Config{ + Settings: llm.Settings{Provider: "openai_compatible", BaseURL: srv.URL, Model: "local-test", Temperature: 0.7}, + APIKey: seededKey, + NewProvider: presets.FromSettings, + Store: emptyHousehold(t), + } + out, err := Run(context.Background(), cfg, echoSkill{}, echoInput{Word: "taco"}) + if err != nil { + t.Fatal(err) + } + if out.Echo != seededKey || turn != 2 { + t.Fatalf("Echo = %q after %d turns, want the provider's key after 2", out.Echo, turn) + } + rows := runRows(t, cfg.Store) + if len(rows) != 1 || rows[0].Status != string(StatusRepaired) { + t.Fatalf("rows = %+v, want one repaired row", rows) + } + text := rows[0].text() + if strings.Contains(text, seededKey) { + t.Fatalf("stored row holds the provider's key:\n%s", text) + } + if !strings.Contains(text, "Your key is "+redactionMarker) || !strings.Contains(text, "model-"+redactionMarker) { + t.Fatalf("stored row lacks the redacted quotes:\n%s", text) + } +} + func TestRedact(t *testing.T) { const key = "sk-abc123" for _, tc := range []struct{ in, want string }{