fix(perfbench): grant write tools so mutating tasks measure real edits

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 <tmpfile>`, 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)
 	}

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