Build the run's provider from the key it redacts; record render failures

Review findings on b0cf6fb:

1. Config took a ready-made Provider and a separate, optional APIKey used
   only for redaction, so a provider holding a decrypted key could run
   with APIKey empty and a reply quoting that key unlabelled was stored.
   Config now holds the household's llm.Settings, the decrypted APIKey and
   a NewProvider constructor (presets.FromSettings in production). Run
   builds the provider from exactly that key and redacts the same key, so
   the two cannot differ. ProviderName, Model and Temperature come from
   Settings. A new test runs presets.FromSettings against a server that
   echoes the bearer key unlabelled in the reply text, reply JSON and
   model name, across a repair turn, and asserts the stored row never
   holds it; with redaction disabled the test fails.

2. A render failure wrote no ai_runs row. Every run with a valid Config
   now writes one: a render failure is bad_output and a provider that
   cannot be built is provider_error, each with the error text and empty
   request and response lists. TestRunRenderError now expects exactly one
   such row. A Config error (no store, provider name or model) still
   writes none, since the row cannot be written without them.

Redaction rules, the four statuses for provider-reaching runs (one row,
both turns) and Purge are unchanged.

Maestro task 53e382d8-1db3-4555-a31d-e44cf55cc98d.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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.

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)

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

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}

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 }{