diff --git a/internal/perfbench/turn_bench.go b/internal/perfbench/turn_bench.go index 839c7362e..83e0069fc 100644 --- a/internal/perfbench/turn_bench.go +++ b/internal/perfbench/turn_bench.go @@ -578,28 +578,54 @@ func WriteTurnBenchJSON(w io.Writer, result TurnBenchResult) error { return encoder.Encode(result) } +// execExitIncomplete mirrors internal/cli.exitIncomplete (the `zero exec` exit +// code, 4) for a headless run the completion gate marked INCOMPLETE: the run +// stopped without a completion signal (e.g. it couldn't self-verify because its +// sandboxed shell was unavailable under --auto member), which is distinct from a +// crash (1), usage error (2), provider failure (3), or interruption (130). It is +// the ONLY nonzero exit an oracle-bearing task may defer to the oracle for; every +// other nonzero exit stays authoritative. Kept as a local literal so perfbench +// doesn't import the cli package; must stay in sync with that constant. +const execExitIncomplete = 4 + // NewTurnExecRunner builds the production turn-benchmark runner: it invokes // headless `zero exec` with stream-json output AND `--trace `, then // parses the emitted NDJSON trace into a *trace.TurnTrace. binary is the path to // the `zero` binary; extraArgs are appended to every invocation. Pass/fail is -// decided from the stream-json run_end exit code (and the task's -// VerificationCommand when present), exactly like NewExecRunner. +// decided by the task's oracle when it has one. The oracle that gates the verdict +// is the VerificationCommand (which compiles and runs the stamped OracleTest via +// `go test`, plus any greps against the fixture); it is ground truth, so an +// oracle-bearing task that applied the right edit but exited INCOMPLETE (exit 4, +// its sandboxed self-verification couldn't run) still passes. Any OTHER nonzero +// exit (crash/usage/provider/interrupt) stays authoritative and fails even an +// oracle-bearing task. A latency-only task has no VerificationCommand, so any +// nonzero exit is its only signal and fails it. func NewTurnExecRunner(binary string, extraArgs ...string) TurnRunner { return func(ctx context.Context, task BenchTask, rc RunContext) TurnTaskOutcome { + // buildTurnExecArgs grants the write + sandboxed-shell tool set to EVERY + // invocation, so a task MUST run inside an isolated fixture copy — never the + // caller's cwd. The grant is unconditional, so the isolation can't be + // optional: a fixtureless task would turn an agent with write/shell tools + // loose in the caller's working directory (their real repo). Enforce the + // invariant instead of trusting it — reject a fixtureless task up front, + // before anything is launched, so the grant and the guard can't drift apart. + // Every shipped task declares a fixture, so this only fires on a malformed or + // newly added task, and it fails loudly rather than silently. + if strings.TrimSpace(task.WorkspaceFixture) == "" { + return TurnTaskOutcome{Err: errors.New("benchmark task has no workspaceFixture: the run grants write/shell tools and must execute in an isolated fixture copy, not the caller's cwd")} + } // Isolate the workspace: copy the fixture into a fresh temp dir so a // mutating task (edit/fix/refactor) can't dirty the shared, checked-in - // fixture or bleed into a later iteration of the same task. When no - // fixture is configured the agent runs in the caller's cwd as before. - if fixture := strings.TrimSpace(task.WorkspaceFixture); fixture != "" { - copyDir, parent, cerr := copyFixture(fixture) - if cerr != nil { - return TurnTaskOutcome{Err: fmt.Errorf("isolate fixture: %w", cerr)} - } - // Clean the whole unique parent (which owns copyDir) so the - // per-invocation scratch dir never leaks. - defer os.RemoveAll(parent) - task.WorkspaceFixture = copyDir + // fixture or bleed into a later iteration of the same task. The guard above + // guarantees WorkspaceFixture is set, so this always runs. + copyDir, parent, cerr := copyFixture(strings.TrimSpace(task.WorkspaceFixture)) + if cerr != nil { + return TurnTaskOutcome{Err: fmt.Errorf("isolate fixture: %w", cerr)} } + // Clean the whole unique parent (which owns copyDir) so the per-invocation + // scratch dir never leaks. + defer os.RemoveAll(parent) + task.WorkspaceFixture = copyDir traceFile, err := os.CreateTemp("", "zero-turn-trace-*.ndjson") if err != nil { @@ -656,10 +682,23 @@ func NewTurnExecRunner(binary string, extraArgs ...string) TurnRunner { } } - // A nonzero agent exit already decided failure; don't run verification or - // mark the task passed. + // Decide what a nonzero exit means. We defer to the oracle for exactly ONE + // case: an oracle-bearing task that exited INCOMPLETE (exit 4). That is the + // completion gate downgrading a run whose edits may be correct but that + // couldn't emit a completion signal (e.g. its sandboxed self-verify couldn't + // run under --auto member) — the oracle below (the compiled test / grep run + // against the actual fixture files) is ground truth, so if the edit really + // landed it passes, and if the run abandoned the task the oracle fails it. + // Every OTHER nonzero exit stays authoritative: a crash (1), usage error + // (2), provider failure (3), or interruption (130) is a genuine failure, so + // a partial edit that happens to satisfy the oracle can't launder it into a + // pass. A latency-only task has no oracle to defer to, so any nonzero exit + // fails it. if outcome.VerifyErr != "" { - return outcome + if len(task.VerificationCommand) == 0 || exitCode != execExitIncomplete { + return outcome + } + outcome.VerifyErr = "" } // Stamp the compiler-backed oracle and capture the agent's final answer // BEFORE running the verification command. Both write into the fixture @@ -790,7 +829,23 @@ func stampOracleAndAnswer(task BenchTask, outBuf []byte) error { } func buildTurnExecArgs(task BenchTask, rc RunContext, tracePath string, extraArgs []string) []string { - args := []string{"exec", "--output-format", "stream-json", "--trace", tracePath} + // --auto member is REQUIRED, not optional: without a permission grant `zero + // exec` runs in its default read-only posture, which exposes no write or shell + // tools (no edit_file/apply_patch/write_file/exec_command/bash). The mutating + // task classes (edit/fix/refactor) then cannot apply any change, so every run + // grinds to the turn ceiling or reports a no-tool blocker, and the only tasks + // that "pass" are ones whose oracle grep matches the stamped answer text rather + // than a real edit. member-auto grants the write + sandboxed-shell tools the + // benchmark needs while keeping the workspace/network/destructive safeguards, so + // it is preferred over the broader --skip-permissions-unsafe (which also drops + // those guards and auto-retries blocked commands unsandboxed). Note the shell it + // grants is sandboxed: on a host without sandbox setup the agent's own + // self-verification (go test/build) can't run and the turn exits INCOMPLETE even + // after a correct edit — the runner treats the stamped oracle, not that exit + // code, as ground truth for oracle-bearing tasks (see NewTurnExecRunner), so a + // correct edit still passes. Each task also runs in an isolated, throwaway + // fixture copy, so the granted tools have nothing outside the task to harm. + args := []string{"exec", "--auto", "member", "--output-format", "stream-json", "--trace", tracePath} if model := strings.TrimSpace(rc.Model); model != "" { args = append(args, "--model", model) } diff --git a/internal/perfbench/turn_bench_test.go b/internal/perfbench/turn_bench_test.go index b123af027..cbb3ae0c4 100644 --- a/internal/perfbench/turn_bench_test.go +++ b/internal/perfbench/turn_bench_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "path/filepath" "strings" @@ -750,6 +751,92 @@ echo '{"type":"run_end","exitCode":0}' } } +// TestOracleAuthoritativeOnIncompleteExit is the companion to the --auto member +// switch: under member-auto the agent's shell is sandboxed, so on a host without +// sandbox setup its self-verification (go test/build) can't run and the turn +// exits INCOMPLETE (exit 4) even after a correct edit. For an ORACLE-bearing task +// the runner must treat the stamped oracle — not that INCOMPLETE exit — as ground +// truth: here the stub applies the real edit-01 rename but reports exitCode 4, and +// the task must still pass because the fixture is correct. This defers ONLY for +// exit 4; TestNonIncompleteExitStaysAuthoritative pins the other side. +func TestOracleAuthoritativeOnIncompleteExit(t *testing.T) { + task := loadBaselineTask(t, "edit-01") + outcome := runTurnStub(t, task, `sed 's/const MaxRetries = 3/const RetryLimit = 3/' main.go > .zero-tmp && mv .zero-tmp main.go +echo '{"type":"run_end","exitCode":4}' +`) + if outcome.Err != nil { + t.Fatalf("incomplete-exit with a correct edit should pass, got harness error: %v", outcome.Err) + } + if !outcome.Passed { + t.Fatalf("a correct edit that exited INCOMPLETE (exit 4) must still pass its oracle: %+v", outcome) + } + if strings.TrimSpace(outcome.VerifyErr) != "" { + t.Fatalf("a passing oracle must clear the exit-code VerifyErr, got %q", outcome.VerifyErr) + } +} + +// TestNonIncompleteExitStaysAuthoritative is the guard the oracle-authoritative +// change MUST NOT weaken: only INCOMPLETE (exit 4) defers to the oracle. A crash +// (1), provider failure (3), or interruption (130) is a genuine failure, so it +// stays authoritative even for an oracle-bearing task — otherwise a partial edit +// that happens to satisfy the oracle would launder a crashed or interrupted run +// into a pass. Each case applies the CORRECT edit-01 rename (so the oracle WOULD +// pass) but reports the failing exit code; the task must still fail, and the +// failure must be attributed to the exit code, not the oracle. +func TestNonIncompleteExitStaysAuthoritative(t *testing.T) { + for _, code := range []int{1, 3, 130} { + t.Run(fmt.Sprintf("exit%d", code), func(t *testing.T) { + task := loadBaselineTask(t, "edit-01") + outcome := runTurnStub(t, task, fmt.Sprintf(`sed 's/const MaxRetries = 3/const RetryLimit = 3/' main.go > .zero-tmp && mv .zero-tmp main.go +echo '{"type":"run_end","exitCode":%d}' +`, code)) + if outcome.Err != nil { + t.Fatalf("a nonzero exit should be a task fail, not a harness error: %v", outcome.Err) + } + if outcome.Passed { + t.Fatalf("a run that exited %d must not pass even when the edit satisfies the oracle: %+v", code, outcome) + } + if want := fmt.Sprintf("exit code %d", code); !strings.Contains(outcome.VerifyErr, want) { + t.Fatalf("a non-INCOMPLETE nonzero exit must stay authoritative and surface its code, got VerifyErr=%q", outcome.VerifyErr) + } + }) + } +} + +// TestIncompleteExitStillFailsWhenOracleFails pins the OTHER half of the exit-4 +// deferral: deferring to the oracle on INCOMPLETE must still MEAN the oracle is +// consulted, not a blanket pass. Here the stub does no real work and exits +// INCOMPLETE (4); the edit-01 rename never happened, so the oracle fails and the +// task fails. Without this, a regression that turned exit 4 into an unconditional +// pass would slip through, because its sibling TestOracleAuthoritativeOnIncomplete- +// Exit applies a CORRECT edit and would pass either way. +func TestIncompleteExitStillFailsWhenOracleFails(t *testing.T) { + task := loadBaselineTask(t, "edit-01") + outcome := runTurnStub(t, task, `echo '{"type":"run_end","exitCode":4}' +`) + assertVerifyFailed(t, "incomplete exit with no edit applied", outcome) +} + +// TestNonzeroExitStillFailsLatencyOnly is the guard on the other side of that +// change: a latency-only task has no oracle to appeal to, so the exit code is +// the ONLY correctness signal and a nonzero exit must still fail it. Without +// this, dropping the exit-code gate for oracle tasks could be misread as +// dropping it everywhere. +func TestNonzeroExitStillFailsLatencyOnly(t *testing.T) { + task := loadBaselineTask(t, "longproc-01") + outcome := runTurnStub(t, task, `echo '{"type":"run_end","exitCode":4}' +`) + if outcome.Err != nil { + t.Fatalf("latency-only nonzero exit should be a verify fail, not a harness error: %v", outcome.Err) + } + if outcome.Passed { + t.Fatalf("a latency-only task that exited nonzero must not pass: %+v", outcome) + } + if strings.TrimSpace(outcome.VerifyErr) == "" { + t.Fatalf("a latency-only nonzero exit must surface a VerifyErr, got none: %+v", outcome) + } +} + // --- Satisfiable tests: the oracle PASSES the right thing (real fix applied) --- // TestStampedOraclePassesWhenRefactorHappened proves the refactor-01 oracle is @@ -1192,6 +1279,57 @@ func TestBuildTurnExecArgsIncludesExecProfile(t *testing.T) { } } +// Every benchmark invocation MUST grant the write/shell tool set via --auto +// member. Without a permission grant the agent runs read-only and cannot apply +// any edit, so the mutating classes measure nothing but oracle/answer +// contamination. member-auto is the RIGHT grant: it exposes the write + +// sandboxed-shell tools the benchmark needs while keeping the workspace/network/ +// destructive safeguards, so the args must NOT reach for the broader +// --skip-permissions-unsafe (which drops those guards). This is a correctness +// contract, not a preference. +func TestBuildTurnExecArgsGrantsWriteTools(t *testing.T) { + args := buildTurnExecArgs(BenchTask{ID: "t", Prompt: "edit the file"}, RunContext{Model: "m"}, "trace.ndjson", nil) + autoMember := false + for i := 0; i < len(args)-1; i++ { + if args[i] == "--auto" && args[i+1] == "member" { + autoMember = true + break + } + } + if !autoMember { + t.Fatalf("benchmark exec args must include --auto member so the agent can apply edits, got %v", args) + } + for _, arg := range args { + if arg == "--skip-permissions-unsafe" { + t.Fatalf("benchmark exec args must NOT use --skip-permissions-unsafe (member-auto keeps the sandbox/network guards), got %v", args) + } + } + if args[len(args)-1] != "edit the file" { + t.Fatalf("prompt must stay the last argument, got %v", args) + } +} + +// TestTurnRunnerRejectsFixturelessTask is the other half of the write-tools +// contract: because buildTurnExecArgs grants the write + sandboxed-shell tool +// set to every invocation, a task with no fixture would run an agent with those +// tools in the caller's cwd. The runner must reject such a task BEFORE launching +// zero exec. Passing a binary path that does not exist proves the rejection is +// pre-launch — the outcome carries the fixture error, not a spawn error — so this +// runs on every OS (it never reaches the POSIX-only exec stub). +func TestTurnRunnerRejectsFixturelessTask(t *testing.T) { + task := BenchTask{ID: "no-fixture", Prompt: "do a thing"} // no WorkspaceFixture set + outcome := NewTurnExecRunner(filepath.Join(t.TempDir(), "does-not-exist-zero"))(context.Background(), task, RunContext{Model: "m"}) + if outcome.Err == nil { + t.Fatalf("a fixtureless task must be rejected before launch, got %+v", outcome) + } + if !strings.Contains(outcome.Err.Error(), "workspaceFixture") { + t.Fatalf("rejection must name the missing workspaceFixture, got: %v", outcome.Err) + } + if outcome.Passed { + t.Fatalf("a rejected task must not pass, got %+v", outcome) + } +} + // The configured profile must reach the runner's RunContext and be stamped // into the result, so a profile A/B report is self-describing. The boundary // canonicalizes (case/whitespace) so equivalent postures always carry the same