mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
fix/opencode-session-header
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Vasanth T
|
727ad4d321 |
perf(turn-bench): Phase 0 — strengthen oracles so pass rate can't be misread as correctness (#712)
* perf(turn-bench): Phase 0 — strengthen oracles so pass rate can't be misread as correctness
The baseline manifest's pass/fail contract had three holes that let a weak
or no-op agent read as a pass:
- edit grep oracles were substring checks: a reworded line (e.g.
fmt.Println("debug: starting") -> fmt.Println("starting up")) passed
`! grep 'debug: starting'`.
- refactor lived in the build tier with a non-positive `go build ./...`,
so a no-op refactor passed.
- the 24 read-only tasks (nav/longproc/longctx/parallel) had no oracle;
a zero-exit only proved the turn ran.
This makes the tiers honest:
- edit/refactor now carry a stamped oracle_test.go compiled by
`go test ./...` — the Go compiler is the structural verifier, so a
no-op refactor, a missing field, or a reworded-but-not-removed line
fails to compile/run. The test is stamped into the fixture copy AFTER
the agent run, so it can't interfere with the agent's own build or be
pre-seen.
- nav gains an answer-oracle: the harness captures the agent's final
answer (.zero-answer.txt) and greps it for determinable facts. Count
tasks (nav-04/05/08) use a structured `count: N` token so the oracle is
tight rather than a loose prose-contains on "0".
- refactor graduates from build to correctness (its oracle is now
positive); nav graduates from latency-only to correctness. The build
tier is empty (buildOnlyClasses: []).
- longproc/longctx/parallel stay permanently latency-only — there is no
deterministic oracle for "report whether the build succeeds" or
"summarize a large file"; documented in MANIFEST.md.
Tier counts: correctness 34 (edit 10 + fix 8 + nav 10 + refactor 6),
build 0, latency 14. Total 48 unchanged.
TurnSchemaVersion 2 -> 3: the result shape is unchanged (only class-list
membership shifts) but correctnessPassRate now means something strictly
stronger, so the bump prevents a v2->v2 cross-version comparison from
misreading the jump as a model improvement.
Prerequisite fix: the mutating fixtures had no go.mod, so `go test ./...`
in the temp copy failed with "cannot find main module". Added go.mod per
fixture (edit/fix/refactor/nav) and moved copyFixture's temp dir one level
below the system temp root — Go ignores go.mod in direct children of the
temp dir (a hijack guard), which would have broken every compiler-backed
oracle. This also fixes the pre-existing fix/refactor oracles, which were
silently broken because the test suite used fake runners that never ran
go test.
Tests: stub-binary gating tests prove each oracle fails the wrong thing
(no-op refactor, missing field, wrong nav answer, reworded debug print) and
passes the right thing (real refactor, added field, correct answer); an
edit-01 test proves the scoped negative grep doesn't false-fail a correct
rename that leaves the doc comment. Aggregation, schema, and unit tests
cover the v3 tier model and the answer-capture helper.
* Address CodeRabbit review on PR #712: tighten oracles + isolation
- nav count oracles (nav-01/04/05/08): anchored line-exact
`^count: N$` with the fixture's exact expected count, so a substring
like "the count: 0" or a wrong count like "count: 3" fails instead
of rubber-stamping. nav-01 added to the structured-count contract.
- stampOracleAndAnswer now errors when a task has an oracle or
verification but no workspace fixture, so verification can't run in
the caller's cwd with the oracle never compiled (false-pass closed).
- copyFixture uses a unique 0700 os.MkdirTemp parent under the temp
root and nests the copy beneath it (grandchild of temp root), so the
Go go.mod hijack guard is satisfied and cleanup removes the whole
parent. Avoids a predictable shared world-readable dir.
- Rejection tests require a non-empty VerifyErr via a shared
assertVerifyFailed helper, proving the failure came from
verification rather than a harness error or a silent non-pass.
- Unify streamJSONExitCode + streamJSONFinalText behind one
parseStreamJSON parser (behavior-preserving; scanner limits and
malformed-event handling now live in one place).
* Address gnanam1990 review on PR #712: close count=0 gameability + stronger oracles
- nav count tasks (nav-04/05/08) had a count=0 ground truth, so an agent
that never opened the workspace and always emitted "count: 0" passed
three correctness tasks -- the exact pass-rate-misread-as-correctness
failure this PR closes. nav-04 now has a real TestGreet (count 1) and
nav-05 a real TODO (count 1); nav-01's file count moves to 5. nav-08
stays at 0 (a real third-party import would need a network fetch and
break the offline suite) but its oracle now requires the answer to name
both stdlib imports (fmt from main.go, testing from main_test.go), so a
blind count:0 or a main.go-only read fails. nav-08's prompt now asks the
agent to enumerate and classify every import, so naming the stdlib
imports is part of a correct answer, not a false-fail.
- refactor-05: stamp a behavioral oracle (TestGreetWrapped) so deleting
both Wrapper and greetWrapped, or stubbing the caller to return "",
fails to compile/run. The negative grep alone passed that wrong edit.
- refactor-04: replace the brittle exact-text shell grep with a stamped
file-reading oracle using a spacing-tolerant regex, so a bare
map[string]int decl under any spacing/:= variant can't dodge it, and
the Stats type must be referenced in main.go.
- edit-05: also assert the "debug: starting" message text is absent, so
a respell onto fmt.Printf / backticks / log.Println / println that
still prints the message fails (the fmt.Println(" check missed those).
- v3 doc comments on ClassSummary / TurnBenchResult /
FormatTurnBenchSummary now describe the v3 tiers (correctness =
edit/fix/nav/refactor, build empty, latency = longproc/longctx/
parallel) instead of the stale v2 model.
- stampOracleAndAnswer failure now mutates the existing outcome instead
of returning a fresh one, so the already-parsed trace and trace issue
survive a stamp error (attribution is preserved, pass/fail unchanged).
- Schema comment provenance: "PR #701" -> neutral "Phase 0 oracle
hardening" (this PR is #712).
- New gating + satisfiable stub tests for refactor-04/05 and edit-05, and
count-oracle zero-guess / inspection-proof tests for nav-04/05/08.
* Fix macOS CI: copyFixture isolation test was slash-sensitive
TestCopyFixtureIsolatesSourceFromMutation compared filepath.Dir(parent)
against os.TempDir() verbatim. On macOS $TMPDIR ends in a trailing slash
("/var/folders/.../T/") while filepath.Dir strips it, so the equality
false-failed even though the parent is genuinely a direct child of the
temp root. Linux (/tmp, no slash) and Windows (no slash in this Go
build) matched by accident.
The production copyFixture nesting is correct on all platforms (the
grandchild copy sits one level below the temp root, satisfying Go's
go.mod hijack guard); only the assertion was slash-sensitive. Normalize
both sides with filepath.Clean.
* Close nav-04/05 blind-count-one gameability (anandh8x review)
nav-04 and nav-05 had a count-only oracle (anchored '^count:1$'). Once the
ground truth became 1, an agent that never inspects the fixture and always
emits 'count: 1' passed both — the blind-count-one surface the prior
non-zero-ground-truth fix opened. Pair the count with a workspace-derived
named fact that proves inspection, mirroring nav-08's fmt+testing:
- nav-04 now also requires 'TestGreet' (the one test function in
main_test.go) — a specific, non-generic name a blind agent can't produce
without reading main.go to learn a greet function exists. Fully closes
the blind-count-one surface.
- nav-05 now also requires 'main.go' (the file holding the one TODO).
main.go is the conventional entrypoint and the most guessable filename, so
this reduces rather than fully closes the surface — a blind 'count: 1, in
main.go' can still pass. Accepted for Phase 0: the blind floor drops below
100% (a bare 'count: 1' fails), so a model still moves the pass rate by
inspecting. MANIFEST.md documents this residual honestly.
Two gating tests per task: RejectsBlindCountOne (bare 'count: 1' now fails)
and, for nav-05, RejectsSubstring (main.go present but unanchored count
still fails — parity with nav-04). Existing accept tests still pass (their
answers already name TestGreet / main.go).
Doc nits also fixed: MANIFEST.md #701 -> #712; refactor-05 removed from the
'no oracleTest' inventory (it now has TestGreetWrapped); the nav-01 named-
fact prose corrected to the three files the oracle actually anchors on;
test comment #701 -> #712.
|
||
|
Vasanth T
|
abe24bcb10 |
Add per-turn tracing and a turn-benchmark harness (Phase 0 baseline) (#700)
* Add per-turn tracing and a turn-benchmark harness (Phase 0 baseline)
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.
* test(trace): make TestAttributionRatio clock-independent
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.
* Address review feedback on the per-turn tracer + benchmark harness
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.
* Address review feedback: exclusive-time attribution + wired spans + fix-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.
* Fix exclusive-time round-trip, identical-interval nesting, and empty-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.
* perf(turn-bench): split pass/fail into oracle tiers so the pass rate 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.
* Address CodeRabbit review on
|