fix(analyst): normalize model citations before the evidence gate - #107
fix(analyst): normalize model citations before the evidence gate#107drewstone wants to merge 1 commit into
Conversation
agent-eval's evidence gate rejected the built-in analysts' own findings on Codex traces: excerpts copied from searchTrace hits keep their JSON escapes, citations of readable harness IDs name no stored span, and a subject from another kind's vocabulary fails the kind check. Wrap the built-in kinds in analyzeSpans with a postProcess hook built over the analyzed spans. It rewrites a citation only to a form proven present in the span it cites: a readable ID must name exactly one span, and a decoded excerpt must appear in that span's attributes or status message. An out-of-kind optional subject is omitted. The gate itself is unchanged and re-checks every rewritten row, so fabricated, unknown, and ambiguous citations stay rejected, and normalization never adds a citation. Wrapped definitions carry a +traces-citations-1 version suffix because agent-eval binds a postProcess hook to the definition version. The module is temporary until the agent-eval gate issue lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drewstone
left a comment
There was a problem hiding this comment.
Adversarial review — no blocking findings
Reviewed 3b2125c in a clean worktree off origin/fix/normalize-analyst-citations (origin/main is an ancestor; the branch merges cleanly).
The change is scoped to the five files in the diff: it adds src/analyst-citations.ts, one export line, a docs section, and one wiring line in analyzeSpans.
No adapter, analyzer, report format, evidence-JSONL, or exit-code path is touched.
Checks I ran myself
pnpm install --frozen-lockfile, then check:source, typecheck, test, build, check:package — all pass (58 files, 822 tests), matching the five steps in .github/workflows/ci.yml.
gh pr checks is green on Node 22.13.0 and 24.18.0.
The safety argument holds
I tried to make normalization admit absent evidence and could not.
postProcessruns before the gate, and agent-eval re-parses the row (parseRawFinding), re-checks the subject, re-counts distinct URIs, and re-resolves every citation against the store (kind-factory.tsacceptFindings/evidenceIsResolvable). Nothing the hook writes is trusted.resolveCitationreturns a span only when the match set has exactly one member, so an ambiguous readable ID stays unresolved.normalizeExcerptrewrites only to textcontainsTextalready found in the cited span, mirroring the gate'scontainsExactTextwalk (string values at any depth, keys excluded, same depth bound).parseCitationreturns null for anything carrying a non-trace://scheme, sofinding://and artifact citations are passed through untouched.- Evidence entries are rebuilt as
{uri}/{uri, excerpt}, which is the whole of the strictRawAnalystEvidenceSchema— no field is dropped. - The store the gate reads is
writeOtlpFile(spans, …)from the same array handed tonormalizeAnalystCitations, so the code comment onanalyze.ts:139is accurate. definitions: DEFAULT_TRACE_ANALYST_KINDSis exactly whatbuildDefaultAnalystRegistryuses whendefinitionsis omitted, and an emptyagenticKindsarray still registers nothing, so the routed and unrouted paths keep their previous composition.computeFindingIdhashesanalyst_id/area/subject/claimand not the version, so the+traces-citations-1suffix does not move finding IDs.- The readable-ID index depends on
traces.<harness>.source_trace_id+source_span_id, whichnormalizeCodexIdsandnormalizeClaudeIdsset on every span, so the rewrite works on real traces rather than only on the fixture. Adapters that do not rewrite IDs (amp, gemini, opencode, copilot, qwen, forge, pi) simply get no aliases; the excerpt path is guarded, so they are unaffected.
Mutation checks
- Hook body replaced with a pass-through: 9 of the 20 new tests fail, matching the PR body.
matches.size !== 1weakened to>= 1: the two-trace ambiguity test fails. Good.normalizeExcerptreduced to an unconditionaldecodeJsonEscapes(excerpt): all 20 tests still pass. See finding 1.
Fixtures and privacy
The fixture is a synthetic inline JSONL rollout in the style of tests/codex-tool-status.test.ts (/fixture, citation-session, pnpm lint, fixture.lock). I grepped the whole diff for home paths, session UUIDs, and bench/map paths and found none. Nothing in the code, tests, commit message, or PR text carries session content.
Non-blocking findings, ranked
1. The one branch that protects correct citations is untested.
normalizeExcerpt's first line — if (containsText(span.content, excerpt)) return excerpt — is what stops a valid excerpt containing a literal backslash sequence from being mangled. Deleting it keeps all 20 tests green, and the case it guards is common: a Codex function_call.arguments attribute literally contains \".
I reproduced it on a synthetic fixture whose command is {"cmd":"echo \"stale\" >> fixture.lock"}. Citing the attribute verbatim with excerpt = 'echo \\"stale\\" >> fixture.lock':
- with the guard,
postProcessreturns the excerpt unchanged (accepted); - with the guard removed, it becomes
echo "stale" >> fixture.lock, which the span does not contain, and the gate rejects the finding.
Please add that shape as a sixth stillAccepted-style case. It is the difference between "normalization is safe" and "normalization does not break honest citations", and only the first is currently covered.
2. agent-eval#TBD ships in the module header.
No matching issue exists in tangle-network/agent-eval yet (I listed the open issues; #713 is the TypeScript-engine issue, not the gate one). Fill the number before merge, or land a follow-up commit the same day — a public file that says #TBD gives a future reader no way to find the deletion trigger, and this module's whole contract is "delete me when that lands".
3. Dropping an out-of-kind subject is silent, and for failure-mode the subject is the cluster key.
KIND_EXPECTED_SUBJECTS['failure-mode'] is ['cluster'], and computeFindingId hashes the subject, so a normalized finding loses its cluster identity and its **Subject:** line in report.ts:593. That is still strictly better than the finding being discarded, and the schema prompt does say "omit rather than guess", so I would keep the behavior — but the report and the JSON should say how many findings had a subject removed and how many citations were rewritten. Today the only record is context.log('finding citation normalized: …'), and analystLog (cli.ts:1158) prints the bare message with none of the from/to/subject fields, so an operator cannot tell that traces edited the model's citations. This is adjacent to brief item T2; a count in the Detail cell would close both.
4. The normalizer reads untruncated attributes; the gate reads viewSpans, capped at perAttributeSpanBudget (16384).
Mostly this only costs a missed rewrite. There is one narrow regression shape: with --source-bundle, an escaped excerpt quoted from a source window is accepted today via sourceWindows; if its decoded form lives past the 16 KiB view cut, normalization rewrites it and the gate then rejects it. TOOL_IO_VALUE_MAX_BYTES is also 16 KiB, so only the truncation marker normally sits past the cut and I could not construct a real case. Worth one line in the module comment, or a cheap belt-and-braces check that the decoded text falls inside the same budget.
5. runTraceStoreInvestigation gets no normalization, and the docs do not say so.
The PR body discloses it, but docs/trace-analysts.md reads as if every built-in --llm analyst is covered. It is unreachable from the CLI and untested, so this is SDK-surface only — one sentence in the docs section is enough.
6. Nit: not guarded against double wrapping.
normalizeAnalystCitations(normalizeAnalystCitations(kinds, spans), spans) — which an SDK caller passing wrapped definitions as agenticKinds would produce — yields …+traces-citations-1+traces-citations-1 and runs the hook twice. The hook is idempotent, so only the version string is wrong; skipping a definition whose version already ends in the suffix would be one line.
Approving in substance: the failure class is real, the fix is the right size, the invariant is genuinely protected, and the tests exercise the gate rather than a mock of it. Finding 1 is the one I would land before merge; finding 2 must be closed before or immediately after.
Failure class
Design brief F6 (item T3): agent-eval's evidence gate rejects the built-in
--llmanalysts' own findings on Codex traces.In the recorded run, all 5 submitted findings were rejected: 4 as "unresolved evidence" and 1 as "subject is not valid for analyst".
Three citation forms cause this, and none of them means the evidence is absent:
searchTracereturns raw OTLP-JSONL text, so an excerpt copied from a hit keeps JSON escapes (\",\n) that the decoded attribute does not contain.tool:<call_id>, the session ID) to hex OTLP IDs and keep the readable form only intraces.<harness>.source_*attributes, so a citation of the readable ID names no stored span.tool-doc:exec_commandon the cluster-only failure-mode kind) fails the kind check and discards the whole finding.Change
src/analyst-citations.ts:normalizeAnalystCitations(definitions, spans)adds apostProcesshook built over the analyzed spans.analyzeSpanswraps the built-in kinds (the routed subset or the full default set), soanalyze --llm,investigate, andimproveall get it.A caller-supplied
agenticRegistryis left alone.<kind version>+traces-citations-1, because agent-eval records apostProcesshook asversion-bound.createTraceAnalyst, and documented indocs/trace-analysts.md.The gate itself is unchanged and re-checks every rewritten row.
Normalization never adds a citation, so it cannot satisfy a kind's minimum citation count.
Tests
tests/analyst-citations.test.tsuses a synthetic inline Codex rollout and a stub engine.The engine calls the real trace tools it receives and builds each citation from their results.
The escaped excerpt is read from a real
searchTracehit, and the readable IDs come fromviewSpans.Five real-evidence shapes: each test runs the unwrapped kind (rejected, with the exact gate reason) and the wrapped kind (accepted, citations rewritten to canonical hex URIs):
tool:<call_id>)Still rejected after normalization:
postProcessruns.One more test covers a readable ID that names spans in two traces: it stays unresolved, while the trace-scoped form resolves.
An integration test runs
runTraceInvestigationand checks that the built-in failure-mode kind accepts the rewritten finding.A mutation check that disabled the hook failed 9 of the 20 new tests, and a check that accepted ambiguous IDs failed the ambiguity test.
pnpm check:source,pnpm typecheck,pnpm test(58 files, 822 tests),pnpm build, andpnpm check:packagepass locally.Needs agent-eval
The module comment names the agent-eval gate issue (agent-eval#TBD); delete this module once traces adopts a release that includes it.
That issue should cover:
Two rejection shapes cannot be fixed from traces:
parseRawFindingrejects the row beforepostProcessruns.On the DSPy path, the Python
finding_codec.pydrops the row asinvalid-subjectbefore it reaches TypeScript.runTraceStoreInvestigation) have no in-memory spans.postProcessis synchronous and receives no store, so a store-backed normalizer needs an agent-eval hook change.🤖 Generated with Claude Code