2 Commits

Include renames

Author	SHA1	Message	Date
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 2bfb3406: omitempty + crash visibility 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. * Address CodeRabbit re-review: anti-averaging README + fixture-isolation test Re-review at d0615249 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.	2026-07-16 21:58:20 +08:00
KRATOS	ae4ce8aae2	wedge build: web_search + LSP client/diagnostics (stages 01-03) (#175) ... * stage 01: web_search tool Adds a web_search tool alongside web_fetch: discovers URLs via a pluggable searchBackend (env-configured generic JSON backend; swappable for Exa/You/ Tavily/Brave) and formats ranked results as a numbered list. Registered in CoreNetworkTools(). Deviation from the stage spec: the spec said readOnlySafety, but the repo enforces (TestCoreNetworkToolsExposePromptMetadata) that every core network tool is prompt-gated network egress — so web_search matches web_fetch's posture instead of being auto-allowed. Necessary fallout from adding the tool: knownToolNames (+web_search), the network-tools metadata test (now covers both), and a stale 'unknown tool' fixture that had used web_search. * stage 02: LSP client foundation New internal/lsp package: a direct (self-spawned, stdio) Language Server Protocol client so unattended runs see real compiler diagnostics without an open editor. - client.go: JSON-RPC 2.0 with LSP Content-Length framing; concurrent Call/Notify, id-matched responses, server->client request auto-reply, notification handler, context cancellation, read-loop teardown. - server.go: process lifecycle (spawn via os/exec, initialize/initialized handshake, graceful shutdown+exit with kill fallback). Handshake factored out (performInitialize) for pipe-based testing. - registry.go: extension -> server command (.go/.ts/.tsx/.js/.jsx/.py/.rs), languageId map, PATH availability via exec.LookPath. - types.go: minimal LSP types + PathToURI/URIToPath (Windows-drive aware). Stdlib-only; tested over in-memory pipes with a stub server (no real gopls), including -race for the concurrent id router. * stage 03: LSP document sync + diagnostics Makes the LSP client produce actionable signal so ZERO can ask 'did my edit compile?' headlessly. - documents.go: per-server session with document lifecycle (didOpen/didChange full-sync/didClose), a URI->diagnostics store fed by publishDiagnostics, and WaitForDiagnostics with a publish-baseline + 300ms quiet debounce (servers never signal 'analysis complete'). - diagnostics.go: FormatDiagnostics (path:line:col: severity: message, 1-based), HasErrors, FilterBySeverity, severity labels. - manager.go: Manager — one lazily-started, reused server per language (keyed by binary), routes a file to the right server, Check(ctx,path,text) -> settled diagnostics, graceful Shutdown; unknown extension -> (nil,nil), not an error. Concurrency-safe; tested under -race over in-memory pipes with a stub server (no real gopls). FormatDiagnostics takes the path explicitly since a Diagnostic carries only a range, not its document. * wedge: address review on #175 (LSP robustness + web_search provider) - lsp/client.go: Close() now disables the client — Call/Notify reject after close instead of leaking a pending entry / writing to a dead conn. - lsp/documents.go: serialize same-document syncs through the Notify write via a per-URI lock and commit version/open only on success; drop delayed publishDiagnostics for a superseded (older positive) version; WaitForDiagnostics now reports whether a fresh publish arrived. - lsp/manager.go: Check returns (nil,nil) when no publish newer than baseline arrives (don't return a stale prior result for new text); Shutdown joins and returns per-server shutdown errors instead of always reporting success. - lsp/diagnostics.go: FormatDiagnostics collapses embedded whitespace so each diagnostic stays one physical line. - tools/web_search.go: forward ZERO_WEBSEARCH_PROVIDER in the request so the config knob isn't inert. Adds tests: close-disables-client, stale-version drop, multi-line message collapse, Shutdown error propagation, httptest backend (provider+parse). * wedge: degrade gracefully when an LSP server binary is missing (#175 review) Manager.Check translated a missing-binary start error (exec.ErrNotFound) into (nil,nil) so a configured extension on a machine without gopls/ pyright/rust-analyzer degrades exactly like an unsupported extension — LSP diagnostics are an opportunistic layer, not a hard dependency. Other start failures still surface. Adds a test injecting an exec.ErrNotFound starter. * wedge stage 04: self-correct loop (#176) * stage 04: self-correct loop After a successful mutating tool call, verify the changed files (LSP diagnostics + project tests) and feed failures back to the model to fix — bounded and autonomy-gated, so an unattended run fixes its own mistakes without looping forever. - internal/agent/selfcorrect.go: SelfCorrector with injectable diagnosticsChecker (lsp.Manager adapter) and projectVerifier (verify.DetectPlan+Run); AfterEdit produces a redacted, model-facing corrective message; per-run attempt ceiling; low autonomy reports without auto-attempting; medium/high auto-correct. - internal/agent/loop.go: invoke AfterEdit after a successful mutating tool call (ChangedFiles non-empty), appending feedback to messages. nil SelfCorrect (the default) is a no-op — read-only tools never trigger it. - internal/agent/types.go: add Options.SelfCorrect *SelfCorrector (the explicit options-struct change). Single-pass verify per call (the agent loop + model edits provide the multi- attempt cycle, not selfverify's same-command re-runs). Tested with fakes (no gopls/shell): fail-then-pass, max-attempts abort, read-only skip, low-autonomy report-only, LSP-only failure, secret redaction; -race clean. CLI activation (a --self-correct flag building the SelfCorrector in exec.go) is intentionally left out to honor this stage's file scope — the engine is exported and ready to wire. * wedge: propagate file-read error in self-correct LSP checker (#176 review) CodeRabbit/golangci-lint (nilerr) flagged lspDiagnosticsChecker.Check returning (nil, nil) after os.ReadFile fails. Propagate the error instead; the sole caller (inspect) already skips files whose Check errors, so a deleted/unreadable file still degrades quietly while future callers can decide. Adds a regression test. * wedge: surface non-degradable verify errors in self-correct (#175 review) SelfCorrector.inspect silently continued on every checker.Check error and ignored verifier errors, so a real LSP startup/sync failure, an unreadable changed file, or a failed plan detection produced report.Failed=false -> OutcomePassed with no signal. Manager.Check already degrades missing servers to (nil,nil) and DetectPlan returns an empty plan (not an error) when nothing is detected, so a non-nil error from either is a genuine failure. Record such errors in CorrectionReport.InspectErrors, fail the pass, and surface them in the model feedback. Adds regression tests for both paths. * wedge: drop third-party search-provider names from web_search comments Keep code/comments provider-neutral: describe the backend interface as 'any hosted search API' rather than naming specific services. * wedge: defer self-correct feedback until after the tool-call batch (#175 review) Self-correct feedback was appended as a user message inside the per-tool-call loop, so a multi-tool-call turn whose first (mutating) call failed verification produced: assistant tool_use batch -> tool_result 1 -> user feedback -> tool_result 2. A user message interleaved between tool_results breaks strict provider replay and later session replay/compaction. Accumulate feedback during the batch and append it once after the loop (same after-batch pattern already used for mid-run model escalation), so every advertised tool call keeps its tool_result contiguous. Adds a two-tool-call regression test asserting the feedback lands after both tool_results. * wedge: batch self-correct AfterEdit once per turn over union of changes (#175 review) Deferring only the feedback emission still ran SelfCorrect.AfterEdit once per mutating tool call, which (a) consumes the per-run attempt budget multiple times in a single turn and (b) verifies an intermediate edit that a later call in the same batch already superseded — AfterEdit is documented as one per-run instance holding attempt state, so this is a correctness issue, not just transcript shape. Aggregate every mutating tool's ChangedFiles during the batch and call AfterEdit once afterward over the deduped union, appending its feedback once (still after all tool_results, preserving contiguity). Adds dedupeStrings and a regression test asserting two writes in one turn consume exactly one attempt and emit one feedback message.	2026-06-12 15:13:05 +05:30