From f1e9ceca5f05185d344c17c43c776acecc1ce983 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 19 Jul 2026 22:49:09 +0530 Subject: [PATCH 1/5] fix(perfbench): grant write tools so mutating tasks measure real edits The turn benchmark invoked `zero exec` in its default read-only posture, which exposes no write or shell tools (no edit_file/apply_patch/ write_file/exec_command/bash). So the mutating classes (edit/fix/ refactor) could never apply a change: a run either ground to the turn ceiling hunting for an edit tool or reported a no-tool blocker, and the only tasks that 'passed' were ones whose oracle grep matched the stamped .zero-answer.txt narration rather than a real edit. This invalidated every mutating-class number in both baselines. A live trace of fix-07 (a one-line admin->user prefix change) showed the model calling only read_file/grep/lsp_navigate/update_plan and then stating it had no file-writing tool. Adding --skip-permissions-unsafe exposes the full tool set; each task already runs in an isolated throwaway fixture copy, so there is nothing to protect. With the flag, edit-01/fix-01/ fix-07/refactor-01 go from failing (or false-passing) to 4/4 real passes, and tool_execution rises from ~0.4% to ~29% of wall as the write tools are actually exercised. Adds TestBuildTurnExecArgsGrantsWriteTools pinning the flag as a correctness contract. --- internal/perfbench/turn_bench.go | 11 ++++++++++- internal/perfbench/turn_bench_test.go | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/perfbench/turn_bench.go b/internal/perfbench/turn_bench.go index 839c7362e..41ffeba9c 100644 --- a/internal/perfbench/turn_bench.go +++ b/internal/perfbench/turn_bench.go @@ -790,7 +790,16 @@ 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} + // --skip-permissions-unsafe is REQUIRED, not optional: without it `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. Each task already runs in an isolated, throwaway fixture + // copy (see NewTurnExecRunner), so granting the full tool set has nothing to + // protect and is the whole point: the benchmark must measure real edits. + args := []string{"exec", "--skip-permissions-unsafe", "--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..256e0d28d 100644 --- a/internal/perfbench/turn_bench_test.go +++ b/internal/perfbench/turn_bench_test.go @@ -1192,6 +1192,28 @@ func TestBuildTurnExecArgsIncludesExecProfile(t *testing.T) { } } +// Every benchmark invocation MUST grant the write/shell tool set. Without +// --skip-permissions-unsafe the agent runs read-only and cannot apply any edit, +// so the mutating classes measure nothing but oracle/answer contamination. Each +// task runs in an isolated throwaway fixture copy, so there is nothing to +// protect. 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) + found := false + for _, arg := range args { + if arg == "--skip-permissions-unsafe" { + found = true + break + } + } + if !found { + t.Fatalf("benchmark exec args must include --skip-permissions-unsafe so the agent can apply edits, got %v", args) + } + if args[len(args)-1] != "edit the file" { + t.Fatalf("prompt must stay the last argument, got %v", args) + } +} + // 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 From 19a18b2320efe31a1bbc60b435c1de134e54c118 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 19 Jul 2026 23:14:28 +0530 Subject: [PATCH 2/5] fix(perfbench): use --auto member and let the oracle judge incomplete exits Reviewer feedback on the write-tools grant: --skip-permissions-unsafe is broader than the benchmark needs. It also drops the workspace, network, and destructive-command guards and auto-retries sandbox-blocked commands unsandboxed. Switch to --auto member, which grants exactly the write + sandboxed-shell tools the mutating classes need while keeping those safeguards. member-auto's shell is sandboxed, though, so 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 used to treat any nonzero exit as failure before consulting the oracle, which would false-fail those correct edits. Make the oracle authoritative for oracle-bearing tasks: the stamped test/grep against the actual fixture files is ground truth, so a correct edit passes regardless of the exit code. Latency-only tasks have no oracle, so a nonzero exit still fails them. Verified with a 4-task glm-5.2 smoke (edit-01/fix-01/fix-07/refactor-01): 4/4 real passes under --auto member. Updates the exec-args test to assert --auto member is present and --skip-permissions-unsafe is absent, and adds TestOracleAuthoritativeOnIncompleteExit and TestNonzeroExitStillFailsLatencyOnly. --- internal/perfbench/turn_bench.go | 46 ++++++++++++----- internal/perfbench/turn_bench_test.go | 73 +++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 24 deletions(-) diff --git a/internal/perfbench/turn_bench.go b/internal/perfbench/turn_bench.go index 41ffeba9c..c5a296d91 100644 --- a/internal/perfbench/turn_bench.go +++ b/internal/perfbench/turn_bench.go @@ -582,8 +582,11 @@ func WriteTurnBenchJSON(w io.Writer, result TurnBenchResult) error { // 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 (OracleTest/VerificationCommand) when it has one: +// the stamped oracle is ground truth, so an oracle-bearing task that applied the +// right edit but exited INCOMPLETE (its sandboxed self-verification couldn't run) +// still passes. For a latency-only task (no oracle) the stream-json run_end exit +// code is the only signal, so a nonzero exit fails it. func NewTurnExecRunner(binary string, extraArgs ...string) TurnRunner { return func(ctx context.Context, task BenchTask, rc RunContext) TurnTaskOutcome { // Isolate the workspace: copy the fixture into a fresh temp dir so a @@ -656,10 +659,20 @@ 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. For a latency-only task (no oracle) the + // exit code is the only correctness signal, so a nonzero exit is the verdict + // and we stop here. For an oracle-bearing task the oracle below — the + // compiled test / grep run against the actual fixture files — is ground + // truth: a run that applied the correct edit but exited INCOMPLETE (e.g. it + // couldn't self-verify because the sandboxed shell was unavailable under + // --auto member) still achieved the task, so we let the oracle decide and + // drop the exit-code failure. A run that really did nothing useful just + // fails that same oracle, so this can't turn a bad run into a pass. if outcome.VerifyErr != "" { - return outcome + if len(task.VerificationCommand) == 0 { + 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,16 +803,23 @@ func stampOracleAndAnswer(task BenchTask, outBuf []byte) error { } func buildTurnExecArgs(task BenchTask, rc RunContext, tracePath string, extraArgs []string) []string { - // --skip-permissions-unsafe is REQUIRED, not optional: without it `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 + // --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. Each task already runs in an isolated, throwaway fixture - // copy (see NewTurnExecRunner), so granting the full tool set has nothing to - // protect and is the whole point: the benchmark must measure real edits. - args := []string{"exec", "--skip-permissions-unsafe", "--output-format", "stream-json", "--trace", tracePath} + // 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 256e0d28d..abff4e7f1 100644 --- a/internal/perfbench/turn_bench_test.go +++ b/internal/perfbench/turn_bench_test.go @@ -750,6 +750,49 @@ 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 (nonzero) even after a correct edit. For an ORACLE-bearing +// task the runner must treat the stamped oracle — not that exit code — 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. +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) + } +} + +// 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,22 +1235,30 @@ func TestBuildTurnExecArgsIncludesExecProfile(t *testing.T) { } } -// Every benchmark invocation MUST grant the write/shell tool set. Without -// --skip-permissions-unsafe the agent runs read-only and cannot apply any edit, -// so the mutating classes measure nothing but oracle/answer contamination. Each -// task runs in an isolated throwaway fixture copy, so there is nothing to -// protect. This is a correctness contract, not a preference. +// 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) - found := false - for _, arg := range args { - if arg == "--skip-permissions-unsafe" { - found = true + autoMember := false + for i := 0; i < len(args)-1; i++ { + if args[i] == "--auto" && args[i+1] == "member" { + autoMember = true break } } - if !found { - t.Fatalf("benchmark exec args must include --skip-permissions-unsafe so the agent can apply edits, got %v", args) + 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) From 8053bc37777af291b7e710c334cf07dd30dc5b4f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 19 Jul 2026 23:21:19 +0530 Subject: [PATCH 3/5] fix(perfbench): reject fixtureless tasks so the write-tools grant can't escape isolation CodeRabbit and gnanam both flagged the same structural gap: buildTurnExecArgs grants the write + sandboxed-shell tools to every invocation, but the fixture isolation was conditional (a task with no workspaceFixture ran in the caller's cwd). So the grant and the guard could drift apart, and a fixtureless task would run an agent with write/shell tools in the caller's real repo dir. Make the isolation a checked precondition: NewTurnExecRunner now rejects a task with no workspaceFixture before launching zero exec, failing loudly rather than silently running in cwd. Every shipped task already declares a fixture, so this only fires on a malformed or newly added one. Adds TestTurnRunnerRejectsFixturelessTask (rejection is pre-launch: it returns the fixture error even with a nonexistent binary). --- internal/perfbench/turn_bench.go | 15 +++++++++++++-- internal/perfbench/turn_bench_test.go | 21 +++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/internal/perfbench/turn_bench.go b/internal/perfbench/turn_bench.go index c5a296d91..2aba10956 100644 --- a/internal/perfbench/turn_bench.go +++ b/internal/perfbench/turn_bench.go @@ -589,10 +589,21 @@ func WriteTurnBenchJSON(w io.Writer, result TurnBenchResult) error { // code is the only signal, so a nonzero exit 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. + // fixture or bleed into a later iteration of the same task. if fixture := strings.TrimSpace(task.WorkspaceFixture); fixture != "" { copyDir, parent, cerr := copyFixture(fixture) if cerr != nil { diff --git a/internal/perfbench/turn_bench_test.go b/internal/perfbench/turn_bench_test.go index abff4e7f1..e8fda6f2f 100644 --- a/internal/perfbench/turn_bench_test.go +++ b/internal/perfbench/turn_bench_test.go @@ -1265,6 +1265,27 @@ func TestBuildTurnExecArgsGrantsWriteTools(t *testing.T) { } } +// 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 From 57d1ae517e48c7e47b2abdec9a8c95b2c0f8b7df Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 19 Jul 2026 23:31:55 +0530 Subject: [PATCH 4/5] fix(perfbench): defer to the oracle only for INCOMPLETE (exit 4), not every nonzero exit anand's follow-up: the previous commit cleared VerifyErr for any nonzero run_end whenever a VerificationCommand was present, which would also convert a crash (1), usage error (2), provider failure (3), or interruption (130) into a passing result whenever a partial edit happened to satisfy the oracle. Gate the oracle override on exitCode == exitIncomplete (4): the completion gate's "work maybe done but not signaled" case, the only one where a correct edit can legitimately exit nonzero (its sandboxed self-verify couldn't run under --auto member). Every other nonzero exit stays authoritative, so a partial edit can't launder a crashed or interrupted run into a pass. The 4-task glm-5.2 smoke stays 4/4 (those tasks exit INCOMPLETE, not some other code). Adds TestNonIncompleteExitStaysAuthoritative (a correct edit that reports exit 1 must still fail) and mirrors internal/cli.exitIncomplete as a local execExitIncomplete constant. --- internal/perfbench/turn_bench.go | 40 ++++++++++++++++++--------- internal/perfbench/turn_bench_test.go | 33 +++++++++++++++++++--- 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/internal/perfbench/turn_bench.go b/internal/perfbench/turn_bench.go index 2aba10956..8025ab8d8 100644 --- a/internal/perfbench/turn_bench.go +++ b/internal/perfbench/turn_bench.go @@ -578,15 +578,26 @@ 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 by the task's oracle (OracleTest/VerificationCommand) when it has one: // the stamped oracle is ground truth, so an oracle-bearing task that applied the -// right edit but exited INCOMPLETE (its sandboxed self-verification couldn't run) -// still passes. For a latency-only task (no oracle) the stream-json run_end exit -// code is the only signal, so a nonzero exit fails it. +// 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. For a +// latency-only task (no oracle) any nonzero exit is the only signal, so it fails. 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 @@ -670,17 +681,20 @@ func NewTurnExecRunner(binary string, extraArgs ...string) TurnRunner { } } - // Decide what a nonzero exit means. For a latency-only task (no oracle) the - // exit code is the only correctness signal, so a nonzero exit is the verdict - // and we stop here. For an oracle-bearing task the oracle below — the - // compiled test / grep run against the actual fixture files — is ground - // truth: a run that applied the correct edit but exited INCOMPLETE (e.g. it - // couldn't self-verify because the sandboxed shell was unavailable under - // --auto member) still achieved the task, so we let the oracle decide and - // drop the exit-code failure. A run that really did nothing useful just - // fails that same oracle, so this can't turn a bad run into a pass. + // 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 != "" { - if len(task.VerificationCommand) == 0 { + if len(task.VerificationCommand) == 0 || exitCode != execExitIncomplete { return outcome } outcome.VerifyErr = "" diff --git a/internal/perfbench/turn_bench_test.go b/internal/perfbench/turn_bench_test.go index e8fda6f2f..70dceb835 100644 --- a/internal/perfbench/turn_bench_test.go +++ b/internal/perfbench/turn_bench_test.go @@ -753,10 +753,11 @@ 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 (nonzero) even after a correct edit. For an ORACLE-bearing -// task the runner must treat the stamped oracle — not that exit code — 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. +// 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 @@ -773,6 +774,30 @@ echo '{"type":"run_end","exitCode":4}' } } +// TestNonIncompleteExitStaysAuthoritative is the guard the oracle-authoritative +// change MUST NOT weaken: only INCOMPLETE (exit 4) defers to the oracle. A crash +// (1), usage error (2), 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. Here the stub applies the CORRECT edit-01 rename +// (so the oracle WOULD pass) but reports a crash (exit 1); the task must still +// fail, and the failure must be attributed to the exit code, not the oracle. +func TestNonIncompleteExitStaysAuthoritative(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":1}' +`) + if outcome.Err != nil { + t.Fatalf("a crash exit should be a task fail, not a harness error: %v", outcome.Err) + } + if outcome.Passed { + t.Fatalf("a run that exited 1 (crash) must not pass even when the edit satisfies the oracle: %+v", outcome) + } + if !strings.Contains(outcome.VerifyErr, "exit code 1") { + t.Fatalf("a non-INCOMPLETE nonzero exit must stay authoritative and surface its code, got VerifyErr=%q", outcome.VerifyErr) + } +} + // 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 From 390460f419994eae3bc77b34d36706c80e6abb97 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 19 Jul 2026 23:56:38 +0530 Subject: [PATCH 5/5] test(perfbench): pin the exit-4 oracle deferral from both sides; drop dead fixture branch Follow-ups from a self-review of the exit-4 gating: - Add TestIncompleteExitStillFailsWhenOracleFails: an INCOMPLETE (exit 4) run that did NO work must still FAIL, because deferring to the oracle still means the oracle is consulted. The sibling TestOracleAuthoritativeOnIncompleteExit applies a correct edit and would pass either way, so without this a regression that turned exit 4 into a blanket pass would go unnoticed. - Broaden TestNonIncompleteExitStaysAuthoritative to a table over exit 1/3/130 (crash/provider/interrupt), not just 1, so every named launder-risk code is covered. - Tighten the NewTurnExecRunner doc: the verdict gate is the VerificationCommand (which runs the stamped OracleTest via go test), not OracleTest on its own. - Drop the now-dead "if WorkspaceFixture != empty" branch: the fixtureless guard added earlier already guarantees it is set. No behavior change; the 4-task glm-5.2 smoke stays 4/4. --- internal/perfbench/turn_bench.go | 35 +++++++++--------- internal/perfbench/turn_bench_test.go | 53 ++++++++++++++++++--------- 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/internal/perfbench/turn_bench.go b/internal/perfbench/turn_bench.go index 8025ab8d8..83e0069fc 100644 --- a/internal/perfbench/turn_bench.go +++ b/internal/perfbench/turn_bench.go @@ -592,12 +592,14 @@ const execExitIncomplete = 4 // 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 by the task's oracle (OracleTest/VerificationCommand) when it has one: -// the stamped oracle 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. For a -// latency-only task (no oracle) any nonzero exit is the only signal, so it fails. +// 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 @@ -614,17 +616,16 @@ func NewTurnExecRunner(binary string, extraArgs ...string) TurnRunner { } // 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. - 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 { diff --git a/internal/perfbench/turn_bench_test.go b/internal/perfbench/turn_bench_test.go index 70dceb835..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" @@ -776,26 +777,44 @@ echo '{"type":"run_end","exitCode":4}' // TestNonIncompleteExitStaysAuthoritative is the guard the oracle-authoritative // change MUST NOT weaken: only INCOMPLETE (exit 4) defers to the oracle. A crash -// (1), usage error (2), 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. Here the stub applies the CORRECT edit-01 rename -// (so the oracle WOULD pass) but reports a crash (exit 1); the task must still -// fail, and the failure must be attributed to the exit code, not the oracle. +// (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, `sed 's/const MaxRetries = 3/const RetryLimit = 3/' main.go > .zero-tmp && mv .zero-tmp main.go -echo '{"type":"run_end","exitCode":1}' + outcome := runTurnStub(t, task, `echo '{"type":"run_end","exitCode":4}' `) - if outcome.Err != nil { - t.Fatalf("a crash exit should be a task fail, not a harness error: %v", outcome.Err) - } - if outcome.Passed { - t.Fatalf("a run that exited 1 (crash) must not pass even when the edit satisfies the oracle: %+v", outcome) - } - if !strings.Contains(outcome.VerifyErr, "exit code 1") { - t.Fatalf("a non-INCOMPLETE nonzero exit must stay authoritative and surface its code, got VerifyErr=%q", outcome.VerifyErr) - } + assertVerifyFailed(t, "incomplete exit with no edit applied", outcome) } // TestNonzeroExitStillFailsLatencyOnly is the guard on the other side of that