Skip to content

fix(analyst): normalize model citations before the evidence gate - #107

Open
drewstone wants to merge 1 commit into
mainfrom
fix/normalize-analyst-citations
Open

fix(analyst): normalize model citations before the evidence gate#107
drewstone wants to merge 1 commit into
mainfrom
fix/normalize-analyst-citations

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Failure class

Design brief F6 (item T3): agent-eval's evidence gate rejects the built-in --llm analysts' 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:

  • searchTrace returns raw OTLP-JSONL text, so an excerpt copied from a hit keeps JSON escapes (\", \n) that the decoded attribute does not contain.
  • The adapters rewrite readable harness IDs (tool:<call_id>, the session ID) to hex OTLP IDs and keep the readable form only in traces.<harness>.source_* attributes, so a citation of the readable ID names no stored span.
  • A subject from another kind's vocabulary (for example tool-doc:exec_command on the cluster-only failure-mode kind) fails the kind check and discards the whole finding.

Change

  • New src/analyst-citations.ts: normalizeAnalystCitations(definitions, spans) adds 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; an ambiguous or unknown ID is left untouched.
    • A decoded excerpt must be a substring of the cited span's attributes or status message.
    • An out-of-kind subject is omitted, because the subject is optional and the schema tells the model to omit it rather than guess.
  • analyzeSpans wraps the built-in kinds (the routed subset or the full default set), so analyze --llm, investigate, and improve all get it.
    A caller-supplied agenticRegistry is left alone.
  • Wrapped definitions report version <kind version>+traces-citations-1, because agent-eval records a postProcess hook as version-bound.
  • The function is exported for custom analysts built with createTraceAnalyst, and documented in docs/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.ts uses 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 searchTrace hit, and the readable IDs come from viewSpans.

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):

Shape Unwrapped gate reason
Escaped excerpt copied from a search hit excerpt is not present in the cited span content
Readable span ID in the hex trace trace span does not exist
Readable trace and span IDs trace span does not exist
Bare readable ID (tool:<call_id>) citation is not a supplied finding or trace span
Subject from another kind subject is not valid for analyst

Still rejected after normalization:

  • a fabricated escaped excerpt;
  • a real excerpt cited on a span that does not contain it;
  • a fabricated readable ID;
  • a single citation on failure-mode, which needs two spans;
  • a readable alias of a span that is already cited, which collapses to one citation;
  • a subject outside the grammar, which the schema rejects before postProcess runs.

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 runTraceInvestigation and 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, and pnpm check:package pass 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:

  • accepting JSON-escaped excerpts in the gate (brief A2);
  • resolving readable source-ID aliases, or letting a store supply them;
  • treating an out-of-kind optional subject as omitted.

Two rejection shapes cannot be fixed from traces:

  • Grammar-invalid subject (for example uppercase or spaces): parseRawFinding rejects the row before postProcess runs.
    On the DSPy path, the Python finding_codec.py drops the row as invalid-subject before it reaches TypeScript.
  • Store-only investigations (runTraceStoreInvestigation) have no in-memory spans.
    postProcess is synchronous and receives no store, so a store-backed normalizer needs an agent-eval hook change.

🤖 Generated with Claude Code

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 drewstone left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  • postProcess runs 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.ts acceptFindings/evidenceIsResolvable). Nothing the hook writes is trusted.
  • resolveCitation returns a span only when the match set has exactly one member, so an ambiguous readable ID stays unresolved.
  • normalizeExcerpt rewrites only to text containsText already found in the cited span, mirroring the gate's containsExactText walk (string values at any depth, keys excluded, same depth bound).
  • parseCitation returns null for anything carrying a non-trace:// scheme, so finding:// and artifact citations are passed through untouched.
  • Evidence entries are rebuilt as {uri} / {uri, excerpt}, which is the whole of the strict RawAnalystEvidenceSchema — no field is dropped.
  • The store the gate reads is writeOtlpFile(spans, …) from the same array handed to normalizeAnalystCitations, so the code comment on analyze.ts:139 is accurate.
  • definitions: DEFAULT_TRACE_ANALYST_KINDS is exactly what buildDefaultAnalystRegistry uses when definitions is omitted, and an empty agenticKinds array still registers nothing, so the routed and unrouted paths keep their previous composition.
  • computeFindingId hashes analyst_id/area/subject/claim and not the version, so the +traces-citations-1 suffix does not move finding IDs.
  • The readable-ID index depends on traces.<harness>.source_trace_id + source_span_id, which normalizeCodexIds and normalizeClaudeIds set 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 !== 1 weakened to >= 1: the two-trace ambiguity test fails. Good.
  • normalizeExcerpt reduced to an unconditional decodeJsonEscapes(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, postProcess returns 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant