Expand agent quality eval module#178
Merged
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 (8)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughAdds local/offline agent-eval features: system-prompt workspace seed injection, an AgentRunner (external command runner), Materializer to copy fixtures and initialize git, Runner to execute verification commands with per-command timeouts and changed-file detection, a Benchmark Harness to run tasks across models, expanded suite/schema (context/trace/forbidden files), CLI run/bench/report wiring, many unit/integration tests, sample fixtures, and documentation. ChangesSystem prompt and CLI
Agent-eval core
Benchmark Harness
Fixture/sample repo and docs
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/agenteval/testdata/fixtures/zero-mini/internal/config/validate_test.go (1)
5-10: ⚡ Quick winAdd a case for “default provider not configured” to prevent regressions.
Line 5-10 covers only one failure path. Add a table test that also asserts an error when
DefaultProvideris set but absent fromProviders.🤖 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/testdata/fixtures/zero-mini/internal/config/validate_test.go` around lines 5 - 10, Update TestValidateRequiresDefaultProvider into a table-driven test that exercises two failure cases for Validate: (1) Providers contains entries but DefaultProvider is empty (current case) and (2) DefaultProvider is set to a name that is missing from Providers. Iterate over test cases with t.Run, construct Config instances referencing Config.DefaultProvider and Config.Providers for each case, call Validate(config) and assert err != nil for both, and include descriptive test names; keep reference to the existing Validate function and Config type.
🤖 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/testdata/fixtures/zero-mini/cmd/zero-release/main.go`:
- Around line 5-9: Replace the no-op main with a call into the release command
path so CLI invocations actually run: change func main() {} to invoke the
release entrypoint (e.g., call release.Main(smokeTarget) or the package's
Run/Execute function) passing the smokeTarget helper (which wraps
release.SmokeTarget), and propagate any error by logging/exit (os.Exit(1)) on
failure so "go run ./cmd/zero-release build" / "smoke" executes the release
flow.
In `@internal/agenteval/testdata/fixtures/zero-mini/internal/config/validate.go`:
- Around line 10-16: The Validate function currently checks that
Config.DefaultProvider is non-empty and that Config.Providers is non-empty but
doesn't verify that the named DefaultProvider exists in the Providers
collection; update Validate (function Validate, type Config, fields
DefaultProvider and Providers) to check that cfg.DefaultProvider is present as a
key/entry in cfg.Providers (after the existing empty checks) and return a
descriptive error (e.g. "default provider '%s' not found") when it is missing so
the config cannot be left in an invalid state.
In
`@internal/agenteval/testdata/fixtures/zero-mini/internal/selfverify/selfverify_test.go`:
- Around line 5-10: The test TestChecksAreLocal currently only ensures check
names are non-empty; change it to assert the exact expected list of checks
returned by Checks() so renames/removals fail the test — call Checks() in
TestChecksAreLocal and compare its result to a canonical slice of expected check
names (use reflect.DeepEqual or cmp.Equal and t.Fatalf/t.Fatalf-style failure)
so the test fails when the returned list differs in content or order from the
expected list.
In
`@internal/agenteval/testdata/fixtures/zero-mini/internal/verify/verify_test.go`:
- Around line 5-9: Update TestEventsIncludeSummary to validate the full trailing
summary event, not just its Type: retrieve the last event from Events() and
assert both last.Type == "summary" and last.Name == the expected summary name
(or compare the entire last event to an expectedEvent struct). Modify the
assertion in TestEventsIncludeSummary to fail unless both the Type and Name (or
full event equality) match the known fixture values so the test
deterministically verifies the summary payload.
---
Nitpick comments:
In
`@internal/agenteval/testdata/fixtures/zero-mini/internal/config/validate_test.go`:
- Around line 5-10: Update TestValidateRequiresDefaultProvider into a
table-driven test that exercises two failure cases for Validate: (1) Providers
contains entries but DefaultProvider is empty (current case) and (2)
DefaultProvider is set to a name that is missing from Providers. Iterate over
test cases with t.Run, construct Config instances referencing
Config.DefaultProvider and Config.Providers for each case, call Validate(config)
and assert err != nil for both, and include descriptive test names; keep
reference to the existing Validate function and Config type.
🪄 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: 6d99e957-8256-443e-bb8e-1e8bcf9dea41
📒 Files selected for processing (31)
docs/AGENT_EVALS.mdinternal/agent/system_prompt.gointernal/agent/system_prompt_test.gointernal/agenteval/agenteval_test.gointernal/agenteval/run.gointernal/agenteval/run_test.gointernal/agenteval/testdata/fixtures/zero-mini/bin/zero.jsinternal/agenteval/testdata/fixtures/zero-mini/cmd/zero-release/main.gointernal/agenteval/testdata/fixtures/zero-mini/cmd/zero-release/main_test.gointernal/agenteval/testdata/fixtures/zero-mini/docs/NPM_WRAPPER_SMOKE.mdinternal/agenteval/testdata/fixtures/zero-mini/docs/RELEASE_SMOKE.mdinternal/agenteval/testdata/fixtures/zero-mini/docs/STREAM_JSON_PROTOCOL.mdinternal/agenteval/testdata/fixtures/zero-mini/go.modinternal/agenteval/testdata/fixtures/zero-mini/internal/agent/denial_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/agent/system_prompt.mdinternal/agenteval/testdata/fixtures/zero-mini/internal/config/validate.gointernal/agenteval/testdata/fixtures/zero-mini/internal/config/validate_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/mcp/config.gointernal/agenteval/testdata/fixtures/zero-mini/internal/mcp/config_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/modelregistry/catalog.gointernal/agenteval/testdata/fixtures/zero-mini/internal/npmwrapper/wrapper.gointernal/agenteval/testdata/fixtures/zero-mini/internal/npmwrapper/wrapper_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/release/release.gointernal/agenteval/testdata/fixtures/zero-mini/internal/release/release_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/selfverify/selfverify.gointernal/agenteval/testdata/fixtures/zero-mini/internal/selfverify/selfverify_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/verify/verify.gointernal/agenteval/testdata/fixtures/zero-mini/internal/verify/verify_test.gointernal/agenteval/testdata/sample_suite.jsoninternal/cli/agent_eval.gointernal/cli/agent_eval_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agenteval/testdata/fixtures/zero-mini/cmd/zero-release/main_test.go (1)
15-33: ⚡ Quick winAdd negative-path tests for command UX contract.
Please add tests for
run([]string{}, ...)andrun([]string{"unknown"}, ...)so the required/unknown command errors stay locked down with the same rigor as the success paths.🤖 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/testdata/fixtures/zero-mini/cmd/zero-release/main_test.go` around lines 15 - 33, Add two negative-path tests mirroring the existing style: create TestRunNoArgsReturnsRequiredCommandError and TestRunUnknownCommandReturnsError that call run([]string{}, &stdout) and run([]string{"unknown"}, &stdout) respectively; in each assert that run returns a non-nil error and that the trimmed output (using bytes.Buffer and strings.TrimSpace as in TestRunSmokeUsesReleasePath/TestRunBuildUsesReleasePath) contains the expected UX phrases (for no args assert the output/error mentions a required command message, for unknown assert it mentions the unknown command name or "unknown command"). Keep the same t.Fatalf style for failures and reuse the run function, stdout buffer, and strings package for consistency.
🤖 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/testdata/fixtures/zero-mini/cmd/zero-release/main_test.go`:
- Around line 15-33: Add two negative-path tests mirroring the existing style:
create TestRunNoArgsReturnsRequiredCommandError and
TestRunUnknownCommandReturnsError that call run([]string{}, &stdout) and
run([]string{"unknown"}, &stdout) respectively; in each assert that run returns
a non-nil error and that the trimmed output (using bytes.Buffer and
strings.TrimSpace as in TestRunSmokeUsesReleasePath/TestRunBuildUsesReleasePath)
contains the expected UX phrases (for no args assert the output/error mentions a
required command message, for unknown assert it mentions the unknown command
name or "unknown command"). Keep the same t.Fatalf style for failures and reuse
the run function, stdout buffer, and strings package for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d39e80b-68b5-403a-8270-0ae1ce7c22fb
📒 Files selected for processing (6)
internal/agenteval/testdata/fixtures/zero-mini/cmd/zero-release/main.gointernal/agenteval/testdata/fixtures/zero-mini/cmd/zero-release/main_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/config/validate.gointernal/agenteval/testdata/fixtures/zero-mini/internal/config/validate_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/selfverify/selfverify_test.gointernal/agenteval/testdata/fixtures/zero-mini/internal/verify/verify_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/agenteval/testdata/fixtures/zero-mini/internal/verify/verify_test.go
- internal/agenteval/testdata/fixtures/zero-mini/internal/config/validate_test.go
gnanam1990
reviewed
Jun 12, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Review — expand agent quality eval module
Multi-agent pass over the real product code (internal/agenteval/run.go, internal/cli/agent_eval.go, system_prompt.go, docs), each finding independently verified; fixture files under testdata/fixtures/zero-mini/ were treated as intentional test data unless a defect would break the run. 20 findings confirmed, 2 refuted. CI is green; nothing here is a blocker, and the headline win is real — eval run now actually executes + scores a suite, closing the validate-only gap from #177. The notes below harden the new executor.
Most important — the executor's runtime boundary
-
No cancellation and no timeout (
cli/agent_eval.go:57,run.go:102-117) — medium. The CLI callsdeps.runAgentEval(context.Background(), …)instead ofsignalContext()like every other long-running command (exec.go:442,cron_run.go:72, …), and nothing derives aWithTimeout. The runner kills children only viaexec.CommandContextcancellation, so a hung verification command (deadlockedgo test, a network/module wait, an accidental watch/REPL in a typo'd suite) hangs the eval forever and Ctrl-C/SIGTERM can't tear it down. Fix: acquirectx, stop := signalContext()at the CLI boundary and add a sensible per-command timeout (optionally configurable onCommand/Suite). -
eval rundefaults--workspaceto.and ignorestask.WorkspaceFixture(cli/agent_eval.go:171-173,run.go:110) — medium (boundary). With--workspaceomitted, commands run in the current repo, and the runner never stages/copies the suite'sWorkspaceFixture(e.g.fixtures/zero-mini). Sozero eval run --suite …/sample_suite.jsonexecutes the suite'sgo test/gitcommands against the real working tree, not the intended fixture. Combined with (1), that's unbounded, un-interruptible command execution in your repo. Fix: stageWorkspaceFixtureinto a temp dir (or require--workspaceexplicitly; at minimum warn when running in.).
Other correctness / contract — medium
- Task-selection error reported twice (
cli/agent_eval.go:284-298). On--task does-not-exist(or omitting--taskfor a multi-task suite),Scoresets bothreport.Errorand a syntheticResult{ID:"task",…}, andagentEvalReportFromRunnerappends both — so the same error shows twice in text and in the--jsonfailures array, whilesummary.errors == 1. De-dupe. - Docs describe a report contract the CLI doesn't emit (
docs/AGENT_EVALS.md:59-117). The docs promisezero.agenteval.report.v1(suiteId/taskId/resultswith per-commandexitCode/stdout/stderr) for--jsonand--report-dir, but both emit the flatagentEvalReportstruct. Either persist the realagenteval.Report, or update the docs to the flat schema. changed_filesfailures lose diagnostics (cli/agent_eval.go:302-310).scoreChangedFilesputs the detail inMissingFiles/UnexpectedFilesand leavesMessageempty, so a mismatch renders as barechanged_files - failwith no "which files". Include missing/unexpected in the message.- Error/duplication path untested (
cli/agent_eval_test.go:176-272).agentEvalReportFromRunner's task-selection-error and de-dup behavior has no test.
Low
score.go:118-131— on partial cancellation, already-passed commands are markedblocked, discarding their real results.run.go:109— verification commands inherit the full parent environment with no allowlist (fine for a local dev tool; worth noting for CI hygiene).run.go:104-108—trimCommandsilently drops blank argument elements, mutating the intended argv.run_test.go— the realexecCommandpath (success/failure/error/empty-command, cancellation) is never exercised directly; theError-field →StatusErrormapping is untested.cli/agent_eval_test.go:189-228—--report-diris only tested with an injected runner, never end-to-end with the real one.docs/AGENT_EVALS.md:8-11— stale "future CLI work / run against copied workspaces" text now that the runner exists.
Nits
cli/agent_eval.go:227— help one-liner still hedges that the runner "may not be wired in."system_prompt.go:70-72—workspace_seedlargely duplicates the environment/repo-map content already in the prompt.
Refuted (not real issues)
- Unbounded stdout/stderr capture — output is in fact capped before it reaches the report.
- Fixture missing
catalog_test.go— intentional test data, not a defect.
Net: strong step that makes the eval real. The one thing I'd fix before relying on it: bound + interrupt command execution and run against the staged fixture rather than . (items 1–2), so an eval run can't execute unbounded commands in the working repo.
gnanam1990
requested changes
Jun 12, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes on the strength of the two runtime-boundary findings from my detailed review above — the rest are non-blocking and can be follow-ups.
Blocking:
eval runexecutes commands in the real working tree.--workspacedefaults to.and the runner ignorestask.WorkspaceFixture, sozero eval run --suite …/sample_suite.jsonruns the suite'sgo test/gitcommands against the current repo, not the staged fixture. Stage the fixture into a temp dir (or require--workspace/ warn on.).- No timeout or signal cancellation. The CLI passes
context.Background()(notsignalContext()) and there's no per-command deadline, so a hung verification command runs unbounded and can't be interrupted. Wire the shutdown context + a per-command timeout.
Strongly recommended (not strictly blocking):
3. Docs (AGENT_EVALS.md) describe zero.agenteval.report.v1 for --json/--report-dir, but the CLI emits the flat agentEvalReport — align one to the other.
4. Task-selection error is reported twice (text + --json) while summary.errors == 1.
Great step overall — this makes the eval actually run and score (closes the validate-only gap from #177). The full list (incl. lows/nits and the 2 refuted items) is in my review comment above.
…tion commands (review #178) Addresses the two runtime-boundary findings: 1. eval run no longer defaults --workspace to '.', which silently ran the suite's verification commands (go test/git) against the real working tree. It is now required; omitting it is a usage error. Docs and help text updated. 2. Wire cancellation + timeouts: runAgentEvalCommand runs under signalContext() so Ctrl+C/SIGTERM cancels in-flight commands, and the runner now applies a per-command timeout (default 10m, overridable via RunInput.CommandTimeout) so a hung verification command can't run unbounded. Tests: require-workspace usage error, cancellable-context wiring, and a per-command-deadline regression.
Collaborator
|
Pushed 1. 2. No timeout / cancellation — Tests: require- Findings #3 (docs report-contract mismatch) and #4 (double-reported task-selection error) were marked non-blocking — happy to follow up on those if you'd like, or leave them for a separate pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/agenteval/run.go (2)
71-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the deadline to changed-file collection too.
runCommandis bounded, butrunner.changedFiles(ctx, workspace)still runsgit statuson the unbounded parent context. A stuck repo operation here can leavezero eval runhung after all verification commands have already finished.Suggested fix
- files, err := runner.changedFiles(ctx, workspace) + filesCtx := ctx + if timeout > 0 { + var cancel context.CancelFunc + filesCtx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } + files, err := runner.changedFiles(filesCtx, workspace) if err != nil { reason := fmt.Sprintf("changed files collection failed: %v", err) return runner.blocked(suite, task.ID, reason, results) }🤖 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/run.go` around lines 71 - 74, The changed-files collection is called with the unbounded parent context (runner.changedFiles(ctx, workspace)) which can hang; create and use the same bounded/timeout context you use for runCommand and pass that to runner.changedFiles (e.g., derive ctxWithDeadline via context.WithTimeout or the existing bounded context variable, use defer cancel()) so git status runs under the deadline and cannot block the whole run; update the call to runner.changedFiles to use that bounded context and ensure you cancel the context when done.
173-175:⚠️ Potential issue | 🟡 MinorDon’t split on
" -> "for everygit status --porcelainentry (it corrupts valid filenames)
parseGitStatusPorcelainalways truncates after the last" -> ":file := strings.TrimSpace(line[3:]) if arrow := strings.LastIndex(file, " -> "); arrow >= 0 { file = file[arrow+4:] }So a normal modified file like
M "a -> b.txt"gets parsed asb.txt"(the suffix after the arrow, with quotes not stripped). Gate the" -> "handling to rename/copy statuses (R/C), or switch togit status --porcelain -z(and parse NUL-delimited paths) to avoid quoted-path issues.🤖 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/run.go` around lines 173 - 175, parseGitStatusPorcelain currently always truncates on the last " -> " which corrupts filenames like `"a -> b.txt"`; modify the parsing logic in parseGitStatusPorcelain so that the " -> " split is only applied for rename/copy entries (status codes 'R' or 'C') when constructing the file variable, or alternatively switch to invoking git with `--porcelain -z` and parse NUL-delimited paths to reliably handle quoted/embedded " -> " in filenames; update any code paths that use the file variable accordingly (look for parseGitStatusPorcelain and the lines creating the file := strings.TrimSpace(line[3:])).
🤖 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.
Outside diff comments:
In `@internal/agenteval/run.go`:
- Around line 71-74: The changed-files collection is called with the unbounded
parent context (runner.changedFiles(ctx, workspace)) which can hang; create and
use the same bounded/timeout context you use for runCommand and pass that to
runner.changedFiles (e.g., derive ctxWithDeadline via context.WithTimeout or the
existing bounded context variable, use defer cancel()) so git status runs under
the deadline and cannot block the whole run; update the call to
runner.changedFiles to use that bounded context and ensure you cancel the
context when done.
- Around line 173-175: parseGitStatusPorcelain currently always truncates on the
last " -> " which corrupts filenames like `"a -> b.txt"`; modify the parsing
logic in parseGitStatusPorcelain so that the " -> " split is only applied for
rename/copy entries (status codes 'R' or 'C') when constructing the file
variable, or alternatively switch to invoking git with `--porcelain -z` and
parse NUL-delimited paths to reliably handle quoted/embedded " -> " in
filenames; update any code paths that use the file variable accordingly (look
for parseGitStatusPorcelain and the lines creating the file :=
strings.TrimSpace(line[3:])).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f552700-7c0c-45a5-a01a-7574e0d74214
📒 Files selected for processing (5)
docs/AGENT_EVALS.mdinternal/agenteval/run.gointernal/agenteval/run_test.gointernal/cli/agent_eval.gointernal/cli/agent_eval_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/AGENT_EVALS.md
- internal/agenteval/run_test.go
- internal/cli/agent_eval_test.go
Collaborator
Author
|
Blocking items from the previous review are addressed in
Local validation on
@coderabbitai review |
|
Triggering a fresh review now to cover ✅ Action performedReview finished.
|
Collaborator
Author
|
@gnanam1990 the two blocking items from your request-change review are addressed in
Please rereview when you get a chance. |
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 ✅
Both blocking items from my earlier review are resolved (verified against HEAD):
- eval run no longer touches the real working tree.
eval runnow requires an explicit--workspace(internal/cli/agent_eval.go:175-179) and errors with "--workspace requires a path for eval run" instead of defaulting to.. - Timeout + cancellation wired. The CLI uses
signalContext()(agent_eval.go:59) rather thancontext.Background(), and each verification command is bounded bycontext.WithTimeoutwith a 10-min default (internal/agenteval/run.go:18,96), overridable per run.
go build ./... and go test ./internal/agenteval/ ./internal/cli/ are green locally. Thanks for the quick turnaround.
* Add agent eval benchmark harness * Upgrade agent eval quality signals * Address agent eval benchmark review feedback 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. * Make timeout-cancellation test strict 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). --------- Co-authored-by: gnanam1990 <gnanasekaran.sekareee@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/AGENT_EVALS.md (1)
243-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport JSON contract docs do not match the CLI artifact actually emitted.
The section currently documents
internal/agenteval.Report(suiteId,taskId,results, etc.), butzero eval run|bench --jsonand--report-diremit the CLI report shape frominternal/cli/agent_eval.go(suite,task_id,total/passed/failed/blocked/errors,failures, optionalbenchmark). This contract mismatch is integration-breaking for consumers following the docs.🤖 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/AGENT_EVALS.md` around lines 243 - 258, Docs describe the report shape for internal/agenteval.Report (fields like suiteId, taskId, results) but the CLI actually emits the artifact produced by internal/cli/agent_eval.go (fields like suite, task_id, total/passed/failed/blocked/errors, failures, optional benchmark) via "zero eval run|bench --json" and --report-dir; update AGENT_EVALS.md to match the CLI contract by replacing the documented field names and structure with the exact keys and semantics emitted by internal/cli/agent_eval.go (include examples of the top-level keys suite, task_id, total/passed/failed/blocked/errors, failures, and benchmark) or alternatively change internal/cli/agent_eval.go to marshal internal/agenteval.Report (pick one approach consistently and update tests/docs accordingly).
🧹 Nitpick comments (4)
internal/cli/agent_eval_test.go (1)
302-336: ⚡ Quick winAdd a regression assertion for duplicate benchmark failures.
Given the benchmark failure aggregation path, add a test that asserts a task-selection/report-level error is surfaced once (not duplicated across result-level and task-level error channels). This will prevent recurrence of duplicated failure lines in CLI output/JSON.
🤖 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/cli/agent_eval_test.go` around lines 302 - 336, The test TestRunEvalBenchDefaultHarnessRunsModelMatrix should be updated to assert that aggregated benchmark/report-level errors are not duplicated across result- and task-level failure channels: after unmarshalling into decoded (agentEvalReport), add a regression assertion that the set of failure IDs (decoded.Failures[*].ID) contains no duplicates (e.g., compare len(decoded.Failures) to the size of a unique-ID set) and specifically assert the task-selection/report-level error ID appears exactly once; this prevents duplicated failure lines in CLI JSON output while keeping the existing checks on decoded.Total, decoded.Benchmark and model order.internal/agenteval/suite.go (1)
189-191: ⚡ Quick winRemove the unused
validateExpectedChangedFileswrapper to prevent validation drift.This helper is no longer called after
validateFileListbecame the shared path, so keeping it adds dead code and future inconsistency risk.🤖 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, Delete the dead wrapper function validateExpectedChangedFiles and its definition since it is no longer used; callers should already use validateFileList directly (no other changes required), and remove any imports or tests that referenced validateExpectedChangedFiles if they exist to avoid compilation errors.Source: Linters/SAST tools
internal/agenteval/context_checks_test.go (1)
31-39: ⚡ Quick winAdd a test for malformed
ContextChecksinputs returningValidationError.
CheckWorkspacehas a dedicated normalization/validation error path for invalid or empty relative paths, but this file currently only tests existence checks. Covering that branch will protect the exported contract.🤖 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/context_checks_test.go` around lines 31 - 39, Add a new unit test that exercises the validation branch of ContextChecks.CheckWorkspace by providing malformed/invalid relative paths (e.g., an empty string or a path with leading slash) in ContextChecks.RequiredFiles, then call CheckWorkspace and assert the returned error is a ValidationError (use the ValidationError type/symbol from the package); name the test clearly (for example TestEvaluateContextChecksReturnsValidationErrorForMalformedInputs) and follow the existing test pattern (use t.TempDir() and check for non-nil error and that it is a ValidationError).internal/agenteval/agenteval_test.go (1)
360-382: ⚡ Quick winAdd
Scoretests forcontext_checksandtrace_eventsresult branches.This file now validates forbidden-file scoring, but the new context/trace scoring paths are still unasserted here. Add pass/fail (and at least one error/blocked) cases to lock report semantics.
🤖 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/agenteval_test.go` around lines 360 - 382, Update the test suite by adding new test cases alongside TestScoreFailsWhenForbiddenFilesChange that exercise the Score function's context_checks and trace_events branches: create one passing and one failing scenario for context_checks and one passing, one failing, and at least one error/blocked scenario for trace_events; for each case call Score with appropriate ScoreInput (set TaskID to the task that contains context/trace rules, populate CommandResults/ChangedFiles/ContextChecks/TraceEvents as needed), then assert report.OK and report.Status (StatusPass/StatusFail/StatusError or StatusBlocked as applicable) and locate the specific result using findResultByID to assert the result ID ("context_checks" or "trace_events") and its fields (e.g., UnexpectedFiles, Error/Blocked markers) match expected outcomes; keep these tests focused and use the existing sampleSuite() to obtain tasks and rules.
🤖 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/agent_command_test.go`:
- Around line 212-213: The two unchecked writes to os.Stdout.WriteString and
os.Stderr.WriteString are causing errcheck failures; update the helper process
to capture and handle the returned values (e.g., _, err :=
os.Stdout.WriteString("agent stdout\n") and _, err :=
os.Stderr.WriteString("agent stderr\n")) and handle non-nil errors (propagate or
fail the test/helper process via panic or t.Fatalf as appropriate). Ensure both
WriteString calls in the helper process are updated to check and handle their
errors so lint no longer flags them.
In `@internal/agenteval/agent_command.go`:
- Around line 49-65: The Run method currently allows an empty
input.WorkspacePath which causes the child process to run in the caller's CWD;
update CommandAgentRunner.Run to validate input.WorkspacePath (e.g., ensure
strings.TrimSpace(input.WorkspacePath) != "") before creating the
exec.CommandContext and setting cmd.Dir, and if it's empty set result.Error (for
example "workspace path is required") and return the result without spawning the
process; make this change in the Run function (referencing Run,
CommandAgentRunner, and input.WorkspacePath).
In `@internal/agenteval/benchmark.go`:
- Around line 63-81: The error-reporting path currently builds a Report with a
default successful agent exit code; update the constructed
BenchmarkTaskReport/Report (the block that appends to report.Tasks) to set a
sentinel Agent exit code and an explicit agent result when the agent never runs:
set Report.Agent.ExitCode = -1 (or another agreed sentinel) and add a Result
entry (e.g., ID: "agent", Name: "Agent run", Kind: ResultProcess/appropriate
kind, Status: StatusError, Message: "Agent did not run") so the report
unambiguously indicates the agent never executed; apply the same change to the
other identical block mentioned (lines 101-117) to keep behavior consistent.
In `@internal/agenteval/materialize.go`:
- Around line 64-87: The path containment check in resolveFixturePath is
bypassable via symlink escapes; update resolveFixturePath to compare
symlink-canonicalized paths: run filepath.EvalSymlinks on the suite base
(computed from filepath.Abs(filepath.Dir(suitePath))) and on the candidate
fixture path before computing filepath.Rel, and reject if the symlink-resolved
relative path escapes the base. If EvalSymlinks on the candidate fails because
the target doesn't exist, fall back to EvalSymlinks on the nearest existing
ancestor (e.g., loop up parents until EvalSymlinks succeeds) and append the
remaining path segments before computing Rel. Also ensure error cases from
EvalSymlinks are returned, and note that copyFixtureDir already rejects symlink
entries so only the root candidate needs this canonicalization.
In `@internal/cli/agent_eval.go`:
- Around line 547-565: The function agentEvalFailuresFromTaskReport currently
appends a task-level error after iterating task.Report.Results which can
duplicate a failure already represented in results; change the logic so before
appending the task-level agentEvalFailure (using ID taskID and Message
task.Report.Error) you check task.Report.Results for a matching entry (compare
the constructed id (taskID or taskID+"."+result.ID) and the message from
agentEvalResultMessage(result)), and only append the task-level failure if no
result matches both id and message; this avoids emitting duplicate failures
while keeping the original result-collection logic intact.
---
Outside diff comments:
In `@docs/AGENT_EVALS.md`:
- Around line 243-258: Docs describe the report shape for
internal/agenteval.Report (fields like suiteId, taskId, results) but the CLI
actually emits the artifact produced by internal/cli/agent_eval.go (fields like
suite, task_id, total/passed/failed/blocked/errors, failures, optional
benchmark) via "zero eval run|bench --json" and --report-dir; update
AGENT_EVALS.md to match the CLI contract by replacing the documented field names
and structure with the exact keys and semantics emitted by
internal/cli/agent_eval.go (include examples of the top-level keys suite,
task_id, total/passed/failed/blocked/errors, failures, and benchmark) or
alternatively change internal/cli/agent_eval.go to marshal
internal/agenteval.Report (pick one approach consistently and update tests/docs
accordingly).
---
Nitpick comments:
In `@internal/agenteval/agenteval_test.go`:
- Around line 360-382: Update the test suite by adding new test cases alongside
TestScoreFailsWhenForbiddenFilesChange that exercise the Score function's
context_checks and trace_events branches: create one passing and one failing
scenario for context_checks and one passing, one failing, and at least one
error/blocked scenario for trace_events; for each case call Score with
appropriate ScoreInput (set TaskID to the task that contains context/trace
rules, populate CommandResults/ChangedFiles/ContextChecks/TraceEvents as
needed), then assert report.OK and report.Status
(StatusPass/StatusFail/StatusError or StatusBlocked as applicable) and locate
the specific result using findResultByID to assert the result ID
("context_checks" or "trace_events") and its fields (e.g., UnexpectedFiles,
Error/Blocked markers) match expected outcomes; keep these tests focused and use
the existing sampleSuite() to obtain tasks and rules.
In `@internal/agenteval/context_checks_test.go`:
- Around line 31-39: Add a new unit test that exercises the validation branch of
ContextChecks.CheckWorkspace by providing malformed/invalid relative paths
(e.g., an empty string or a path with leading slash) in
ContextChecks.RequiredFiles, then call CheckWorkspace and assert the returned
error is a ValidationError (use the ValidationError type/symbol from the
package); name the test clearly (for example
TestEvaluateContextChecksReturnsValidationErrorForMalformedInputs) and follow
the existing test pattern (use t.TempDir() and check for non-nil error and that
it is a ValidationError).
In `@internal/agenteval/suite.go`:
- Around line 189-191: Delete the dead wrapper function
validateExpectedChangedFiles and its definition since it is no longer used;
callers should already use validateFileList directly (no other changes
required), and remove any imports or tests that referenced
validateExpectedChangedFiles if they exist to avoid compilation errors.
In `@internal/cli/agent_eval_test.go`:
- Around line 302-336: The test TestRunEvalBenchDefaultHarnessRunsModelMatrix
should be updated to assert that aggregated benchmark/report-level errors are
not duplicated across result- and task-level failure channels: after
unmarshalling into decoded (agentEvalReport), add a regression assertion that
the set of failure IDs (decoded.Failures[*].ID) contains no duplicates (e.g.,
compare len(decoded.Failures) to the size of a unique-ID set) and specifically
assert the task-selection/report-level error ID appears exactly once; this
prevents duplicated failure lines in CLI JSON output while keeping the existing
checks on decoded.Total, decoded.Benchmark and model order.
🪄 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: 3599a714-de2e-4623-86a0-06d7c44ceb0b
📒 Files selected for processing (20)
docs/AGENT_EVALS.mddocs/superpowers/plans/2026-06-12-agent-eval-benchmark-harness.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/materialize.gointernal/agenteval/materialize_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 (2)
- internal/agenteval/testdata/sample_suite.json
- internal/agenteval/run.go
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 runwith task/workspace/report-dir/JSON outputValidation
gofmt -l internal/agent internal/agenteval internal/cligit diff --check HEAD~1..HEADgo test -count=1 ./internal/agenteval ./internal/cli ./internal/agent ./internal/workspaceseedgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokego run ./cmd/zero eval --suite internal/agenteval/testdata/sample_suite.jsonSummary by CodeRabbit
New Features
Quality & Scoring
Documentation
Tests