Add per-turn tracing and a turn-benchmark harness (Phase 0 baseline) - #700
Conversation
Introduce an opt-in `internal/trace` package that attributes a run's wall time to named spans — prompt build, provider connect/queue, generation, tool queue/execution, permission wait, verification, compaction, persistence — plus counters for model requests, tool calls, retries, reconnects, compactions, completion nudges, acceptance checks, model switches, and input/cached/output tokens. The recorder is nil-safe (a nil *Recorder is a no-op), so every stamp site can call it unguarded; an untraced run is byte-identical to today. The recorder threads into agent.Run via a new `Options.Trace` field and a single `trace.WithContext` assignment, so the shared providerio seam (connect around client.Do, queue around OAuth resolve, first token at the first non-keepalive SSE payload) reaches it through `trace.FromContext` without any provider-interface change. All instrumentation is opt-in: when `Options.Trace` is nil the hot path is untouched. Headless `zero exec` gains `--trace <path>` (and `ZERO_TRACE=<path>`), writing an agenteval-contract-compatible NDJSON trace (`-` for stderr). The trace is pure observation; enabling it does not change behavior. A new per-turn benchmark harness extends `internal/perfbench` (reusing the existing TaskSet/NumericStats/subprocess runner shapes): `RunTurnBench` aggregates per-span median/P95 across iterations, ranks the top three controllable latency sources by share of attributed time, rolls up per task class, and totals tokens/counts. A checked-in 42-task manifest across seven classes (nav, edit, fix, refactor, longproc, longctx, parallel) and local offline fixtures back it, and `make baseline` runs it against the built `zero`, writing `internal/perfbench/reports/baseline.json`. The report is machine-specific and regenerated, not hand-edited. The manifest is JSON rather than YAML to avoid adding a dependency; per-task verification targets the specific test (`go test -run <name>`) so fix tasks are independent. Each task is a fresh `zero exec` process, so iterations are cold-start samples; a warm in-process path is left for follow-up. Verified: `go build ./...`, `go vet ./...`, `gofmt -l` clean on touched files; `go test -race` green for internal/trace, internal/agent, internal/perfbench, and cmd/zero-perf-bench.
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 Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds opt-in per-turn tracing across agent and provider execution, trace serialization/parsing, a ChangesTrace foundation and runtime instrumentation
Per-turn benchmark workflow
Estimated code review effort: 4 (Complex) | ~70 minutes 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: 18
🧹 Nitpick comments (3)
internal/perfbench/turn_bench_test.go (1)
283-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate that every workspace fixture exists and is a directory.
NewTurnExecRunneruses this path ascmd.Dir; the current substring check lets a typo pass CI and fail only during a model-backed baseline run.Proposed test coverage
import ( "bytes" "context" "encoding/json" + "os" "path/filepath" "strings" @@ if !strings.Contains(task.WorkspaceFixture, "testdata") { t.Fatalf("task %q fixture %q not under testdata", task.ID, task.WorkspaceFixture) } + info, err := os.Stat(task.WorkspaceFixture) + if err != nil { + t.Fatalf("task %q fixture %q: %v", task.ID, task.WorkspaceFixture, err) + } + if !info.IsDir() { + t.Fatalf("task %q fixture %q is not a directory", task.ID, task.WorkspaceFixture) + } }🤖 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/perfbench/turn_bench_test.go` around lines 283 - 292, Extend the task validation loop in the benchmark test to stat the path in task.WorkspaceFixture and fail when it does not exist or is not a directory, while retaining the existing testdata containment check. Use the filesystem error and task ID/path in the failure message so invalid cmd.Dir values are reported during validation.internal/agent/loop.go (1)
118-136: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd focused coverage for the tracing wrapper.
This wrapper feeds all token counters and must preserve the caller’s
OnUsagecallback. Add tests for nil versus non-nilTrace, callback forwarding, token aggregation across multiple requests, and an error/retry path.🤖 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/agent/loop.go` around lines 118 - 136, Add focused tests around the tracing setup in the loop entry point, covering nil versus non-nil Trace behavior, preservation and invocation of the caller’s OnUsage callback, aggregation of input/cached-input/output token counters across multiple requests, and counter behavior through an error/retry path.internal/agent/types.go (1)
283-289: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine recorder reuse semantics before exposing
Tracepublicly.
Runstarts but does not reset or finish the caller-owned recorder, while spans and counters accumulate. Reusing one*trace.Recorderacross runs will merge both runs and retain the first run’s timestamps. Either require a fresh recorder perRunin this contract or add an explicit reset/new-run boundary.🤖 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/agent/types.go` around lines 283 - 289, Clarify the public `Trace` contract near its field declaration to require a fresh `*trace.Recorder` for each `Run`, since `Run` neither resets nor finishes the caller-owned recorder. State that recorder reuse across runs is unsupported and prevents spans, counters, and timestamps from being merged.
🤖 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/agent/compaction.go`:
- Around line 422-424: Update the CounterCompactionCount increment in Compact so
it occurs only after the no-op checks and a history reduction has actually
completed successfully. Preserve the existing trace context and err guard, and
leave the counter unchanged for paths that return the input unchanged.
In `@internal/cli/exec.go`:
- Around line 493-497: Update the trace lifecycle around the agent.Run boundary
in runExec: finish or snapshot traceRecorder immediately after agent.Run
returns, before final output, notifications, warnings, and cleanup execute. Keep
the existing deferred emitTrace serialization afterward, using the completed
snapshot so wall_ms excludes post-run latency.
- Around line 484-498: Update the early options.useSpec return path to honor
tracing configuration: either initialize and emit a trace recorder there using
the same tracePath/emitTrace behavior, or explicitly reject --trace and
ZERO_TRACE for spec-draft runs with a clear error. Ensure tracing is never
silently ignored while preserving the existing normal-run setup.
In `@internal/perfbench/manifests/baseline.json`:
- Around line 4-58: The tasks array in the baseline manifest defines 48 entries
while the advertised baseline contract requires 42. Remove six task entries,
preserving the intended category coverage and existing task structure, or
consistently update the baseline contract and documentation to 48; ensure all
benchmark aggregation and reported metrics use the resulting count.
- Around line 5-14: Add deterministic correctness validation for the read-only
nav, longproc, longctx, and parallel manifest tasks by populating their
verificationCommand fields with checks that validate the reported answer or
output; ensure NewTurnExecRunner does not count zero-exit tasks as passed
without validation. If reliable validators cannot be defined, exclude these
tasks from correctness metrics instead.
- Around line 16-25: Strengthen the verificationCommand entries for mutation
tasks, especially edit-01, edit-03, and edit-08, so they validate the requested
rename, Config.Label field, and Config.GetLabel signature rather than merely
matching substrings. Add task-specific tests, negative assertions, or structural
checks that confirm old symbols are removed and declarations or behavior are
correct. Replace build-only verification for refactor tasks with checks that
validate the requested extraction, splitting, package/type changes, inlining,
and error consolidation.
- Around line 16-41: Update the task execution flow that runs the edit-, fix-,
and refactor-* manifest entries so each task receives an isolated copy or reset
of its workspaceFixture before setting cmd.Dir and running verification. Ensure
mutations from one task cannot affect subsequent tasks, while preserving the
existing fixture paths and verification commands.
In `@internal/perfbench/reports/README.md`:
- Around line 11-14: Update the shell command code fence in the README section
containing the make baseline examples to specify a shell language tag, such as
sh or bash, while leaving the command contents unchanged.
In `@internal/perfbench/testdata/longctx/big.go`:
- Around line 10-14: Update the handler generation template so the parity check
in functions like Handle001 uses n%2 != 0, covering both positive and negative
odd inputs. Regenerate all generated handlers while preserving the existing
errBad return and success behavior for even values.
In `@internal/perfbench/testdata/longproc/main_test.go`:
- Around line 11-15: Update BenchmarkProcess so the result of Process(100) is
assigned to a package-level sink variable on each iteration, ensuring the
benchmark work remains observable while preserving the existing iteration count
and input.
In `@internal/perfbench/turn_bench.go`:
- Around line 372-379: Update the trace-loading block in the benchmark flow
around os.Open and trace.ReadNDJSON to surface both file-open and parse failures
instead of silently ignoring them. Return the encountered error or record a
result warning through the existing benchmark result mechanism, while preserving
successful assignment to outcome.Trace and file cleanup.
- Around line 348-350: Update the task execution flow around WorkspaceFixture
and cmd.Dir to copy the fixture into a fresh temporary directory for every
invocation, then set cmd.Dir to that isolated copy. Ensure edit and fix
operations, including verification, use the temporary workspace while preserving
the existing behavior when no fixture is configured.
- Around line 361-388: The agent outcome flow must not mark a nonzero exit as
passed. In the run-result handling block, after trace capture and before
optional runVerification, return the outcome whenever VerifyErr is already set
from a nonzero run_end exit; preserve trace loading while skipping verification
and the final Passed assignment.
- Around line 169-200: Update the iteration tracking in the task loop around
passedForTask so a task is marked passed only when every iteration completes
successfully with outcome.Passed true. Preserve the existing handling of errors
and metrics, but initialize or update the pass state to fail on any unsuccessful
iteration and use it when incrementing result.TasksPassed.
In `@internal/providers/providerio/providerio.go`:
- Around line 370-372: Move trace.FromContext(ctx).StampFirstToken() out of the
generic non-keepalive payload branch and into the handle(item.data) processing
path after the payload is successfully parsed and accepted as text, reasoning,
or tool-output output. Ensure error and metadata payloads do not stamp
FirstTokenAt, while preserving the no-op behavior for untraced runs.
In `@internal/trace/emit.go`:
- Around line 82-117: Update WriteText to check every fmt.Fprintf and
fmt.Fprintln result and return the first encountered write error immediately,
including errors from span and counter output. Preserve the existing formatting
and nil-input behavior so TextSink.Emit receives failures instead of always
seeing success.
In `@internal/trace/recorder.go`:
- Around line 92-176: Update all Recorder mutators, including Counter,
StampFirstToken, StampFirstVisibleEvent, StampFirstUsefulAction, and addSpan, to
check finished while holding r.mu and return without changing r.tr once Finish
has set it. Preserve the existing first-call timestamp guards and ensure Finish
continues returning the original frozen snapshot on every call.
In `@internal/trace/trace_test.go`:
- Around line 204-220: Update TestAttributionRatio to make the trace wall time
match the two recorded 10ms spans: set the resulting TurnTrace’s CompletedAt to
StartedAt.Add(20*time.Millisecond), or otherwise advance its wall clock before
checking AttributionRatio. Preserve the existing attributed-duration assertion
and ratio validation.
---
Nitpick comments:
In `@internal/agent/loop.go`:
- Around line 118-136: Add focused tests around the tracing setup in the loop
entry point, covering nil versus non-nil Trace behavior, preservation and
invocation of the caller’s OnUsage callback, aggregation of
input/cached-input/output token counters across multiple requests, and counter
behavior through an error/retry path.
In `@internal/agent/types.go`:
- Around line 283-289: Clarify the public `Trace` contract near its field
declaration to require a fresh `*trace.Recorder` for each `Run`, since `Run`
neither resets nor finishes the caller-owned recorder. State that recorder reuse
across runs is unsupported and prevents spans, counters, and timestamps from
being merged.
In `@internal/perfbench/turn_bench_test.go`:
- Around line 283-292: Extend the task validation loop in the benchmark test to
stat the path in task.WorkspaceFixture and fail when it does not exist or is not
a directory, while retaining the existing testdata containment check. Use the
filesystem error and task ID/path in the failure message so invalid cmd.Dir
values are reported during validation.
🪄 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: ea1b7e7d-3db2-4ec5-86df-c6d21eab5591
📒 Files selected for processing (51)
Makefilecmd/zero-perf-bench/main.gocmd/zero-perf-bench/turn.gointernal/agent/compaction.gointernal/agent/loop.gointernal/agent/reconnect.gointernal/agent/types.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/perfbench/manifests/baseline.jsoninternal/perfbench/reports/.gitkeepinternal/perfbench/reports/README.mdinternal/perfbench/taskbench.gointernal/perfbench/testdata/fix/bugs.gointernal/perfbench/testdata/fix/bugs_test.gointernal/perfbench/testdata/longctx/big.gointernal/perfbench/testdata/longproc/main.gointernal/perfbench/testdata/longproc/main_test.gointernal/perfbench/testdata/nav/README.mdinternal/perfbench/testdata/nav/config.jsoninternal/perfbench/testdata/nav/main.gointernal/perfbench/testdata/parallel/a.txtinternal/perfbench/testdata/parallel/b.txtinternal/perfbench/testdata/parallel/c.txtinternal/perfbench/testdata/parallel/config1.jsoninternal/perfbench/testdata/parallel/config2.jsoninternal/perfbench/testdata/parallel/config3.jsoninternal/perfbench/testdata/parallel/config4.jsoninternal/perfbench/testdata/parallel/config5.jsoninternal/perfbench/testdata/parallel/config6.jsoninternal/perfbench/testdata/parallel/d.txtinternal/perfbench/testdata/parallel/dir1/notes.mdinternal/perfbench/testdata/parallel/dir2/notes.mdinternal/perfbench/testdata/parallel/dir3/notes.mdinternal/perfbench/testdata/parallel/dir4/notes.mdinternal/perfbench/testdata/parallel/dir5/notes.mdinternal/perfbench/testdata/parallel/dir6/notes.mdinternal/perfbench/testdata/parallel/e.txtinternal/perfbench/testdata/parallel/f.txtinternal/perfbench/testdata/refactor/main.gointernal/perfbench/turn_bench.gointernal/perfbench/turn_bench_test.gointernal/providers/providerio/auth.gointernal/providers/providerio/providerio.gointernal/providers/providerio/retry.gointernal/trace/context.gointernal/trace/emit.gointernal/trace/parse.gointernal/trace/recorder.gointernal/trace/trace.gointernal/trace/trace_test.go
The test recorded two synthetic 10ms spans with no real delay and asserted the attribution ratio stayed <= 1.0. On a precise clock (Linux/macOS CI) the wall is microseconds while attributed is 20ms, so attributed/wall blows past 1.0 and the test failed — even though the ratio legitimately exceeds 1.0 when spans overlap (parallel tool execution). On Windows the coarse timer masked it, so the failure only surfaced in CI. Assert the contract instead: attributed is the deterministic sum of span durations, and the ratio equals attributed/wall (0 when wall is zero). Add a zero-wall case for the divide-by-zero guard.
Benchmark runner (internal/perfbench): - A nonzero agent exit code no longer counts as Passed. The runner set VerifyErr but then fell through to outcome.Passed = true, so a crashed run could be recorded as a pass. Return before the verification block when VerifyErr is already set. - A task now passes only when every iteration passes. The previous "any iteration passes" rule let a flaky fail hide behind one good run. - Each invocation copies its workspace fixture to a fresh temp dir and runs the agent + verifier there, so mutating tasks (edit/fix/refactor) can't dirty the checked-in fixtures or bleed into the next iteration. - Trace load failures (missing/malformed trace file) are surfaced as result warnings via a new TraceIssue field instead of being swallowed. Tracer (internal/trace): - Finish freezes the recorder: Counter, addSpan, and the first-event stamps are dropped after Finish returns, so a snapshot captures exactly one turn and late stamps can't mutate it. - WriteText propagates the first write error instead of silently swallowing sink failures. Provider seam (internal/providers/providerio): - FirstTokenAt is stamped only after a payload is accepted as real model output, not before handle() decides to drop it as an error payload. Agent loop (internal/agent): - The compaction counter increments only when the history actually shrinks, in both the proactive and reactive paths — paid no-ops no longer inflate the compaction total. - Document on Options.Trace that a fresh Recorder is required per Run. CLI (internal/cli/exec): - Finish the trace at the agent.Run boundary and defer only the serialization, so the snapshot covers exactly the turn. - Reject --trace / ZERO_TRACE for spec-draft runs with a clear error instead of silently accepting and writing nothing. Manifest + fixtures: - Strengthen the edit verifiers: rename and value-bump tasks now assert the old symbol is gone, and the getter task greps the method signature. - Add the missing testdata/edit fixture (10 edit tasks referenced a directory that was never committed, so the whole edit class errored out at run time). - The manifest test now stats each fixture so a missing dir fails at load time instead of at every run. - longproc benchmark keeps Process live across the loop; README code fence gets a language tag and documents the per-invocation fixture copy. - Add tests for the tracing wrapper (nil vs wired recorder, OnUsage forwarding, token aggregation) and the frozen-recorder contract.
|
Pushed Benchmark runner: a nonzero agent exit no longer counts as Tracer: Provider seam: Compaction: the counter now increments only on an actual history shrink (both the proactive and reactive paths). CLI: the trace is finished at the Manifest + fixtures: strengthened the edit verifiers rename and value-bump tasks now assert the old symbol is gone, and the getter task greps the method signature. Added the missing Two nitpicks I left on purpose:
The |
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/testdata/edit/main.go`:
- Around line 22-27: Update greet to incorporate its name argument when
constructing the returned greeting, replacing the constant-only "hello" result
while preserving the documented hello-message format.
🪄 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: db33eae6-93d9-4151-8114-74794ab61c89
📒 Files selected for processing (15)
internal/agent/compaction.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/exec.gointernal/perfbench/manifests/baseline.jsoninternal/perfbench/reports/README.mdinternal/perfbench/testdata/edit/main.gointernal/perfbench/testdata/edit/version.gointernal/perfbench/testdata/longproc/main_test.gointernal/perfbench/turn_bench.gointernal/perfbench/turn_bench_test.gointernal/providers/providerio/providerio.gointernal/trace/emit.gointernal/trace/recorder.gointernal/trace/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/perfbench/reports/README.md
- internal/perfbench/testdata/longproc/main_test.go
- internal/agent/compaction.go
- internal/providers/providerio/providerio.go
- internal/perfbench/manifests/baseline.json
- internal/agent/types.go
- internal/cli/exec.go
- internal/perfbench/turn_bench_test.go
- internal/trace/emit.go
- internal/trace/recorder.go
- internal/perfbench/turn_bench.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Review
This is the Phase 0 baseline PR the perf split work gated on, and the shape is right: opt-in tracer, span taxonomy, benchmark manifest + fixtures, NDJSON compatible with the existing agenteval contract, no behavior change for untraced runs. The addressing commit b3e6ef1 closed 13 of the 19 review threads. I'm fine with landing the tracer + harness code on the merits, but the baseline manifest is the gate, and the manifest as it stands cannot answer "where does a turn spend time?" with trustworthy numbers. I'd like to see the manifest issues below fixed (or explicitly deferred with a follow-up issue) before this lands.
Verdict
Ship the tracer, the harness, and the manifest as-is is not OK — the manifest is the program keystone, and it overstates the pass rate. Two options:
- (A) Block merge on the manifest fixes below. Strongly preferred —
make baselinewill be re-run on every perf change, so the manifest's contract matters for months. - (B) Land the tracer + harness now, open a follow-up issue for the manifest, and explicitly mark
baseline.jsonas draft in the file. Workable if the team is willing to do the manifest work in the next 1-2 days before any actual baseline is taken. The follow-up issue must land before any "M1 baseline established" sign-off.
Either way, the tracer land should not block on the benchmark being run against a real model — the harness is good, the questions are about what the harness measures.
Manifest — task count
The PR body says "42-task manifest across seven classes." The manifest has 48 tasks (10 nav + 10 edit + 8 fix + 6 refactor + 4 longproc + 4 longctx + 6 parallel). b3e6ef1 was supposed to address this per the "Addressed" marker on the manifest-count thread, but the count is still 48. Either:
- Update the body/docs to say 48, and update the "≥30-task gate" wording in the harness tests, or
- Drop 6 tasks to land at 42 (e.g. trim 2 each from
navandparallel, or 4 fromeditand 2 fromparallel).
I'd take the latter — 42 is the number the program gate is written against, and changing the gate at the same time as the first baseline is a way to lose the contract.
Manifest — verifiers (this is the real one)
Counting tasks by class and verifier:
| class | count | with verificationCommand |
verifier shape |
|---|---|---|---|
| nav | 10 | 0 | none — read-only, success = zero exec exit 0 |
| edit | 10 | 10 | grep for the new string (positive) or ! grep for the old (negative) |
| fix | 8 | 8 | go test -run <name> — strong, scoped |
| refactor | 6 | 6 | go build ./... — proves it compiles, doesn't prove the refactor |
| longproc | 4 | 0 | none |
| longctx | 4 | 0 | none |
| parallel | 6 | 0 | none |
| total | 48 | 24 | 24/48 tasks have no correctness signal |
Three sub-issues:
-
24 tasks with no verifier inflate
passRate. A task where the model says "the file has 5 lines" when it has 3 still passes today, because the only check is "didzero execexit 0." For the read-only task classes this is the majority of the manifest. Pick one:- Add deterministic verifiers. For
navthis is realistic — averificationCommandofbash -c "diff -q <(expected) <(agent-output)"works if the harness pipes the model's final text to a known path. Forlongproc/longctxit's harder (the model "summarizes" — there's no canonical answer), so document that those classes are excluded from the pass-rate metric and only contribute to the latency/span attribution. - Exclude them from
tasksPassedand only count them in latency. I.e.TasksAttemptedincludes them,TasksPasseddoes not, and the JSON explicitly says which classes are correctness-measured. The harness should already know the class — it's in the manifest.
Without one of these, the first baseline report's
passRateis a junk number, and the program will be making decisions off of it. - Add deterministic verifiers. For
-
The
editverifiers are weak.grep -R Label .does not prove the field was added to the Config struct — it proves the string "Label" appears somewhere. Foredit-01("renameMaxRetriestoRetryLimit"),grep -R RetryLimit .will pass if the model added a comment line or a string literal. Foredit-08(addGetLabelmethod),grep -R GetLabel .passes if the model wrote a function namedGetLabelin the wrong package. Two cheap upgrades:- For renames, also assert the old name is gone:
bash -c "test ! -f /tmp/marker && grep -R RetryLimit . && ! grep -R MaxRetries .". The harness already supports compound commands. - For adds (fields, methods, headers), assert with
go vetorgo buildin addition to the grep, so a syntactically broken addition is caught.
- For renames, also assert the old name is gone:
-
The
refactorverifiers are non-positive assertions.go build ./...only catches the case where the refactor broke compilation. A no-op refactor (model says "I extracted the helper" but didn't) passes. I don't have a clean universal verifier for refactors, so I'd mark the refactor class as "structure-uncorrected" in the report and not include it inpassRate. Latency + span attribution are still useful for it; correctness is not.
Manifest — workspace isolation (already addressed in b3e6ef1)
I see the addressed markers, but I want to flag that this is the kind of fix that belongs in a regression test, not just code. Can you add a turn_bench_test.go case that runs the same mutating task twice and asserts the second run's fixture contents are identical to the first? Right now the isolation is verified by reading the code; the test would verify it. Skip if too much for this PR — just want it on the record.
Tracer — minor stuff (defer or fix in this PR)
internal/agent/compaction.go:422-424and:469-471: theCounterCompactionCountincrement is gated onerr == nil, but the no-op path inCompact(returns the input unchanged whennewSize >= sizeorlen(messages) <= preserveLast+2) still increments. The CodeRabbit finding is right. Either move the increment below the no-op check, or rename toCompactionAttemptsin the report. (I'd rename — counting attempts vs. reductions is a question the baseline report should answer, not one we silently choose.)internal/cli/exec.go:506(Finish atrunExecboundary, not atagent.Run): the addressing commit moved the Finish to immediately afteragent.Runreturns, which is correct. Good.internal/providers/providerio/providerio.go:372(first-token stamp): the addressing commit moved the stamp into thehandle(item.data)path after parse, which is correct. Good.internal/trace/recorder.go(freeze onFinish): the addressing commit made all mutators no-op oncefinished. Good.internal/trace/emit.go:117(propagate write errors fromWriteText): the addressing commit returns the first write error. Good.
Plan-level concern
The PR drops the 16-span taxonomy the split work spec'd (prompt build, provider connect, provider queue, generation, tool planning/queue/exec, permission wait, process wait, compaction, retries/reconnects, verification, completion nudges, persistence, model switching). The PR has 10 of those. Process wait, tool planning, and tool queue are missing. Are they intentionally out of scope for Phase 0? If yes, mention it in the PR description under "Notes / deviations" so the next PR (whichever one owns the missing spans) doesn't have to dig. If no, add them — the seam is in place, it's three more Span() calls.
What I'd accept today, with the manifest deferred
If the team wants to unblock the tracer+bench-harness code now, this is the minimum to ship:
- Tracer + harness + NDJSON emit: ship as-is.
Makefiletarget: ship as-is.- Manifest: keep the 48 tasks, but in
internal/perfbench/reports/README.md(or a newinternal/perfbench/MANIFEST.md), document:- Total 48, class breakdown.
- 24 tasks are "latency-only" (no
verificationCommand) and are excluded frompassRate. - 6 refactor tasks are "structure-uncorrected" and are excluded from
passRate(latency + spans only). passRateis therefore computed over 18 tasks (10 edit + 8 fix) until the read-only verifiers are added.
- Open a follow-up issue "Phase 0 baseline: add read-only verifiers for nav/longproc/longctx/parallel" and assign to whoever picks up the manifest hardening.
This is a less-good path than fixing the manifest in this PR, but it unblocks the tracer for the rest of the program. The follow-up must be tracked; if it slips past the first baseline report, the program is making decisions on a junk passRate number.
What I won't accept
- "LGTM, the manifest is fine." It isn't.
- Drop the manifest entirely and run the harness ad-hoc. The point of the PR is the durable manifest.
- "We can fix the verifiers once we have a real baseline." No — the verifier is the contract; without it, we don't know whether the numbers move because the model improved, the harness changed, or we're measuring the wrong thing.
Verdict
Request changes. Either fix the manifest in this PR, or land the tracer+bench-harness only and explicitly defer the manifest with a tracked follow-up. Either is fine. Shipping the manifest as-is is not.
— kevin
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Review
This is the Phase 0 baseline PR the perf split work gated on, and the shape is right: opt-in tracer, span taxonomy, benchmark manifest + fixtures, NDJSON compatible with the existing agenteval contract, no behavior change for untraced runs. The addressing commit b3e6ef1 closed 13 of the 19 review threads. I'm fine with landing the tracer + harness code on the merits, but the baseline manifest is the gate, and the manifest as it stands cannot answer "where does a turn spend time?" with trustworthy numbers. I'd like to see the manifest issues below fixed (or explicitly deferred with a follow-up issue) before this lands.
Verdict
Ship the tracer, the harness, and the manifest as-is is not OK — the manifest is the program keystone, and it overstates the pass rate. Two options:
- (A) Block merge on the manifest fixes below. Strongly preferred —
make baselinewill be re-run on every perf change, so the manifest's contract matters for months. - (B) Land the tracer + harness now, open a follow-up issue for the manifest, and explicitly mark
baseline.jsonas draft in the file. Workable if the team is willing to do the manifest work in the next 1-2 days before any actual baseline is taken. The follow-up issue must land before any "M1 baseline established" sign-off.
Either way, the tracer land should not block on the benchmark being run against a real model — the harness is good, the questions are about what the harness measures.
Manifest — task count
The PR body says "42-task manifest across seven classes." The manifest has 48 tasks (10 nav + 10 edit + 8 fix + 6 refactor + 4 longproc + 4 longctx + 6 parallel). b3e6ef1 was supposed to address this per the "Addressed" marker on the manifest-count thread, but the count is still 48. Either:
- Update the body/docs to say 48, and update the "≥30-task gate" wording in the harness tests, or
- Drop 6 tasks to land at 42 (e.g. trim 2 each from
navandparallel, or 4 fromeditand 2 fromparallel).
I'd take the latter — 42 is the number the program gate is written against, and changing the gate at the same time as the first baseline is a way to lose the contract.
Manifest — verifiers (this is the real one)
Counting tasks by class and verifier:
| class | count | with verificationCommand |
verifier shape |
|---|---|---|---|
| nav | 10 | 0 | none — read-only, success = zero exec exit 0 |
| edit | 10 | 10 | grep for the new string (positive) or ! grep for the old (negative) |
| fix | 8 | 8 | go test -run <name> — strong, scoped |
| refactor | 6 | 6 | go build ./... — proves it compiles, doesn't prove the refactor |
| longproc | 4 | 0 | none |
| longctx | 4 | 0 | none |
| parallel | 6 | 0 | none |
| total | 48 | 24 | 24/48 tasks have no correctness signal |
Three sub-issues:
-
24 tasks with no verifier inflate
passRate. A task where the model says "the file has 5 lines" when it has 3 still passes today, because the only check is "didzero execexit 0." For the read-only task classes this is the majority of the manifest. Pick one:- Add deterministic verifiers. For
navthis is realistic — averificationCommandofbash -c "diff -q <(expected) <(agent-output)"works if the harness pipes the model's final text to a known path. Forlongproc/longctxit's harder (the model "summarizes" — there's no canonical answer), so document that those classes are excluded from the pass-rate metric and only contribute to the latency/span attribution. - Exclude them from
tasksPassedand only count them in latency. I.e.TasksAttemptedincludes them,TasksPasseddoes not, and the JSON explicitly says which classes are correctness-measured. The harness should already know the class — it's in the manifest.
Without one of these, the first baseline report's
passRateis a junk number, and the program will be making decisions off of it. - Add deterministic verifiers. For
-
The
editverifiers are weak.grep -R Label .does not prove the field was added to the Config struct — it proves the string "Label" appears somewhere. Foredit-01("renameMaxRetriestoRetryLimit"),grep -R RetryLimit .will pass if the model added a comment line or a string literal. Foredit-08(addGetLabelmethod),grep -R GetLabel .passes if the model wrote a function namedGetLabelin the wrong package. Two cheap upgrades:- For renames, also assert the old name is gone:
bash -c "test ! -f /tmp/marker && grep -R RetryLimit . && ! grep -R MaxRetries .". The harness already supports compound commands. - For adds (fields, methods, headers), assert with
go vetorgo buildin addition to the grep, so a syntactically broken addition is caught.
- For renames, also assert the old name is gone:
-
The
refactorverifiers are non-positive assertions.go build ./...only catches the case where the refactor broke compilation. A no-op refactor (model says "I extracted the helper" but didn't) passes. I don't have a clean universal verifier for refactors, so I'd mark the refactor class as "structure-uncorrected" in the report and not include it inpassRate. Latency + span attribution are still useful for it; correctness is not.
Manifest — workspace isolation (already addressed in b3e6ef1)
I see the addressed markers, but I want to flag that this is the kind of fix that belongs in a regression test, not just code. Can you add a turn_bench_test.go case that runs the same mutating task twice and asserts the second run's fixture contents are identical to the first? Right now the isolation is verified by reading the code; the test would verify it. Skip if too much for this PR — just want it on the record.
Tracer — minor stuff (defer or fix in this PR)
internal/agent/compaction.go:422-424and:469-471: theCounterCompactionCountincrement is gated onerr == nil, but the no-op path inCompact(returns the input unchanged whennewSize >= sizeorlen(messages) <= preserveLast+2) still increments. The CodeRabbit finding is right. Either move the increment below the no-op check, or rename toCompactionAttemptsin the report. (I'd rename — counting attempts vs. reductions is a question the baseline report should answer, not one we silently choose.)internal/cli/exec.go:506(Finish atrunExecboundary, not atagent.Run): the addressing commit moved the Finish to immediately afteragent.Runreturns, which is correct. Good.internal/providers/providerio/providerio.go:372(first-token stamp): the addressing commit moved the stamp into thehandle(item.data)path after parse, which is correct. Good.internal/trace/recorder.go(freeze onFinish): the addressing commit made all mutators no-op oncefinished. Good.internal/trace/emit.go:117(propagate write errors fromWriteText): the addressing commit returns the first write error. Good.
Plan-level concern
The PR drops the 16-span taxonomy the split work spec'd (prompt build, provider connect, provider queue, generation, tool planning/queue/exec, permission wait, process wait, compaction, retries/reconnects, verification, completion nudges, persistence, model switching). The PR has 10 of those. Process wait, tool planning, and tool queue are missing. Are they intentionally out of scope for Phase 0? If yes, mention it in the PR description under "Notes / deviations" so the next PR (whichever one owns the missing spans) doesn't have to dig. If no, add them — the seam is in place, it's three more Span() calls.
What I'd accept today, with the manifest deferred
If the team wants to unblock the tracer+bench-harness code now, this is the minimum to ship:
- Tracer + harness + NDJSON emit: ship as-is.
Makefiletarget: ship as-is.- Manifest: keep the 48 tasks, but in
internal/perfbench/reports/README.md(or a newinternal/perfbench/MANIFEST.md), document:- Total 48, class breakdown.
- 24 tasks are "latency-only" (no
verificationCommand) and are excluded frompassRate. - 6 refactor tasks are "structure-uncorrected" and are excluded from
passRate(latency + spans only). passRateis therefore computed over 18 tasks (10 edit + 8 fix) until the read-only verifiers are added.
- Open a follow-up issue "Phase 0 baseline: add read-only verifiers for nav/longproc/longctx/parallel" and assign to whoever picks up the manifest hardening.
This is a less-good path than fixing the manifest in this PR, but it unblocks the tracer for the rest of the program. The follow-up must be tracked; if it slips past the first baseline report, the program is making decisions on a junk passRate number.
What I won't accept
- "LGTM, the manifest is fine." It isn't.
- Drop the manifest entirely and run the harness ad-hoc. The point of the PR is the durable manifest.
- "We can fix the verifiers once we have a real baseline." No — the verifier is the contract; without it, we don't know whether the numbers move because the model improved, the harness changed, or we're measuring the wrong thing.
Verdict
Request changes. Either fix the manifest in this PR, or land the tracer+bench-harness only and explicitly defer the manifest with a tracked follow-up. Either is fine. Shipping the manifest as-is is not.
— kevin
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Review
This is the Phase 0 baseline PR the perf split work gated on, and the shape is right: opt-in tracer, span taxonomy, benchmark manifest + fixtures, NDJSON compatible with the existing agenteval contract, no behavior change for untraced runs. The addressing commit b3e6ef1 closed 13 of the 19 review threads. I'm fine with landing the tracer + harness code on the merits, but the baseline manifest is the gate, and the manifest as it stands cannot answer "where does a turn spend time?" with trustworthy numbers. I'd like to see the manifest issues below fixed (or explicitly deferred with a follow-up issue) before this lands.
Verdict
Ship the tracer, the harness, and the manifest as-is is not OK — the manifest is the program keystone, and it overstates the pass rate. Two options:
- (A) Block merge on the manifest fixes below. Strongly preferred —
make baselinewill be re-run on every perf change, so the manifest's contract matters for months. - (B) Land the tracer + harness now, open a follow-up issue for the manifest, and explicitly mark
baseline.jsonas draft in the file. Workable if the team is willing to do the manifest work in the next 1-2 days before any actual baseline is taken. The follow-up issue must land before any "M1 baseline established" sign-off.
Either way, the tracer land should not block on the benchmark being run against a real model — the harness is good, the questions are about what the harness measures.
Manifest — task count
The PR body says "42-task manifest across seven classes." The manifest has 48 tasks (10 nav + 10 edit + 8 fix + 6 refactor + 4 longproc + 4 longctx + 6 parallel). b3e6ef1 was supposed to address this per the "Addressed" marker on the manifest-count thread, but the count is still 48. Either:
- Update the body/docs to say 48, and update the "≥30-task gate" wording in the harness tests, or
- Drop 6 tasks to land at 42 (e.g. trim 2 each from
navandparallel, or 4 fromeditand 2 fromparallel).
I'd take the latter — 42 is the number the program gate is written against, and changing the gate at the same time as the first baseline is a way to lose the contract.
Manifest — verifiers (this is the real one)
Counting tasks by class and verifier:
| class | count | with verificationCommand |
verifier shape |
|---|---|---|---|
| nav | 10 | 0 | none — read-only, success = zero exec exit 0 |
| edit | 10 | 10 | grep for the new string (positive) or ! grep for the old (negative) |
| fix | 8 | 8 | go test -run <name> — strong, scoped |
| refactor | 6 | 6 | go build ./... — proves it compiles, doesn't prove the refactor |
| longproc | 4 | 0 | none |
| longctx | 4 | 0 | none |
| parallel | 6 | 0 | none |
| total | 48 | 24 | 24/48 tasks have no correctness signal |
Three sub-issues:
-
24 tasks with no verifier inflate
passRate. A task where the model says "the file has 5 lines" when it has 3 still passes today, because the only check is "didzero execexit 0." For the read-only task classes this is the majority of the manifest. Pick one:- Add deterministic verifiers. For
navthis is realistic — averificationCommandofbash -c "diff -q <(expected) <(agent-output)"works if the harness pipes the model's final text to a known path. Forlongproc/longctxit's harder (the model "summarizes" — there's no canonical answer), so document that those classes are excluded from the pass-rate metric and only contribute to the latency/span attribution. - Exclude them from
tasksPassedand only count them in latency. I.e.TasksAttemptedincludes them,TasksPasseddoes not, and the JSON explicitly says which classes are correctness-measured. The harness should already know the class — it's in the manifest.
Without one of these, the first baseline report's
passRateis a junk number, and the program will be making decisions off of it. - Add deterministic verifiers. For
-
The
editverifiers are weak.grep -R Label .does not prove the field was added to the Config struct — it proves the string "Label" appears somewhere. Foredit-01("renameMaxRetriestoRetryLimit"),grep -R RetryLimit .will pass if the model added a comment line or a string literal. Foredit-08(addGetLabelmethod),grep -R GetLabel .passes if the model wrote a function namedGetLabelin the wrong package. Two cheap upgrades:- For renames, also assert the old name is gone:
bash -c "test ! -f /tmp/marker && grep -R RetryLimit . && ! grep -R MaxRetries .". The harness already supports compound commands. - For adds (fields, methods, headers), assert with
go vetorgo buildin addition to the grep, so a syntactically broken addition is caught.
- For renames, also assert the old name is gone:
-
The
refactorverifiers are non-positive assertions.go build ./...only catches the case where the refactor broke compilation. A no-op refactor (model says "I extracted the helper" but didn't) passes. I don't have a clean universal verifier for refactors, so I'd mark the refactor class as "structure-uncorrected" in the report and not include it inpassRate. Latency + span attribution are still useful for it; correctness is not.
Manifest — workspace isolation (already addressed in b3e6ef1)
I see the addressed markers, but I want to flag that this is the kind of fix that belongs in a regression test, not just code. Can you add a turn_bench_test.go case that runs the same mutating task twice and asserts the second run's fixture contents are identical to the first? Right now the isolation is verified by reading the code; the test would verify it. Skip if too much for this PR — just want it on the record.
Tracer — minor stuff (defer or fix in this PR)
internal/agent/compaction.go:422-424and:469-471: theCounterCompactionCountincrement is gated onerr == nil, but the no-op path inCompact(returns the input unchanged whennewSize >= sizeorlen(messages) <= preserveLast+2) still increments. The CodeRabbit finding is right. Either move the increment below the no-op check, or rename toCompactionAttemptsin the report. (I'd rename — counting attempts vs. reductions is a question the baseline report should answer, not one we silently choose.)internal/cli/exec.go:506(Finish atrunExecboundary, not atagent.Run): the addressing commit moved the Finish to immediately afteragent.Runreturns, which is correct. Good.internal/providers/providerio/providerio.go:372(first-token stamp): the addressing commit moved the stamp into thehandle(item.data)path after parse, which is correct. Good.internal/trace/recorder.go(freeze onFinish): the addressing commit made all mutators no-op oncefinished. Good.internal/trace/emit.go:117(propagate write errors fromWriteText): the addressing commit returns the first write error. Good.
Plan-level concern
The PR drops the 16-span taxonomy the split work spec'd (prompt build, provider connect, provider queue, generation, tool planning/queue/exec, permission wait, process wait, compaction, retries/reconnects, verification, completion nudges, persistence, model switching). The PR has 10 of those. Process wait, tool planning, and tool queue are missing. Are they intentionally out of scope for Phase 0? If yes, mention it in the PR description under "Notes / deviations" so the next PR (whichever one owns the missing spans) doesn't have to dig. If no, add them — the seam is in place, it's three more Span() calls.
What I'd accept today, with the manifest deferred
If the team wants to unblock the tracer+bench-harness code now, this is the minimum to ship:
- Tracer + harness + NDJSON emit: ship as-is.
Makefiletarget: ship as-is.- Manifest: keep the 48 tasks, but in
internal/perfbench/reports/README.md(or a newinternal/perfbench/MANIFEST.md), document:- Total 48, class breakdown.
- 24 tasks are "latency-only" (no
verificationCommand) and are excluded frompassRate. - 6 refactor tasks are "structure-uncorrected" and are excluded from
passRate(latency + spans only). passRateis therefore computed over 18 tasks (10 edit + 8 fix) until the read-only verifiers are added.
- Open a follow-up issue "Phase 0 baseline: add read-only verifiers for nav/longproc/longctx/parallel" and assign to whoever picks up the manifest hardening.
This is a less-good path than fixing the manifest in this PR, but it unblocks the tracer for the rest of the program. The follow-up must be tracked; if it slips past the first baseline report, the program is making decisions on a junk passRate number.
What I won't accept
- "LGTM, the manifest is fine." It isn't.
- Drop the manifest entirely and run the harness ad-hoc. The point of the PR is the durable manifest.
- "We can fix the verifiers once we have a real baseline." No — the verifier is the contract; without it, we don't know whether the numbers move because the model improved, the harness changed, or we're measuring the wrong thing.
Verdict
Request changes. Either fix the manifest in this PR, or land the tracer+bench-harness only and explicitly defer the manifest with a tracked follow-up. Either is fine. Shipping the manifest as-is is not.
— kevin
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review (local build + live smoke + code read)
Reviewed at b3e6ef1 (merged cleanly with current main). Gate satisfied: @anandh8x has formal review comments on this PR.
Local verification
| Check | Result |
|---|---|
gofmt / go vet on touched packages |
clean |
go build ./cmd/zero + ./cmd/zero-perf-bench |
OK |
go test -race -count=1 on trace, perfbench, agent, providerio |
pass |
zero-perf-bench turn --suite … --dry-run |
pass |
--use-spec --trace rejection |
correct |
Live zero exec --trace … --max-turns 1 "reply with only the word pong" |
works |
Live smoke numbers (real concern)
wall_ms=1614.8 attributed_ms=2364.1 attribution=1.464 (146%)
generation=1607ms provider_connect=756ms prompt_build=0.07ms
first_visible_at=empty (StampFirstVisibleEvent never called in production)
generation + provider_connect alone already exceed wall — spans overlap (provider client.Do runs in a goroutine while the agent holds SpanGeneration around CollectStream). Summing span durations for Phase 0 “top latency sources” is misleading until exclusive/non-overlapping attribution exists.
What looks good
- Opt-in nil-safe recorder; untraced path is a real no-op
- Thin providerio seam (no Provider interface change)
- Follow-ups already landed: Finish at
agent.Runboundary, freeze after Finish, nonzero exit ≠ pass, all-iterations-must-pass, fixture isolation, TraceIssue, compaction count only on real shrink - Unit coverage for recorder / harness aggregation is solid
Verdict: CHANGES_REQUESTED
Treat as good instrumentation scaffolding, not a trustworthy Phase 0 baseline until the must-fix items below land (especially fix-fixture compile independence + attribution model).
Inline comments cover the must-fix / should-fix items.
| // Label returns a display label for the given name. BUG: uses the %d format | ||
| // verb for a string, producing the wrong text. | ||
| func Label(name string) string { | ||
| return fmt.Sprintf("user-%d", name) |
There was a problem hiding this comment.
Blocking — fix suite independence is false
This fmt.Sprintf("user-%d", name) with a string is a compile error under current Go:
go test ./internal/perfbench/testdata/fix/
# bugs.go:75: format %d has arg name of wrong type string
FAIL [build failed]
Package comments claim the initial package compiles and each task is independent via go test -run <Test>. None of fix-01…fix-06 / fix-08 can verify until Label is fixed first.
Fix: make Label a pure runtime bug (wrong prefix string that still type-checks), or isolate compile-time bugs so other -run targets still build.
| if wall <= 0 { | ||
| return 0 | ||
| } | ||
| return float64(t.AttributedDuration()) / float64(wall) |
There was a problem hiding this comment.
Major — raw sum attribution is not wall-correct (proven live)
Doc says a run is “well-attributed when AttributedDuration >= 95% of WallDuration”. Live smoke on this branch produced attribution = 1.464 because concurrent spans are summed:
provider_connectstampsclient.Doinside the provider stream goroutinegenerationstampsCollectStreamon the agent loop at the same wall timepermission_waitis nested insidetool_executionand is summed again
Phase 0 ranks “top controllable latency sources by share of attributed time”. With ratio ≫ 1, shares are inflated.
Please either:
- stamp non-overlapping exclusive regions only, or
- model a span tree / exclusive time, or
- stop using raw attributed share as the Phase 0 decision metric until exclusive time exists,
and update the “≥ 95%” contract so concurrent/nested spans are honest.
| return collected, nil | ||
| } | ||
|
|
||
| generationSpan := options.Trace.Span(trace.SpanGeneration) |
There was a problem hiding this comment.
Major — generation overlaps provider_connect
SpanGeneration wraps only CollectStream*, but providers return a channel and perform client.Do / SSE setup in a background goroutine (where SpanProviderConnect is open). Those two spans run concurrently in wall time and are both added into AttributedDuration.
Live smoke: generation≈1607ms + provider_connect≈756ms vs wall≈1615ms.
Stamp connect only for the synchronous pre-return path, or exclude connect time already covered by generation, otherwise baseline rankings will systematically double-count provider wait.
| return PermissionDecision{Action: PermissionDecisionDeny, Reason: request.Reason}, nil | ||
| } | ||
| return options.OnPermissionRequest(ctx, request) | ||
| permSpan := options.Trace.Span(trace.SpanPermissionWait) |
There was a problem hiding this comment.
Major — nested double-count with tool_execution
requestPermission is only reached from inside executeToolCall, which is already wrapped in SpanToolExecution. Stamping permission_wait here is useful as a nested breakdown, but summing both into AttributedDuration / top-latency shares double-counts the same wall interval.
If permission_wait is kept, exclusive attribution (or a parent/child model) needs to exclude it from the sum used for “share of attributed time”.
| SpanProcessWait = "process_wait" | ||
| SpanVerification = "verification" | ||
| SpanCompaction = "compaction" | ||
| SpanPersistence = "persistence" |
There was a problem hiding this comment.
Major — span vocabulary oversells Phase 0 coverage
These names are defined as “the single source of truth for what a run's wall time is attributed to”, but in production code I cannot find any stamps for:
tool_queueprocess_waitverificationpersistence
Also StampFirstVisibleEvent is never called outside tests (live smoke left first_visible_at empty).
Longproc tasks (go test / build) will dump shell wait into tool_execution only, so the harness cannot separate model vs process wait as advertised.
Either wire real stamps for the named phases, or drop/rename until they exist so readers don’t trust empty categories.
| "span:" + SpanToolExecution, | ||
| "span:" + SpanPermissionWait, | ||
| "span:" + SpanCompaction, | ||
| "span:" + SpanPersistence, |
There was a problem hiding this comment.
Should-fix — RequiredEventKeys includes never-emitted spans
span:persistence (and typically tool/permission spans) are not produced by a normal successful short run. A live “pong” trace only had prompt_build / compaction / generation / provider_*.
Callers using MissingTraceEvents(RequiredEventKeys(), …) will false-fail healthy traces. Trim to keys that are actually guaranteed (or document per-run optional keys).
| // the tool-definition tokens (they ride on every request) in its estimate. | ||
| // partitionTools depends only on registry/permissions/options/loaded, not on | ||
| // the messages, so computing it before compaction is safe. | ||
| promptBuildSpan := options.Trace.Span(trace.SpanPromptBuild) |
There was a problem hiding this comment.
Medium — prompt_build is misnamed / near-noise
This only wraps partitionToolsCached, not system-prompt construction (built earlier). Live smoke: 0.07ms.
Rename (e.g. tool_partition) or expand to real prompt assembly so top-span tables aren’t confusing.
| @@ -0,0 +1,60 @@ | |||
| { | |||
There was a problem hiding this comment.
Medium — baseline validity / doc drift
- PR text says 42 tasks; this file has 48 (10 nav + 10 edit + 8 fix + 6 refactor + 4 longproc + 4 longctx + 6 parallel).
- 24 read-only tasks (nav / longproc / longctx / parallel) have no
verificationCommand→ exit 0 can pass with wrong answers; latency samples may be from failed cognitive work. - refactor-* only requires
go build→ no-op can pass. - edit oracles are mostly substring
grep(weak but maybe OK if suite is explicitly “latency probes”).
Please either strengthen oracles, or clearly label the suite as latency-only (not correctness) and fix the 42 vs 48 count everywhere.
Also: refactor-04 prompt mentions stats.go but the fixture is a single main.go.
| } | ||
| var obj map[string]any | ||
| if err := json.Unmarshal([]byte(line), &obj); err != nil { | ||
| continue |
There was a problem hiding this comment.
Medium — soft-parse can hide bad traces
Invalid JSON lines are continued; a truncated/corrupt file can yield an empty-ish TurnTrace with nil error. The harness then may not set TraceIssue, so a measurement gap looks like a valid empty attribution sample.
Prefer requiring a type:trace header (error if missing after non-empty input), or error when no spans/counters were recovered from a non-empty file.
There was a problem hiding this comment.
Review
This is the Phase 0 baseline PR the perf split work gated on, and the shape is right: opt-in tracer, span taxonomy, benchmark manifest + fixtures, NDJSON compatible with the existing agenteval contract, no behavior change for untraced runs. The addressing commit b3e6ef1 closed 13 of the 19 review threads. I'm fine with landing the tracer + harness code on the merits, but the baseline manifest is the gate, and the manifest as it stands cannot answer "where does a turn spend time?" with trustworthy numbers. I'd like to see the manifest issues below fixed (or explicitly deferred with a follow-up issue) before this lands.
Verdict
Ship the tracer, the harness, and the manifest as-is is not OK — the manifest is the program keystone, and it overstates the pass rate. Two options:
- (A) Block merge on the manifest fixes below. Strongly preferred —
make baselinewill be re-run on every perf change, so the manifest's contract matters for months. - (B) Land the tracer + harness now, open a follow-up issue for the manifest, and explicitly mark
baseline.jsonas draft in the file. Workable if the team is willing to do the manifest work in the next 1-2 days before any actual baseline is taken. The follow-up issue must land before any "M1 baseline established" sign-off.
Either way, the tracer land should not block on the benchmark being run against a real model — the harness is good, the questions are about what the harness measures.
Manifest — task count
The PR body says "42-task manifest across seven classes." The manifest has 48 tasks (10 nav + 10 edit + 8 fix + 6 refactor + 4 longproc + 4 longctx + 6 parallel). b3e6ef1 was supposed to address this per the "Addressed" marker on the manifest-count thread, but the count is still 48. Either:
- Update the body/docs to say 48, and update the "≥30-task gate" wording in the harness tests, or
- Drop 6 tasks to land at 42 (e.g. trim 2 each from
navandparallel, or 4 fromeditand 2 fromparallel).
I'd take the latter — 42 is the number the program gate is written against, and changing the gate at the same time as the first baseline is a way to lose the contract.
Manifest — verifiers (this is the real one)
Counting tasks by class and verifier:
| class | count | with verificationCommand |
verifier shape |
|---|---|---|---|
| nav | 10 | 0 | none — read-only, success = zero exec exit 0 |
| edit | 10 | 10 | grep for the new string (positive) or ! grep for the old (negative) |
| fix | 8 | 8 | go test -run <name> — strong, scoped |
| refactor | 6 | 6 | go build ./... — proves it compiles, doesn't prove the refactor |
| longproc | 4 | 0 | none |
| longctx | 4 | 0 | none |
| parallel | 6 | 0 | none |
| total | 48 | 24 | 24/48 tasks have no correctness signal |
Three sub-issues:
-
24 tasks with no verifier inflate
passRate. A task where the model says "the file has 5 lines" when it has 3 still passes today, because the only check is "didzero execexit 0." For the read-only task classes this is the majority of the manifest. Pick one:- Add deterministic verifiers. For
navthis is realistic — averificationCommandofbash -c "diff -q <(expected) <(agent-output)"works if the harness pipes the model's final text to a known path. Forlongproc/longctxit's harder (the model "summarizes" — there's no canonical answer), so document that those classes are excluded from the pass-rate metric and only contribute to the latency/span attribution. - Exclude them from
tasksPassedand only count them in latency. I.e.TasksAttemptedincludes them,TasksPasseddoes not, and the JSON explicitly says which classes are correctness-measured. The harness should already know the class — it's in the manifest.
Without one of these, the first baseline report's
passRateis a junk number, and the program will be making decisions off of it. - Add deterministic verifiers. For
-
The
editverifiers are weak.grep -R Label .does not prove the field was added to the Config struct — it proves the string "Label" appears somewhere. Foredit-01("renameMaxRetriestoRetryLimit"),grep -R RetryLimit .will pass if the model added a comment line or a string literal. Foredit-08(addGetLabelmethod),grep -R GetLabel .passes if the model wrote a function namedGetLabelin the wrong package. Two cheap upgrades:- For renames, also assert the old name is gone:
bash -c "test ! -f /tmp/marker && grep -R RetryLimit . && ! grep -R MaxRetries .". The harness already supports compound commands. - For adds (fields, methods, headers), assert with
go vetorgo buildin addition to the grep, so a syntactically broken addition is caught.
- For renames, also assert the old name is gone:
-
The
refactorverifiers are non-positive assertions.go build ./...only catches the case where the refactor broke compilation. A no-op refactor (model says "I extracted the helper" but didn't) passes. I don't have a clean universal verifier for refactors, so I'd mark the refactor class as "structure-uncorrected" in the report and not include it inpassRate. Latency + span attribution are still useful for it; correctness is not.
Manifest — workspace isolation (already addressed in b3e6ef1)
I see the addressed markers, but I want to flag that this is the kind of fix that belongs in a regression test, not just code. Can you add a turn_bench_test.go case that runs the same mutating task twice and asserts the second run's fixture contents are identical to the first? Right now the isolation is verified by reading the code; the test would verify it. Skip if too much for this PR — just want it on the record.
Tracer — minor stuff (defer or fix in this PR)
internal/agent/compaction.go:422-424and:469-471: theCounterCompactionCountincrement is gated onerr == nil, but the no-op path inCompact(returns the input unchanged whennewSize >= sizeorlen(messages) <= preserveLast+2) still increments. The CodeRabbit finding is right. Either move the increment below the no-op check, or rename toCompactionAttemptsin the report. (I'd rename — counting attempts vs. reductions is a question the baseline report should answer, not one we silently choose.)internal/cli/exec.go:506(Finish atrunExecboundary, not atagent.Run): the addressing commit moved the Finish to immediately afteragent.Runreturns, which is correct. Good.internal/providers/providerio/providerio.go:372(first-token stamp): the addressing commit moved the stamp into thehandle(item.data)path after parse, which is correct. Good.internal/trace/recorder.go(freeze onFinish): the addressing commit made all mutators no-op oncefinished. Good.internal/trace/emit.go:117(propagate write errors fromWriteText): the addressing commit returns the first write error. Good.
Plan-level concern
The PR drops the 16-span taxonomy the split work spec'd (prompt build, provider connect, provider queue, generation, tool planning/queue/exec, permission wait, process wait, compaction, retries/reconnects, verification, completion nudges, persistence, model switching). The PR has 10 of those. Process wait, tool planning, and tool queue are missing. Are they intentionally out of scope for Phase 0? If yes, mention it in the PR description under "Notes / deviations" so the next PR (whichever one owns the missing spans) doesn't have to dig. If no, add them — the seam is in place, it's three more Span() calls.
What I'd accept today, with the manifest deferred
If the team wants to unblock the tracer+bench-harness code now, this is the minimum to ship:
- Tracer + harness + NDJSON emit: ship as-is.
Makefiletarget: ship as-is.- Manifest: keep the 48 tasks, but in
internal/perfbench/reports/README.md(or a newinternal/perfbench/MANIFEST.md), document:- Total 48, class breakdown.
- 24 tasks are "latency-only" (no
verificationCommand) and are excluded frompassRate. - 6 refactor tasks are "structure-uncorrected" and are excluded from
passRate(latency + spans only). passRateis therefore computed over 18 tasks (10 edit + 8 fix) until the read-only verifiers are added.
- Open a follow-up issue "Phase 0 baseline: add read-only verifiers for nav/longproc/longctx/parallel" and assign to whoever picks up the manifest hardening.
This is a less-good path than fixing the manifest in this PR, but it unblocks the tracer for the rest of the program. The follow-up must be tracked; if it slips past the first baseline report, the program is making decisions on a junk passRate number.
What I won't accept
- "LGTM, the manifest is fine." It isn't.
- Drop the manifest entirely and run the harness ad-hoc. The point of the PR is the durable manifest.
- "We can fix the verifiers once we have a real baseline." No — the verifier is the contract; without it, we don't know whether the numbers move because the model improved, the harness changed, or we're measuring the wrong thing.
Verdict
Request changes. Either fix the manifest in this PR, or land the tracer+bench-harness only and explicitly defer the manifest with a tracked follow-up. Either is fine. Shipping the manifest as-is is not.
…ix-suite compile
Rewrite the trace attribution model so concurrent/nested spans no longer
double-count. Spans record wall intervals; the recorder derives a parent for
each by interval containment and computes exclusive time (duration minus the
union of its children). AttributedDuration sums only top-level spans; Coverage
is the union of intervals over wall, capped at 1, the honest ">=95%" metric.
Top-latency ranking uses exclusive time so shares sum to ~1 instead of being
inflated by overlapping provider_connect/generation and nested
permission_wait/tool_execution.
Wire the spans that were named but never stamped: verification around the
self-correct verify pass, and StampFirstVisibleEvent on the first forwarded
text event. Drop the tool_queue/process_wait/persistence constants that had no
stamps, and rename the misnamed near-noise prompt_build span to tool_partition.
Trim RequiredEventKeys to events a healthy short run actually emits; move
conditional phases (tools, permission, compaction, verification) to
OptionalEventKeys so a short trace does not false-fail.
Make ReadNDJSON fail loudly on a corrupt file: require a type:trace header,
error on span/counter lines that appear before it, and error when a header
yields no spans or counters.
Fix the blocking fix-suite compile error: Label's %d format verb on a string
tripped go test's vet before any fix task could verify. Make it a runtime
wrong-prefix bug ("admin-" vs "user-") that still type-checks, so each fix
task is independent again.
Manifest: correct refactor-04's stats.go to main.go, reword fix-07 to the
wrong-prefix bug, add a latency-only description (read-only tasks carry no
verification oracle by design), and frame the report README on exclusive time
and coverage.
Tests updated for the occurrence/exclusive/coverage model; trace, perfbench,
and agent green under -race.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review @ (after exclusive-time + fix-suite commit)
Gate: @anandh8x has formal reviews on this PR. Reviewed against latest HEAD with merged cleanly.
Local verification (this pass)
| Check | Result |
|---|---|
| FAIL . [setup failed] trace [options] - record system behavior |
trace record: record a trace file
$ trace record myworkload
[... Ctrl-C to stop ...]
$ trace record myworkload --Logging:enable-logs --end-after-duration 5s
$ trace record myworkload --plan profile --omit Symbolication
$ trace record myworkload --end-on-notification stop-myworkload-trace
[... elsewhere notifyutil -p stop-myworkload-trace ...]
$ trace record /tmp/trace-path.atrc --compress
trace amend: add data to a file
$ trace amend myworkload-003.atrc --add Symbolication
trace trim: trim a file based on kdebug event times
$ trace trim myworkload-002.atrc --from +1s --to +2s
trace providers: print information about Logging, Symbolication, etc.
trace plans: print detailed information about tracing approaches
See man trace for more information. / / / | pass |
| | pass |
| fix package compiles ( is runtime-only bug) | pass |
| FAIL . [setup failed] / | fail as independent targeted bugs (expected) |
| Live | pass |
Live smoke (real provider) — attribution fixed
Previous review smoke had attribution=1.464. That double-count is gone.
Prior findings vs this commit
| Prior issue (gnanam1990 / others) | Status @ |
|---|---|
| Fix suite compile ( + string) | Fixed — runtime bug |
| Nested/concurrent double-count | Fixed — interval nesting + exclusive time + Coverage |
| generation ∩ provider_connect | Fixed — parent/exclusive derived at Finish |
| permission_wait nested double-count | Fixed (same model) |
| Dead span vocabulary (process_wait etc.) | Addressed — dropped unused names; wired verification + first_visible |
| Misnamed | Fixed → |
| RequiredEventKeys false-fails short runs | Fixed — Required vs Optional split |
| Soft NDJSON parse | Fixed — requires ; empty body after header errors |
| refactor-04 stats.go typo | Fixed → main.go |
| Manifest “latency-only” framing | Documented in manifest + reports README |
Remaining real concerns
1. Medium — still treats latency-only tasks as correctness passes
Manifest description correctly says read-only tasks have no oracle and pass/fail must not be read as correctness. The harness still does:
- no → if exit 0 → → increments
So the headline in / JSON will still be dominated by 24 exit-0 latency tasks (+ weak refactor ). That is the same structural issue @anandh8x raised: passRate remains a junk gate metric even if prose warns not to trust it.
Ask (minimal, Phase 0–honest): either
- (A) only count tasks with toward / class pass rates, and report latency-only as (or exclude from passRate), or
- (B) keep current counting but rename fields in the report to / never call it “passed”, and print an explicit over verified classes only.
Prose alone is not enough — the JSON field name will be what dashboards consume.
2. Low — missing process-wait / tool-queue spans (scope OK if documented in PR body)
Phase 0 plan listed process wait + tool queue. They remain unstamped (constants removed). Fine for Phase 0 if the PR description “Notes / deviations” says so and names the follow-up owner. Please add one sentence so the next stream doesn’t rediscover the gap.
3. Low — weak edit/refactor oracles (accepted for latency suite)
Still weak (, ). Acceptable only under the latency-only contract and if concern #1 is fixed so they don’t inflate a “pass” KPI.
4. Nit — mutual equal-interval parent edge case
uses interval containment; two spans with identical can parent each other and drop out of top-level . Unlikely in real stamps; a unit test with equal intervals would lock the expected behavior.
What looks solid now
- Opt-in nil-safe tracer, exclusive-time model, NDJSON with exclusive/parent/start/end
- Harness ranks top latency by exclusive ms
- Finish freeze, agent.Run boundary, fixture isolation, nonzero exit ≠ pass (prior commits)
- Live coverage ≈ 99.5% on a short turn
Verdict: CHANGES_REQUESTED (narrow)
I would approve the tracer + exclusive-time model + fix-suite compile as soon as #1 is addressed (or anandh’s option B: land with follow-up issue filed and report fields that cannot be misread as correctness). Everything else is non-blocking for Phase 0.
Not blocking: CI still finishing smoke on this HEAD when I reviewed; unit/race/local smoke green here.
| } | ||
| result.TasksAttempted++ | ||
| if passedForTask { | ||
| result.TasksPassed++ |
There was a problem hiding this comment.
Medium (residual) — still includes latency-only exit-0
Even with the manifest calling the suite latency-only, this counter still increments for tasks with no whenever exits 0.
That means the JSON / summary line will be dominated by the 24 nav/longproc/longctx/parallel tasks and is still not a usable Phase 0 correctness signal (same gate concern as @anandh8x).
Please either:
- Only increment when and verification succeeded (and report latency-only attempts separately), or
- Rename the public metric away from “passed” and add an explicit over verified classes only.
Docs alone won’t stop dashboards from reading as success rate.
| continue | ||
| } | ||
| // b contains a when a starts at/after b and ends at/before b. | ||
| if !a.Start.Before(b.Start) && !a.End.After(b.End) { |
There was a problem hiding this comment.
Nit — equal-interval mutual containment
If two spans share identical , each can select the other as parent ( both ways). Both then have and drop out of top-level .
Real provider stamps are unlikely to hit this; synthetic might. A tiny unit test (or a tie-break: prefer earlier index / longer name rule) would lock intent.
| { | ||
| "id": "zero-baseline-turn", | ||
| "name": "Zero per-turn baseline (Phase 0)", | ||
| "description": "Latency-only baseline: measures where a turn spends wall time, not task correctness. Read-only tasks (nav, longproc, longctx, parallel) carry no verification oracle by design — a zero-exit does not prove the answer was right, only that the turn ran. Edit/fix/refactor oracles are intentionally lightweight (substring grep or `go build`) so the suite stays a latency probe; do not read pass/fail as a correctness verdict.", |
There was a problem hiding this comment.
Note — description is good; count still 48
Latency-only framing here is clear and matches the program’s “measure first” goal. Count is still 48, not 42 — keep PR body/docs aligned (48 is fine if consistent).
The remaining gap is not this prose — it’s the harness still counting latency-only tasks as (see ).
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review at 0f372f3 (exclusive-time + fix-suite follow-up)
Gate: anandh8x has formal reviews. Reviewed HEAD 0f372f3 with origin/main merged cleanly.
Note: An earlier re-review submission had a corrupted body due to shell expansion. This is the authoritative re-review.
Local verification
| Check | Result |
|---|---|
| go test -race on trace, perfbench, agent, providerio | pass |
| go build ./cmd/zero | pass |
| fix package compiles (Label is runtime-only bug) | pass |
| targeted go test -run on Sum / Label | fail as independent bugs (expected) |
| Live zero exec --trace on a one-turn pong prompt | pass |
Live smoke (real provider) — prior double-count fixed
- wall ~2210 ms
- coverage / attribution ~0.995 (was 1.464 on previous HEAD)
- provider_connect parent = generation, exclusive times look sane
- first_visible_at is set
Prior findings vs this commit
| Issue | Status |
|---|---|
| Fix suite compile (%d + string) | Fixed (admin-%s runtime bug) |
| Nested/concurrent double-count | Fixed (interval nesting + exclusive + Coverage) |
| generation / provider_connect overlap | Fixed |
| permission_wait nested double-count | Fixed (same model) |
| Dead span names | Addressed (dropped unused; wired verification + first_visible) |
| Misnamed prompt_build | Fixed to tool_partition |
| RequiredEventKeys false-fails | Fixed (Required vs Optional) |
| Soft NDJSON parse | Fixed (requires type:trace header) |
| refactor-04 stats.go typo | Fixed |
| Latency-only framing | Documented in manifest description + reports README |
Remaining real concern
Medium — tasksPassed still treats latency-only exit-0 as a pass
Manifest prose correctly says read-only tasks have no oracle. The harness still increments TasksPassed when there is no verificationCommand and zero exec exits 0. That means the headline tasks N/M passed and JSON tasksPassed stay dominated by ~24 latency-only tasks. Dashboards will misread this as correctness. Same structural gate anandh8x raised.
Ask (minimal): only count verified tasks toward TasksPassed (and report latency-only separately), or rename the metric and add an explicit correctnessPassRate over verified classes only.
Low / nit
- Document process_wait / tool_queue as intentionally out of Phase 0 scope in the PR body.
- Weak edit/refactor oracles OK only under latency contract after pass metric is honest.
- deriveNesting equal-interval mutual-parent edge case: add a unit test or tie-break (nit).
What is solid now
Opt-in tracer, exclusive-time model, NDJSON with exclusive/parent/start/end, top-latency by exclusive ms, Finish freeze, agent.Run boundary, fixture isolation, nonzero exit != pass, live coverage ~99.5% on a short turn.
Verdict: CHANGES_REQUESTED (narrow)
I would approve the tracer + exclusive-time model + fix-suite compile as soon as the TasksPassed / pass-rate honesty fix lands (or anandh option B: land with a filed follow-up AND report field names that cannot be misread as correctness). Everything else is non-blocking for Phase 0.
| } | ||
| result.TasksAttempted++ | ||
| if passedForTask { | ||
| result.TasksPassed++ |
There was a problem hiding this comment.
Medium (residual) — TasksPassed still includes latency-only exit-0
Even with the manifest description calling the suite latency-only, this counter still increments for tasks with no verificationCommand whenever zero exec exits 0.
JSON tasksPassed / summary 'tasks: N/M passed' will be dominated by the 24 nav/longproc/longctx/parallel tasks and is still not a usable Phase 0 correctness signal (same gate concern as anandh8x).
Please either:
- Only increment TasksPassed when verificationCommand is present and succeeds (report latency-only attempts separately), or
- Rename the public metric away from 'passed' and add an explicit correctnessPassRate over verified classes only.
Docs alone will not stop dashboards from reading tasksPassed as success rate.
| continue | ||
| } | ||
| // b contains a when a starts at/after b and ends at/before b. | ||
| if !a.Start.Before(b.Start) && !a.End.After(b.End) { |
There was a problem hiding this comment.
Nit — equal-interval mutual containment
If two spans share identical Start/End, each can select the other as parent (containment is symmetric on equality). Both then have Parent set and drop out of top-level AttributedDuration.
Unlikely with real provider stamps; synthetic RecordSpan might hit it. A tiny unit test or tie-break (prefer earlier index) would lock intent.
| { | ||
| "id": "zero-baseline-turn", | ||
| "name": "Zero per-turn baseline (Phase 0)", | ||
| "description": "Latency-only baseline: measures where a turn spends wall time, not task correctness. Read-only tasks (nav, longproc, longctx, parallel) carry no verification oracle by design — a zero-exit does not prove the answer was right, only that the turn ran. Edit/fix/refactor oracles are intentionally lightweight (substring grep or `go build`) so the suite stays a latency probe; do not read pass/fail as a correctness verdict.", |
There was a problem hiding this comment.
Note — description is good; count still 48
Latency-only framing here is clear. Count is still 48 not 42 — keep PR body/docs aligned (48 is fine if consistent).
The remaining gap is not this prose — it is the harness still counting latency-only tasks as tasksPassed (see turn_bench.go).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agent/loop.go`:
- Around line 287-300: Update the forwarding handlers in the loop setup so they
are installed whenever options.Trace is non-nil, even when options.OnText and
options.OnReasoning are unset. Keep stamping first-token timing for streamed
text and reasoning, while invoking each user callback only when its
corresponding option is non-nil; preserve forwardedVisibleText behavior for text
callbacks.
In `@internal/trace/parse.go`:
- Around line 72-75: The trace pipeline must preserve an explicit zero exclusive
duration instead of replacing it with the inclusive duration. In
internal/trace/parse.go lines 72-75, update the parsing fallback around
parseDurationMs and s.Exclusive to apply only when exclusive_ms is absent; in
internal/perfbench/turn_bench.go lines 206-213, rank spans using the parsed
Exclusive value directly without another duration fallback.
🪄 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: 85ea29cd-2ad5-480f-8c40-764ab94bcbb7
📒 Files selected for processing (12)
internal/agent/loop.gointernal/agent/selfcorrect.gointernal/perfbench/manifests/baseline.jsoninternal/perfbench/reports/README.mdinternal/perfbench/taskbench.gointernal/perfbench/testdata/fix/bugs.gointernal/perfbench/turn_bench.gointernal/trace/emit.gointernal/trace/parse.gointernal/trace/recorder.gointernal/trace/trace.gointernal/trace/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/perfbench/reports/README.md
- internal/perfbench/testdata/fix/bugs.go
- internal/perfbench/manifests/baseline.json
- internal/trace/trace_test.go
…trace parse Three correctness bugs in the attribution model found by an adversarial review of the prior commit, all undermining the no-double-count guarantee: 1. exclusive_ms:0 was overwritten with the inclusive Duration on re-parse. A parent whose children tile its interval legitimately has exclusive time 0; WriteNDJSON emits exclusive_ms:0, but ReadNDJSON's "<= 0 means missing" fallback turned that back into the full duration, so every re-parsed trace re-introduced the double-counting the model exists to prevent (the benchmark harness, which ranks re-parsed traces, inherits it). Gate the fallback on key ABSENCE, so a written 0 is preserved. 2. deriveNesting formed a 2-cycle for two spans with identical [start, end] intervals: the symmetric containment check made each parent the other, dropping both from top-level and zeroing AttributedDuration. Break the tie deterministically — strict containment is an unambiguous parent; for identical intervals, only the lower-indexed span may parent the other, so one stays top-level. 3. ReadNDJSON returned a valid empty TurnTrace for empty/blank-only input, so a run that crashed before emitting a trace (or where --trace was not honored) masqueraded as a clean zero-attribution sample. Empty input now errors, so the harness records a TraceIssue. Also: decode counters via json.Number so int64 values above 2^53 round-trip exactly instead of losing precision through float64; and drop the harness's own "exclusive <= 0 -> Duration" ranking fallback, which was the same double-count on the re-parsed path. Added regression tests covering the zero-exclusive round-trip, the identical-interval nesting, empty-input rejection, and counter precision.
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 (1)
internal/trace/parse.go (1)
94-99: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed counter values instead of fabricating totals.
Line 99appends counters even whenvalueis absent or invalid:parseInt64(json.Number("1.5"))becomes1, while a missing/string value becomes0. That single malformed counter bypasses the final corrupt-trace guard and lets benchmarks report false token/counter totals without aTraceIssue. Require a nonempty name and an exactint64value before appending; otherwise skip it so an input with no valid events still errors.Also applies to: 143-164
🤖 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/trace/parse.go` around lines 94 - 99, Update the counter handling in the trace parser to require a nonempty string name and an exact int64 value before appending to t.Counters; do not use parseInt64 for validation because it accepts malformed or fractional values. Skip invalid counter objects, preserving the existing final corrupt-trace guard so traces with no valid events produce a TraceIssue.
🤖 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/trace/parse.go`:
- Around line 94-99: Update the counter handling in the trace parser to require
a nonempty string name and an exact int64 value before appending to t.Counters;
do not use parseInt64 for validation because it accepts malformed or fractional
values. Skip invalid counter objects, preserving the existing final
corrupt-trace guard so traces with no valid events produce a TraceIssue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3ccea87-45ac-4cd5-9f02-fc4d1914811f
📒 Files selected for processing (4)
internal/perfbench/turn_bench.gointernal/trace/parse.gointernal/trace/recorder.gointernal/trace/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/trace/recorder.go
- internal/trace/trace_test.go
- internal/perfbench/turn_bench.go
|
Pushed a follow-up commit (0f372f3) that addresses the review. Walk-through:
2–4. double-counting (attribution = 1.46× wall). Rewrote the model: each span records a wall interval, the recorder derives a parent by interval containment, and a span's exclusive time is its duration minus the union of its children.
A second commit (40806aa) fixes three correctness bugs an adversarial self-review turned up in the new model, all variants of the same double-count risk:
CI is green across all checks. The honest framing is now coverage (union of span intervals over wall, capped at 1) as the "≥95% accounted for" metric, with exclusive-time ranking — the ≥95% contract no longer breaks on concurrent/nested spans. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review at 40806aa
Gate: anandh8x formal reviews present. Local re-check of HEAD after exclusive-time round-trip / nesting follow-up.
Local verification
| Check | Result |
|---|---|
| go test -race on trace, perfbench, agent | pass |
| go build ./cmd/zero | pass |
| CI smoke (macOS/Ubuntu/Windows) + security + CodeQL | all green |
| Live zero exec --trace one-turn pong | pass; coverage ~0.993; provider_connect parent=generation |
What 40806aa fixed (from prior threads)
| Item | Status |
|---|---|
| exclusive_ms:0 re-parse inflated to Duration | Fixed (fallback only when key absent) |
| Identical-interval mutual parent cycle | Fixed (strict containment + index tie-break) |
| Empty/blank NDJSON accepted as valid empty trace | Fixed (errors; harness gets TraceIssue) |
| Harness exclusive less-equal 0 fell back to Duration | Fixed (use Exclusive only) |
| Counter int64 precision via float64 | Fixed (json.Number) |
| All prior gnanam technical blocks on 0f372f3 (fix compile, nesting model, dead spans, soft parse, etc.) | Still hold as fixed |
Residual (unchanged) — still the only merge gate from my side
Medium: tasksPassed still counts latency-only exit-0 as pass
In NewTurnExecRunner, after a zero exit with no verificationCommand:
outcome.Passed = true
So nav/longproc/longctx/parallel (~24 tasks) still dominate TasksPassed and the summary line "tasks: N/M passed", despite the manifest description saying the suite is latency-only and pass/fail is not a correctness verdict.
This is the same structural concern as anandh8x. Prose does not fix dashboard/JSON consumers of tasksPassed.
Minimal ask (either is fine):
- Only increment TasksPassed when verificationCommand is present and succeeds; report latency-only attempts separately, or
- Rename the public metric (e.g. tasksCompleted) and add correctnessPassRate over verified classes only.
Non-blocking
- Weak edit/refactor oracles accepted under a true latency-only contract once the metric name/counting is honest.
- process_wait / tool_queue still out of Phase 0 — please one PR-body note under deviations if not already.
Verdict: CHANGES_REQUESTED (narrow, same residual)
Tracer + exclusive-time model look solid and production-ready for Phase 0 measurement. I will approve once the TasksPassed / pass-rate honesty fix lands (or anandh option B: follow-up issue filed and report fields that cannot be misread as correctness).
| return outcome | ||
| } | ||
| } | ||
| outcome.Passed = true |
There was a problem hiding this comment.
Still open — latency-only tasks mark Passed=true
When VerificationCommand is empty, a clean zero-exec exit still sets Passed=true, which then rolls into TasksPassed. That is the residual gate from the prior re-review.
Please only treat verification-backed tasks as Passed for the pass-rate KPI, or rename the metric so it cannot be read as correctness. 40806aa fixed exclusive-time round-trip nicely; this harness metric is the remaining issue.
…can't be misread as correctness The turn benchmark counted any exit-0 task as passed, including the 24 read-only tasks (nav/longproc/longctx/parallel) that carry no verification oracle — so the JSON tasksPassed / "N/M passed" headline could read as a correctness verdict it never was. Split pass/fail into three oracle tiers: - correctness (edit grep + fix go test): the only pass rate that can move with model quality (tasksVerified / tasksPassed / correctnessPassRate) - build-only (refactor go build): a non-positive oracle, reported as buildPassRate and excluded from correctnessPassRate; the manifest declares buildOnlyClasses: ["refactor"] - latency-only (no oracle): counted in latencyOnlyTasks, never in any pass rate NewTurnExecRunner no longer sets Passed=true for tasks with no verificationCommand, and the summary headline names each tier explicitly so an exit-0 read-only run cannot inflate a correctness number. Bumps TurnSchemaVersion to 2. Class breakdown and tier contract documented in internal/perfbench/MANIFEST.md and reports/README.md. Also install the trace first-token/visible-event forwarding handlers in agent.Run whenever options.Trace is set, not only when a UI OnText / OnReasoning callback is set, so a headless traced run (zero exec --trace) still captures FirstTokenAt. forwardedVisibleText stays tied to the user callback so stall-retry semantics are unchanged. Adds a regression assertion to the existing traced-run test.
|
Pushed Pass-rate honesty — the gateThe benchmark now reports pass/fail in three oracle tiers so an exit-0 read-only task can never inflate a number that reads as correctness:
The tier is decided per task from oracle presence first: a task with no The class breakdown and tier contract are written down in Weak-oracle hardening (strengthening edit grep to CodeRabbit findingsAddressed:
Already addressed in earlier commits (CodeRabbit self-marked these, or fixed in Skipped (not valid against the current code / would break the suite):
Notes / deviations (added to the PR body)Three spans from the plan's 16-span taxonomy are intentionally out of Phase 0 scope: CI is green across the touched packages ( |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review at 2bfb340 — APPROVE
Gate: anandh8x formal reviews present. Local re-check of latest HEAD.
Local verification
| Check | Result |
|---|---|
| go test -race on trace, perfbench, agent | pass |
| TestRunTurnBenchLatencyOnlyNeverPassed | pass |
| go build ./cmd/zero | pass |
| Live zero exec --trace one-turn pong | pass; coverage ~0.997; first_token_at set; provider_connect parent=generation |
| CI (smoke platforms, security, CodeQL, Zero Review) | green (windows smoke was finishing when checked) |
Residual from prior review — resolved in 2bfb340
| Prior ask | Status |
|---|---|
| tasksPassed inflated by latency-only exit-0 | Fixed — three oracle tiers; latency-only never sets Passed; correctnessPassRate / buildPassRate / latencyOnlyTasks; schema v2; MANIFEST.md + summary headline |
| NewTurnExecRunner Passed=true without oracle | Fixed — only path sets Passed after verificationCommand succeeds |
| Headless FirstTokenAt when no OnText | Fixed — forwarding handlers install when Trace is set |
Still solid from earlier commits
Exclusive-time nesting model, exclusive_ms:0 round-trip, identical-interval tie-break, empty-trace hard fail, fix-suite compile independence, wired verification / first_visible / tool_partition, Required vs Optional event keys.
Non-blocking notes (do not block merge)
- Edit oracles remain substring-grep (weak); acceptable under documented latency/correctness split where only edit+fix feed correctnessPassRate.
- process_wait / tool_queue still out of Phase 0 span vocabulary — already an intentional gap; optional PR-body one-liner if not present.
- Weak refactor build-only oracle is correctly excluded from correctnessPassRate.
Verdict: APPROVE
Phase 0 tracer + harness are ready to land for measurement. Pass-rate metrics can no longer be misread as correctness for the 24 no-oracle tasks. Thank you for the thorough follow-ups.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/perfbench/turn_bench.go (2)
530-546: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
cmd.Run()error is discarded, losing the real failure reason on process-launch failures.If
Start()fails (binary missing, exec permission denied, etc.), nothing is written toerrBuf, so the reported error collapses to the generic"missing terminal run_end event"instead of the actual OS error.Proposed fix
} else if !haveExit { detail := strings.TrimSpace(errBuf.String()) - if detail == "" { + if detail == "" && runErr != nil { + detail = runErr.Error() + } + if detail == "" { detail = "missing terminal run_end event" } outcome.Err = fmt.Errorf("zero exec failed: %s", detail) return outcome }🤖 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/perfbench/turn_bench.go` around lines 530 - 546, Update the command execution handling around cmd.Run() to preserve and report its returned error when process launch or execution fails. Before falling back to the errBuf-derived “missing terminal run_end event” detail in the !haveExit branch, use runErr as the failure detail when it is non-nil, while retaining existing stderr and terminal-event handling for cases where cmd.Run() succeeds.
245-248: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEmit a warning for
outcome.Err
outcome.Erronly clearspassedForTaskand skips the rest of the sample. On latency-only tasks that leaves no warning at all, so a crashed run can still look like a normal benchmark with just fewer measurements. Add aWarninghere, mirroring theTraceIssuepath.🤖 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/perfbench/turn_bench.go` around lines 245 - 248, Update the outcome.Err branch in the benchmark sample processing flow to emit a warning before marking passedForTask false and continuing. Mirror the warning behavior and context used by the nearby TraceIssue path, while preserving the existing control flow.
🤖 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 137-138: Align JSON serialization for the pass-rate fields in the
relevant result struct by removing omitempty from BuildPassRate, matching
CorrectnessPassRate so an explicit 0.0 is preserved. Keep both fields serialized
consistently, including when their rates are zero.
---
Outside diff comments:
In `@internal/perfbench/turn_bench.go`:
- Around line 530-546: Update the command execution handling around cmd.Run() to
preserve and report its returned error when process launch or execution fails.
Before falling back to the errBuf-derived “missing terminal run_end event”
detail in the !haveExit branch, use runErr as the failure detail when it is
non-nil, while retaining existing stderr and terminal-event handling for cases
where cmd.Run() succeeds.
- Around line 245-248: Update the outcome.Err branch in the benchmark sample
processing flow to emit a warning before marking passedForTask false and
continuing. Mirror the warning behavior and context used by the nearby
TraceIssue path, while preserving the existing control flow.
🪄 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: 6f80d499-a4a0-4332-a0c1-cfdc78b52ff4
📒 Files selected for processing (8)
internal/agent/loop.gointernal/agent/loop_test.gointernal/perfbench/MANIFEST.mdinternal/perfbench/manifests/baseline.jsoninternal/perfbench/reports/README.mdinternal/perfbench/taskbench.gointernal/perfbench/turn_bench.gointernal/perfbench/turn_bench_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/agent/loop_test.go
- internal/perfbench/reports/README.md
- internal/perfbench/manifests/baseline.json
- internal/perfbench/turn_bench_test.go
- internal/perfbench/taskbench.go
- internal/agent/loop.go
Three findings from CodeRabbit's review of the 3-tier pass-rate fix: - BuildPassRate dropped its omitempty so an explicit 0.0 (all refactor tasks fail to compile) is no longer indistinguishable from 'no build tasks ran' in the JSON. Matches CorrectnessPassRate's always-serialize behavior, which is the whole point of the tier split: the report cannot be misread. - A crashed sample (outcome.Err) now emits a Warning mirroring the TraceIssue path, so a run that died every iteration can't look like a normal benchmark with fewer measurements instead of a failure. - cmd.Run()'s returned error is preserved on the no-run_end path: when stderr is empty and Run failed (binary missing, exec permission denied), the actual OS error is reported instead of collapsing to the generic 'missing terminal run_end event'. go build/vet/gofmt clean; go test -race green for perfbench/agent/trace.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-review at 2bfb340
Three new commits since my last review: 0f372f38 (wired spans + fix-suite compile), 40806aa4 (exclusive-time round-trip, nesting, empty-trace parse), and 2bfb3406 (split pass/fail into oracle tiers). The headline concern from the prior review — that the pass rate could be misread as a blanket correctness verdict — is now addressed structurally. The v2 schema separates correctnessPassRate (edit/fix with positive oracles), buildPassRate (refactor with go build oracle), and latencyOnlyTasks (nav/longproc/longctx/parallel with no oracle), and the per-class ClassSummary carries Verified and LatencyOnly counts so consumers can see which tasks are excluded.
The tracer also got the structural fixes it needed: strict-parse on the trace NDJSON file (parse.go:14-121 — empty input is now an error, not a silent empty trace), exclusive-time attribution derived at Finish (recorder.go:224-285, strict containment to break the equal-interval 2-cycle), and the span vocabulary is realistic now (tool_partition instead of the misnamed prompt_build).
What still needs work before this lands
-
CorrectnessPassRatedenominator is correct, but the "passed" semantics across tiers are not equal.correctnessPassRate = tasksPassed / tasksVerified(correctness tier) andbuildPassRate = buildPassedTasks / buildCheckedTasks(build tier) are both denominators, but a consumer who doesn't read the schema doc and reports "Zero's pass rate is X" will conflate them. Two options:- (a) Require consumers to pick a tier explicitly and refuse to provide a "headline" pass rate. The JSON already does this — it just needs the report README to be louder about it.
- (b) Ship a single
passRatefield that is undefined when multiple tiers are present, and apassRateTierfield naming which one. Then consumers can't accidentally average.
I'd take (a) but I won't block on this — the schema is already correct. The point is the README must say loudly "do not average these, do not report a single number, the schema forces you to pick a tier."
-
BuildOnlyClassesis configured per-manifest, but the runner's classification also depends on whether an individual task has averificationCommand. The current rule (turn_bench.go:202-203) is "the latency-only tier is always driven by oracle presence, never by the declared list, so a declared build-only class with a missing oracle still doesn't pollute the build rate." That's the right rule. Document it inTurnBenchConfigso manifest authors don't have to read the runner to know the precedence. Two-line doc comment. -
TopLatencyranking still says "by share of total exclusive span time" (turn_bench.go:78-82), but the JSON field is namedLatencySource. Both are fine, but the field nametopLatencyis the public surface. Consider renamingSharetoexclusiveShareto make explicit that the share is computed on exclusive time, not raw sum. One line. -
Refactor verifier is still
go build ./.... I called this out last time and it stands. A no-op refactor passes. Two options:- (a) Document the refactor class as "structure-uncorrected" in
BuildOnlyClassesand labelbuildPassRateaccordingly. The class is build-checked, not correctness-checked. - (b) Add structural verifiers per refactor task. More work.
I'd take (a). The schema already has the machinery —
buildPassRateis a build rate, not a correctness rate, and the docstring should make that explicit. The plan-level concern is that "improving refactor success" is a goal that requires actual verifiers, and right now the metric cannot move because no refactor fails thego buildcheck. - (a) Document the refactor class as "structure-uncorrected" in
-
No regression test for "re-running the same mutating task does not pollute the fixture for the next run." This was raised last time and addressed in code, but I do not see a test asserting it. Add a
turn_bench_test.gocase that runs the sameedit-01twice and asserts the second run's fixture directory state matches the first's at the byte level. The fix is verified by reading the code today; a test makes it durable. -
The PR description still says "42-task manifest." The manifest has 48. Either update the description or drop 6 tasks. Trivial.
What I would accept
If the team addresses items 1, 2, and 6 in this PR (all are doc/README + one schema rename), and items 3, 4, 5 are tracked as follow-up issues with clear owners, I would approve. The tracer, the harness, the v2 oracle schema, and the exclusive-time math are all in good shape. The remaining issues are documentation, the build-only metric's known weakness, and a missing test.
If items 1, 2, 6 are not addressed in this PR, I would still want one of:
- a follow-up issue filed before the M1 baseline is taken, so the first published
baseline.jsonis read correctly by downstream consumers, or - the report README updated to make the tier separation loud enough that a careless reader can't misread it.
The M1 gate is "≥30 benchmark tasks run repeatably + top-3 controllable latency sources named + a baseline report published." The schema is what makes the report publishable. A reader who averages correctnessPassRate and buildPassRate is going to report a number that means nothing; the README has to forbid that explicitly.
Plan-level
The plan's 16-span taxonomy is now reflected in the code's OptionalEventKeys: provider_connect, provider_queue, generation, tool_execution, permission_wait, compaction, verification (plus the renamed tool_partition). The RequiredEventKeys keeps the always-present ones (tool_partition, generation, provider_connect + counters) so a short healthy trace doesn't false-fail. process_wait, tool_planning, tool_queue from the plan are not in the code — they are out of scope for Phase 0. The PR description under "Notes / deviations" should mention this so a follow-up PR knows it owns those spans.
Verdict
Request changes. Land when items 1, 2, 6 are addressed in this PR (or in a follow-up issue filed before the M1 gate, with the report README loud about the tier separation). Items 3, 4, 5 are not blockers but should be tracked.
The work is real. The headline concern from the prior review is closed. The remaining items are about the durability of the contract for downstream consumers, not about whether the tracer works.
|
uhh cmon guys |
…on test Re-review at d061524 asked for the pass-rate contract to be loud to downstream consumers and for the fixture-isolation property to be asserted, not just read from code. - reports/README.md gains a prominent 'do not average the tier pass rates / do not report a single pass rate' warning. The schema offers no headline pass rate on purpose; a consumer must name the tier it is reporting. This is the durability fix for the misread the tier split exists to prevent. - turn_bench_test.go gains TestCopyFixtureIsolatesSourceFromMutation: copies a fixture, mutates the copy, and asserts the source is byte-identical (and that a file added to the copy never appears in the source). Proves the 'fixtures stay clean across runs' property without needing the binary. Items the re-review also raised that were already satisfied: the oracle-presence-over-declared-list precedence is documented on the BuildOnlyClasses field (taskbench.go), and the PR body already says '48-task manifest'. The Share->exclusiveShare rename is tracked in #701. go build/vet/gofmt clean; go test -race green for perfbench.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-review at c4de52a
Two commits since 2bfb3406:
d0615249—BuildPassRatelost itsomitempty(the field is always present, so consumers don't confuse absence with zero), and a crashed run now appendsWarning{Metric: "run", Message: ...}instead of silently dropping.runErris no longer discarded —binary missing/exec permission deniednow surface as the real reason. Good.c4de52a5— the per-tier README is in (internal/perfbench/reports/README.md, "Pass/fail is reported per oracle tier" + a> Do not average the tier pass rates. Do not report a single "pass rate"callout that names what the schema is not doing on purpose), and a regression test for the fixture-isolation contract is atturn_bench_test.go::TestCopyFixtureIsolatesSourceFromMutation(creates a source withmain.go+sub/a.go, copies it, mutates the copy, asserts the source is byte-identical).
The three items I asked to be addressed in this PR are closed:
- Anti-averaging README. Done, and louder than I asked for: the README explicitly says the schema offers no headline pass rate on purpose, and names the three tiers with the field names a consumer must quote. Consumers who try to combine the two rates now have to ignore a documented warning.
BuildOnlyClassesprecedence doc comment. Done, inturn_bench.gonext to the manifest-declaration loop: "the latency-only tier is always driven by oracle presence, never by the declared list, so a declared build-only class with a missing oracle still doesn't pollute the build rate." Right rule, in the right place.- PR body "42-task" claim. Done, body now says "48-task manifest" with the tier breakdown and a pointer to
MANIFEST.md.
Plus the regression test for fixture isolation (item 5) — bonus, since I marked it as a nice-to-have. The test asserts byte-identical source after a copy mutation, which is exactly the contract a future refactor of the runner's isolation path would have to break to fail the test. Good hygiene.
What I would still track as follow-up
These were the non-blocking items from the prior review. None of them are dealbreakers for the M1 gate, but they are real and the team should know.
Sharefield name inLatencySource. It's an exclusive share, computed at Finish by subtracting nested children. Rename toExclusiveShareso a downstream consumer can't misread it as "raw sum share." The semantics changed (raw sum was wrong), but the field name didn't. Two lines.- Refactor class as "structure-uncorrected". The refactor oracle is
go build ./...— it catches compile failures but not no-op refactors. The schema already separatescorrectnessPassRatefrombuildPassRate, but a reader who seesbuildPassRate = 1.0may still think the refactors are correct. I'd add one sentence toMANIFEST.mdnext to the refactor class: "Refactor passes are build-only; the oracle does not verify the refactor achieved its goal. AbuildPassRateof 1.0 means the refactored code compiles." One sentence. - Plan-level: 3 of the 16 spans in the program spec are intentionally out of scope for Phase 0.
process_wait,tool_planning,tool_queueare not in the code. TheRequiredEventKeysandOptionalEventKeyslists are the implicit phase-0 contract, and they're correct (always-on spans are required, conditional ones are optional). A follow-up PR that wants those spans will need to know the contract this PR established. Worth a one-line "out of scope for Phase 0" note in theNotes / deviationssection of the PR description so the next PR doesn't have to dig.
These are all doc-level fixes, no code changes. Tracked as a follow-up issue, they should not block this PR.
Verdict
Approve. The tracer, the harness, the v2 oracle schema, the exclusive-time math, the anti-averaging contract, and the fixture-isolation test are all in good shape. The remaining items are documentation, not engineering. The M1 gate ("≥30 benchmark tasks run repeatably + top-3 controllable latency sources named + a baseline report published") is achievable against this PR as it stands; the report will be read correctly by downstream consumers because the schema and the README force them to pick a tier.
Two things to confirm before merge:
go test -race ./internal/trace/... ./internal/perfbench/... ./internal/agent/...green on a clean checkout (the PR description says it is, but a re-run after the two new commits is cheap).- The actual M1 baseline run produces a
reports/baseline.jsonwhose tier class lists and the three pass rates match the manifest, end-to-end. That is the M1 evidence; nothing in this PR validates that, only the next run does.
Solid work.
|
Pushed Done in this commit
Already satisfied (the re-review didn't see the latest state)
Tracked as follow-up (non-blocking, per the re-review)
|
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
great work team! thank you so much
4 participants
What this does
Establishes the Phase 0 performance baseline: where does a turn actually spend time, recorded repeatably so later work is measurable instead of guessed.
It adds an opt-in per-turn tracer (
internal/trace) that attributes wall time to named spans — tool partition, provider connect, provider queue, generation, tool execution, permission wait, verification, compaction — plus counters for model requests, tool calls, retries, reconnects, compactions, completion nudges, acceptance checks, model switches, and input/cached/output tokens. The recorder is nil-safe, so every stamp site calls it unguarded and a run without tracing is byte-identical to today.Spans record wall intervals, not raw durations. At finish the recorder derives each span's parent by interval containment and its exclusive time (duration minus the union of its children), so a
provider_connectrunning concurrently insidegenerationor apermission_waitnested insidetool_executionno longer double-counts the same wall. Top-latency ranking uses exclusive time, so shares of the top sources sum to ~1; coverage (union of span intervals over wall, capped at 1) is the honest "≥95% of wall accounted for" metric.The recorder threads into
agent.Runthrough a newOptions.Tracefield and onetrace.WithContextcall, so the shared providerio seam (connect aroundclient.Do, queue around the OAuth resolve, first token at the first non-keepalive SSE payload) reaches it viatrace.FromContextwith no provider-interface change. Headlesszero execgets--trace <path>(orZERO_TRACE=<path>,-for stderr), emitting an NDJSON trace that matches the existing agenteval trace contract.A turn-benchmark harness extends
internal/perfbench(reusing its TaskSet / NumericStats / subprocess-runner shapes):RunTurnBenchaggregates per-span median/P95 across iterations, ranks the top three controllable latency sources by exclusive time, rolls up per task class, and totals tokens and counts. A checked-in 48-task manifest across seven classes (nav, edit, fix, refactor, longproc, longctx, parallel) and local offline fixtures back it;make baseline ZERO_BENCH_MODEL=<model>runs it against the builtzeroand writesinternal/perfbench/reports/baseline.json. Pass/fail is reported per oracle tier (seeinternal/perfbench/MANIFEST.md): 18 correctness tasks (edit grep + fixgo test), 6 build-only tasks (refactorgo build), and 24 latency-only tasks (nav/longproc/longctx/parallel, no oracle) — so a read-only exit 0 can never inflate a pass rate that reads as correctness.Notes / deviations
LoadTaskSetalready parses JSON.zero execprocess, so iterations are cold samples. A warm in-process path is left for follow-up — called out in the code and the reports README.go test -run <name>so the eight bug fixes are independent (fixing one never depends on another). Each bug is a runtime defect that still type-checks and vets clean, so the package compiles in both the buggy and fixed state.reports/keeps a README +.gitkeepand regenerates onmake baseline. The manifest and fixtures are the durable artifacts.process_wait(subprocess collect latency),tool_planning(tool-selection think time), andtool_queue(parallel-read semaphore wait). The seam is in place (Options.Trace,trace.FromContext); wiring these three is follow-up work for whichever PR owns them.provider_queueis wired as the pre-send OAuth/auth resolve, since no send-side semaphore exists today.Verification
go build ./...,go vet ./..., andgofmt -lare clean on the touched files.go test -raceis green forinternal/trace,internal/agent,internal/perfbench, andcmd/zero-perf-bench. The trace tests cover span accumulation, concurrency, nil-recorder no-op, exclusive-time subtraction for nested spans, coverage capping, and NDJSON round-trip through the agenteval contract (plus hard-fail on corrupt input with no trace header); the harness tests use a fake runner to verify aggregation, top-three ranking, per-class roll-ups, and JSON shape, plus a manifest-load test asserting the seven required classes and the ≥30-task gate.Summary by CodeRabbit
zero execvia--trace <path>orZERO_TRACE, producing NDJSON snapshots (or stderr when-).zero-perf-bench turnandmake baselineto run the per-turn benchmark suite with dry-run and JSON-only options.