fix(perfbench): grant write tools so mutating tasks measure real edits#763
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTurn benchmarks now require workspace fixtures, defer only INCOMPLETE oracle-bearing exits to oracle verification, preserve other non-zero failures, and use member-mode execution arguments. Tests cover verdict handling, fixture validation, and argument construction. ChangesTurn benchmark execution semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TurnRunner
participant AgentProcess
participant Oracle
TurnRunner->>AgentProcess: Launch task with workspace fixture
AgentProcess-->>TurnRunner: Return exit code and verification error
TurnRunner->>Oracle: Verify oracle after INCOMPLETE exit
Oracle-->>TurnRunner: Return verdict used for pass status
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/perfbench/turn_bench.go`:
- Around line 793-802: Ensure the turn runner grants --skip-permissions-unsafe
only after verified fixture isolation; otherwise reject the task before
launching zero exec. Update the runner logic around NewTurnExecRunner in
internal/perfbench/turn_bench.go:793-802, and update the related tests in
internal/perfbench/turn_bench_test.go:1200-1210 to use a populated
WorkspaceFixture for the positive case and cover rejection of unisolated tasks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94dea3f8-d304-4bb6-946d-298348ba0115
📒 Files selected for processing (2)
internal/perfbench/turn_bench.gointernal/perfbench/turn_bench_test.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the diagnosis is excellent and the fix is right. One thing I'd like tightened before merge, and it's about three lines.
The investigation here is the good part: catching that the mutating classes were passing on oracle/answer contamination rather than real edits, confirming the oracles themselves are sound with a hand-applied fix, and quantifying it with tool_execution going from ~0.4% to ~29% of wall time. Flagging both baselines as invalid rather than quietly re-running is the right call. The --skip-permissions-unsafe conclusion is correct — a headless run can't answer a permission prompt, so there is no narrower posture that both exposes the write tools and completes unattended.
The safety argument depends on an invariant that isn't enforced
The comment and the test both justify the flag the same way — "Each task already runs in an isolated, throwaway fixture copy … so granting the full tool set has nothing to protect." That is true for every task shipped today. It is not true structurally.
The isolation is conditional on the task having a fixture (turn_bench.go:593, 615):
if fixture := strings.TrimSpace(task.WorkspaceFixture); fixture != "" {
copyDir, parent, cerr := copyFixture(fixture) // isolate
...
}
// "When no fixture is configured the agent runs in the caller's cwd as before."
...
if dir := strings.TrimSpace(task.WorkspaceFixture); dir != "" {
cmd.Dir = dir // only then is cwd redirected
}WorkspaceFixture is optional (json:"workspaceFixture,omitempty", no validation requires it) and the runner explicitly contemplates the empty case. The new flag, by contrast, is added unconditionally to every invocation — which the PR description states as a deliberate property.
So the guard is conditional and the grant is not. A task added without a fixture would run an LLM with the full write and shell tool set, no permission prompting, in the caller's working directory — the user's real repo.
I checked how bad that actually is rather than assuming the worst: the workspace sandbox survives unsafe mode. TestRunAppliesSandboxEvenInUnsafeMode (loop_test.go:2838) asserts an out-of-workspace write under PermissionModeUnsafe is denied with BlockOutsideWorkspace at critical risk and never hits the disk. So unsafe disables prompting, not confinement, and the blast radius is the repo rather than the filesystem. That's a meaningful mitigation and it's why this isn't a blocker.
I also confirmed the hazard is latent, not live: both shipped catalogs are fully covered — internal/perfbench/manifests/baseline.json has 48 tasks and cmd/zero-perf-bench/testdata/terminal-bench-sample.json has 2, with zero lacking a workspaceFixture.
Suggested tightening, in the spirit of the PR's own "this is a correctness contract, not a preference": make the runner enforce what the comment asserts, so the grant and the guard can't drift apart. Something like refusing to launch when unsafe permissions are being granted and the task has no fixture:
if strings.TrimSpace(task.WorkspaceFixture) == "" {
return TurnTaskOutcome{Err: errors.New(
"benchmark tasks must declare a workspaceFixture: the run grants unsafe permissions and would otherwise execute in the caller's cwd")}
}That turns "nothing to protect" from a comment into a checked precondition, and it fails loudly at the moment someone adds a fixture-less task rather than silently letting an unprompted agent loose in their repo. A companion test would pair naturally with TestBuildTurnExecArgsGrantsWriteTools — the two together would pin both halves of the contract instead of just the grant.
I'm approving rather than blocking because nothing is broken today and the sandbox bounds the damage, but I do think this belongs in this PR rather than a follow-up: it's the other half of the same invariant, and it's cheaper to add now than to remember later.
Minor
buildTurnExecArgsappendsextraArgsas well, so a caller that already passes--skip-permissions-unsafeproduces it twice. Harmless with current parsing, but if the runner is going to own this flag unconditionally it'd be tidy to de-duplicate, or to document thatextraArgsshouldn't carry it.
Verification
gofmt and go vet clean. internal/perfbench and cmd/zero-perf-bench both green, including the new TestBuildTurnExecArgsGrantsWriteTools. The added assertion that the prompt stays the final argument is a nice touch — that ordering is easy to break when prepending flags.
Agreed on both follow-ups, and thanks for scoping them out explicitly. The .zero-answer.txt grep contamination in edit-03/06/08/09/10 is the one I'd prioritize — until that's excluded, those tasks can still false-pass on narration, which is the same class of defect this PR just fixed for the tool-availability side.
Merge is kevin's call per the program gate.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Blocking: --skip-permissions-unsafe is broader than this benchmark needs. The fixture copy protects benchmark inputs from persistent edits, but unsafe mode also pre-grants permission and automatically retries sandbox-restricted commands unsandboxed (including network-related sandbox failures), so model-generated commands are not confined to the copied fixture. Please invoke the benchmark with --auto member instead. That existing headless mode advertises write and sandboxed-shell tools for mutating tasks while retaining workspace, network, and destructive-operation safeguards. Please update the test to assert that mode and that the unsafe flag is absent.\n\nValidation on f1e9cec: focused perfbench tests pass; make fmt-check and diff hygiene pass.
… 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.
…'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).
|
Thanks all. Pushed two commits ( anand's blocker: dropped One consequence of member-auto worth flagging. member-auto's shell is sandboxed, so on a host without sandbox setup the agent's own self-verification (its CodeRabbit + gnanam: the grant/guard drift. You both caught the same thing, and it survives the member-auto switch: the tool grant is unconditional but fixture isolation was conditional, so a fixtureless task would run write/shell tools in the caller's cwd. Fixed it the way gnanam suggested. gnanam's minor and the follow-ups. gnanam, heads up: your approval got auto-dismissed when I pushed (branch protection drops stale approvals on new commits), not by me by hand. Re-approve if the tightening reads right. Merge stays kevin's call per the program gate. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
The original safety blocker is resolved: --auto member is the correct narrow mode, the unsafe flag is absent, and fixtureless tasks are rejected before launch.
One new blocker remains in the exit-code handling. The comments and test say only INCOMPLETE (exit 4) should defer to the oracle, but the implementation clears VerifyErr for every nonzero run_end whenever VerificationCommand is present. That also converts provider failure (3), crash (1), and interruption (130) into a passing benchmark result when a partial edit happens to satisfy the oracle. Please allow the oracle override only when exitCode == 4; keep every other nonzero exit authoritative, and add a regression test for at least one non-INCOMPLETE code.
Validation on 8053bc3: focused perfbench tests and vet pass; make fmt-check and diff hygiene pass.
… 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.
… 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.
|
Good catch, fixed in You're right on the semantics: the previous commit cleared On tests: One thing I left as a local mirror deliberately: Smoke stays 4/4 (those tasks exit INCOMPLETE, so the tighter gate costs no real passes). Also dropped a now-dead fixture branch and tightened the runner doc while I was in there. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Approved on 390460f. The benchmark now uses the narrow --auto member posture, enforces fixture isolation before launch, and defers to the oracle only for INCOMPLETE exit 4. Crash, provider-failure, and interruption exits remain authoritative, with regression coverage on both sides of the exit-4 boundary.
Validated: focused perfbench tests, focused vet, make fmt-check, and diff hygiene all pass. All GitHub checks are green.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE. Re-reviewed at 390460f. This is a materially better PR than the one I approved at f1e9cec — the permission posture changed for the better, the isolation invariant is now enforced rather than assumed, and the new exit-4 logic holds up under scrutiny.
--auto member is the right call, and it's not a small difference
Swapping --skip-permissions-unsafe for --auto member addresses my concern at its root instead of fencing around it. I measured the actual tool exposure rather than taking the comment's word for it:
| posture | tools | write/shell |
|---|---|---|
| default | 12 | none |
--auto member |
17 | apply_patch, bash, edit_file, exec_command, write_file |
--skip-permissions-unsafe |
35 | same five |
So the original diagnosis is exactly right — the default posture genuinely exposes zero write/shell tools, which is why the mutating classes could never apply an edit. And member grants precisely the five tools the benchmark needs at half the attack surface, while keeping the workspace/network/destructive guards that --skip-permissions-unsafe drops. That's a better answer than the one I signed off on.
The isolation invariant is now enforced
NewTurnExecRunner rejects a fixtureless task before anything launches, and the dead conditional branch is gone so isolation is unconditional rather than best-effort. That's exactly the "grant and guard can't drift apart" property I was after.
I mutation-tested it — neutralised the guard, kept the test:
--- FAIL: TestTurnRunnerRejectsFixturelessTask (0.00s)
It fires. Real regression test, not a decorative one.
The exit-4 deferral — checked carefully, and it's sound
This is new since my last review and it's the piece that deserved the most scrutiny, because on its face it risks reintroducing the exact false-pass class this PR exists to eliminate. Before, any nonzero exit failed the task outright. Now an oracle-bearing task that exits INCOMPLETE has its verdict handed to the oracle — so a run that narrated without editing could, in principle, pass on an oracle that greps the stamped answer text.
I checked whether that's reachable across all 24 mutating tasks in manifests/baseline.json. It isn't:
- Every one of the 24 is gated on
go test ./...with&&, so the compiled oracle must pass against the real fixture source before any grep is even evaluated. Narration alone cannot satisfy any of them. refactor-01,refactor-02,refactor-06do carry positive greps without--exclude=.zero-answer.txt— but each sits behind thego testgate, so the answer file can't carry them either.- The four that reference the answer file at all (
edit-01,edit-02,refactor-03,refactor-05) reference it precisely in order to--excludeit. That's the correct pattern, already in use. - The
nav-*oracles do grep the answer file, which is right for those — the narration is the deliverable.
So the deferral is safe as the corpus stands, and the reasoning in the comment at turn_bench.go:685-696 matches what the code actually does.
One thing worth writing down for the follow-up, though, because it's a genuine change in where the safety comes from: the deferral's correctness is now a property of the oracle corpus, not of the runner. Today every mutating oracle is go test-gated, so the runner can safely defer. A future mutating task whose oracle is a bare positive grep with no compiled-test gate would be able to pass on narration alone at exit 4 — where under the old "any nonzero exit fails" rule it would have failed regardless. That makes the --exclude=.zero-answer.txt convention (and the go test && prefix) worth promoting from convention to something enforced when tasks are loaded. It was already on your follow-up list; this change raises its priority a little rather than creating a new problem.
Test coverage
Five new tests pin all four directions, which is what I'd want for logic like this:
TestOracleAuthoritativeOnIncompleteExit— exit 4 + real edit + passing oracle → passTestIncompleteExitStillFailsWhenOracleFails— exit 4 + failing oracle → failTestNonIncompleteExitStaysAuthoritative— crash/usage/provider/interrupt → fail regardless of oracleTestNonzeroExitStillFailsLatencyOnly— no oracle to defer to → any nonzero failsTestTurnRunnerRejectsFixturelessTask— the isolation guard
Nit
const execExitIncomplete = 4 (turn_bench.go:589) hand-mirrors the unexported cli.exitIncomplete, protected only by a "must stay in sync" comment. internal/cli doesn't import internal/perfbench, so there's no cycle in the way — but the constant is unexported, so a compile-time guard needs exporting it (or adding an exported alias). Worth doing at some point: if the CLI's exit codes are ever renumbered, this silently reclassifies every incomplete run as a hard failure, and the symptom would be a benchmark-wide pass-rate drop with no obvious cause.
Verification
gofmt and go vet ./internal/perfbench/... clean. Full internal/perfbench and cmd/zero-perf-bench suites pass. The five new tests pass individually. Guard mutation-tested as above. Tool-exposure numbers measured against a binary built from this head, not inferred.
Nice iteration — the posture change in particular is the kind of fix that makes the original concern moot rather than mitigated.
Merge is kevin's call per the program gate.
3 participants
The bug
The turn benchmark invokes
zero execin its default read-only posture, which exposes no write or shell tools.zero exec --list-toolsin that mode lists only read tools plusweb_fetch, but noedit_file,apply_patch,write_file,exec_command, orbash. So the mutating task classes (edit, fix, refactor) can never apply a change. A run either grinds to the turn ceiling hunting for an edit tool, or reports a no-tool blocker and exits incomplete, and the only tasks that "pass" are ones whose oracle grep happens to match the stamped.zero-answer.txtnarration rather than a real edit.This invalidates every mutating-class number the benchmark has produced, in both the grok-4.5 and glm-5.2 baselines. The read classes (nav) were unaffected because they only need read tools.
How it surfaced
A live per-turn trace of fix-07 (a one-line
admin-touser-prefix change, about as trivial as an edit gets) showed the model calling onlyread_file,grep,lsp_navigate, andupdate_plan, then ending with:It identified the exact fix and had no tool to apply it. Re-running the same prompt with
--skip-permissions-unsafelanded the edit and passed the oracle. Probes also confirmed the oracles themselves are sound (a correct hand-applied fix passes fix-01, fix-07, and refactor-01), so the failure was the missing tools, not the checks.The fix
Add
--skip-permissions-unsafeto the benchmark's exec invocation. Each task already runs in an isolated, throwaway fixture copy (the runner copies the fixture into a fresh temp dir per task), so granting the full tool set has nothing to protect, and it is the whole point: the benchmark is supposed to measure real edits. This is a correctness contract for the harness, not a preference, so it is unconditional and pinned by a test.This changes only the benchmark's own
zero execinvocation. It does not touch the agent, the provider layer, or the product's permission model.Verification
Re-running four representative mutating tasks (edit-01, fix-01, fix-07, refactor-01) with the fix:
tool_executionrose from ~0.4% to ~29% of wall time, i.e. the write and shell tools are now actually being exercised rather than sitting unavailable.go build,go vet,gofmt, and theinternal/perfbench+cmd/zero-perf-benchsuites are green, including the newTestBuildTurnExecArgsGrantsWriteTools.Follow-ups (separate)
.zero-answer.txt(edit-03/06/08/09/10, the remaining half of the fix(perfbench): keep the stamped answer file out of negative oracle greps #737 class) let a narration false-pass even without real edits; worth excluding the answer file from those greps too, in a follow-up.Summary by CodeRabbit