Add agent quality eval foundation#177
Conversation
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 (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds an offline agent-eval framework (suite model, loader/validator, scoring/reporting, CLI integration, and sample fixtures), updates the autonomous agent system prompt with a verification test, and introduces a deterministic workspace-seed package with rendering and tests. ChangesAgent Eval Framework
Agent eval CLI
Agent System Prompt Refinements
Workspace Context Seed Package
Agent Evals User Documentation
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: 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.
Inline comments:
In `@internal/agenteval/suite.go`:
- Around line 122-140: Suite.normalize currently canonicalizes
ExpectedChangedFiles via normalizeFiles but does not apply the same
normalizeEvalPath cleaning used during Validate, causing paths like
"docs/a/../b.md" to mismatch; update normalizeFiles (or Suite.normalize) to call
normalizeEvalPath on each file before trimming/replacing, deduplicate, and sort
so stored ExpectedChangedFiles matches validation normalization (use the
existing normalizeEvalPath function and keep existing dedupe/sort logic).
- Around line 52-55: The decoder currently only decodes the first JSON value and
ignores trailing JSON; after the existing Decode(&suite) call, attempt a second
decode (e.g., var extra interface{}; err = decoder.Decode(&extra)) and if that
second decode returns nil (meaning there was extra data) or returns an error
other than io.EOF, return an error indicating "trailing JSON after suite"
(include original err when appropriate). Keep references to json.NewDecoder,
decoder.DisallowUnknownFields, and the suite variable so you update the code
right after the Decode(&suite) call.
In `@internal/workspaceseed/workspaceseed.go`:
- Around line 171-180: The normalizePath logic incorrectly converts absolute
paths to non-absolute when cwd is empty; update normalizePath (the block that
checks cwd and filepath.IsAbs(value)) to immediately return "" for any absolute
value if cwd is empty instead of falling through to cleaning and trimming, and
add a regression test that calls Build with Input{Paths:
[]string{"/home/alice/repo/go.mod"}} and an empty CWD to assert the resulting
seed omits that path (i.e., normalizePath returns ""), ensuring host-specific
absolute paths are rejected when they cannot be relativized.
🪄 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: 5f933abd-a056-4c43-9de7-7e254beb824f
📒 Files selected for processing (12)
docs/AGENT_EVALS.mdinternal/agent/system_prompt.mdinternal/agent/system_prompt_test.gointernal/agenteval/agenteval_test.gointernal/agenteval/score.gointernal/agenteval/suite.gointernal/agenteval/testdata/sample_suite.jsoninternal/cli/agent_eval.gointernal/cli/agent_eval_test.gointernal/cli/app.gointernal/workspaceseed/workspaceseed.gointernal/workspaceseed/workspaceseed_test.go
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review — agent quality eval foundation
Deep pass across the four areas (eval core, eval CLI, workspaceseed, prompt/docs), with each finding independently verified/reproduced before inclusion. CI is green; everything below is correctness / contract / coverage — no blockers, and the one security-flagged item was refuted down to a cosmetic determinism issue. 21 findings confirmed, 2 refuted.
The structural theme behind several findings
The zero eval CLI doesn't actually score — defaultRunAgentEval only loads + validates the suite and returns a hardcoded OK: true, Status: "ready"; it never calls agenteval.Score. That's a reasonable "foundation" scope, but the docs, help text, and part of the test suite read as if it scores. This single fact is the root of findings #2, #7–#9, and the scoring↔exit-code coverage gap. Making the validator-only scope explicit (until Score is wired in) would resolve most of the doc/help items at once.
Real bugs (latent — exported API, no production caller yet)
-
score.go:245— single-task selection skips path normalization (medium; found independently by two reviewers, reproduced).selectTask's by-ID branch normalizesExpectedChangedFiles, but the single-task fast path (taskID == "" && len(suite.Tasks) == 1) returns the task verbatim.Scorealways normalizes the actual side (score.go:86), so a hand-built single-task suite with an emptyTaskIDyields spurious mismatches (e.g. expectedinternal/a/../b.govs actualinternal/b.go→fail).LoadSuite.normalize()masks it today, butScoreis exported and documented to accept an arbitrarySuite. This is the residual half of the earlierexpectedChangedFilescanonicalization concern — the divergence lives inselectTask. Fix: normalize in the single-task branch too, and add aScoretest with an unnormalized in-memory single-task suite + emptyTaskID. -
internal/cli/agent_eval.go:22—--jsondrops zero-valued counters.Tasks/Checks/Total/Passed/Failed/Errorsall carryomitempty, so a wholly-failing run emits JSON with nopassedkey — a consumer can't distinguish "0 passed" from "not reported," and the field most wanted on failure vanishes. Note the package's ownagenteval.Summaryuses noomitemptyon the same counters. Fix: dropomitemptyfrom the six counter fields (keep it on optionalName/Status/Failures). -
score.go:175— blocked run still scores unknown command results as fail/error (low).unknownCommandResultsignoresinput.Blocked, so a blocked run with a stray command result yieldsSummary{Blocked:2, Failed:1}—report.Statusstaysblockedbut the summary is self-contradictory. -
score.go:175— duplicate unknown command IDs produce duplicate result IDs (low). Expected commands dedupe viacommandResultsByID; unknown ones don't, so two results with the same unknown ID both emitunknown_command.<id>and inflate the summary. Reachable via the exportedScoreInput. -
suite.go:151— validation accepts entries that normalize to the same path (low).["a/b.go", "a/./b.go"]both pass validation, thennormalizeFilessilently collapses them to one — inconsistent with the duplicate-id detection already done for tasks/commands. -
workspaceseed.go:166— backslash paths bypass the../absolute guards (low). The "Windows escape" framing was refuted — Go'sfilepathhandles backslashes correctly on Windows, so there's no traversal. The real impact is non-Windows output pollution / non-determinism (..\..\etcflows through as a literal layout entry). Cheap fix: convert\→/at the top ofnormalizePath(and treat a leading\\as absolute).
Docs & help mismatches (low)
docs/AGENT_EVALS.mddocuments thepass/fail/blocked/error"Score Interpretation" right after telling maintainers to runzero eval— which only printsstatus: ready. Those statuses come solely fromagenteval.Score, which has no CLI entry point. State that scoring is library-only for now.- Help verb inconsistency —
app.golists "eval Run offline agent eval suites" while the subcommand help says "Validates …". Behavior is validate. defaultRunAgentEvalhelp/description overstates what is currently a suite validator.
Test-coverage gaps
| Sev | Area | Gap |
|---|---|---|
| med | CLI | No test asserts the exact JSON key set (hides #2), nor --help, unknown-flag/stray-arg, or the default runner on an invalid suite file |
| med | workspaceseed |
The relative .. traversal guard (workspaceseed.go:192) has zero coverage |
| med | score |
Task-selection error path (typo'd task id → StatusError, report.Error, Summary.Errors) entirely untested |
| med | CLI | The real runner can't produce a failing report, so scoring↔exit-code (exitProvider) is exercised only via mocks |
| low | workspaceseed |
Truncated/MaxLayoutEntries, the dirty-summary branches, and the Windows-drive branch untested |
| low | agenteval |
DisallowUnknownFields + empty-tasks validation untested (a typo'd key → empty ExpectedChangedFiles → tasks fail for the wrong reason) |
| low | agenteval |
Command Error-field and unknown-command fail/error statuses unasserted |
| low | agenteval |
The shipped sample fixture is already clean/sorted, so it doesn't guard normalization |
| nit | agenteval |
Duplicate-task-id behavior at scoring time (first-match) is undocumented/untested |
| nit | prompt | system_prompt.md:54 "verify after edits" is redundant with the trigger clause and duplicates line 34 (which already satisfies the test) |
Status of the earlier flagged items
Re-checked against the latest commit: trailing-JSON rejection and absolute-path rejection (CWD unavailable) appear addressed; expectedChangedFiles canonicalization is addressed except the residual selectTask single-task divergence (#1).
Refuted (not real issues)
- Colon-containing relative paths rejected on Unix — intentional, defensible hardening against drive-letter ambiguity.
- CLI report can't represent
Blocked— purely hypothetical; the CLI never produces a blocked result today.
Net: a solid, deterministic foundation. Highest-value fixes are small — the selectTask normalization (#1) and --json omitempty (#2) — plus aligning the docs/help with the validator-only scope.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving — the new commits address the review (incl. the selectTask single-task normalization and the --json counter serialization), CI is green, and CodeRabbit has approved. Thanks for the thorough foundation.
2 participants
Summary
internal/workspaceseed, a pure deterministic workspace context seed builder for future prompt/session wiringinternal/agenteval, an offline eval suite/scoring contract with sample fixture data and docszero eval --suite <path> [--json]to validate and summarize offline eval suitesNotes
This PR does not run live model evals yet. It creates the suite/scoring contract and a CLI validation surface so future PRs can wire captured runs or headless executions without changing the JSON/report shape.
Validation
go test -count=1 ./internal/agenteval ./internal/cli ./internal/workspaceseed ./internal/agentgo run ./cmd/zero eval --suite internal/agenteval/testdata/sample_suite.json --jsongo test ./...git diff --checkgofmt -l internal/agent internal/agenteval internal/cli internal/workspaceseedgo run ./cmd/zero-release buildgo run ./cmd/zero-release smokeSubagent review
Used five scoped subagents for prompt, workspace seed, eval core, eval CLI, and docs/sample work, then a final reviewer pass. The final reviewer initially found two issues around overclaiming eval readiness and expectedChangedFiles validation; both were fixed and re-reviewed with no blockers.
Summary by CodeRabbit
New Features
Documentation
Tests