Skip to content

feat(judges): add reference equivalence scoring#358

Open
drewstone wants to merge 3 commits into
mainfrom
feat/reference-equivalence-judge
Open

feat(judges): add reference equivalence scoring#358
drewstone wants to merge 3 commits into
mainfrom
feat/reference-equivalence-judge

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

Products compare free-text expected answers with ad hoc prompts or misuse concept-coverage judges. That loses semantic equivalence, contradiction detection, usage, and cost.

Change

Add one transport-injected runReferenceEquivalenceJudge with structured output, untrusted-input isolation, strict parsing, cancellation, usage, cost, model, and duration. Export it from the root and contract entry points.

Proof

  • pnpm test: 278 files, 2,947 passed, 2 skipped
  • pnpm typecheck: passed
  • pnpm lint: passed with four existing warnings
  • pnpm build: passed
  • pnpm verify:package: passed
  • merge-tree against origin/main: clean

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 401c90f7

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-13T08:14:48Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 3 (3 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 744.6s (2 bridge agents)
Total 744.6s

💰 Value — sound-with-nits

Adds a standalone, transport-injected judge that scores a free-text agent output against a free-text reference answer — fills a real gap (no existing judge does free-text answer equivalence) and reuses the right primitives; ship.

  • What it does: A new runReferenceEquivalenceJudge(input, {chat, model, signal}) makes one ChatClient call, parses a strict {score, rationale} JSON object, validates score∈[0,1], checks finishReason==='stop', and returns {kind:'reference-equivalence', version, score, rationale, usage, costUsd, model, durationMs}. It hard-fails with JudgeParseError (and propagates transport errors) rather than emitting a s
  • Goals it achieves: (1) Give products a correct way to compare free-text expected answers with agent outputs — the PR body notes the current alternatives (ad-hoc prompts, or misusing the concept-coverage judge) lose semantic equivalence, contradiction detection, usage, and cost. (2) Isolate untrusted input: the three text fields travel as a JSON data object in the user message, never interpolated into the system inst
  • Assessment: Sound. It fills a genuine gap: every existing LLM judge judges a BUILT ARTIFACT (source files / served HTML) — intent-match (src/intent-match-judge.ts:30-40), semantic-concept (src/semantic-concept-judge.ts:69-81), keyword-coverage (src/keyword-coverage-judge.ts:25-38). None compares free-text candidate-vs-reference answers. Transport choice (ChatClient) follows the codebase's stated forward direc
  • Better / existing approach: Considered whether llmJudge (src/llm-judge.ts) is a better base — it is NOT. llmJudge is a factory that returns a campaign-shaped JudgeConfig whose score() yields {dimensions, composite, notes}; the new judge is a standalone one-shot returning {kind, version, score, rationale, usage, costUsd, model, durationMs}, matching the runIntentMatchJudge/runSemanticConceptJudge family for direct-call consum
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

A coherent, specialized reference-answer semantic-equivalence judge filling a real gap, built on the codebase's preferred ChatClient seam with strong adversarial error/injection handling; only nit is discoverability.

  • Integration: Exported from both public entry points (src/index.ts:1163, src/contract/index.ts:138) and verified by an explicit test (reference-equivalence-judge.test.ts:236). No in-repo caller exists yet, but the consumption paths are concrete and established: direct call by product agents, an analyst adapter analogous to createSemanticConceptJudgeAdapter (src/analyst/adapters.ts:281), or alongside llmJudge. T
  • Fit with existing patterns: Fits the grain. Uses ChatClient, which chat-client.ts:1-20 designates as the preferred seam for new LLM-calling code ('New analyst code uses ChatClient'), matching llmJudge (llm-judge.ts:27). The standalone result shape (kind/version/score/durationMs/costUsd) mirrors runSemanticConceptJudge and runIntentMatchJudge. No existing equivalent found — completion-verifier scores artifact presence (a diff
  • Real-world viability: Holds up well past the happy path. Tested adversarially for prompt-injection isolation (untrusted input kept as JSON data, kept out of system instructions — reference-equivalence-judge.test.ts:84-130), malformed/truncated JSON rejection (:144-188), out-of-range score rejection (:171-188), non-'stop' finishReason rejection (:177-188), abort signal pass-through (:208-235), network-error propagation
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 index.ts export has no section header and sits adjacent to the unrelated 'Reference replay' block [maintenance] ``

In src/contract/index.ts:129 the export gets its own '// ── Reference-answer judge ──' header, but in src/index.ts:1156-1164 it is dumped with no header immediately before '// ── Reference replay ──'. A reader scanning the root barrel could conflate reference-equivalence (answer scoring) with reference-replay (run replay/steering). Trivial: add a one-line section comment matching the contract barrel. Does not gate shipping.

🟡 Standalone judges are now split across two LLM transports [maintenance] ``

runIntentMatchJudge/runSemanticConceptJudge take LlmClientOptions+callLlmJson (src/intent-match-judge.ts:26, src/semantic-concept-judge.ts:22) and soft-fail to available:false; the new judge and llmJudge (src/llm-judge.ts:27,41) take injected ChatClient and hard-fail. This is the codebase's acknowledged forward direction (chat-client.ts:16-19), not against the grain, but it does leave two judge families with different error semantics (soft available:false vs thrown JudgeParseError) until the old

🎯 Usefulness Audit

🟡 New judge not added to the agent-eval SKILL.md judge menu, so skill-driven adopters won't discover it [ergonomics] ``

The /agent-eval skill is the canonical discovery surface: .claude/skills/agent-eval/SKILL.md:103 lists runSemanticConceptJudge + runKeywordCoverageJudge as the 'did the artifact implement the concepts?' judges. runReferenceEquivalenceJudge is correctly exported from src/index.ts:1163 and src/contract/index.ts:138, but is absent from the SKILL.md judge table, so LLM agents building integrations via the skill will not find it when choosing a free-text-answer comparison judge — the exact use case i


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260713T082910Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 401c90f7

Review health 100/100 · Reviewer score 77/100 · Confidence 80/100 · 6 findings (6 low)

glm: Correctness 77 · Security 77 · Testing 77 · Architecture 77

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.

🟡 LOW Contract docstring does not mention the new reference-answer judge surface — src/contract/index.ts

The file-level JSDoc (lines 16-53) advertises 'The five types you need to know' and 'The five functions you'll call', but the new ReferenceEquivalenceJudge exports added at lines 129-139 are neither listed nor referenced anywhere in the overview. Consumers reading only the docstring will miss this public judge. Fix: add a short bullet under a new '## Reference-answer judge' section, or extend the relevant list. Pure doc nit; no runtime impact.

🟡 LOW No test for rationale whitespace trimming — src/reference-equivalence-judge.test.ts

parseResponse returns parsed.rationale.trim() (src/reference-equivalence-judge.ts:141) and rejects whitespace-only strings (line 137). No test verifies either behavior: a valid rationale with leading/trailing whitespace is not asserted to be trimmed in the result, and a whitespace-only rationale is not asserted to throw JudgeParseError. Add a case with { score: 0.5, rationale: ' equivalent ' } expecting result.rationale === 'equivalent', and a case with { score: 0.5, rationale: ' ' } expecting rejection.

🟡 LOW No test that options.model forwards to the ChatClient — src/reference-equivalence-judge.test.ts

ReferenceEquivalenceJudgeOptions.model (src/reference-equivalence-judge.ts:20) is a documented public field that overrides the ChatClient default. No test captures the resolved request.model to verify a caller-supplied model reaches the transport. The existing prompt-injection test captures request and could additionally assert request?.model === 'caller-model' when { chat, model: 'caller-model' } is passed.

🟡 LOW parseResponse edge-case branches lack coverage — src/reference-equivalence-judge.test.ts

The implementation (src/reference-equivalence-judge.ts:111-142) guards six failure modes that have NO corresponding test: (1) non-finite score — Number.isFinite(parsed.score) at line 131 (JSON.parse of 1e309 yields Infinity, or a model emitting NaN); (2) non-object JSON root — Array.isArray(value) / typeof value !== 'object' at line 122 (e.g. model returns [1,2,3] or "hello"); (3) missing rationale field; (4) wrong-type score (string "0.5"); (5) wrong-type rationale (number); (6) whitespace-only ratio

🟡 LOW No input-size truncation, unlike sibling semantic-concept-judge — src/reference-equivalence-judge.ts

userRequest/expectedAnswer/candidateOutput are passed straight into JSON.stringify with no cap. Sibling runSemanticConceptJudge (src/semantic-concept-judge.ts:171-208) truncates source/HTML via maxSourceChars/maxHtmlChars. For operator-supplied scenario data this is fine, but an unbounded candidateOutput could exceed the model context window — provider would return 400 (LlmCallError propagates, still fail-loud). The asymmetry is intentional iff callers are trusted to bound inputs; worth either documenting that invariant on ReferenceEquivalenceJudgeInput or mirroring the truncation pattern. No silent failure either way.

🟡 LOW maxTokens 400 is tight vs schema rationale.maxLength 1000 — src/reference-equivalence-judge.ts

RESPONSE_SCHEMA caps rationale at 1000 chars (~250 tokens) but the request sets maxTokens: 400. A verbose model emitting a near-max rationale plus JSON framing can brush the 400-token ceiling; the resulting finishReason=length is correctly rejected by parseResponse (lines 115-117) — so this fails CLOSED, never silently truncates — but it inflates the judge's failure rate on wordy models. Bumping to ~600 would give headroom without changing the schema. Not a correctness bug; a tunability nit.


tangletools · 2026-07-13T09:49:54Z · trace

tangletools
tangletools previously approved these changes Jul 13, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 6 non-blocking findings — 401c90f7

Full multi-shot audit completed 4/4 planned shots over 4 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-13T09:49:54Z · immutable trace

tangletools
tangletools previously approved these changes Jul 13, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 7a58c62e

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-13T10:11:18Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 248.3s (2 bridge agents)
Total 248.3s

💰 Value — sound

Adds a canonical semantic reference-answer equivalence judge composed over the existing llmJudge bridge, plus a correctly-scoped upgrade that makes paid judge calls visible in campaign cost accounting — no duplication, follows the grain.

  • What it does: Adds two exports: createReferenceEquivalenceJudge (a campaign-native JudgeConfig factory) and runReferenceEquivalenceJudge (a direct-call product adapter). Both compose the existing llmJudge bridge (src/llm-judge.ts) with a single 'equivalence' dimension, a Zod strict response schema, transport-injected ChatClient, input bounding (8k/32k/32k), and prompt-injection isolation (untrusted values ride
  • Goals it achieves: (1) Give products a canonical expected-answer equivalence judge so they stop rolling ad-hoc prompts or misusing concept-coverage judges for free-text comparison. (2) Make paid judge calls first-class cost citizens in campaigns — previously judge cost was invisible to totalCostUsd, costCeiling, and the aggregates dump, which is a real accounting bug for any campaign with LLM judges. (3) Make strict
  • Assessment: Strong, coherent, in-grain. The reference judge composes llmJudge instead of reimplementing the call+parse loop (src/llm-judge.ts:85), so the new module is ~150 lines of prompt + schema + bounding, not a parallel transport. The responseSchema option is generic — any future strict-schema judge gets it free. The cost-accounting refactor is independently correct: totalCellCost (src/campaign/run-campa
  • Better / existing approach: none — this is the right approach. Verified the codebase has no existing reference-answer equivalence judge: intent-match-judge.ts scores holistic app-correctness (source+HTML, soft-fail), semantic-concept-judge.ts scores per-concept presence in built artifacts, agreement-judge.ts is a pure no-LLM injected comparator for distillation, keyword-coverage-judge.ts is deterministic substring matching.
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

A well-integrated reference-answer equivalence judge built on the existing llmJudge/ChatClient seam, filling a real gap with cohesive supporting improvements to cost accounting, structured output, and cancellation.

  • Integration: Fully reachable. Exported from src/index.ts, src/contract/index.ts, and src/campaign/index.ts. Plugs into runCampaign as a JudgeConfig (createReferenceEquivalenceJudge) and has a direct product-call adapter (runReferenceEquivalenceJudge). Signal, cost, cache, and trace paths all wired. As a substrate package, its job is to export for consumers — no internal caller is expected, and none is needed.
  • Fit with existing patterns: Builds directly on llmJudge (llm-judge.ts:85) + ChatClient, the documented transport-agnostic seam. The supporting changes (responseSchema option, LlmCallMetadata on JudgeScore/JudgeParseError, campaign totalCellCost/failedJudgeCall, enforceDispatchUsage relocation) are cohesive and reusable — any future llmJudge-based judge inherits structured output, cost tracking, and fail-loud parsing for free
  • Real-world viability: Holds up across error paths. Prompt injection: system prompt explicitly marks userRequest/expectedAnswer/candidateOutput as untrusted data; test confirms adversarial values stay in the user message. Truncation: parseResponse (llm-judge.ts:238) rejects non-stop finishReason. Parse failure: JudgeParseError carries llmCall so the paid call is still accounted (campaign stores it in failedJudgeCall). C
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Contract subpath exports the judge but not createChatClient [ergonomics] ``

src/contract/index.ts exports runReferenceEquivalenceJudge and createReferenceEquivalenceJudge, whose ReferenceEquivalenceJudgeOptions requires a ChatClient — but createChatClient is only exported from the root entry (src/index.ts:62) and src/analyst/index.ts, NOT from /contract. The contract header (contract/index.ts:80-85) tells consumers that reaching for any non-contract subpath is 'using internals.' A contract-only consumer therefore cannot construct the required transport without leaving t


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260713T101844Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 92889e76

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-13T10:23:19Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 527.8s (2 bridge agents)
Total 527.8s

💰 Value — sound-with-nits

Adds a campaign-native reference-answer equivalence judge built on the existing llmJudge bridge, plus threads cost/usage metadata through the campaign so paid judge calls are never silently dropped — coherent and in-grain.

  • What it does: Adds one new judge, createReferenceEquivalenceJudge / runReferenceEquivalenceJudge (src/reference-equivalence-judge.ts:65-134), that scores a free-text candidate against an expected answer by semantic meaning on a 0–1 scale, with untrusted-input isolation, strict zod response schema, input-size bounds, cancellation, and a versioned result carrying usage/cost/model/duration. It is built as a th
  • Goals it achieves: (1) Give products a transport-injected, campaign-native judge for expected-answer scoring so they stop comparing free-text with ad-hoc prompts or misusing concept-coverage judges. (2) Make judge cost/usage fully visible in campaign accounting — no paid judge call (including ones that fail to parse) is silently dropped, and the stub-detection guard no longer misfires on judge tokens. Both are visib
  • Assessment: Sound and in the codebase's grain. The judge reuses llmJudge (the documented single-call bridge, src/llm-judge.ts:85) rather than reimplementing the LLM call, injects transport via ChatClient (the substrate's transport-agnostic seam), fails loud (JudgeParseError + failedJudgeCall, matching the repo's no-silent-zero / no-fake-zero doctrine in CLAUDE.md), and exports from root + /contract
  • Better / existing approach: none — this is the right approach. Considered the three existing purpose-built judges (semantic-concept-judge.ts, intent-match-judge.ts, keyword-coverage-judge.ts); none does reference-answer equivalence, so no reuse opportunity was missed. The reference judge is correctly layered on llmJudge rather than standalone callLlmJson, which is the higher-reuse path. One maintenance note (not a redire
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

A new reference-equivalence judge fills a real gap (free-text semantic equivalence, not covered by any existing judge) by composing the established llmJudge + injected ChatClient pattern, with correct cost-accounting fixes for judge spend that previously leaked out of campaign totals.

  • Integration: Properly exported from three entry points — src/index.ts:1163, src/contract/index.ts:142, src/campaign/index.ts:44 — with the companion createChatClient export added to contract (src/contract/index.ts:131) so product callers can build the transport the judge requires. The judge returns a standard JudgeConfig (src/reference-equivalence-judge.ts:67) so it slots directly into runCampaign({ judges: [.
  • Fit with existing patterns: Composes llmJudge (src/llm-judge.ts:85) — the canonical single-call judge factory — rather than reinventing it, and adds a generic responseSchema option (src/llm-judge.ts:65) to llmJudge that any future judge benefits from. Uses the injected ChatClient seam rather than the older LlmClientOptions shape used by intent-match-judge.ts:55 and semantic-concept-judge.ts:113; this divergence is intentiona
  • Real-world viability: Handles the realistic load: untrusted-input isolation via JSON-data user message + explicit anti-injection system prompt (src/reference-equivalence-judge.ts:51-62), per-field length bounds enforced before the paid call (boundedField, line 136), provider json_schema→json_object degrade tested at reference-equivalence-judge.test.ts:163, AbortSignal propagation to the underlying fetch verified at tes
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 Third judge-construction pattern introduced without consolidating [maintenance] ``

src/reference-equivalence-judge.ts composes llmJudge and exposes a runX/createX direct adapter, a third shape alongside standalone-callLlmJson+soft-fail (src/semantic-concept-judge.ts:235, src/intent-match-judge.ts:132) and raw llmJudge (src/llm-judge.ts:85). It's the best of the three and the right choice here, but semantic-concept and intent-match could now be expressed as llmJudge configs to collapse toward one pattern. Not a gate — flagging for a future consolidation, not thi


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260713T103409Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 92889e76

Review health 100/100 · Reviewer score 40/100 · Confidence 95/100 · 16 findings (1 medium, 15 low)

glm: Correctness 40 · Security 40 · Testing 40 · Architecture 40

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 11 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM finishReason enforcement applies to all llmJudge callers, not just responseSchema opt-in — src/llm-judge.ts

parseResponse now throws JudgeParseError whenever response.finishReason != null && response.finishReason !== 'stop', BEFORE any schema or regex parsing. This check runs for every llmJudge caller, including those who do not pass opts.responseSchema. Previously a caller receiving finishReason='length' but with a parseable JSON body would still get a score via the objMatch regex fallback; now they get a hard JudgeParseError. The direction is correct (truncated responses should fail loud, matching callLlmJson's truncated-JSON handling in src/llm-client.ts:717-720), but the behavior change is invisible from the public llmJudge docstring at src/llm-judge.ts:75-84 which still describes only the dimension-presence/range invariants. In-repo impact: none (the only consumer, createReferenceEquiva

🟡 LOW Caller-abort error shape changed (Error -> DOMException) — src/analyst/chat-client.ts

Old path rejected via toAbortError() returning new Error('ChatClient.chat: aborted') with name='AbortError'. New path delegates to inner.call -> callLlm, which throws new DOMException('callLlm aborted by caller signal', 'AbortError') (llm-client.ts:440). DOMException is the spec-compliant abort error and has name==='AbortError', so consumers using err.name (e.g. src/adapters/http.ts:326) are unaffected. Only breaks consumers doing instanceof Error branch + name check on chat aborts specifically; ripgrep found no such consumer. Informational; document in ChatClient.chat docstring.

🟡 LOW No test covers the LlmClient-backed signal forwarding path — src/analyst/chat-client.ts

The only signal-related test (analyst.test.ts:740 'ChatClient signal racing') uses transport:'mock', which bypasses wrapLlmClient entirely. The new signal-forwarding logic in wrapLlmClient.chat (the actual production code path for router/cli-bridge/direct-provider) is uncovered — regressions in the per-call LlmClientOptions signal wiring would not be caught. Add a unit test using a fake fetch (LlmClientOptions.fetch override) that resolves only on signal abort, then assert the chat() promise rejects with name==='AbortError'. The existing test's describe name and comments are also now stale (it claims 'races live in wrapLlmClient'), but the test file is outside this shot's scope.

🟡 LOW No dedicated unit tests for run-campaign.ts internal helpers — src/campaign/run-campaign.ts

totalCellCost, traceJudgeCost, and the renamed enforceDispatchUsage are exercised only through reference-equivalence-judge.test.ts integration tests. Edge cases like multiple judges with mixed llmCall presence, warn-mode enforcement behavior, and totalCellCost on cells with partial judgeScores are not directly unit-tested. Coverage is adequate for merge but a run-campaign.test.ts would harden against future regressions.

🟡 LOW traceJudgeCost span has ~0ms duration by construction — src/campaign/run-campaign.ts

The span is created and immediately .end()-ed synchronously, so durationMs is always ~0. The span serves as a cost-amount marker (amountUsd attribute), not a timing measurement, so this is a design choice not a bug. If downstream tooling expects cost spans to carry meaningful durations, this will read as noise. No action required unless trace consumers interpret duration.

🟡 LOW New public section lacks a doc entry in the contract header JSDoc — src/contract/index.ts

The file's top JSDoc (lines 1-85) enumerates the public-surface groups ('The five types', 'The five functions', storage backends, deployment-outcome store, RL bridge) but does not mention the newly public Reference-answer judge surface now exported at lines 131-147. Since /contract is the documented frozen API and the header is the discovery path for consumers, the new section is invisible to anyone reading only the header. Impact: discoverability/documentation gap only — no correctness, type, or runtime impact. Fix: add a short '## Reference-an

🟡 LOW LlmClient.call silently degrades schema on 400 but neither the method nor callLlm docstring reflects it — src/llm-client.ts

LlmClient.call now routes to callLlmStructured when req.jsonSchema is set, meaning a 400 schema-rejection is silently retried as json_object. This is a behavior change from the prior pure-delegation-to-callLlm. LlmClient has no method-level docstring (line 892 comment only describes it as a 'thin wrapper'), and callLlm's docstring (lines 411-414) explicitly says 'does NOT degrade schema here — callers that want graceful degrade use callLlmJson'. The two statements now contradict for the jsonSchema path. Impact: a caller reading callLlm's contract and rout

🟡 LOW No test exercises the new LlmClient.call → callLlmStructured schema-degrade path — src/llm-client.ts

The only LlmClient wrapper test (src/llm-client.test.ts:585-596) covers per-call apiKey override on a no-jsonSchema request, so it still hits the callLlm branch. The new ternary req.jsonSchema ? callLlmStructured(req, options) : callLlm(req, options) has zero direct coverage — the existing schema-degrade test (llm-client.test.ts:444) only exercises callLlmJson. Impact: a regression that breaks the LlmClient.call routing (e.g. someone reverts the ternary or swaps the branch order) would pass CI. Fix: add a test mirroring llm-client.test.ts:444 but invoking new LlmClient({ fetch }).call({ model, messages, jsonSchema }) and asserting (1) two fetch calls, (2) second request body has response_format.type === 'json_object', (3) result.content is non-empty.

🟡 LOW Shallow clone of zod-emitted JSON Schema shares nested object references with zod internals — src/llm-judge.ts

const schema = { ...(z.toJSONSchema(opts.responseSchema.schema) as Record<string, unknown>) } shallow-clones only the top level. delete schema.$schema correctly removes the only top-level meta field (verified zod 4.3.6 emits $schema only at root). Nested nodes (e.g. properties.dimensions) remain shared references. Today the code only reads the object, so this is safe; flagging because any future mutation of nested fields would leak into zod's cached representation. Cheap fix if touched later: deep-clone via structuredClone(z.toJSONSchema(...)).

🟡 LOW Cache-mutation step in 'rechecks dispatch-only usage' is observationally hollow — src/reference-equivalence-judge.test.ts

Lines 297-302 rewrite the cached-result.json to add error: "judge 'reference-equivalence' failed" then re-run with expectUsage:'assert'. The only assertion is the SAME BackendIntegrityError already asserted on line 293. Nothing in the test observes that the legacy error-tagged cache shape was actually loaded and honored — the assertion would pass even if the cache rewrite were a no-op. Either assert dispatch is NOT called again on the third run (proving cache hit), or assert the specific recheck code path

🟡 LOW Negative-score range boundary not exercised — src/reference-equivalence-judge.test.ts

The schema-rejection table covers equivalence: 1.01 (max violation) but not equivalence: -0.01 (min violation at reference-equivalence-judge.ts:46 .min(0)). Asymmetric boundary coverage on a [0,1] scoring dimension. Trivial fix: add a row.

🟡 LOW runReferenceEquivalenceJudge signal cancellation not directly tested — src/reference-equivalence-judge.test.ts

The cancellation test routes through createReferenceEquivalenceJudge().score() (scoreWith helper), not through runReferenceEquivalenceJudge() — the direct-call adapter product callers actually use (reference-equivalence-judge.ts:108-122). The signal path is shared, but the adapter's options.signal ?? new AbortController().signal line is only exercised with the undefined branch. Add one case passing an aborting signal through runReferenceEquivalenceJudge to lock the product-facing contract.

🟡 LOW Defensive AbortController allocation for missing signal — src/reference-equivalence-judge.ts

options.signal ?? new AbortController().signal allocates a controller purely to extract a never-aborting signal so the JudgeConfig.score signature (which requires signal) is satisfied. Functionally correct, but a tiny wart: a fresh controller can never be cancelled by the caller, so the direct adapter silently swallows caller-side cancellation when no signal is passed. Acceptable for a one-shot adapter, but if a product caller forgets the signal they lose the ability to cancel a long-running judge call. Cosmetic / ergonomic only.

🟡 LOW Hardcoded scenario.id in direct adapter — src/reference-equivalence-judge.ts

The direct adapter constructs the scenario with id: 'reference-equivalence-direct' (a constant). The function is documented as a single-call product adapter so this is fine for one-shot use, but if any caller wires runReferenceEquivalenceJudge into a multi-scenario loop (it's exported from /contract and /index, so the misuse is reachable), every call collides on the same id and any id-keyed caching/dedup logic downstream would silently collapse distinct evaluations. Consider accepting an optional id on ReferenceEquivalenceJudgeInput (defaulting to 'reference-equivalence-direct') so the contract documents and survives reuse. Low because current contract is single-call and fail-loud elsewhere protects the campaign path.

🟡 LOW No appliesTo filter — mixing scenario kinds throws — src/reference-equivalence-judge.ts

The returned JudgeConfig omits appliesTo, so in a campaign that mixes ReferenceEquivalenceScenario with other kinds, this judge will be invoked on every scenario and boundedField will throw TypeError on missing userRequest/expectedAnswer. The campaign engine records the throw as a failed cell (fail-loud, correct), but a careless consumer will see mysterious failed-cell counts. A one-line appliesTo: (s) => s.kind === 'reference-equivalence' would make the contract explicit and match the JudgeConfig docstring at campaign/types.ts:106. Low because fail-loud is the doctrine and the type parameter already constrains TypeScript callers.

🟡 LOW maxTokens 400 is tight vs notes cap of 1000 chars — src/reference-equivalence-judge.ts

RESPONSE_SCHEMA.notes allows up to 1000 chars; JSON envelope {"dimensions":{"equivalence":<num>},"notes":"..."} plus a verbose 1000-char note approaches the 400-token (~1600 char) cap. If the model writes a long note AND hits the cap, finishReason='length' triggers JudgeParseError and the cell is recorded as a failed judge call with paid-call metadata preserved (test line 143 proves the path). Fail-loud makes this self-correcting rather than silently corrupting, so impact is bounded; still, 500-600 tokens would give the schema more headroom and avoid wasted paid calls on edge cases.


tangletools · 2026-07-13T11:13:22Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 16 non-blocking findings — 92889e76

Full multi-shot audit completed 8/8 planned shots over 11 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-13T11:13:22Z · immutable trace

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.

2 participants