Add agent eval benchmark harness#180
Merged
gnanam1990 merged 5 commits intoJun 13, 2026
Merged
Conversation
Contributor
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a local “bench” harness: materializes fixture workspaces with a deterministic Git baseline, runs an agent command (argv placeholders, output truncation), extends suite/task schema and scoring with context/trace/forbidden-file checks, adds Harness orchestration for model-matrix runs, wires CLI ChangesOffline Agent Evaluation Benchmark Harness
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (2)
docs/superpowers/plans/2026-06-12-agent-eval-benchmark-harness.md (1)
147-147: ⚡ Quick winClarify the
agentRunnerFunctype adapter in the implementation spec.The test in Task 3 (line 147) uses
agentRunnerFunc(func(...) AgentRunResult {...})as a type adapter to satisfy theAgentRunnerinterface, but this is not mentioned in the Task 2 implementation specification. Add it to Task 2's// Add:bullet point, or document that the test helper should be defined inbenchmark_test.gorather thanagent_command.go.Suggestion: Add the helper type to Task 2 spec (or benchmark_test.go)
In Task 2's "Implement runner" section, add:
// Optional: type adapter for testing type agentRunnerFunc func(context.Context, AgentRunInput) AgentRunResult func (f agentRunnerFunc) Run(ctx context.Context, input AgentRunInput) AgentRunResult { return f(ctx, input) }Or clarify in Task 3 that the test defines its own helper.
🤖 Prompt for 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. In `@docs/superpowers/plans/2026-06-12-agent-eval-benchmark-harness.md` at line 147, Add documentation for the test helper type adapter so readers know how the test satisfies the AgentRunner interface: update Task 2's "Add:" bullet to include an optional type adapter declaration named agentRunnerFunc that implements AgentRunner by defining a Run(ctx context.Context, input AgentRunInput) AgentRunResult method (or state that the helper may instead be defined locally in benchmark_test.go). Mention the exact symbol names agentRunnerFunc, AgentRunInput, AgentRunResult, and AgentRunner so implementers can locate where to add the adapter or where the test helper should live.internal/agenteval/materialize_test.go (1)
13-42: 💤 Low valueUse
exec.CommandContextfor consistency with project conventions.The linter flagged
exec.Commandat line 39. While this is test code and the command is quick, usingCommandContextwith a timeout would make the test more robust against git hangs.Proposed fix
- if output, err := exec.Command("git", "-C", workspace.Path, "status", "--porcelain").CombinedOutput(); err != nil || strings.TrimSpace(string(output)) != "" { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if output, err := exec.CommandContext(ctx, "git", "-C", workspace.Path, "status", "--porcelain").CombinedOutput(); err != nil || strings.TrimSpace(string(output)) != "" {You'll also need to add
"time"to the imports.🤖 Prompt for 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. In `@internal/agenteval/materialize_test.go` around lines 13 - 42, Replace the exec.Command call in TestMaterializeTaskCopiesFixtureAndInitializesGit with exec.CommandContext using a context with a short timeout (e.g., context.WithTimeout) so the git status invocation can't hang the test; update the test to cancel the context after use and add the "time" import (and "context" import if not present) as needed. Locate the exec invocation that runs git -C workspace.Path status --porcelain and wrap it with a context timeout, then use the CombinedOutput from the returned *Cmd as before and handle the context cancellation.Source: Linters/SAST tools
🤖 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/agenteval/benchmark.go`:
- Around line 108-110: The deferred call defer os.RemoveAll(workspace.Path)
discards its returned error and fails errcheck; replace it with an explicit
deferred function that calls os.RemoveAll(workspace.Path), captures its error,
and handles it (e.g., log.Printf or wrap into an existing logger) so the return
value is not ignored. Specifically, where input.KeepWorkspaces and
workspace.Path are used, change the defer to something like: defer func() { if
!input.KeepWorkspaces { if err := os.RemoveAll(workspace.Path); err != nil { /*
log the error via existing logger */ } } } so the cleanup error is handled
rather than dropped.
In `@internal/agenteval/materialize.go`:
- Around line 118-134: The function copyFixtureFile currently defers
target.Close() which hides any error returned by Close (e.g., flush failures);
change the logic in copyFixtureFile to not defer target.Close(), instead perform
the io.Copy into a variable (e.g., n, copyErr := io.Copy(target, source)), then
call closeErr := target.Close(); return copyErr if non-nil else return closeErr;
keep the deferred source.Close() as-is and ensure OpenFile uses the same mode
and flags (function name: copyFixtureFile, variables: target, source, io.Copy).
---
Nitpick comments:
In `@docs/superpowers/plans/2026-06-12-agent-eval-benchmark-harness.md`:
- Line 147: Add documentation for the test helper type adapter so readers know
how the test satisfies the AgentRunner interface: update Task 2's "Add:" bullet
to include an optional type adapter declaration named agentRunnerFunc that
implements AgentRunner by defining a Run(ctx context.Context, input
AgentRunInput) AgentRunResult method (or state that the helper may instead be
defined locally in benchmark_test.go). Mention the exact symbol names
agentRunnerFunc, AgentRunInput, AgentRunResult, and AgentRunner so implementers
can locate where to add the adapter or where the test helper should live.
In `@internal/agenteval/materialize_test.go`:
- Around line 13-42: Replace the exec.Command call in
TestMaterializeTaskCopiesFixtureAndInitializesGit with exec.CommandContext using
a context with a short timeout (e.g., context.WithTimeout) so the git status
invocation can't hang the test; update the test to cancel the context after use
and add the "time" import (and "context" import if not present) as needed.
Locate the exec invocation that runs git -C workspace.Path status --porcelain
and wrap it with a context timeout, then use the CombinedOutput from the
returned *Cmd as before and handle the context cancellation.
🪄 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 Plus
Run ID: 04fa5108-dc33-49a5-ae24-bd25bf631050
📒 Files selected for processing (10)
docs/AGENT_EVALS.mddocs/superpowers/plans/2026-06-12-agent-eval-benchmark-harness.mdinternal/agenteval/agent_command.gointernal/agenteval/agent_command_test.gointernal/agenteval/benchmark.gointernal/agenteval/benchmark_test.gointernal/agenteval/materialize.gointernal/agenteval/materialize_test.gointernal/cli/agent_eval.gointernal/cli/agent_eval_test.go
gnanam1990
requested changes
Jun 12, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Review — agent eval benchmark harness (stacked on #178)
Multi-agent pass over the new code (materialize.go, agent_command.go, benchmark.go, the bench CLI), each finding independently verified and diffed against this PR's real base (feat/agent-eval-module-upgrade), so base/#178 issues are excluded. 23 confirmed, 2 refuted — no critical/high. Nice design: materialize-into-temp → run agent → score is the right shape, and MaterializeTask correctly copies the fixture into a temp dir (addressing the run-in-. concern from my #178 review at this layer). Requesting changes for the mediums below; the rest are non-blocking.
Blocking (medium)
- The benchmarked agent command has no timeout / cancellation.
CommandAgentRunner.Run→exec.CommandContext(ctx, …)(agent_command.go:48), but no deadline is ever set:cli/agent_eval.go:60→Harness.Run(benchmark.go:46-49, nil→Background) →runTask→Agent.Run, with noWithTimeoutand no--timeoutflag. Since bench mode exists precisely to run an external agent that may make live model calls, a wedged/interactive agent makeszero eval benchblock forever. Add a per-task/per-agent timeout (e.g.BenchmarkInput.Timeout+--timeout) and thread a cancelable context. (The CLI'scontext.Background()itself is inherited from #178 — fix that separately; the new unbounded agent-process path is #180's.) - Unbounded agent stdout/stderr capture (
agent_command.go:50-57). Output is buffered into plainbytes.Bufferwith no cap and stored inAgentRunResult/the JSON report. A chatty or runaway agent can exhaust memory and bloat the report. Cap captured output (and note in the report when truncated). - Bench text output hides the scores (
cli/agent_eval.go:233-237, 357-362).agentEvalReportFromBenchmarkpopulatesTotal/Passed/Failed/Blocked/Errors, butformatAgentEvalReportbranches onTasks>0 || Checks>0and prints the validate-stylesummary: N tasks, 0 checks— so a benchmark run never shows pass/fail counts in text. The whole point of the harness is the score; surface it. - Temp workspace leaked on error (
materialize.go:49-58).MaterializeTaskdoesos.MkdirTempthen returns early oncopyFixtureDir/initGitBaselinefailure withoutRemoveAll; the caller onlydefers cleanup after a successful return, so every materialization error leaks a temp dir. Clean up on the error paths insideMaterializeTask. - Benchmark error paths untested (
benchmark_test.go). No coverage for bad task id, materialization failure, or theErrorTaskssummary path.
Non-blocking (low)
materialize.go:107-113— symlinks (and other non-regular entries) in a fixture are silently dropped (no copy, no rejection, no test), so a fixture relying on them materializes incompletely. Decide: reject or copy, and test it.agent_command.go:48— the agent inherits the harness's full environment (including secrets) with no scoping.materialize_test.go/agent_command_test.go— missing tests for symlink fixtures, error-path cleanup, and an actually-hanging agent (cancellation is only tested via a pre-canceled context).cli/agent_eval.go:174-182—--agent-command=<value>(inline=form) collapses to a single-token argv, so a multi-word command can't be expressed that way.cli/agent_eval.go:345-348—--keep-workspaceswithout--work-rootkeeps dirs under an unnamed temp location that's never printed, so they can't be found.cli/agent_eval.go:301-308— aMkdirTemp/work-root IO failure is reported as a usage error (exit 2) rather than a runtime error.docs/AGENT_EVALS.md:89-111— bench docs omit that an agent which commits its changes defeats changed-files scoring.
Nits
agent_command.go:75-84— placeholder expansion is order-dependent and can re-expand injected placeholders.benchmark.go:109— workspace cleanup error is silently ignored.cli/agent_eval.go:165-173—--agent-command(space form) swallows all later args, including--suite/--json/--report-dir.materialize_test.go:88-93— copied-file mode (executable bit) preservation is untested.
Refuted (not real issues)
resolveFixturePathsymlink escape — not an actual escape at HEAD.- Bench runs under
context.Background()— that line is inherited from #178, not introduced here (out of scope for this PR).
Net: solid harness; the materialize-to-temp approach is the right call. Blocking only on bounding/capping the agent process (1–2), surfacing the benchmark scores in text (3), and not leaking temp dirs on error (4). Since this is stacked on #178 (currently CHANGES_REQUESTED), it can't merge until that lands anyway.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agenteval/suite.go (1)
189-191: ⚡ Quick winRemove unused
validateExpectedChangedFilesfunction.This wrapper is no longer called after refactoring line 110 to use
validateFileListdirectly. Static analysis correctly flags it as unused.♻️ Proposed fix
-func validateExpectedChangedFiles(taskPath string, files []string) []string { - return validateFileList(taskPath, "expectedChangedFiles", files, true) -} -🤖 Prompt for 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. In `@internal/agenteval/suite.go` around lines 189 - 191, Remove the now-unused wrapper function validateExpectedChangedFiles; locate the function definition validateExpectedChangedFiles (which simply calls validateFileList) and delete it so only validateFileList remains in use, verifying no other references to validateExpectedChangedFiles exist and running tests/build to ensure no regressions.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@internal/agenteval/suite.go`:
- Around line 189-191: Remove the now-unused wrapper function
validateExpectedChangedFiles; locate the function definition
validateExpectedChangedFiles (which simply calls validateFileList) and delete it
so only validateFileList remains in use, verifying no other references to
validateExpectedChangedFiles exist and running tests/build to ensure no
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9c0f6f0-91ee-43e7-8d4e-91537828976e
📒 Files selected for processing (17)
docs/AGENT_EVALS.mddocs/superpowers/plans/2026-06-12-agent-eval-d-level-upgrades.mdinternal/agenteval/agent_command.gointernal/agenteval/agent_command_test.gointernal/agenteval/agenteval_test.gointernal/agenteval/benchmark.gointernal/agenteval/benchmark_test.gointernal/agenteval/context_checks.gointernal/agenteval/context_checks_test.gointernal/agenteval/run.gointernal/agenteval/score.gointernal/agenteval/suite.gointernal/agenteval/testdata/sample_suite.jsoninternal/agenteval/trace.gointernal/agenteval/trace_test.gointernal/cli/agent_eval.gointernal/cli/agent_eval_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/AGENT_EVALS.md
- internal/agenteval/agent_command.go
- internal/agenteval/agent_command_test.go
- internal/agenteval/benchmark.go
Blocking review items: - Bound each bench task with a --timeout flag + BenchmarkInput.Timeout, threading a per-task context; report a clear timeout error instead of "exited with code -1". - Cap captured agent stdout/stderr per stream (default 1 MiB, configurable) and flag truncation via AgentRunResult.Truncated, surfaced in the CLI report (text note + JSON). - Show scored pass/fail/blocked/error tallies for bench/run text output (formatAgentEvalReport now keys the validate-style summary on Checks). - Clean up materialized temp workspaces on copy/git-baseline failure instead of leaking them. - Cover unknown-task-id, materialization-failure, per-task-timeout, and timeout-cancellation paths with tests. Lows/nits: - Reject symlink/non-regular fixture entries instead of silently dropping them. - Check copyFixtureFile's target.Close()/source.Close(); fix cleanup-defer errcheck in benchmark.go and cli. - Surface the work root when --keep-workspaces is set so kept dirs are findable. - Route work-root creation failures to the crash exit code, not the usage code. - Avoid re-expanding injected placeholder values via strings.NewReplacer. - Document that committing agent changes defeats changed-files scoring; preserve executable bits; add work-root and truncation tests.
Collaborator
|
Pushed Blocking (medium)
Lows/nits
Validation: |
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/agenteval/benchmark_test.go`:
- Around line 425-465: TestHarnessTimeoutCancelsBlockedAgent currently gates the
blocked-status checks behind "if agentReached { ... }", letting pre-agent
timeouts hide regressions; change it so the test fails if the agent never
started (add a t.Fatal when agentReached is false immediately after the call to
harness.Run) and move the assertion that report.Tasks[0].Report.Status equals
StatusBlocked outside the conditional so it always runs; keep the existing
checks for sawCancel and report.OK but ensure the blocked-status check and the
hard failure on !agentReached are unconditional after obtaining report from
harness.Run.
🪄 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 Plus
Run ID: d7fbd250-b878-4cf6-bc84-1695863e357f
📒 Files selected for processing (9)
docs/AGENT_EVALS.mdinternal/agenteval/agent_command.gointernal/agenteval/agent_command_test.gointernal/agenteval/benchmark.gointernal/agenteval/benchmark_test.gointernal/agenteval/materialize.gointernal/agenteval/materialize_test.gointernal/cli/agent_eval.gointernal/cli/agent_eval_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/agenteval/benchmark.go
- internal/agenteval/materialize.go
- internal/cli/agent_eval.go
Use a generous (2s) per-task timeout so the agent is always reached, then hard-assert that it observed cancellation and the task is blocked, instead of gating those assertions behind agentReached (which let a pre-agent timeout regression pass silently).
Collaborator
|
Follow-up
|
Collaborator
Author
|
Updated this stacked PR in Current state keeps:
Local validation on
@coderabbitai review |
|
✅ Action performedReview finished.
|
Collaborator
Author
|
@gnanam1990 this stacked PR now includes latest #178 via @coderabbitai approve |
|
✅ Action performedComments resolved and changes approved. |
gnanam1990
approved these changes
Jun 13, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — approved ✅
All five blocking items from my earlier review are resolved (verified against HEAD):
- Per-task timeout + cancellation.
BenchmarkInput.Timeoutthreads throughrunTaskviacontext.WithTimeout(benchmark.go:16-18,96-98), exposed as--timeout(agent_eval.go:227-253). - Captured output is capped.
capWriterbounds stdout/stderr to 1 MiB/stream with aTruncatedflag in the report (agent_command.go:38-73). - Bench text output surfaces the scores.
formatAgentEvalReportnow printstotal/passed/failed/blocked/errorsfor run/bench modes (agent_eval.go:357-363). - No temp-dir leak on error.
MaterializeTaskcallsos.RemoveAllon both the copy and git-baseline error paths (materialize.go:54-59). - Error paths tested.
benchmark_test.gocovers unknown task id, materialization failure, agent-run failure, and per-task timeout cancellation.
Nice: the placeholder nit is also handled (single-pass strings.NewReplacer). go build ./... + full agenteval/cli tests green locally.
Note: this is stacked on #178, so it should merge after #178 lands.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
zero eval benchto materialize fixture workspaces, run an agent command, then score results{workspace},{prompt}, and{task_id}placeholder expansionStacking
Stacked on #178 (
feat/agent-eval-module-upgrade). After #178 merges, this can be retargeted tomain.Validation
gofmt -l internal/agenteval internal/cligit diff --checkgo vet ./...go test -count=1 ./internal/agenteval ./internal/cligo test ./...frominternal/agenteval/testdata/fixtures/zero-minigo run ./cmd/zero eval --suite internal/agenteval/testdata/sample_suite.jsongo run ./cmd/zero eval bench ... --agent-command ...with deterministic local fake-agent commandgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokeSummary by CodeRabbit
New Features
Evaluation Enhancements
Documentation
Tests