diff --git a/internal/runner/llm_source.go b/internal/runner/llm_source.go index 65f7625..f5be99f 100644 --- a/internal/runner/llm_source.go +++ b/internal/runner/llm_source.go @@ -64,11 +64,14 @@ type llmSource struct { // can stamp the trace. lastSource is "llm" only when the LLM (not setup) // chose the action; lastReasoning is the model's rationale. lastChoice is the // 1-based number it picked and lastChosenAction the description it echoed, so - // the trace shows what the model believed it was doing. + // the trace shows what the model believed it was doing. lastFromSetup says + // the spec's setup produced the action, which is the app being put in + // position rather than the generator exploring it. lastSource string lastReasoning string lastChoice int lastChosenAction string + lastFromSetup bool } // llmSelection is the outcome of one LLM selection call. @@ -95,12 +98,14 @@ func (s *llmSource) NextAction(ctx context.Context, stepIndex int) (verifier.Act s.lastReasoning = "" s.lastChoice = 0 s.lastChosenAction = "" + s.lastFromSetup = false s.history.completeLast(s.verifier.CurrentScreen()) // Setup precedence only: the LLM replaces the seeded action root, so we run // setup (e.g. login) first but never the weighted picker. action, err := s.verifier.SetupAction() if err == nil { + s.lastFromSetup = true s.record(stepIndex, trace.LLMCall{Outcome: trace.LLMOutcomeSetupAction}) s.history.add(describeAction(action)) return action, nil @@ -509,6 +514,16 @@ func stampActionSource(traceAction *trace.Action, source ActionSource) { traceAction.LLMChosenAction = llm.lastChosenAction } +// generatorChoseAction reports whether the action the source just returned came +// from the generator rather than from the spec's setup driving the app into +// position. Only the model source can tell them apart: the seeded picker +// resolves setup precedence inside the one JS call it makes, so everything it +// returns counts as the generator's. +func generatorChoseAction(source ActionSource) bool { + llm, ok := source.(*llmSource) + return !ok || !llm.lastFromSetup +} + // screenshotDataURL downscales the PNG and encodes it as a data URL for the // image content part. func screenshotDataURL(pngBytes []byte, maxEdge int) (string, bool) { diff --git a/internal/runner/llm_source_test.go b/internal/runner/llm_source_test.go index 328ef41..016d756 100644 --- a/internal/runner/llm_source_test.go +++ b/internal/runner/llm_source_test.go @@ -818,6 +818,168 @@ func TestRunner_EveryModelCallFailingIsNotACleanRun(t *testing.T) { } } +// llmLoginSetupSpec is the shape every spec with a login has: setup drives the +// app for its first steps and then yields nothing, leaving the rest of the run +// to the generator. +const llmLoginSetupSpec = ` +import { llm, always, actions, taps, typing, weighted, Tap } from "@sanderling/spec"; +globalThis.properties = { ok: always(() => true) }; +let setupTapsLeft = 2; +globalThis.setup = actions(() => (setupTapsLeft-- > 0 ? [Tap({ on: "id:Submit" })] : [])); +globalThis.actions = weighted([1, taps], [1, typing]); +globalThis.generator = llm({ model: "test/model" }); +` + +// TestRunner_SetupActionsAreNotTheGeneratorDrivingTheApp is the folio run: the +// spec's login setup dispatches the first steps, then every model call fails. +// Two actions reached the app and none of them explored it, and a run counted +// by dispatched actions alone reports that as a clean run. +func TestRunner_SetupActionsAreNotTheGeneratorDrivingTheApp(t *testing.T) { + fake := newFakeOpenRouter(t) + fake.server.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + }) + t.Setenv("OPENROUTER_API_KEY", "test-key") + t.Setenv("OPENROUTER_BASE_URL", fake.server.URL) + + state := newHarnessWithSpec(t, llmLoginSetupSpec) + state.mock.HierarchyJSON = llmTreeJSON + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + summary, err := Run(ctx, Options{ + Duration: 30 * time.Second, + IdleTimeout: 20 * time.Millisecond, + MaxSteps: 4, + Driver: state.mock, + Verifier: state.verifier, + TraceWriter: state.writer, + Generator: "llm", + LabelSource: verifier.LabelSourceVisibleText, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("Run: %v", err) + } + + wantOutcomes := []string{ + trace.LLMOutcomeSetupAction, trace.LLMOutcomeSetupAction, + trace.LLMOutcomeRequestFailed, trace.LLMOutcomeRequestFailed, + } + calls := readLLMCalls(t, state.writer.Directory()) + if len(calls) != len(wantOutcomes) { + t.Fatalf("recorded %d selection records, want %d", len(calls), len(wantOutcomes)) + } + for index, call := range calls { + if call.Outcome != wantOutcomes[index] { + t.Fatalf("call at step %d ended %q, want %q", call.Step, call.Outcome, wantOutcomes[index]) + } + } + + lines := readTraceLines(t, state.writer.Directory()) + if len(lines) != 4 { + t.Fatalf("wrote %d trace lines, want 4", len(lines)) + } + for _, line := range lines[:2] { + if line.NextAction == nil || line.ActionSkipped != "" { + t.Errorf("step %d = %+v, want setup's action dispatched", line.Step, line) + } + } + for _, line := range lines[2:] { + if line.ActionSkipped != string(actionSkippedNoActionProduced) { + t.Errorf("step %d action_skipped = %q, want %q", + line.Step, line.ActionSkipped, actionSkippedNoActionProduced) + } + } + + if summary.DispatchedActions != 2 { + t.Errorf("DispatchedActions = %d, want 2: setup drove the app twice", + summary.DispatchedActions) + } + if summary.GeneratorActions != 0 { + t.Errorf("GeneratorActions = %d, want 0: every model call failed", + summary.GeneratorActions) + } + if got := summary.SkippedActions[string(actionSkippedNoActionProduced)]; got != 2 { + t.Errorf("summary counted %d step(s) as %q, want 2: %v", + got, actionSkippedNoActionProduced, summary.SkippedActions) + } + taps := 0 + for _, action := range state.mock.Actions() { + if action.Kind == mockdriver.ActionTap || action.Kind == mockdriver.ActionTapSelector { + taps++ + } + } + if taps != 2 { + t.Errorf("the run drove the app %d time(s), want the 2 setup taps only", taps) + } +} + +// TestRunner_SetupAndGeneratorBothDrivingIsAHealthyRun is the same spec with a +// provider that answers: setup drives its steps and the model drives the rest, +// which is what an ordinary login-fronted run looks like. Counting only the +// generator's actions must not turn it red. +func TestRunner_SetupAndGeneratorBothDrivingIsAHealthyRun(t *testing.T) { + fake := newFakeOpenRouter(t) + t.Setenv("OPENROUTER_API_KEY", "test-key") + t.Setenv("OPENROUTER_BASE_URL", fake.server.URL) + + state := newHarnessWithSpec(t, llmLoginSetupSpec) + state.mock.HierarchyJSON = llmTreeJSON + pushSnapshotTree(t, state.verifier, llmTreeJSON) + tap := candidateByKind(t, + mustCandidates(t, state.verifier, verifier.LabelSourceVisibleText), + verifier.ActionKindTap) + fake.choice = tap.Index + fake.chosenAction = tap.Description + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + summary, err := Run(ctx, Options{ + Duration: 30 * time.Second, + IdleTimeout: 20 * time.Millisecond, + MaxSteps: 4, + Driver: state.mock, + Verifier: state.verifier, + TraceWriter: state.writer, + Generator: "llm", + LabelSource: verifier.LabelSourceVisibleText, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("Run: %v", err) + } + + if summary.DispatchedActions != 4 { + t.Errorf("DispatchedActions = %d, want 4: every step drove the app", + summary.DispatchedActions) + } + if summary.GeneratorActions != 2 { + t.Errorf("GeneratorActions = %d, want 2: the model drove the two steps setup left it", + summary.GeneratorActions) + } + lines := readTraceLines(t, state.writer.Directory()) + if len(lines) != 4 { + t.Fatalf("wrote %d trace lines, want 4", len(lines)) + } + for _, line := range lines { + if line.NextAction == nil || line.ActionSkipped != "" { + t.Fatalf("step %d = %+v, want an action dispatched", line.Step, line) + } + } + for _, line := range lines[:2] { + if line.NextAction.Source != "" { + t.Errorf("step %d action source = %q, want none: setup chose it", + line.Step, line.NextAction.Source) + } + } + for _, line := range lines[2:] { + if line.NextAction.Source != "llm" { + t.Errorf("step %d action source = %q, want llm", line.Step, line.NextAction.Source) + } + } +} + // llmSetupFixtureSpec drives the first action from setup, so the model is never // consulted for that step. const llmSetupFixtureSpec = ` diff --git a/internal/runner/runner.go b/internal/runner/runner.go index fa8f907..2723637 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -79,6 +79,13 @@ type Summary struct { // A run at zero never touched the app, whatever its step count says, so its // empty violation list is the reading of an instrument that measured nothing. DispatchedActions int + // GeneratorActions counts the dispatched actions the generator chose. The + // spec's setup drives the app into its starting position before the + // generator is consulted, so a run at zero here explored nothing however + // many actions its login fired. Only the model generator separates the two: + // the seeded picker resolves setup precedence inside the one JS call it + // makes, so everything it returns counts as the generator's. + GeneratorActions int } type ViolationRecord struct { @@ -499,6 +506,9 @@ func Run(ctx context.Context, options Options) (Summary, error) { } if nextErr == nil && !applySkipped { summary.DispatchedActions++ + if generatorChoseAction(actionSource) { + summary.GeneratorActions++ + } } summary.Steps = stepIndex if len(violations) > 0 { diff --git a/internal/testrun/testrun.go b/internal/testrun/testrun.go index b4d36d9..64d6c73 100644 --- a/internal/testrun/testrun.go +++ b/internal/testrun/testrun.go @@ -265,13 +265,16 @@ func Execute(ctx context.Context, options Options, stdout io.Writer) error { // fraction of them: a screen that composes now and then costs a healthy android // run a step or two, and a check that fired on those would be red on every run. // -// A run that dispatched no action at all fails on the same grounds, and the +// A run whose generator dispatched no action fails on the same grounds, and the // threshold is zero for the same reason: a generator with nothing to offer on // some screens is ordinary, one with nothing to offer on every screen of a whole -// run drove nothing. --exit-on-violation keeps precedence over it so a run that -// found something still exits on its evidence, and the property-free opt-out -// exempts the sweeps, whose measurement is where a generator reaches and for -// which "nowhere on this build" is a result rather than a broken run. +// run drove nothing. The count is the generator's alone because a spec's setup +// drives the app before the generator is consulted, so a login that ran leaves +// dispatched actions behind whatever the generator then did. +// --exit-on-violation keeps precedence over it so a run that found something +// still exits on its evidence, and the property-free opt-out exempts the sweeps, +// whose measurement is where a generator reaches and for which "nowhere on this +// build" is a result rather than a broken run. func runOutcome(options Options, summary runner.Summary) error { if summary.Steps > 0 && summary.SkippedVerification == summary.Steps { return VacuousRunError{Steps: summary.Steps} @@ -279,8 +282,8 @@ func runOutcome(options Options, summary runner.Summary) error { if options.ExitOnViolation && len(summary.Violations) > 0 { return ViolationsError{Count: len(summary.Violations)} } - if !options.AllowNoProperties && summary.Steps > 0 && summary.DispatchedActions == 0 { - return NoActionsDispatchedError{ + if !options.AllowNoProperties && summary.Steps > 0 && summary.GeneratorActions == 0 { + return NoGeneratorActionsError{ Steps: summary.Steps, SkippedActions: summary.SkippedActions, } @@ -352,13 +355,14 @@ func (e NoPropertiesError) Error() string { e.Spec) } -// NoActionsDispatchedError reports a run not one of whose steps drove the app. -// Every screen it judged was the one it launched on, so its empty violation list -// says as much about the app as a spec with no properties would: the run -// observed, judged the same state over and over, and exercised nothing. It stays -// untyped to the CLI's violation path like VacuousRunError, so it exits 1 as a -// run that holds no verdict rather than 2. -type NoActionsDispatchedError struct { +// NoGeneratorActionsError reports a run not one of whose steps was driven by the +// action generator. Setup can put the app in position, but only the generator +// explores it, so every screen this run judged was one setup left it on and its +// empty violation list says as much about the app as a spec with no properties +// would: the run observed, judged the same state over and over, and exercised +// nothing. It stays untyped to the CLI's violation path like VacuousRunError, so +// it exits 1 as a run that holds no verdict rather than 2. +type NoGeneratorActionsError struct { Steps int // SkippedActions is the runner's per-reason count of actions that never // reached the app, which is where the cause is: a picker with no candidate @@ -366,11 +370,12 @@ type NoActionsDispatchedError struct { SkippedActions map[string]int } -func (e NoActionsDispatchedError) Error() string { +func (e NoGeneratorActionsError) Error() string { return fmt.Sprintf( - "%d step(s) ran and none of them dispatched an action: the run observed the app "+ - "and never drove it, so it judged its launch screen over and over and its "+ - "violation count says nothing about the rest of the app%s", + "%d step(s) ran and the action generator drove the app in none of them: whatever "+ + "the spec's setup did to get the app into position, nothing explored it from "+ + "there, so the run judged one screen over and over and its violation count "+ + "says nothing about the rest of the app%s", e.Steps, skipReasonSuffix(e.SkippedActions)) } diff --git a/internal/testrun/testrun_test.go b/internal/testrun/testrun_test.go index 5c7ac94..5a87309 100644 --- a/internal/testrun/testrun_test.go +++ b/internal/testrun/testrun_test.go @@ -299,9 +299,10 @@ func TestRunOutcome_ReportsViolationsOnlyUnderTheFlag(t *testing.T) { violated := runner.Summary{ Steps: 7, DispatchedActions: 7, + GeneratorActions: 7, Violations: []runner.ViolationRecord{{StepIndex: 3, Properties: []string{"balanceMoves"}}}, } - clean := runner.Summary{Steps: 7, DispatchedActions: 7} + clean := runner.Summary{Steps: 7, DispatchedActions: 7, GeneratorActions: 7} if err := runOutcome(Options{}, violated); err != nil { t.Errorf("without --exit-on-violation a violated run must succeed, got %v", err) @@ -339,7 +340,12 @@ func TestRunOutcome_ARunThatJudgedNothingIsNotASuccess(t *testing.T) { // A screen that composes now and then costs a run steps, not its verdict. A // check that fired here would turn every healthy android run red. - mostlyJudged := runner.Summary{Steps: 6, SkippedVerification: 5, DispatchedActions: 1} + mostlyJudged := runner.Summary{ + Steps: 6, + SkippedVerification: 5, + DispatchedActions: 1, + GeneratorActions: 1, + } if err := runOutcome(Options{}, mostlyJudged); err != nil { t.Errorf("a run that judged one of its 6 steps must succeed, got %v", err) } @@ -355,10 +361,10 @@ func TestRunOutcome_ARunThatDroveNothingIsNotASuccess(t *testing.T) { SkippedActions: map[string]int{"no_action_produced": 200}, } err := runOutcome(Options{}, droveNothing) - var dead NoActionsDispatchedError + var dead NoGeneratorActionsError if !errors.As(err, &dead) { t.Fatalf("a run that dispatched none of its 200 steps' actions came back %v, "+ - "want a NoActionsDispatchedError", err) + "want a NoGeneratorActionsError", err) } if dead.Steps != 200 { t.Errorf("steps: got %d, want 200", dead.Steps) @@ -367,11 +373,12 @@ func TestRunOutcome_ARunThatDroveNothingIsNotASuccess(t *testing.T) { t.Errorf("the error never names why nothing was dispatched: %v", dead) } - // One action is exploration, however little. A check that fired here would - // be red on any run whose screen offers the generator nothing for a while. - droveOnce := runner.Summary{Steps: 200, DispatchedActions: 1} + // One generator action is exploration, however little. A check that fired + // here would be red on any run whose screen offers the generator nothing for + // a while. + droveOnce := runner.Summary{Steps: 200, DispatchedActions: 1, GeneratorActions: 1} if err := runOutcome(Options{}, droveOnce); err != nil { - t.Errorf("a run that dispatched one action must succeed, got %v", err) + t.Errorf("a run whose generator dispatched one action must succeed, got %v", err) } // The sweeps that measure what a spec extracts and where a generator reaches @@ -401,6 +408,41 @@ func TestRunOutcome_ARunThatDroveNothingIsNotASuccess(t *testing.T) { } } +// A spec whose setup logs in drives the app before the generator is ever +// consulted, so a run whose every model call failed still reached the driver a +// few times. Those actions are the harness getting into position: counting them +// as the run driving the app passed 86 steps of folio that explored nothing. +func TestRunOutcome_SetupActionsDoNotCarryARunWhoseGeneratorDroveNothing(t *testing.T) { + loginThenNothing := runner.Summary{ + Steps: 86, + DispatchedActions: 3, + GeneratorActions: 0, + SkippedActions: map[string]int{"no_action_produced": 83}, + } + err := runOutcome(Options{}, loginThenNothing) + var dead NoGeneratorActionsError + if !errors.As(err, &dead) { + t.Fatalf("a run whose 3 dispatched actions all came from setup came back %v, "+ + "want a NoGeneratorActionsError", err) + } + if dead.Steps != 86 { + t.Errorf("steps: got %d, want 86", dead.Steps) + } + if !strings.Contains(dead.Error(), "no_action_produced") { + t.Errorf("the error never names why the generator dispatched nothing: %v", dead) + } + + // The same run under --exit-on-violation still reports its evidence: CI + // reads exit 2 as "the run found the bug", and a setup-only run that found + // one must not be downgraded to a broken harness. + found := loginThenNothing + found.Violations = []runner.ViolationRecord{{StepIndex: 2, Properties: []string{"balanceMoves"}}} + var violations ViolationsError + if err := runOutcome(Options{ExitOnViolation: true}, found); !errors.As(err, &violations) { + t.Errorf("a setup-only run that found a violation came back %v, want a ViolationsError", err) + } +} + // wedgedLaunchDriver never returns from Launch, standing in for a driver whose // device-side session is stuck. type wedgedLaunchDriver struct { diff --git a/skills/sanderling-run-triage/SKILL.md b/skills/sanderling-run-triage/SKILL.md index 856846b..d879ab7 100644 --- a/skills/sanderling-run-triage/SKILL.md +++ b/skills/sanderling-run-triage/SKILL.md @@ -25,7 +25,10 @@ complete and useful answer. - **1** means the harness broke. A bad target gives `error: launch app: page load error net::ERR_UNSAFE_PORT` and exit 1, and writes no run directory at all, because the trace is created after the launch - succeeds. + succeeds. A run that finished also exits 1 when it holds no verdict to report: + a spec with no properties, a run no step of which reached the verifier, or one + whose action generator never drove the app (section 5). Those do leave a full + run directory behind. Anything other than 0 and 2 means the run did not complete, and a missing `trace.jsonl` under a 0 or a 2 means there is nothing to judge rather than @@ -182,7 +185,15 @@ seconds a step for a run that is driving something real. **It spent its budget on one action.** Count `next_action` by kind and selector. A run whose actions are one selector explored nothing, whatever its step count. -None of these change the exit code. All of them change what the run proves, +**The generator never drove the app.** A model run whose every call fails still +dispatches the actions its spec's setup produced, so a login-fronted spec leaves +a trace and a summary that read like a run that acted. This one the run refuses +itself: exit 1 and `N step(s) ran and the action generator drove the app in none +of them`, followed by the per reason tally of what never reached the app. +`llm-calls.jsonl` carries the cause, one record per step, and the trace's +`action_skipped` names it on each step that produced nothing. + +None of the others change the exit code. All of them change what the run proves, which is nothing. ## 6. `skipped_verification`, `transitional`, and the judged count