diff --git a/internal/runner/llm_source_test.go b/internal/runner/llm_source_test.go index f3372b1..a1e8bb1 100644 --- a/internal/runner/llm_source_test.go +++ b/internal/runner/llm_source_test.go @@ -16,6 +16,8 @@ import ( "strings" "testing" + "github.com/priyanshujain/sanderling/internal/driver" + mockdriver "github.com/priyanshujain/sanderling/internal/driver/mock" "github.com/priyanshujain/sanderling/internal/hierarchy" "github.com/priyanshujain/sanderling/internal/llmclient" "github.com/priyanshujain/sanderling/internal/verifier" @@ -276,6 +278,100 @@ func TestPickSourcesSeededByDefault(t *testing.T) { } } +// seededFixtureSpec declares no generator = llm(...), so --generator llm has +// nothing to build a picker from. +const seededFixtureSpec = ` +import { always, taps, typing, weighted } from "@sanderling/spec"; +globalThis.properties = { ok: always(() => true) }; +globalThis.actions = weighted([1, taps], [1, typing]); +` + +func newSeededVerifier(t *testing.T) *verifier.Verifier { + t.Helper() + verifierInstance, err := verifier.New() + if err != nil { + t.Fatal(err) + } + if err := verifierInstance.Load(bundleSpec(t, seededFixtureSpec)); err != nil { + t.Fatal(err) + } + if _, ok := verifierInstance.LLMConfig(); ok { + t.Fatal("seeded fixture spec must not register an llm action backend") + } + return verifierInstance +} + +// TestPickSourcesRejectsLLMWithoutSpecConfig pins the abort. Falling back to the +// seeded picker here completes the run, writes a well-formed output directory, +// and records it under the requested arm, so a campaign cell silently reports +// the wrong policy's numbers. +func TestPickSourcesRejectsLLMWithoutSpecConfig(t *testing.T) { + for name, activeDriver := range map[string]driver.DeviceDriver{ + "native": nil, + "web": &webMockDriver{Driver: mockdriver.New()}, + } { + t.Run(name, func(t *testing.T) { + action, extractor, err := pickSources(Options{ + Driver: activeDriver, + Verifier: newSeededVerifier(t), + Generator: "llm", + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err == nil { + t.Fatalf("pickSources = (%T, %T), want an error", action, extractor) + } + if action != nil || extractor != nil { + t.Errorf("sources = (%v, %v), want both nil alongside the error", action, extractor) + } + if !strings.Contains(err.Error(), "generator = llm(...)") { + t.Errorf("error = %q, want it to name the missing spec declaration", err) + } + }) + } +} + +// TestPickSourcesOnWebComposesLLMWithWebExtractors covers the second half of the +// same claim: the picker is chosen by --generator and the extractor source by +// the driver, so the llm policy runs on web instead of being silently replaced +// by the V8 picker. +func TestPickSourcesOnWebComposesLLMWithWebExtractors(t *testing.T) { + fake := newFakeOpenRouter(t) + _, verifierInstance := newLLMSource(t, fake) + action, extractor, err := pickSources(Options{ + Driver: &webMockDriver{Driver: mockdriver.New()}, + Verifier: verifierInstance, + Generator: "llm", + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("pickSources: %v", err) + } + if _, ok := action.(*llmSource); !ok { + t.Errorf("action source = %T, want *llmSource on web with --generator llm", action) + } + if _, ok := extractor.(webSource); !ok { + t.Errorf("extractor source = %T, want webSource so overrides still come from V8", extractor) + } +} + +func TestPickSourcesOnWebSeededKeepsBothOnV8(t *testing.T) { + action, extractor, err := pickSources(Options{ + Driver: &webMockDriver{Driver: mockdriver.New()}, + Verifier: newSeededVerifier(t), + Generator: "seeded", + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("pickSources: %v", err) + } + if _, ok := action.(webSource); !ok { + t.Errorf("action source = %T, want webSource for the seeded web path", action) + } + if _, ok := extractor.(webSource); !ok { + t.Errorf("extractor source = %T, want webSource for the seeded web path", extractor) + } +} + func TestLLMSourceDrivesExecutedActions(t *testing.T) { fake := newFakeOpenRouter(t) source, verifierInstance := newLLMSource(t, fake) diff --git a/internal/runner/source.go b/internal/runner/source.go index 570c5ff..486904c 100644 --- a/internal/runner/source.go +++ b/internal/runner/source.go @@ -3,6 +3,7 @@ package runner import ( "context" "encoding/json" + "errors" "fmt" "log/slog" @@ -64,39 +65,48 @@ func (s webSource) ExtractorOverrides(ctx context.Context) (map[int]json.RawMess // pickSources selects the runtime's action and extractor sources ONCE at setup // from the driver's capabilities, the --generator flag, and the spec, so the -// step loop never type-asserts. With --generator llm and a spec-declared -// generator = llm({...}) it constructs the chat-completions client and returns -// an llmSource for selection while extractor overrides still come from the goja -// path. Otherwise the seeded goja picker drives. +// step loop never type-asserts. +// +// The two axes are independent. The DRIVER decides where extractor overrides +// come from: the web path reads the values its extractors computed in V8, every +// other path has none. The --generator flag decides who picks the action, and +// the llm picker composes with either extractor source because it reads the +// goja-side candidate list and screenshot, both of which the runner populates on +// every platform. +// +// A missing generator = llm(...) is fatal rather than a fallback to the seeded +// picker. Falling back silently corrupts a comparison campaign: the run +// completes and the output directory looks correct while the wrong policy drove +// it. func pickSources(options Options) (ActionSource, ExtractorSource, error) { + seeded := ActionSource(gojaSource{verifier: options.Verifier}) + extractor := ExtractorSource(gojaSource{verifier: options.Verifier}) if web, ok := options.Driver.(driver.WebDriver); ok { source := webSource{web: web} - return source, source, nil + seeded, extractor = source, source + } + if options.Generator != "llm" { + return seeded, extractor, nil + } + config, ok := options.Verifier.LLMConfig() + if !ok { + return nil, nil, errors.New("--generator llm: the spec declares no generator = llm(...); add one to the spec or drop --generator llm") + } + client, err := llmclient.New() + if err != nil { + return nil, nil, fmt.Errorf("llm action generator: %w", err) } logger := options.Logger if logger == nil { logger = slog.Default() } - if options.Generator == "llm" { - config, ok := options.Verifier.LLMConfig() - if !ok { - logger.Warn("--generator llm requested but spec declares no generator = llm(); using the seeded picker") - } else { - client, err := llmclient.New() - if err != nil { - return nil, nil, fmt.Errorf("llm action generator: %w", err) - } - action := &llmSource{ - verifier: options.Verifier, - client: client, - model: config.Model, - instructions: config.Instructions, - logger: logger, - history: newActionHistory(llmHistorySize), - } - return action, gojaSource{verifier: options.Verifier}, nil - } + action := &llmSource{ + verifier: options.Verifier, + client: client, + model: config.Model, + instructions: config.Instructions, + logger: logger, + history: newActionHistory(llmHistorySize), } - source := gojaSource{verifier: options.Verifier} - return source, source, nil + return action, extractor, nil }