feat(multishot): freeze the loop's behaviour as golden records by drewstone · Pull Request #627 · tangle-network/agent-eval

drewstone · 2026-08-16T19:08:09Z

Step 1 of #617. The loop-to-graph parity claim is proven and merged — tax 4/4 deep-equal, gtm 4/4 shot-level and 8/8 matrix-level. What keeps ongoing value is regression detection on the graph engine, and that needs the loop's RECORDED outputs, not a second live orchestrator that somebody has to maintain forever.

This freezes the oracle. The loop deletion follows once the three consumer parity tests move onto these records.

What ships

New subpath @tangle-network/agent-eval/multishot/golden.

13 shot scenarios + 1 matrix scenario, the union of the behaviours the merged parity proofs covered. Two scripts:

script	what it pins
`delegation-*`	silent multi-tool turns, an unknown tool with unparseable arguments, two typed artifact kinds, both cost paths, `maxTurns` 0 / NaN / 1 / 3 / 10, driver fallback rotation, `MultishotFatalToolError`, `MultishotDriverEmptyError`, the dispatch-cap message
`sampling-contract-*`	distinct per-leg token budgets, driver retry-on-empty, a whitespace-only driver reply, an empty assistant follow-up with no usage at all (the uncaptured cost path), full model rotation, the same three error paths

A record holds two things. The request ledger — every transport call each leg received, in issue order: model, temperature, token budget, advertised tool names, whether the tools array arrived by reference, and the full message log. That is where two orchestrators diverge without their return value changing. And the outcome — the MultishotResult without wall-clock durationMs, or the throw reduced to its class, its message, and the cell spend it declares for the cost ceiling.

The matrix record adds the returned MatrixResult, the judge calls, and all 14 files the run persisted.

A test helper any engine can point at, framework-free:

for (const scenario of multishotGoldenScenarios()) {
  it(scenario.description, async () => {
    await assertMultishotGoldenScenario({ engine: runMyEngine, scenario })
  })
}

Deterministic only. Scripted transports, scripted executors, fixed personas and budgets. No network, no clock in a recorded field, no random number. Matrix cells run one at a time so the ledger is a property of the conversation engine rather than of how two engines interleave microtasks.

Records are frozen

scripts/record-multishot-golden.ts refuses to overwrite an existing version. A golden record that can be regenerated over itself proves nothing — a regression is simply re-recorded as the new truth. A behaviour change mints a NEW version file registered beside the old one, and the diff between them is the reviewable evidence.

The recorder never picks an engine for you (--engine <module>#<export> is required, and the module path may point outside this repo), so the script survives the loop deletion unchanged. Every scenario is captured twice and the two captures must agree, so an unreproducible scenario fails instead of freezing a coin flip.

v1 was captured from ./multishot's loop at 0.145.21 — the engine the merged parity proofs compared against.

The records are load-bearing

Two proofs, both in the suite.

Exhaustive. For all 14 records, every single leaf of {outcome, requests} (and {matrix, requests, judgeRequests, files}) is mutated one at a time; every mutation must be reported. Zero fields escape comparison.

Engine-level. Eight mutants of a real engine, 8/8 detected, each naming the field:

### M1 drop the last transcript row — DETECTED
  - outcome.result.transcript: expected 11 entries, received 10

### M2 stop metering tool-executor cost — DETECTED
  - outcome.result.costProvenance.usd: expected 0.0536, received 0.0196
  - outcome.result.costUsd: expected 0.0536, received 0.0196

### M3 raise the follow-up token budget — DETECTED
  - requests[1].maxTokens: expected 222, received 9999
  - requests[5].maxTokens: expected 222, received 9999

### M4 skip the driver fallback rotation — DETECTED
  - outcome.kind: expected "result", received "error"

### M5 raise the tool dispatch cap — DETECTED
  - outcome.error.message: expected "multishot: tool dispatch cap exceeded (3/2) on turn 0", received "agent transport: unscripted call 2"

### M6 swallow the fatal tool error — DETECTED
  - outcome.kind: expected "error", received "result"

### M7 rebuild the tools array instead of forwarding it — DETECTED
  - requests[0].toolsPassedByReference: expected true, received false

### M8 drop the contentQuality judge slot — DETECTED
  - matrix.byAxis.persona.retail-founder.meanScore: expected 6.5, received 7.5
  … 56 more mismatched fields

8/8 mutants detected

Proof

pnpm test — 5367 passed, 2 skipped. One unrelated timeout in reference-equivalence-judge.test.ts under full-suite load; it passes alone (17/17) and reproduces on origin/main.
pnpm typecheck, pnpm typecheck:scripts, pnpm lint — clean.
pnpm build + pnpm verify:package — exit 0; ./multishot/golden resolves 🟢 from ESM and bundler, records inline into the bundle.

Not in this PR

No existing export changes. runMultishot and runMultishotMatrix are untouched and still published — consumers migrate onto the records first, and the loop deletion is the next PR so no repo is ever pointing at something that does not exist.

The loop-to-graph parity claim is proven and merged. What keeps value is regression detection on the graph engine, and that needs the loop's recorded outputs, not a second live orchestrator. `@tangle-network/agent-eval/multishot/golden` ships: - 13 deterministic shot scenarios and 1 matrix scenario. Scripted transports, scripted executors, fixed personas and token budgets. No network, no clock in a recorded field, no random number. - A record per scenario holding the full request ledger of both legs (model, temperature, token budget, advertised tools, tools-by-reference, message log) and the outcome: the result without wall-clock duration, or the throw reduced to its class, message and declared cell spend. - `assertMultishotGoldenScenario` / `checkMultishotGolden` and the matrix pair, which throw or report every field that moved. No test framework needed. - `scripts/record-multishot-golden.ts`, which never picks an engine for you, refuses to overwrite a released version, and captures every scenario twice so an unreproducible scenario fails instead of freezing a coin flip. v1 is captured from `./multishot`'s loop at 0.145.21 — the engine the merged parity proofs compared against. The records are load-bearing: a single-field mutation of any recorded leaf, across all 14 records, is reported as a named mismatch. Docs: docs/multishot-golden-records.md

…lyst digest The golden matrix scenario answers its judge leg from a score table on its own fetch wire, so `test/judge-model` never reaches a provider and has no served id to assert. The analyst dependency lock covers package.json, which now declares the ./multishot/golden subpath.

drewstone · 2026-08-16T19:08:17Z

@tangletools review now

tangletools

✅ Auto-approved drewstone PR — `574bf400`

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

_{tangletools · auto-approval · reason: drewstone_author · 2026-08-16T19:08:21Z}

tangletools

✅ Auto-approved drewstone PR — `574bf400`

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

_{tangletools · auto-approval · reason: drewstone_author · 2026-08-16T19:08:30Z}

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	2 of 2 lenses (value, usefulness)
Concerns	4 (4 weak-concern)
Heuristic	0.0s
Duplication	0.2s
Interrogation	386.9s (2 bridge agents)
Total	387.1s

💰 Value — sound

Freezes the multishot loop's observable behavior (per-leg wire requests + outcomes) into append-only golden records with a framework-free consumer-facing check harness — the right oracle design for deleting the loop while keeping regression detection on successor engines; verified passing and load-b

What it does: Adds subpath @tangle-network/agent-eval/multishot/golden: 13 deterministic shot scenarios + 1 matrix scenario (src/multishot/golden/scenarios.ts, matrix-scenarios.ts) whose scripted transports fill a request ledger (model, temperature, token budget, tool names, tools-by-reference, full message log, in issue order); a recorder script (scripts/record-multishot-golden.ts) that captures the ledger plu
Goals it achieves: Convert the loop-to-graph parity arrangement from 'maintain a second live orchestrator forever' to 'maintain a frozen fixture': the records become the regression oracle any successor engine (in consumer repos, per docs/multishot-golden-records.md:75-82 the recorder can capture from ../gtm-agent) checks itself against, unblocking deletion of the loop from this substrate package. Secondary goals ach
Assessment: Good change, in the grain of the repo. The layering is exactly right per CLAUDE.md's substrate doctrine: verification primitive + fixtures live in agent-eval (the bottom), conversation engines stay in consumers; the harness depends only on in-repo ../multishot and ../matrix. Design choices are unusually disciplined for golden testing: append-only versions (recorder refuses to overwrite, so a regre
Better / existing approach: none — this is the right approach. Searched for existing equivalents before concluding: src/golden-matcher.ts (fuzzy phrase matching for expected findings — different mechanism and purpose), src/reference-replay.ts (scores candidates against withheld historical outcomes — live re-scoring, not a wire-level frozen oracle), tests/multishot/*.test.ts (live loop unit tests with ad-hoc stubs — die with
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 2
Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A correctly-built frozen-oracle: golden records that pin the multishot loop's full observable behavior (request ledger + outcome + matrix files), consumed in-repo today by a 49-test regression suite and ready for the graph engine through the already-existing runShot seam.

Integration: Fully wired and already consumed. Live caller in this PR: src/multishot/golden/golden.test.ts runs the reference loop (runMultishot, runMultishotMatrix) against the records — verified 49/49 passing in 4.47s — including mutation proofs that every recorded leaf fails the check when perturbed (golden.test.ts:206-268), so the records cannot silently rot. The imminent external caller is concrete, not h
Fit with existing patterns: Matches the codebase's grain exactly: per-module subpath export + build-entries registration + one docs file per capability, mirroring every sibling module. No competing snapshot/golden mechanism exists (grep for toMatchSnapshot/toMatchInlineSnapshot across src: zero hits), so this invents nothing that's already there. Architecturally it is the right answer to the stated problem (#617): the merged
Real-world viability: Built for realistic use, with the hard parts handled: wall-clock and run-identity keys stripped before comparison (recording.ts:22-27), judge fan-out nondeterminism normalized by sorted comparison (recording.ts:117-129), matrix concurrency pinned to 1 with the reason written down (matrix-scenarios.ts:7-10), rendered durations masked in summary Markdown (recording.ts:113-115). Error and edge paths
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 1

💰 Value Audit

🟡 ? [better-architecture] ``

The request ledger pins req.tools === options.tools array identity (scenarios.ts:54, types.ts:34). The stated justification ('an engine that rebuilds the array can silently change what the agent is offered') is weak — a semantically identical clone changes nothing offered — so the field freezes an implementation detail of the loop rather than observable behavior. A conforming successor engine that defensively copies the tools array will fail the golden check and need a deliberate v2 to relax i

🟡 ? [maintenance] ``

v1.json is 183KB committed in src and shipped through the new subpath, and versions are append-only by design (docs/multishot-golden-records.md:54-61), so every future behavior change adds another full fixture file to the published package. This is documented, deliberate (the old contract stays runnable), and the fixture IS the product — but the package will accumulate versioned records indefinitely with no stated pruning policy. A note on when old versions may be dropped would future-proof the

🎯 Usefulness Audit

🟡 toolsPassedByReference freezes JS object identity into the cross-engine contract [problem-fit] ``

recording.ts:66 pins req.tools === scenario.tools into the frozen record. An engine that reconstructs the tools array (e.g., serializes node state across a graph boundary) would fail this field while being observably identical. It is deliberate (PR body names it) and satisfiable in-process today, and the merged parity proofs covered it — but when step 2 moves consumers onto these records, this is the single field most likely to force a behavior-only engine to fake an implementation detail. Con

🟡 Matrix judge wire swaps globalThis.fetch process-wide [robustness] ``

matrix-scenarios.ts:238-271 installs the deterministic judge on globalThis.fetch for the run's duration. Safe as used (vitest worker isolation, sequential scenarios), and the fail-loud guard on non-judge URLs is good, but a consumer running the matrix check concurrently with other fetch traffic in the same process would interleave wires. Documented behavior (docs/multishot-golden-records.md:43); worth one sentence in the doc saying 'run matrix checks serially per process' when consumers adopt

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 · 20260816T191558Z}

The request ledger pinned `req.tools === options.tools`. Array identity is not observable behaviour: an engine that deep-copies the tools before dispatch offers the agent exactly the same thing, and would have failed the check for a reason no caller can see. Both review lenses named it as the field most likely to force a conforming engine to fake an implementation detail. The ledger now records the tool DEFINITIONS and compares them by value. A rebuilt array passes; a changed name, description or parameter schema is a mismatch. v1 is re-recorded against the same loop. Also documented: the matrix judge wire is process-wide, so matrix checks run serially per process; and when an old record version may be dropped.

drewstone · 2026-08-16T19:19:56Z

Addressed the toolsPassedByReference finding both lenses raised — it is the right catch, and it mattered before consumers adopt these records.

The ledger now records the tool DEFINITIONS and compares them by value instead of pinning req.tools === options.tools. Array identity is not observable: an engine that deep-copies tools before dispatch offers the agent exactly the same thing.

Proof both directions:

### M7 advertise a renamed tool — DETECTED
  - requests[0].tools[0].function.description: expected "research", received "renamed"
  - requests[1].tools[0].function.description: expected "research", received "renamed"
  … 3 more

PASS — an engine that rebuilds the tools array is no longer a false mismatch

8/8 mutants still detected. v1 re-recorded against the same loop.

Also took the two doc findings: the matrix judge wire is process-wide (run matrix checks serially per process, and the wire fails loud on any request it does not recognise), and a version-retirement policy is now written down.

@tangletools review now

tangletools · 2026-08-16T19:24:22Z

⚠️ Review Interrupted — `574bf400`

The review runner stopped before publishing a final verdict: webhook_restarted.

State	Detail
Interrupted	webhook restarted

No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.

_{tangletools · #627 · model: kimi-for-coding · updated 2026-08-16T19:24:19Z}

tangletools

🟡 Value Audit — sound-with-nits


Verdict	sound-with-nits
Coverage	2 of 2 lenses (value, usefulness)
Concerns	4 (4 weak-concern)
Heuristic	0.1s
Duplication	0.3s
Interrogation	472.0s (2 bridge agents)
Total	472.4s

💰 Value — sound-with-nits

Freezes the multishot loop's observable behavior (request ledger + outcomes + matrix artifacts) into frozen, mutation-verified JSON fixtures shipped as a consumable subpath so the loop can be deleted while its contract stays runnable — a well-built regression oracle, no existing equivalent in the re

What it does: Adds @tangle-network/agent-eval/multishot/golden: 13 deterministic shot scenarios + 1 matrix scenario (scripted transports, scripted executors, no clock/network/random — scenarios.ts:1-17), a one-time recorder script that double-captures and refuses to overwrite a frozen version (scripts/record-multishot-golden.ts:35-46,108-111), a 206KB frozen record (records/v1.json), and a framework-free chec
Goals it achieves: Enable deleting the loop orchestrator (stated step 1 of #617) without losing its behavioral contract: the graph engine replacing it gets regression detection against the loop's RECORDED outputs instead of requiring a second live orchestrator maintained forever. This builds directly on the MultishotShot engine seam from #621 (multishot.ts:74-85, matrix.ts:211 defaults to the loop). Secondary gain
Assessment: Good, and in the grain of the codebase. (1) It follows the repo's established frozen-oracle pattern — scripts/generate-statistics-oracle.py pins statistics.ts the same deliberate-regeneration way ('a changed value on an UNCHANGED case is a regression, not a fixture that needs updating'). (2) Recorder and check share one normalizer implementation (recording.ts), so record/replay cannot drift. (3) T
Better / existing approach: none — this is the right approach. Searched: src/golden-matcher.ts (fuzzy phrase matching, unrelated), scripts/generate-statistics-oracle.py + tests/fixtures (same frozen-fixture PATTERN, followed rather than reinvented), vitest snapshot usage in src/analyst/* (framework-locked, unshippable), meta-eval/sentinel + judge-calibration 'golden' references (labeled score sets, unrelated). Alternatives c
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 6
Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A deterministic frozen-fixture regression detector for the multishot engine, wired end-to-end (exports, build, tests, recorder) and live in this repo's own suite on day one, with the harness contract exactly matching the documented engine-swap seam the graph consumer will plug into.

Integration: Fully wired and already called. Export subpath added (package.json:92-96) and built (scripts/build-entries.mjs:17; verified dist/multishot/golden/index.js + .d.ts emitted). Immediate caller in-repo: src/multishot/golden/golden.test.ts runs the reference loop against all 14 records on every pnpm test — ran it, 49/49 pass — so the regression detector is live now, not waiting on a consumer. Recorder
Fit with existing patterns: Fits the codebase's grain. Frozen-fixture pinning has direct precedent: scripts/generate-statistics-oracle.py regenerates the scipy fixture that pins src/statistics.ts; this extends the same idea to the conversation engine. No competing engine-parity or snapshot infrastructure exists in src (grep for replay/snapshot/golden found only unrelated judge-calibration golden sets and golden-matcher, diff
Real-world viability: Built for determinism, and the edge cases are the product: maxTurns 0/NaN/1/3/10, unparseable tool args, whitespace-only driver replies, empty-usage cost paths, three error classes, and rotation exhaustion are all IN the catalog (scenarios.ts, matrix-scenarios.ts). The recorder captures each scenario twice and refuses non-reproducible fixtures, and refuses to overwrite a released version file (scr
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 2

🎯 Usefulness Audit

🟡 Matrix judge wire patches globalThis.fetch, so concurrent matrix scenarios in one process collide [robustness] ``

installJudgeWire() (src/multishot/golden/matrix-scenarios.ts:238-271) swaps globalThis.fetch and restores it in finally (harness.ts:169-175). Serial use — the harness's shape and how golden.test.ts drives it — is safe, and the wire fails loud on unexpected URLs. But a consumer that runs checkMultishotMatrixGoldenScenario for two scenarios concurrently in one process would interleave wires and cross-record judge calls. Not gating: document the serial requirement (or key the wire off the URL per-s

💰 Value Audit

🟡 Judge wire monkeypatches globalThis.fetch instead of an injectable seam [better-architecture] ``

matrix-scenarios.ts:238-271 replaces globalThis.fetch process-wide for the judge leg. It fails loud on unexpected traffic and the constraint is documented (docs/multishot-golden-records.md:44-45: run matrix checks serially), and JudgeConfig (multishot/judges.ts) has no fetch/transport seam to use instead — adding one would be a wider refactor outside this PR's goal. Note for the reviewer: if judges ever gain a transport seam like the agent/driver legs have, this wire should move onto it.

🟡 Frozen fixture pins edge-case semantics (NaN maxTurns behaves as zero) [maintenance] ``

scenarios.ts:427-430 records maxTurns: Number('not-a-number') collapsing to zero turns, freezing arguably-surprising behavior into the contract. This is inherent to a freeze and the versioned-mint process handles a deliberate later change (new version file, reviewable diff, old version stays runnable via goldenRecords('v1')) — acceptable, just be aware v1 cements it.

🟡 206KB JSON bundled into the dist subpath, growing per version [proportion] ``

records/v1.json is ~206KB, statically imported by records/index.ts and shipped via the new ./multishot/golden entry (package.json:92-96, build-entries.mjs). It is an opt-in subpath so only golden consumers pay, and each version is one file retained deliberately (old contracts stay runnable). Fine at v1; worth watching if version count grows.

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 · 20260816T192847Z}

The wire holds globalThis.fetch for the run, so two matrix checks at once in one process would cross their judge ledgers and report a mismatch nobody can explain. The second install now throws instead. The serial rule is enforced, not only written down.

drewstone · 2026-08-16T19:30:08Z

Took the judge-wire nit one step further than documenting it: a second concurrent installJudgeWire() in the same process now throws instead of crossing two runs' judge ledgers. Serial use is enforced, not only written down.

The other three nits are recorded as accepted rather than fixed, with reasons:

judges have no transport seam — correct, and adding one is a wider refactor than this PR's goal. If JudgeConfig ever gains a transport like the agent and driver legs have, this wire moves onto it.
v1 cements NaN maxTurns behaving as zero turns — that is the point of a freeze. It is what the loop does today and what the merged parity proofs compared against; a deliberate change mints v2 and the diff is the evidence.
206KB fixture in an opt-in subpath — only golden consumers pay it, and the retirement policy is now in the doc.

@tangletools review now

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	1 of 2 lenses (usefulness)
Concerns	0 (none)
Heuristic	0.1s
Duplication	0.1s
Interrogation	293.8s (2 bridge agents)
Total	294.0s

⚠️ Partial audit — the verdict covers only usefulness. value: cli-bridge admission rejected (queue saturated). Treat the missing lens as unexamined, not as clear.

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 5
Bridge error: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=7/48 — no model was started

🎯 Usefulness — sound

A well-built frozen-oracle regression layer that pins the multishot loop's request ledger and outcomes exactly at the already-designed engine seam, verified live (50/50 tests pass, typecheck and both repinned gates green), with the consumer migration path explicitly wired.

Integration: Fully wired and reachable today: package.json:92 exports ./multishot/golden (bundled via scripts/build-entries.mjs), the in-repo suite src/multishot/golden/golden.test.ts runs the reference loop against all 13 shot + 1 matrix records (I ran it: 50/50 pass), and the recorder ships as pnpm record:multishot-golden. The imminent caller is the consumer graph engine from the loop-deletion plan (#617): t
Fit with existing patterns: Fits the codebase's grain rather than competing: the freeze-then-version doctrine (recorder refuses to overwrite, records/index.ts keeps old versions runnable) matches the sealed-experiment charter in docs/experiment.md; the framework-free throw-based harness (harness.ts:1-7) matches the package's cross-language consumer posture. I checked for overlap: src/golden-matcher.ts is unrelated (fuzzy tex
Real-world viability: Holds up beyond the happy path because determinism and coverage are enforced, not assumed: the recorder captures every scenario twice and fails on drift (scripts/record-multishot-golden.ts:132-140), the process-wide judge wire refuses a second concurrent install (matrix-scenarios.ts:242-251, tested at golden.test.ts:302) and fails loud on unrecognized fetch traffic, judge calls fanned out via Prom
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 2

No concerns from the lens that ran (usefulness). The missing lens examined nothing, so this is not a full clean bill of health.

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 · 20260816T193622Z}

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	1 of 2 lenses (value)
Concerns	1 (1 weak-concern)
Heuristic	0.1s
Duplication	0.7s
Interrogation	366.4s (2 bridge agents)
Total	367.2s

⚠️ Partial audit — the verdict covers only value. usefulness: cli-bridge admission rejected (queue saturated). Treat the missing lens as unexamined, not as clear.

💰 Value — sound

Freezes the multishot loop's behavior as a published, framework-free golden-record oracle (request ledger + outcome + matrix files) so the loop can be deleted and any future engine can prove conformance — well-built, verified, and the right architecture for the goal.

What it does: Adds the @tangle-network/agent-eval/multishot/golden subpath: 13 deterministic shot scenarios plus 1 matrix scenario (scripted transports, scripted executors, fixed budgets — no clock/network/random in recorded fields), a frozen v1.json record set (206KB) captured from the loop (runMultishot/runMultishotMatrix at 0.145.21), a recorder script (scripts/record-multishot-golden.ts) that refuses
Goals it achieves: Convert a proven-but-transient parity claim (loop-to-graph, already merged in consumer repos) into a durable regression detector. Once the golden records exist, the loop orchestrator can be deleted from this repo (step 1 of #617) and consumer parity tests point at the frozen records instead of a second live orchestrator that must be maintained forever. The ledger is the key design choice: two orch
Assessment: Good on its merits and executed with unusual rigor. (1) The records are proven load-bearing: golden.test.ts:206-268 mutation-tests every leaf of all 14 records — every single-field perturbation is asserted to be caught — plus an exact-replay-passes proof. (2) Recorder/checker share one normalizer implementation (recording.ts), so the two sides cannot drift; volatile fields (durationMs, runId, matr
Better / existing approach: none — this is the right approach. Searched for overlap before concluding: src/golden-matcher.ts is a fuzzy phrase matcher (recall/precision over text) — different purpose; src/reference-replay.ts is fuzzy reference scoring across train/dev/test splits — different purpose; no structural comparer with path-reporting exists in src; the loop's own tests under tests/multishot/ (2,482 lines) die with t
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 3
Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 4
Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=5/48 — no model was started

💰 Value Audit

🟡 Append-only version files grow the published package ~200KB per behaviour change [maintenance] ``

v1.json is 206KB, bundles inline into dist/multishot/golden/index.js, and ships to every consumer; records/index.ts:1-13 is designed so old versions are kept runnable beside new ones, so each deliberate behaviour change adds another version file permanently. This is an accepted trade (the version diff is the reviewable evidence and consumers need the records importable), but if versions accumulate the maintainer should eventually retire the oldest ones behind a major version rather than carrying

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 · 20260816T193658Z}

tangletools · 2026-08-16T20:03:04Z

✅ No Blockers — `574bf400`

Review health 100/100 · Reviewer score 55/100 · Confidence 95/100 · 20 findings (1 medium, 19 low)

	opencode GLM 5.2	opencode DeepSeek v4 Flash	aggregate
Readiness	68	55	55
Confidence	95	95	95
Correctness	68	55	55
Security	68	55	55
Testing	68	55	55
Architecture	68	55	55

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 18 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 18 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM checkMultishotGolden with a typo'd only id silently returns ok:true with zero scenarios — src/multishot/golden/harness.ts

const scenarios = multishotGoldenScenarios().filter((s) => !wanted || wanted.has(s.id)) — an only: ['delegatio-three-turns'] (typo) matches nothing, reports stays empty, and ok: reports.every(...) is true. The single-scenario path fails loud (requireRecord throws, harness.ts:78-89) and requireMatrixRecord fails loud, but the batch API silently greens on zero runs — exactly the 'silent fallback' the repo's CLAUDE.md forbids ('No fallbacks. Fail loud.'). A CI job wired to checkMultishotGolden({ only }) can be vacuously green after a scenario rename. Fix: after filtering, throw if any wanted id is absent from the catalog.

🟡 LOW "whole catalog" overstates checkMultishotGolden coverage — docs/multishot-golden-records.md

The doc says "checkMultishotGolden runs the whole catalog in one call", but the function (src/multishot/golden/harness.ts) accepts only engine (no matrix-engine) and iterates multishotGoldenScenarios() alone — the matrix scenarios from multishotMatrixGoldenScenarios() are never run by it. The record-set integrity test covers both catalogs, so "catalog" in this module plausibly includes matrix. Impact: a consumer could expect checkMultishotGolden to cover matrix drift too. The next sentence carves out the matrix pair, so confusion is unlikely. Fix: "runs every shot scenario in one call" or "runs the whole shot catalog (matrix scenarios have their own pair below)".

🟡 LOW Regenerate example implies --version v2 alone mints a runnable record — docs/multishot-golden-records.md

Running the Regenerate command verbatim with --version v2 writes src/multishot/golden/records/v2.json but does NOT register it in records/index.ts or move CURRENT_MULTISHOT_GOLDEN_VERSION — those are manual steps described only in the 'Records are frozen' section (line 60). After the command, goldenRecords() still resolves v1 and the v2 file is inert. Suggest a one-line pointer after the bash block, e.g. 'then register v2.json in records/index.ts and move CURRENT_MULTISHOT_GOLDEN_VERSION.' Everything else in the doc verified accurate against the implementation.

🟡 LOW record:multishot-golden npm script supplies no required args — package.json

The script records/record-multishot-golden.ts requires --version, --engine, and --matrix-engine (requireArg throws '--engine is required'), and the package.json script is just 'tsx scripts/record-multishot-golden.ts' with no args and no default engine (refusal is deliberate per the script header). Running pnpm record:multishot-golden therefore always fails. Not a functional bug — it is a developer-facing convenience entry; suggest wiring the documented example args, or dropping the script and keeping the header's pnpm tsx invocation as canonical.

🟡 LOW One waiver also covers the fabricated served-id occurrence — scripts/model-id-request-allowlist.json

The reason text describes only the request leg ('the literal never reaches a provider'), but the same file+literal waiver also silences the second occurrence at matrix-scenarios.ts:262, where the golden wire fabricates model: 'test/judge-model' in a fake HTTP response — a served-side position the gate cannot distinguish. This is the gate's documented by-design semantics ('matched by file and literal, not by line') and applies equally to the five pre-existing waivers, so it is a breadth note, not a defect: a future genuine request using this exact literal in this exact file would be waived without an allowlist change. No action required; optionally note both occurrences in the reason.

🟡 LOW Freeze invariant is check-then-write; concurrent run can overwrite a frozen record — scripts/record-multishot-golden.ts

existsSync(outFile) at line 146 guards the write at line 197, but the two are separated by the entire capture run (all engines, all scenarios, ~seconds). A second recorder invoked with the same version while the first is capturing would pass its own existsSync check and the later writeFileSync silently overwrites — exactly the re-record-over-truth hazard the file header says the script exists to prevent. Fix: write with writeFileSync(outFile, data, { flag: 'wx' }) so the OS fails the create atomically if the file appeared; kee

🟡 LOW Matrix scenario has no error-outcome path; an engine throw aborts the whole recording run — scripts/record-multishot-golden.ts

captureScenario catches engine throws and records { kind: 'error' }, but captureMatrixScenario lets an engine rejection propagate out of the inner try/finally, aborting all recording (the MultishotMatrixGoldenRecord type has no error branch). A deliberate behaviour change where the reference matrix engine throws cannot be golden-recorded at all — the maintainer must change the engine first. Asymmetric with the shot path and undocumented in the docstring. Not a correctness bug for the current engine (runMultishotMatrix resolves on every scenario), but a sharp edge for the tool's stated purpose of capturing behaviour changes.

🟡 LOW Record write is not atomic; a partial file permanently blocks re-recording — scripts/record-multishot-golden.ts

writeFileSync(outFile, ...) writes directly to the final path. A crash or kill mid-write leaves a truncated version file that the existsSync frozen-guard (line 146) then treats as a released golden, forcing manual deletion. Fix: write to a sibling temp file (e.g. ${outFile}.tmp) then renameSync into place.

🟡 LOW Recorder does not enforce the durationMs contract the checker asserts — scripts/record-multishot-golden.ts

captureScenario records any resolved result via recordResult(), which strips durationMs without checking it. The replay checker (src/multishot/golden/harness.ts:98-107) rejects a result whose durationMs is not finite >= 0 on every live run. A reference engine returning durationMs: NaN is stable across the double capture (NaN === NaN under Object.is is false, but compareJson only compares what the record holds — durationMs is dropped, so the two captures still match) and would be frozen as truth, producing a golden the reference engine itself cannot pass. Impact: a minted record that fails its own check; CI catches it later but the record is wrong. Fix: assert Number.isFinite(durationMs) && durationMs >= 0 in captureScenario before recording, mirroring the harness.

🟡 LOW Recorder skips the durationMs contract check the replay harness enforces — scripts/record-multishot-golden.ts

harness.ts:98-107 rejects a live result whose durationMs is not a finite number >= 0 ('durationMs is wall clock and is excluded from the record, but it is still part of the contract'), yet captureScenario only calls recordResult, which strips durationMs before freezing. If the reference engine ever reported NaN/undefined durationMs, the recorder would happily freeze a record set that the harness then fails for EVERY engine — including the reference loop itself — with no hint the fixture was captured bad. Fix: apply the identical Number.isFinite(result.durationMs) && result.durationMs >= 0 check in captureScenario before accepting the capture. One line, keeps recorder and harness enforcing the same contract.

🟡 LOW Shot-capture path installs no network guard, unlike the matrix path — scripts/record-multishot-golden.ts

captureMatrixScenario wraps the engine in installJudgeWire(), whose stub throws on any fetch to a non-judge URL ('unexpected request to ...'), so a wiring defect in the matrix engine fails loud. captureScenario has no equivalent: if the shot engine bypassed its scripted transports and called global fetch, the recorder would let the call out. Today it cannot reach a real provider because every scenario pins baseUrl to the reserved router.invalid TLD and a DNS failure lands in the recorded error outcome — but that containment is incidental, not enforced. Defense-in-depth only; consider a shared fetch guard that throws on ANY network call during shot captures.

🟡 LOW Unsanitized --version interpolated into a filesystem path — scripts/record-multishot-golden.ts

const outFile = join(outDir, ${version}.json) — a --version value containing a path separator (e.g. '../x') writes outside outDir, and readArg treats a following flag as a value ('--out --version v1' makes --out's value '--version'). Operator-supplied dev tool so no privilege boundary is crossed, but a malformed value silently creates files in unexpected places instead of failing loud. Fix: validate version against /^[a-zA-Z0-9._-]+$/ and reject flag-looking values in readArg.

🟡 LOW Invalid durationMs early-return hides all other divergence detail — src/multishot/golden/harness.ts

When result.durationMs is non-finite or negative, the function returns immediately with only the duration mismatch and never runs compareJson on outcome or requests. An engine that both mis-reports duration AND diverges on transcript/ledger reports just one line, so the reviewer fixes duration first and re-discovers the rest on the next run. Impact is diagnostic only (still ok:false). Fix: append the duration line to the mismatches array and continue to the full comparison.

🟡 LOW checkMultishotGolden with a typo'd only id vacuously reports ok — src/multishot/golden/harness.ts

multishotGoldenScenarios().filter((s) => !wanted || wanted.has(s.id)) silently drops unknown ids, and reports.every(r => r.ok) on an empty array is true. I reproduced this with a probe test: checkMultishotGolden({ engine: async () => { throw new Error('x') }, only: ['typo-scenario-id'] }) returns { ok: true, scenarios: [] }. A CI gate or consumer using only with a stale id after a scenario rename gets a green check while running zero scenarios — the exact silent-pass failure this harness exists to prevent. Fix: after filtering, throw (or set ok:false naming) any only ids that matched no scenario, mirroring requireRecord's fail-loud stance.

🟡 LOW Judge wire swaps globalThis.fetch process-wide during matrix runs — src/multishot/golden/matrix-scenarios.ts

The stub replaces global fetch and any non-judge request in the process during the engine run throws unexpected request to ... (intended fail-loud) or, worse, is recorded into judgeRequests if it happens to target JUDGE_BASE_URL-shaped URL. Restore is correctly in a finally in both harness.ts:173 and the recorder, and current usage is sequential (maxConcurrency 1, vitest file isolation), so this is latent. It becomes a real hazard the day matrix golden checks run under describe.concurrent or alongside other fetching tests in the same process. Acceptable as-is; worth a doc comment on MultishotMatrixGoldenCase.installJudgeWire stating it is process-global and the caller must not run concurrent fetchers.

🟡 LOW Matrix judge recording is coupled to globalThis.fetch, and assertMultishotMatrixGoldenScenario is untested — src/multishot/golden/matrix-scenarios.ts

The deterministic judge wire only intercepts a judge leg that goes through the global fetch (which the reference routerCompletion does). A consumer whose matrix judges via an injected client or a non-global transport gets judgeRequests: [], which can never match the recorded 12 judge calls -> permanent mismatch, and the harness cannot even detect the divergence meaningfully. Documented in the docs, so acceptable for the reference engine's own regression use. Separately, golden.test.ts only exercises checkMultishotMatrixGoldenScenario; the exported throwing assertMultishotMatrixGoldenScenario (harness.ts:185) has no test, so the throwing-variant wiring is unproven.

🟡 LOW Duration mask is coupled to the summary renderer's integer-second format — src/multishot/golden/recording.ts

The regex /\*\*Duration\*\*: \d+s/g matches only integer seconds; the renderer at src/multishot/matrix.ts:411 uses .toFixed(0) so it works today and the passing matrix golden test pins that. If someone changes the renderer to .toFixed(1) (e.g. **Duration**: 1.5s), the mask stops matching and every replay reports a wall-clock mismatch under files[summary.md] — loud, but confusing. Cheaper failure mode: mask \d+(\.\d+)?s.

🟡 LOW maskVolatileMarkdown only matches the exact **Duration**: Ns token — format drift becomes a permanent false mismatch — src/multishot/golden/recording.ts

text.replace(/\*\*Duration\*\*: \d+s/g, ...) only masks that one token. The matrix summary.md is produced by the ENGINE UNDER TEST (matrix.ts writes it), so a consumer engine that renders duration differently (decimal, ms unit, different label, different spacing) fails the mask, and the raw wall-clock value stays in the comparison — every run then mismatches the frozen record on a field that is definitionally non-reproducible. The mask also silently no-ops instead of failing loud when it does not match. Consider masking the whole duration-bearing line or asserting the regex matched.

🟡 LOW recordResult JSON round-trip silently coerces NaN/Infinity to null — src/multishot/golden/recording.ts

JSON.parse(JSON.stringify(rest)) turns a non-finite costUsd/toolCalls into null and drops undefined keys. A divergent engine returning costUsd: NaN is still detected (record holds 0.0166, observed null -> mismatch), but the diagnostic reads 'expected 0.0166, received null' — the confusing shape of a silent-coercion failure — and a non-finite value that the matrix would have rejected (runner.ts billableResult) is presented as a clean null in the record path. Low severity: detection is preserved, only the reported shape is misleading.

🟡 LOW stripVolatile removes wall-clock/run-id keys at ANY depth, which can mask a real divergence — src/multishot/golden/recording.ts

VOLATILE_KEYS (durationMs, meanDurationMs, matrixId, runId) are stripped at every object level, not just the run envelope. If a scenario's recorded tool arguments, artifact content, driver POV text, or a judge prompt legitimately contained a field literally named runId or durationMs, a change to that field would silently vanish from the comparison. The 'every recorded field is compared' mutation tests (golden.test.ts) cannot catch this class because they mutate the raw record values, not the post-strip view. Current records do not hit it, and the choice is documented in types.ts, so this is a tradeoff to keep on record rather than a defect to fix now.

_{tangletools · 2026-08-16T20:02:59Z · trace}

tangletools

✅ Approved — 20 non-blocking findings — `574bf400`

Full multi-shot audit completed 8/8 planned shots over 18 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 18 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-08-16T20:02:59Z · immutable trace}

tangletools · 2026-08-16T20:08:34Z

⚠️ Review Incomplete — `64aacc41`

At least one required reviewer lane failed closed. No approval or request-changes review was published. This is a reviewer run failure, not a PR quality score.

Trigger a fresh review on the current PR head.

_{tangletools · 2026-08-16T20:08:32Z}

tangletools · 2026-08-16T20:15:22Z

⚠️ Review Incomplete — `ccae432d`

At least one required reviewer lane failed closed. No approval or request-changes review was published. This is a reviewer run failure, not a PR quality score.

Trigger a fresh review on the current PR head.

_{tangletools · 2026-08-16T20:15:21Z}

From the multi-shot audit. The medium finding was a real silent pass; the rest harden the freeze invariant the module exists to hold. - `checkMultishotGolden({ only })` reported ok for a run of ZERO scenarios when an id named nothing. A stale id after a rename now stops the check. - A durationMs outside the contract returned alone and hid every other divergence in the same run. It joins the mismatch list instead. - The recorder now refuses a --version that is not a bare file name, refuses a flag where a value belongs, refuses the network during a shot capture, and refuses to freeze a result whose durationMs the check would reject. - The freeze guard was check-then-write across the whole capture run. The record is written to a sibling and linked into place, so a concurrent recorder gets EEXIST and a crash leaves a `.partial` nobody mistakes for a released record. - The summary-duration mask reads fractional and millisecond renderings, and fails loud on a duration it cannot mask rather than leaving wall clock in the comparison. - Dropped the `record:multishot-golden` script: its three required arguments have no defaults by design, so the bare script could only fail. The doc carries the canonical invocation. - Docs name what `checkMultishotGolden` covers and the registration step a new version needs.

drewstone · 2026-08-16T20:21:29Z

Took the medium and most of the low findings.

Fixed

checkMultishotGolden({ only }) reported ok on a run of ZERO scenarios when an id matched nothing. That is the silent pass this module exists to prevent — a stale id after a rename now throws, and a real id still runs. Test added, both directions.
A bad durationMs returned alone and hid every other divergence. It now joins the mismatch list; the test asserts both lines come back together.
Recorder: --version must be a bare file name, a flag where a value belongs is refused, the network is refused during a shot capture, and a result whose durationMs the check would reject can no longer be frozen.
The freeze guard was check-then-write across the whole capture run. The record is written to a sibling and linkSynced into place — a concurrent recorder gets EEXIST, and a crash leaves a .partial nobody mistakes for a released record.
The duration mask reads fractional and millisecond renderings and fails loud on one it cannot mask, instead of silently leaving wall clock in the comparison.
assertMultishotMatrixGoldenScenario now has a test (both the throw-through and the mismatch path).
Dropped record:multishot-golden: its three required arguments have no defaults by design, so the bare script could only ever fail.
Docs: what checkMultishotGolden covers, and the registration step a new version needs.

Accepted, with reasons on record

Matrix capture has no error branch. True and asymmetric. Adding an outcome union to the matrix record for a case no engine exhibits is speculative surface; the shot path covers every error class we have.
recordResult coerces NaN to null. Detection is preserved (record holds a number, observed reads null); only the diagnostic wording is odd. The recorder now refuses to freeze a non-finite duration, which was the path that mattered.
stripVolatile removes volatile keys at any depth. A recorded field literally named runId would vanish from comparison. No record hits it and the tradeoff is written down.

@tangletools review now

…lden records

drewstone · 2026-08-16T20:23:59Z

Version-locked to 0.146.0 across package.json, clients/python/pyproject.toml, agent_eval_rpc/init.py and uv.lock; analyst dependency digest re-pinned; CHANGELOG written.

pnpm verify:package exits 0 on the packed tarball — ./multishot/golden resolves 🟢 from ESM and bundler, records inline into the bundle.

@tangletools review now

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	1 of 2 lenses (usefulness)
Concerns	1 (1 weak-concern)
Heuristic	0.0s
Duplication	0.1s
Interrogation	417.5s (2 bridge agents)
Total	417.6s

⚠️ Partial audit — the verdict covers only usefulness. value: cli-bridge admission rejected (queue saturated). Treat the missing lens as unexamined, not as clear.

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 4
Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=4/48 — no model was started

🎯 Usefulness — sound

A well-built golden-record regression harness that freezes the multishot loop's request ledger and outcomes, is load-bearing in this repo's CI today, and slots exactly onto the pre-existing runShot engine seam that consumer graph engines will implement.

Integration: Reachable now and imminently. In-repo, src/multishot/golden/golden.test.ts:79-106 runs the reference loop against the records (54/54 pass), so the freeze is load-bearing CI surface from day one, not dormant. Export wiring is complete: package.json subpath ./multishot/golden, scripts/build-entries.mjs:17, tsdown entry, and dist/multishot/golden/index.js verified to bundle the v1.json records inline
Fit with existing patterns: Fits the codebase grain rather than competing. The repo's established orchestration-correctness pattern is request-body parity (src/analyst/definition-parity.test.ts:158, 'prime arm parity' asserting byte-identical requests); this extends the same idea — an issue-ordered request ledger compared field-for-field — to the multishot orchestrator, with the outcome added because the loop's value contrac
Real-world viability: Holds up well past the happy path. Determinism is enforced, not assumed: every scenario is a closed system (scripted transports/executors), the recorder captures each scenario twice and fails on drift (scripts/record-multishot-golden.ts:176-186), and the network is refused during capture. Volatility is handled with fail-loud guards: VOLATILE_KEYS stripped, and maskVolatileMarkdown throws on a dura
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 4

🎯 Usefulness Audit

🟡 Public subpath exports the recorder's internal normalizers, which have no plausible external caller [ergonomics] ``

src/multishot/golden/index.ts:50-61 re-exports recordMessage, recordRequest, recordJudgeRequest, maskVolatileMarkdown, VOLATILE_KEYS, etc. The only in-repo consumers of these are the recorder script and the harness itself (which import from source paths), and a consumer minting a new version runs this repo's recorder with --engine pointing at their engine (docs/multishot-golden-records.md:72-86) — they never call the normalizers through the package. Harmless (pure functions, stable), but a tight

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 · 20260816T203147Z}

tangletools · 2026-08-16T20:33:56Z

⚠️ Review Incomplete — `ccae432d`

At least one required reviewer lane failed closed. No approval or request-changes review was published. This is a reviewer run failure, not a PR quality score.

Trigger a fresh review on the current PR head.

_{tangletools · 2026-08-16T20:33:55Z}

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	2 of 2 lenses (value, usefulness)
Concerns	1 (1 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	259.9s (2 bridge agents)
Total	259.9s

💰 Value — sound

Freezes the multishot loop's wire behavior (13 shot + 1 matrix scenarios, request ledger + outcome + persisted files) into versioned append-only golden records with a framework-free check harness, published as a subpath so any replacement engine can be regression-tested without keeping the loop aliv

What it does: Adds @tangle-network/agent-eval/multishot/golden: a catalog of 14 deterministic scenarios (scenarios.ts:408-525, matrix-scenarios.ts:186-195) with scripted agent/driver transports that record every transport call they receive; a frozen fixture records/v1.json (~206KB) holding, per scenario, the full request ledger (model, temperature, per-leg token budget, advertised tools, message log) plus t
Goals it achieves: Enable deleting the legacy loop orchestrator without losing its behavior as a regression contract. The loop-to-graph parity was proven once (merged); this converts that proof into a durable, replayable oracle so the graph engine — in this repo or a consumer — can be checked continuously against the loop's recorded wire behavior. The request ledger specifically catches the divergences that don't ch
Assessment: Good on its merits, and unusually rigorous for a fixture freeze. Strengths: (1) freeze integrity — double-capture determinism check, network forbidden, atomic claim-via-hardlink, no-overwrite of a released version, all in scripts/record-multishot-golden.ts; (2) one shared normalizer for recorder and checker (recording.ts:1-6) so a field can't normalize differently on each side; (3) framework-free
Better / existing approach: none — this is the right approach. Searched for overlap: src/golden-matcher.ts is a fuzzy phrase-severity matcher for expected findings, unrelated to byte-exact orchestration fixtures; the repo's many compare*/diff* helpers (src/analyst/findings-store.ts:115, src/contract/diff.ts:150, src/reference-replay.ts:390, etc.) are all domain-specific and none is a generic structural comparator emitting do
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 3
Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Freezes the multishot loop's observable contract (per-leg request ledger, outcome, matrix artifacts) as append-only golden records with a framework-free checker — already load-bearing in this repo's CI and purpose-built for the consumer parity tests that unblock loop deletion (step 2 of #617).

Integration: Reachable now and by design later. Immediate caller: src/multishot/golden/golden.test.ts:79-106 runs the live runMultishot/runMultishotMatrix against the frozen v1 records in CI — I ran the suite (54/54 pass), so the loop's orchestration is pinned the moment this merges. External wiring is complete: subpath export in package.json (./multishot/golden), build entry in scripts/build-entries.mjs:17,
Fit with existing patterns: Fits the codebase's grain rather than competing with it. There is no existing golden-record mechanism to duplicate: docs/eval-fixtures.md is a different concept (campaign scenario folders), and the tb-* scripts are one-off research scripts. The freeze-then-pin posture mirrors established repo patterns — pinned digests (analyst benchmark), the model-id literal allowlist it correctly extends (script
Real-world viability: Built for drift, not the happy path. Determinism is enforced rather than assumed: every scenario is captured twice and an unstable capture fails the recorder (scripts/record-multishot-golden.ts:184-193), network is refused during capture (line 58), unscripted transport calls fail loud (scenarios.ts:292-299), and the process-global judge wire refuses a second concurrent install (tested at golden.te
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 1

💰 Value Audit

🟡 Delegation transport scripts near-duplicated between scenarios.ts and matrix-scenarios.ts [duplication] ``

scenarios.ts:100-170 and matrix-scenarios.ts:93-156 carry almost identical agent/driver scripts (same turn structure, same canned replies). The differences are intentional — the shot script's turn 1 adds an unknown tool with unparseable args to pin error paths, while the matrix script keeps turn 1 clean so every cell yields both artifact kinds and all judge slots fire — so merging them behind flags would couple the two catalogs and hurt per-file legibility. Note for the reviewer only; not worth

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 · 20260816T203952Z}

0.145.22 landed on main mid-flight. The version-lock files take main's values and move forward together; the CHANGELOG keeps 0.145.22's entry and adds this release above it; the analyst dependency digest is re-pinned for the moved package.json.

drewstone · 2026-08-16T20:45:14Z

Rebased onto 0.145.22 (#626 landed mid-flight). Version-lock files take main's values and move forward together to 0.146.0; the CHANGELOG keeps 0.145.22's entry and adds this release above it; the analyst dependency digest is re-pinned for the moved package.json.

pnpm verify:package exits 0 on the packed tarball. Typecheck, lint and the golden suite (54 tests) are green on the merged head.

@tangletools review now

tangletools · 2026-08-16T20:48:48Z

✅ No Blockers — `247fab07`

Review health 100/100 · Reviewer score 59/100 · Confidence 95/100 · 27 findings (27 low)

	opencode GLM 5.2	opencode DeepSeek v4 Pro	opencode DeepSeek v4 Flash	aggregate
Readiness	65	80	59	59
Confidence	95	95	95	95
Correctness	65	80	59	59
Security	65	80	59	59
Testing	65	80	59	59
Architecture	65	80	59	59

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 18 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 18 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 18 changed files. Global verifier still owns final merge decision.

🟡 LOW 'No clock in a recorded field' is overbroad: the frozen fixture envelope carries recordedAt — docs/multishot-golden-records.md

Determinism rules say 'No network, no clock in a recorded field, no random number.' The fixture envelope does contain a clock: records/v1.json line 5 recordedAt (plus recordedFrom/recordedFromPackageVersion). This does not break determinism because the check never compares envelope fields — checkMultishotGoldenScenario compares only outcome/requests, the matrix path adds matrix/judgeRequests/files (harness.ts:113-114,190-193) — so the guarantee holds for compared fields. Suggest tightening to 'no clock in a field that is compared' so the statement is not literally false.

🟡 LOW Doc asserts the package holds no conversation engine, but the loop exists at this commit — docs/multishot-golden-records.md

Line 70 says 'This package holds no conversation engine, so the reference has to be named.' At this commit src/multishot/multishot.ts#runMultishot and src/multishot/matrix.ts#runMultishotMatrix exist, v1 was captured from exactly those (records/v1.json line 3, recordedFrom), and golden.test.ts:82/98 replays them. The true rule — the recorder never defaults an engine (requireArg forces --engine, scripts/record-multishot-golden.ts:191-192) — does not need the false premise; the script's own header hedges 'after the loop is deleted' ([l

🟡 LOW New doc is not linked from any doc index (README, concepts, CLAUDE.md) — docs/multishot-golden-records.md

grep for 'multishot' across README.md, docs/concepts.md, and CLAUDE.md returns zero matches, so this 102-line subsystem doc is unreachable from the repo's documented entry points. CLAUDE.md's own convention says 'Update the doc closest to the change... cross-link', and CLAUDE.md maintains the doc map. Impact: a consumer evaluating the new multishot/golden export finds it only via the module docstring (index.ts:30 links back to this file, but nothing links forward from the human-facing docs). Fix: add one line to README.md's or CLAUDE.md's doc list pointing at docs/multishot-golden-records.md.

🟡 LOW --out value is not flag-guarded — scripts/record-multishot-golden.ts

readArg('--out') is used raw: resolve(repoRoot, readArg('--out') ?? 'src/multishot/golden/records'). Unlike requireArg (line 84) there is no startsWith('--') check, so --out --version v1 (out value omitted) resolves outDir to <repo>/--version and writes the record there, and --out as the last token silently falls back to the default. No security impact (operator-controlled, version is regex-validated) but it is an inconsistent parse and a confusing silent mis-target. Fix: route --out through requireArg or add the same flag-guard.

🟡 LOW Concurrent recorders share one .partial path — scripts/record-multishot-golden.ts

Two recorders minting the same version both writeFileSync to the identical ${outFile}.partial; cross-process interleave could in principle let one process linkSync a file the other is mid-truncating. linkSync's EEXIST still guarantees exactly one claimant of the final name and the loser exits 1, so the freeze itself holds; the residue risk is a corrupted linked record in a two-maintainer race. Theoretical for a single-maintainer dev script. Fix: embed pid+timestamp in the partial name (.partial.<pid>).

🟡 LOW Concurrent same-version recorders share the .partial temp path — scripts/record-multishot-golden.ts

tempFile = ${outFile}.partial (line 246) is not unique per recorder process. If two recorders claim the same version into the same outdir concurrently: A writes temp, B truncates+rewrites temp, A's linkSync(temp,outFile) then publishes B's content while A reports success, and B's linkSync fails EEXIST with the misleading 'another recorder may hold it'. The frozen-record invariant still holds (outFile is never overwritten once linked), and the race needs two operators recording the same version simultaneously — an operator error — so impact is negligible. A per-process suffix on the temp file (or writeFileSync with 'wx') would close it.

🟡 LOW Empty-string --out writes the record into the repo root — scripts/record-multishot-golden.ts

readArg('--out') returns '' for a trailing empty value; '' ?? 'src/multishot/golden/records' keeps the empty string (only nullish triggers the default), so resolve(repoRoot, '') = repoRoot and the record lands at /v1.json, an unregistered location the freeze/registration flow does not expect. Contrast requireArg, which rejects empty via if (!value). Fix: use || instead of ?? for --out, or route it through the same emptiness check.

🟡 LOW Engine returning a non-object is frozen as an incidental TypeError outcome — scripts/record-multishot-golden.ts

If the loaded engine resolves undefined/null, result.durationMs throws 'Cannot read properties of undefined (reading durationMs)', which lacks the sentinel prefix, so the catch freezes that incidental message as the golden error outcome instead of rejecting the broken engine. The harness side (harness.ts:98) records a different destructuring message for the same defect ('Cannot destructure property durationMs of result'), so every later check mismatches — fail-loud, no silent green, but the frozen record is confusing garbage. Fix: before touching fields, throw a sentinel error when result is not a non-null object.

🟡 LOW Matrix engine errors propagate without scenario or recorder context — scripts/record-multishot-golden.ts

Unlike captureScenario (which classifies engine throws as error outcomes), the matrix path has no catch around await engine(runCase.options) (line 160). A matrix-engine throw reaches main().catch with a raw stack and no mention of which matrix scenario was being captured or that the failure happened during recording. This is consistent with the harness (checkMultishotMatrixGoldenScenario also propagates engine errors), so it is a diagnostics gap, not a correctness bug.

🟡 LOW Network guard only replaces globalThis.fetch, and bypass errors get recorded as outcomes — scripts/record-multishot-golden.ts

forbidNetwork reassigns only globalThis.fetch (lines 66-69). An engine that reaches the network via node:http/https or undici bypasses it; in captureScenario such a call to baseUrl http://router.invalid fails with a DNS error whose message has no record-multishot-golden: prefix, so line 137-138 records it as a {kind:'error'} outcome and freezes it, contradicting the docstring's claim that 'a capture that reaches the network... fails loud instead of leaving'. Mitigated in practice by the two-capture stability ch

🟡 LOW Network tripwire stubs only globalThis.fetch — scripts/record-multishot-golden.ts

The guard replaces globalThis.fetch, but an engine that imports undici's fetch directly, uses node:http/https, or captured a fetch reference at module load (loadEngine imports the engine module BEFORE any capture installs the stub) bypasses it. The header comment 'A capture that reaches the network ... the call fails loud' overstates the guarantee. Impact is low because scenarios inject scripted transports, so any wire call is a wiring defect and known engines use globalThis.fetch — verified the fetch path aborts correctly. Fix: soften the comment to 'globalThis.fetch', or move the engine import after installing a process-wide guard.

🟡 LOW Recorder script has no automated test coverage — scripts/record-multishot-golden.ts

The 265-line recorder (arg validation, version-freeze via existsSync+linkSync, double-capture stability enforcement, duration guard, error-outcome recording) is exercised only manually; tsconfig.script.json typechecks it and biome lint only covers src/. Its output is validated downstream by golden.test.ts (54 tests, including the perturbation suite proving every recorded field is compared), and this review confirmed the reference engine reproduces v1 byte-for-byte, so the risk is bounded. A small vitest that runs the script against a fixture engine asserting the freeze/refusal paths would close the gap.

🟡 LOW network guard only intercepts globalThis.fetch — scripts/record-multishot-golden.ts

forbidNetwork (lines 65-73) and the matrix judge wire (matrix-scenarios.ts installJudgeWire) both guard only globalThis.fetch. The script's engine is arbitrary — loaded from any module via --engine/--matrix-engine — so an engine that reaches the network through a captured fetch binding, undici, node:http/https, or an axios-like client bypasses the guard and its non-scripted response is recorded. The double-capture stability check is the authoritative backstop for non-determinism, but a deterministic remote response would be frozen silently, contrary to the comment's 'fails loud instead of leaving' claim. Low because the shipped reference engine goes t

🟡 LOW same-version concurrent recorders race on a shared .partial path — scripts/record-multishot-golden.ts

tempFile is ${outFile}.partial, keyed only by version. The atomic-freeze intent (comment lines 240-245) is that linkSync EEXIST prevents overwrite, but writeFileSync to that shared path is not atomic: if two recorders run the same version concurrently, recorder B can overwrite recorder A's .partial before A's linkSync fires, so A freezes B's bytes as the record. The final file is still only claimed once (no overwrite), but the winning record's content is not guaranteed to be the winner's capture. Probability requires deliberately invoking the same version twice at once; noted for completeness. Fix: derive tempFile from the process id or a random suffi

🟡 LOW Exact float equality on cost sums makes records brittle to summation reordering — src/multishot/golden/compare.ts

walk() compares scalar leaves with Object.is (line 74), and recording.ts keeps costUsd as-is. The fixture freezes floating-point artifacts such as delegation-driver-empty cellSpend costUsd = 0.016659999999999998. A future engine that sums the same contributions in a different order (a+b+c vs c+b+a) yields a different representable double and fails the check even though the result is semantically identical. This is inherent to the golden approach and not a defect in the current engine (the reference loop reproduces the record byte-for-byte), but it means any cost-aggregation refactor forces a new fixture version. Acceptable; flagged so reviewers know the comparis

🟡 LOW compareJson limit is approximate; large divergences can emit unbounded mismatch lines — src/multishot/golden/compare.ts

The limit option (default 25) is checked at walk entry and after the row-branch recursive walk, but the missing-key pushes (expected X, received nothing / expected nothing, received Y, lines 60-66) and the array length-mismatch push (line 41) continue/return without re-checking out.length >= limit. A structural divergence high in the tree can therefore exceed the cap, and MultishotGoldenMismatchError (harness.ts:51-58) prints every line. Impact is cosmetic — the ok flag is unaffected — but the failure message can b

🟡 LOW In-repo golden test is self-referential; it cannot catch a coordinated engine+fixture regression — src/multishot/golden/golden.test.ts

The 'reference loop reproduces the golden records' tests (79-106) run the very engine that captured v1.json (recordedFrom = runMultishot + runMultishotMatrix). That makes the suite a true regression detector for future engine changes and for consumer engines, but if an engine bug and a fixture regeneration land in the same PR, the tests green. The recorder's append-only freeze (record-multishot-golden.ts linkSync EEXIST, refuses overwrite) is the main mitigation; consider a CI check that the fixture matches an external pinned engine.

🟡 LOW Matrix engine throws propagate raw while shot throws are normalized to outcomes — src/multishot/golden/harness.ts

The shot check converts an engine throw into {kind:'error'} and reports mismatches; the matrix check lets the throw escape before any comparison runs, so a partially-divergent matrix run that also throws reports only the throw, never the requests/judgeRequests/files it already collected. The behavior is deliberate (matrix records have no error variant; golden.test.ts:362 pins it) and fail-loud, but it loses already-captured divergence evidence. Fix if desired: wrap the engine call, run the four comparisons on whatever ledgers filled, then append engine: threw <name>: <message> as a mismatch line.

🟡 LOW Judge wire accepts any URL with the JUDGE_BASE_URL prefix, not just the real judge endpoint — src/multishot/golden/matrix-scenarios.ts

String(url).startsWith(JUDGE_BASE_URL) (line 258) accepts e.g. http://router.invalid/v1-other/... as a judge call and records it into the judge ledger. Inert in the closed scenario — routerCompletion produces exactly <base>/chat/completions — but a stricter match on the /chat/completions suffix would remove the ambiguity and keep the 'unexpected request' fail-loud guard precise.

🟡 LOW Judge wire replaces globalThis.fetch process-wide and is non-reentrant — src/multishot/golden/matrix-scenarios.ts

installJudgeWire() overwrites globalThis.fetch (line 254) and guards re-entry with a module-level boolean judgeWireInstalled (line 247). The wire throws on ANY fetch whose URL is not JUDGE_BASE_URL (line 258). This is intentional and the single-wire guard is tested, but it means: (a) matrix golden checks must run serially within one process (a second concurrent install throws '

🟡 LOW Judge-wire URL check is a string prefix, so sibling paths under the same prefix are accepted — src/multishot/golden/matrix-scenarios.ts

String(url).startsWith(JUDGE_BASE_URL) accepts 'http://router.invalid/v1-evil/chat/completions'. The wire would then parse the body and record it as a judge request, and the divergence would appear as a judgeRequests/files mismatch rather than the intended 'unexpected request to ...' fail-loud error. No security impact (test-only wire, .invalid host), but the guard's stated intent is to reject wiring defects immediately. Fix: compare the normalized base (url.startsWith(JUDGE_BASE_URL + '/')) or URL-parse and compare origin+path prefix.

🟡 LOW Volatile-key name stripping is a blind spot for consumer fields that collide with the volatile names — src/multishot/golden/recording.ts

stripVolatile (95-106) drops any key literally named durationMs, runId, matrixId, or meanDurationMs at any depth, on both the record and live sides. A consumer engine whose transcript/result carries an extended field with one of these names would have that field removed identically on both sides, so a divergence in it would be invisible to the check. Documented as intentional (types.ts:20-27), but it is a genuine detection gap; name-colliding extensions should be renamed or explicitly surfaced.

🟡 LOW readRunDir surfaces an opaque ENOENT when runDir does not exist — src/multishot/golden/recording.ts

checkMultishotMatrixGoldenScenario calls readRunDir(opts.runDir), whose walkFiles does readdirSync(dir) first. A caller that passes a nonexistent or not-yet-created runDir gets a raw 'ENOENT: no such file or directory' with no mention of the golden check or the offending scenario, unlike every other failure path in this harness which names its cause. Impact: confusing first-run DX for external consumers, not a correctness bug (it still fails loud). Fix: stat the dir up front and throw multishot golden: runDir <path> does not exist — create an empty directory before the check.

🟡 LOW recordJudgeRequest silently coerces malformed wire bodies into placeholder fields — src/multishot/golden/recording.ts

A judge fetch with no body (init?.body ?? '{}') or missing fields records {model:'(missing model)', temperature:null, messages:[]} instead of throwing. This contradicts the repo's stated 'no fallbacks, fail loud' doctrine: a wiring defect that drops the body would surface later as a confusing judgeRequests mismatch ('(missing model)') rather than an immediate 'judge wire received a bodyless request'. Contrast with maskVolatileMarkdown and the wire's unknown-judge guard, which do fail loud. Fix: throw in the wire (or in recordJudgeRequest) when model/messages are absent from a body that reached the judge URL.

🟡 LOW Fixture JSON cast to MultishotGoldenRecordSet without structural validation — src/multishot/golden/records/index.ts

v1 as MultishotGoldenRecordSet is an unchecked cast; a hand-edited or truncated v1.json would degrade into deep, misleading compareJson mismatches (or a TypeError mid-walk) instead of 'fixture v1 is malformed'. The test file only asserts id alignment and a few outcome-kind presences, so a broken requests array shape is caught only at first use. Impact is low because the recorder script is the only sanctioned writer and refuses overwrites. Fix (cheap): validate the parsed set once at load (version string, non-empty scenarios with id/requests/outcome) and throw a named error naming the file.

🟡 LOW goldenRecords() returns the process-wide fixture by reference; nothing enforces the documented freeze — src/multishot/golden/records/index.ts

VERSIONS and its nested v1 object are module singletons returned directly to callers (line 31). The types.ts:82-85 doc declares records 'append-only / frozen', but nothing freezes them: a consumer that mutates the returned set (e.g. set.scenarios.push(...) or edits a record in place) corrupts the shared fixture for every later check in the same process and can silently mask future mismatches. Fix: Object.freeze the set and its nested records, or return a structured clone.

🟡 LOW v1.json is cast to MultishotGoldenRecordSet without a runtime shape check — src/multishot/golden/records/index.ts

v1 as MultishotGoldenRecordSet (line 12) is an unchecked cast: a fixture that drifts from the type (renamed or missing field) compiles cleanly and only fails later at a check site. The in-repo golden test does exercise every fixture field (golden.test.ts:27-77, 209-399), so drift would surface there; a light runtime validation in goldenRecords() would catch it earlier for consumers who never run the fixture-integrity tests.

_{tangletools · 2026-08-16T20:48:45Z · trace}

tangletools

✅ Approved — 27 non-blocking findings — `247fab07`

Full multi-shot audit completed 8/8 planned shots over 18 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 18 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 18 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-08-16T20:48:45Z · immutable trace}

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	2 of 2 lenses (value, usefulness)
Concerns	0 (none)
Heuristic	0.0s
Duplication	0.1s
Interrogation	552.9s (2 bridge agents)
Total	553.0s

💰 Value — sound

Freezes the multishot loop's observable behavior (per-leg request ledger + outcomes + matrix files) into append-only golden fixtures published as a package subpath, with a self-calibrating, framework-free replay harness — the right way to keep regression detection after the loop is deleted; no exist

What it does: Adds @tangle-network/agent-eval/multishot/golden: 13 deterministic shot scenarios plus 1 matrix scenario (scripted transports, fixed budgets, no clock/network/random — scenarios.ts:1-17), a recorder script (scripts/record-multishot-golden.ts) that captures each scenario TWICE and refuses unstable or overwritable outputs and writes atomically via linkSync, a 206KB frozen v1.json captured from the
Goals it achieves: Make it possible to delete the loop orchestrator (PR states deletion follows) without losing regression detection on the graph engine that replaces it, and let consumer repos verify their engines against published frozen records instead of each maintaining a second live orchestrator as oracle. The request ledger is the load-bearing insight: two orchestrators can diverge (turn boundary, token budge
Assessment: Good on its merits, and in the grain of the codebase. (1) Layering: golden imports only from ../matrix, ../types, ../multishot and @tangle-network/agent-interface — no upward dependencies, per the repo's substrate rule. (2) The oracle is calibrated, not assumed: golden.test.ts:209-271 proves an exact replay passes AND every single leaf of every record is covered by some comparison (uncomparedLeave
Better / existing approach: none — this is the right approach. Searched for existing equivalents: src/golden-matcher.ts / tests/golden-matcher.test.ts shares the word 'golden' but is a different concept (golden failure specs matched against judge candidates — no overlap). No vitest snapshot usage exists in the repo, and snapshots are repo-local and vitest-bound, so they cannot serve as a published cross-repo contract for c
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 8
Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A working, already-wired regression harness that freezes the multishot loop's observable behavior (request ledger + outcome + matrix files) into frozen records with a mutation-proving test suite and a replayable consumer API — exactly the substrate the planned loop deletion needs.

Integration: Reachable now and by named imminent callers. (1) In-repo: src/multishot/golden/golden.test.ts:79-106 runs all 13 shot + 1 matrix scenarios against the live runMultishot/runMultishotMatrix — verified green (54/54, 3.8s) — so the records regression-detect the loop in CI today. (2) External: the subpath @tangle-network/agent-eval/multishot/golden is registered in package.json exports and scripts/buil
Fit with existing patterns: Fits the codebase's established engine-swap seam: the matrix runner already accepts a runShot override (src/multishot/matrix.ts:211), and MultishotGoldenEngine (src/multishot/golden/engine.ts:14) is precisely the shape a runGraph adapter satisfies. The harness asserts by throwing, not by a test framework, matching cross-repo consumption. No competing golden-record mechanism exists — src/golden-mat
Real-world viability: Built for determinism under real conditions, not just the happy path: double-capture reproducibility enforced at record time; wall-clock/run-identity keys stripped with a fail-loud mask for unrecognized duration formats (recording.ts:115-125); judge Promise.all fan-out compared as a sorted set (recording.ts:131-139); a second concurrent process-global judge wire refused instead of corrupting the l
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 2

No concerns — sound change, no better or existing approach found. ✅

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 · 20260816T205519Z}

tangletools · 2026-08-16T20:55:55Z

✅ No Blockers — `47befa2b`

Review health 100/100 · Reviewer score 59/100 · Confidence 95/100 · 18 findings (18 low)

	opencode GLM 5.2	opencode DeepSeek v4 Pro	aggregate
Readiness	59	74	59
Confidence	95	95	95
Correctness	59	74	59
Security	59	74	59
Testing	59	74	59
Architecture	59	74	59

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 22 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 22 changed files. Global verifier still owns final merge decision.

🟡 LOW "No existing export changed" contradicted by the PR's own analyst digest repin — CHANGELOG.md

The 0.146.0 entry states 'No existing export changed.' But this same PR changes the value of the existing export ANALYST_BENCHMARK_DEPENDENCY_LOCK_SHA256 in src/analyst/benchmark-implementation.ts (fd0de8a8… → 67f8190c…, commit 574bf40 'repin the analyst digest'). The repo's own convention treats this as changelog-worthy: the 0.145.20 entry logged 'Refreshed the analyst benchmark dependency-lock hash for this version' under Changed (CHANGELOG.md:65). Impact: a reader diffing 0.145.21..0.146.0 to assess upgrade risk is told no export changed when one exported constant did. Fix: either scope the sentence ('No existing multishot export changed…') or add the conventional 'Refreshed the analyst benchmark dependency-lock hash for this version' bullet.

🟡 LOW Request-ledger prose says 'four places' then lists five — CHANGELOG.md

'with its model, temperature, token budget, advertised tool definitions and full message log — the four places two orchestrations diverge' enumerates five fields (and MultishotRecordedRequest carries a sixth, leg). Cosmetic miscount in a shipped changelog; fix by dropping the count or correcting it.

🟡 LOW Duplicated version string in init.py fallback is drift-prone — clients/python/src/agent_eval_rpc/__init__.py

The fallback version = "0.146.0" must be manually kept in sync with pyproject.toml on every release. Correct in this commit, but a release script or test asserting pyproject/uv.lock/init parity would prevent silent drift. Pre-existing pattern, not introduced by this PR; no action required to merge.

🟡 LOW Doc claims the package holds no conversation engine; the loop exists at head — docs/multishot-golden-records.md

Line 70: 'This package holds no conversation engine, so the reference has to be named' is stated as present fact, but src/multishot/multishot.ts exports runMultishot at head, and this PR's own test suite runs it as 'the reference loop' (src/multishot/golden/golden.test.ts:79-84). The recorder script states the accurate rationale as a future conditional ('after the loop is deleted this repository holds no conversation engine', scripts/record-multishot-golden.ts:103). A reader at this commit is told no engine exists while v1.json and the suite both point at one. Impact: minor doc/repo-state contradiction in an otherwise line-accurate doc. Fix: reword to the scrip

🟡 LOW Doc overstates 'every field that moved' vs 25-mismatch cap — docs/multishot-golden-records.md

The doc states assertMultishotGoldenScenario throws MultishotGoldenMismatchError 'listing every field that moved'. compareJson defaults to limit=25 mismatches (src/multishot/golden/compare.ts:22), so a heavily divergent run reports at most 25 lines, not every moved field. Impact: minor doc inaccuracy; a reader may expect an unbounded diff. Fix: say 'listing the first moved fields' or 'up to 25 moved fields'.

🟡 LOW --out value is not flag-checked and the forbidNetwork doc sentence is truncated — scripts/record-multishot-golden.ts

readArg('--out') (unlike requireArg) accepts a following flag as its value, so '--out --engine' would silently write to a directory named '--engine' after failing arg parsing elsewhere; and the forbidNetwork doc at line 33 ends mid-sentence ('fails loud instead of leaving.'). Both cosmetic for an operator-run dev script; requireArg-style validation for --out and finishing the sentence would polish it.

🟡 LOW Recorder guards have no automated test coverage — scripts/record-multishot-golden.ts

golden.test.ts (399 lines) thoroughly covers the harness, compareJson, scenarios, and record integrity, but nothing exercises this script: requireArg/requireVersion path-traversal regex, the existsSync+linkSync freeze, the double-capture requireStable failure path, forbidNetwork refusal, or .partial cleanup. v1.json proves one successful run, not that the guards stay correct. Recommend extracting the pure guards (requireVersion, the freeze/write step, stability check) into testable helpers, or a single integration test that runs the script with a temp --out dir and asserts: (a) an existing version refuses, (b) a non-deterministic engine fails, (c) an invalid version is rejected. Impact: regression risk on a tool whose whole purpose is producing trustworthy frozen fixtures.

🟡 LOW Script-error classification relies on a message-prefix convention — scripts/record-multishot-golden.ts

The catch rethrows only errors whose message starts with 'record-multishot-golden:'. Two edge directions: (a) an engine-under-test error that happens to carry that prefix would abort recording instead of being frozen as the scenario outcome; (b) recordResult(result) runs inside the try, so a recorder-side failure (e.g. JSON.stringify on a circular result field throwing TypeError) would be frozen as {kind:'error'} — a record asserting the engine threw when the recorder did. Direction (b) stays replay-consistent because the checker (harness.ts:97-110) applies the same normalizer inside its own try, so it degrades to a permanent mismatch rather than a false green. A typed marker (e.g. a symbol-carried flag or throwing the duration/normalization checks before the try) would remove the ambiguit

🟡 LOW Shared .partial temp path: concurrent same-version recorders can link the wrong process's bytes — scripts/record-multishot-golden.ts

The freeze relies on ${outFile}.partial being written (line 247) then linkSync'd (line 249). The EEXIST on linkSync correctly protects the final frozen record, but the .partial path is shared mutable state keyed only by version. Two recorders of the SAME version that both pass the existsSync check at line 196 (both started during the multi-second capture window) write to the same .partial file

🟡 LOW forbidNetwork guards globalThis.fetch only, and a swallowed fetch error escapes it — scripts/record-multishot-golden.ts

The network guard replaces globalThis.fetch; an engine that calls node:http/https directly bypasses it, and an engine that catches the guard's throw internally (retry wrapper) defeats it silently while the record captures normally. Mitigated in depth: every scenario pins baseUrl to http://router.invalid and injects scripted transports, and the matrix judge wire rejects any non-judge URL (matrix-scenarios.ts:258), so a real network egress would fail DNS anyway — the guard is tripwire, not boundary. No evidence any engine under test uses non-fetch transports; recorded as a limitation, not a defect.

🟡 LOW linkSync failure (non-EEXIST) misdiagnosed as a competing recorder — scripts/record-multishot-golden.ts

The catch around linkSync wraps EVERY error as 'could not claim ... another recorder may hold it'. On filesystems without hard-link support (some network/overlay mounts, FAT), linkSync throws EPERM/ENOSYS and the operator is pointed at a phantom second recorder. Fix: re-throw EEXIST with the intended message and surface other errno values verbatim. Impact: diagnostic-only; recording correctly fails either way.

🟡 LOW Empty only: [] filter greens a zero-scenario run — src/multishot/golden/harness.ts

The guard at line 150 rejects only ids absent from the catalog, but an explicitly empty list slips through. With only: [], wanted is a truthy empty Set, unknown is empty, and scenarios = catalog.filter((s) => !wanted || wanted.has(s.id)) yields []; reports.every(r => r.ok) on an empty array is true, so the check reports ok with zero scenarios run. The code's own comment (line 148-149) states a zero-scenario run must not green, but only the stale-id path is guarded. Fix: also throw when only is present and empt

🟡 LOW Matrix check propagates engine throws instead of reporting a mismatch row — src/multishot/golden/harness.ts

Unlike checkMultishotGoldenScenario (harness.ts:96-110), which catches an engine throw and diffs it against the record's error outcome, the matrix check lets await opts.engine(runCase.options) reject straight through, so a throwing matrix engine never produces a MultishotGoldenScenarioReport and the 'report every divergence' contract in the doc comment does not hold for the most broken engine. The behavior is demonstrated deliberately in golden.test.ts:365-373 (rejects.toThrow('matrix engine exploded')), so it is a design choice, but a consumer looping scenarios gets an unhandled rejection rather than a red report row. Fix: catch, record the throw via recordError, and compare against the record (or add an error kind to MultishotMatrixGoldenRecord) — or at minimum state the asymmetry in

🟡 LOW Matrix harness never validates matrix duration usability — src/multishot/golden/harness.ts

The shot harness treats durationMs as part of the contract: it asserts isUsableDuration(result.durationMs) (line 103) even though durationMs is excluded from comparison. The matrix harness has no equivalent — stripVolatile(matrix.matrix) (line 190) silently deletes summary.durationMs and per-cell durationMs, so a custom matrix engine returning durationMs: NaN or a negative value passes the check with no divergence reported. Asymmetric coverage of the same stated invariant. Fix: after matrix = await opts.engine(...), asse

🟡 LOW Judge-wire guard is per module instance, not per process — src/multishot/golden/matrix-scenarios.ts

The judgeWireInstalled flag guards concurrent installs within one module instance. If a consumer's environment loads two copies of the module (dual CJS/ESM package instances, or a bundler duplicating the subpath), each copy holds its own flag while sharing the process-wide globalThis.fetch, so two matrix checks could still cross their judge ledgers — exactly the silent mixture the guard exists to prevent. The saved previous fetch would also chain the first wire under the second. Practically irrelevant for the documented vitest/node:test serial usage and the guard correctly refuses the same-instance case (tested at golden.test.ts:305-318); noting the boundary. A belt-and-braces marker on the installed function (e.g. a symbol property checked instead of the boolean) would make the guard

🟡 LOW readRunDir JSON.parse error carries no file context — src/multishot/golden/recording.ts

stripVolatile(JSON.parse(text)) at line 150 throws a raw SyntaxError (Unexpected token ... in JSON at position N) when the engine under test writes malformed JSON into its run directory. The rest of the harness reports divergences through MultishotGoldenMismatchError naming the scenario and the dotted field path; this path fails with no file name, no scenario id, and no path. Fail-loud is intended and correct, but the message is not actionable for diagnosing which file is corrupt. Fix: wrap the parse and rethrow with the relative key in the message.

🟡 LOW readRunDir throws raw ENOENT when the engine never created runDir — src/multishot/golden/recording.ts

walkFiles calls readdirSync(dir) (recording.ts:160) which throws ENOENT if the directory does not exist. A third-party matrix engine that fails before creating its run directory (runMultishotMatrix does mkdirSync at matrix.ts:211, but the contract allows any engine) turns the check into an unreadable filesystem error instead of a named mismatch like files: expected 14 entries, received nothing. The existing 'writes nothing' test (golden.test.ts:273) only covers an engine run against a pre-made mkdtemp dir. Fix: if statSync fails with ENOENT at the top-level call, return {} so the files comparison reports the divergence.

🟡 LOW Fixture JSON cast to the record-set type without runtime validation — src/multishot/golden/records/index.ts

v1 as MultishotGoldenRecordSet trusts the 7k-line JSON completely. A hand-edited or corrupted fixture (wrong outcome shape, non-array requests) surfaces downstream as baffling per-field mismatches or a TypeError inside walk, not as 'fixture v1 is invalid'. The test suite's structural assertions (one record per catalog id, in order, golden.test.ts:28-36) catch gross drift only when tests run. A cheap shape assertion at load (version string, scenarios/matrixScenarios arrays, every entry has id+requests+outcome) would fail loud at the boundary. Low risk since the recorder is the only writer and freezes via linkSync.

_{tangletools · 2026-08-16T20:55:51Z · trace}

tangletools

✅ Approved — 18 non-blocking findings — `47befa2b`

Full multi-shot audit completed 8/8 planned shots over 22 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 22 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-08-16T20:55:51Z · immutable trace}

…it findings All low, none blocking; these are the ones that harden the claim the module makes about itself. - The record set is validated on load and deep-frozen. It was handed to every caller by reference, so "frozen" was a convention rather than a property, and a malformed fixture would have surfaced as a mismatch on every scenario instead of naming where it broke. - The judge wire answers its own endpoint, not every URL sharing its prefix. - `readRunDir` says which run directory is missing instead of raising ENOENT. - `compareJson` honours its cap exactly; one frame could push past it. - The recorder guards `--out` like `--version` and takes a per-process temp path, so two recorders cannot meet on one `.partial`. - The doc names `recordedAt` as provenance nothing compares, states the engine-naming rule without claiming a state this commit has not reached, and is linked from the doc index.

drewstone · 2026-08-16T20:56:26Z

Closed the substantive part of the 27. All were low; these are the ones that harden what the module claims about itself.

Fixed

The record set is now validated on load and deep-frozen. It was handed to every caller by reference, so "frozen" was a convention rather than a property of the value. A malformed fixture now names where it breaks instead of surfacing as a mismatch on all 14 scenarios.
Judge wire answers its own endpoint (${JUDGE_BASE_URL}/chat/completions), not every URL sharing the prefix.
readRunDir says which run directory is missing instead of raising a bare ENOENT.
compareJson honours its cap exactly — one frame could push several lines past it.
Recorder guards --out like --version (no empty value, no flag-as-value) and takes a per-process .partial path, so two recorders cannot meet on one temp file before the exclusive link.
Docs: recordedAt is named as provenance nothing compares; the engine-naming rule no longer claims a state this commit has not reached; the doc is linked from the index.

Accepted, with reasons

In-repo golden test is self-referential. True and inherent — a fixture cannot validate itself. That is precisely why the fixture is frozen and the consumers are the real check.
Exact float equality on cost sums. Deliberate. The records ARE the arithmetic; a tolerance would hide the reordering it is meant to catch.
Network guard only stubs globalThis.fetch. A node:http bypass stays possible. Every scenario pins router.invalid, so a bypass fails DNS; widening the guard is a bigger surface than the risk.
Recorder has no automated test. Its normalizers are the ones the 54-test golden suite exercises on every run; the script is the thin argument-and-file layer over them.

@tangletools review now

tangletools

⚠️ Value Audit did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.


Status	audit-incomplete (could not run)
Why	value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)
Lenses answered	0 of 2
What to do	re-run once the CLI bridge has capacity: `pr-reviewerctl trigger <repo>#<pr> --force`

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 4
Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=8/48 — no model was started

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 4
Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=9/48 — no model was started

No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.

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 · 20260816T210043Z}

Value audit could not run (value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)). This approval was provisional on that audit running, so it is dismissed. Re-run the reviewer once the CLI bridge has capacity.

tangletools

⚠️ Value Audit did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.


Status	audit-incomplete (could not run)
Why	value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)
Lenses answered	0 of 2
What to do	re-run once the CLI bridge has capacity: `pr-reviewerctl trigger <repo>#<pr> --force`

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 4
Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=14/48 — no model was started

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

Model: opencode/deepseek/deepseek-v4-pro
Bridge attempts: 4
Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=13/48 — no model was started

No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.

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 · 20260816T210403Z}

tangletools

🟢 Value Audit — sound


Verdict	sound
Coverage	2 of 2 lenses (value, usefulness)
Concerns	0 (none)
Heuristic	0.0s
Duplication	0.1s
Interrogation	532.0s (2 bridge agents)
Total	532.1s

💰 Value — sound

Freezes the multishot loop's observable behaviour (transport request ledger + outcome + matrix artifacts) as an append-only, versioned golden-record set with a framework-free check harness and a guarded recorder — the right oracle-freeze pattern for a reference engine about to be deleted, built clea

What it does: Adds a published subpath @tangle-network/agent-eval/multishot/golden (package.json:92-96, scripts/build-entries.mjs:17) containing: (1) a catalog of 13 deterministic shot scenarios + 1 matrix scenario (src/multishot/golden/scenarios.ts:408-525, matrix-scenarios.ts), each a closed system of scripted transports/executors that also fills a request ledger of every transport call (model, temperature,
Goals it achieves: Preserve the loop engine's behaviour as a regression oracle BEFORE the loop is deleted (PR body: 'The loop deletion follows once the three consumer parity tests move onto these records'), so consumer graph engines (the gtm/tax parity work referenced in scenarios.ts:9-10) can detect orchestration drift — a wrong token budget, an early-stopping rotation, a lost tool row, a changed error class — with
Assessment: Good on its merits. The design hits the load-bearing points: one normalization implementation shared by recorder and check so the two sides can't drift (src/multishot/golden/recording.ts:1-6); frozen append-only versioning where the diff between version files IS the reviewable behaviour change (docs/multishot-golden-records.md:58-65); recorder cannot freeze a record its own reference would fail (d
Better / existing approach: none — this is the right approach. Searched for existing equivalents before concluding: no vitest snapshot usage anywhere in src/ or tests/ (git grep toMatchSnapshot/toMatchInlineSnapshot → nothing), and vitest snapshots couldn't serve external consumers anyway; src/golden-matcher.ts is phrase-matching for judge goldens (unrelated); src/meta-eval/sentinel.ts goldens are human-score calibration set
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 8
Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A verified, load-bearing golden-record oracle that freezes the multishot loop's observable behavior as data so the loop can be deleted while consumer parity tests keep a runnable regression contract — exactly the right architecture for issue #617's stated goal.

Integration: Fully wired and already live. The new subpath is exported in package.json:92-96 and registered in scripts/build-entries.mjs:17, matching every other subpath pattern; pnpm typecheck passes and the golden suite (src/multishot/golden/golden.test.ts) runs 54/54 green in this checkout, with the reference loop itself replaying every record — so the regression detector is active in CI today, not waitin
Fit with existing patterns: In the grain of the codebase on every axis I checked. Subpath-export + build-entry wiring matches ./matrix, ./campaign, and peers; the layering rule holds (imports only from ../multishot, ../matrix, and agent-interface types — no upward deps); the fail-loud doctrine is followed everywhere the repo demands it (freeze enforced via linkSync EEXIST rather than an overwrite check alone, double-capture
Real-world viability: Built for the hard paths, not just the happy one. Determinism is engineered: volatile keys (durationMs, matrixId, runId, meanDurationMs) stripped before comparison, judge-call issue order normalized as a sorted set because Promise.all ordering isn't observable behavior (recording.ts:131), summary-markdown durations masked with fail-loud on unrecognized forms (recording.ts:115), tools compared by v
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 4

No concerns — sound change, no better or existing approach found. ✅

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 · 20260816T211236Z}

tangletools · 2026-08-16T21:25:02Z

⚠️ Review Incomplete — `1199f28f`

At least one required reviewer lane failed closed. No approval or request-changes review was published. This is a reviewer run failure, not a PR quality score.

Trigger a fresh review on the current PR head.

_{tangletools · 2026-08-16T21:25:00Z}

tangletools · 2026-08-16T21:30:29Z

✅ No Blockers — `47befa2b`

Review health 100/100 · Reviewer score 56/100 · Confidence 95/100 · 13 findings (13 low)

glm: Correctness 56 · Security 56 · Testing 56 · Architecture 56

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 22 changed files. Global verifier still owns final merge decision.

🟡 LOW 'No existing export changed' contradicts the refreshed analyst lock SHA — CHANGELOG.md

The entry states 'No existing export changed.' but src/analyst/benchmark-implementation.ts changes the exported const ANALYST_BENCHMARK_DEPENDENCY_LOCK_SHA256 (fd0de8a8... -> 67f8190c...) in this same PR, and repo precedent (the 0.145.20 entry) lists such refreshes as a Changed line ('Refreshed the analyst benchmark dependency-lock hash for this version.'). Impact: a consumer diffing exported values sees a changed export the changelog denies. Fix: either add the hash-refresh line under Changed, or scope the claim to 'No existing export's surface or signature changed.'

🟡 LOW 'an unknown tool with unparseable arguments' conflates two distinct fixtures — CHANGELOG.md

The scenario set has a KNOWN tool with unparseable arguments (list_source_documents, arguments 'not-json', scenarios.ts tc-2) and a separate UNKNOWN tool with parseable arguments (mystery_tool, arguments '{}', scenarios.ts tc-3). The changelog sentence reads as one tool that is both unknown and unparseable; the code's own doc comment separates them correctly ('one unparseable argument payload ... a text-plus-unknown-tool turn'). Coverage is real; the phrasing misdescribes it. Fix: 'an unknown tool, and a known tool called with unparseable arguments'.

🟡 LOW 'both cost provenances' is ambiguous against a three-kind type — CHANGELOG.md

CostProvenance.kind has three values (observed/estimated/uncaptured, src/multishot/types.ts assertCostProvenance), but records/v1.json exercises only 'estimated' (10 occurrences) and 'uncaptured' (1); no scenario produces 'observed'. 'Both' implies the type has exactly two or that the two most important are covered, and a reader cannot tell which. Fix: name them — 'the estimated and uncaptured cost provenances'.

🟡 LOW Fallback version duplicates pyproject version and relies on manual sync — clients/python/src/agent_eval_rpc/__init__.py

The PackageNotFoundError fallback hardcodes "0.146.0", duplicating pyproject.toml's version. Nothing in the test suite asserts these stay in sync; a future release that bumps one but not the other would silently report a stale version when the package is imported from source (not installed). Currently in sync (both 0.146.0), so no live defect — pre-existing pattern, correctly updated by this commit. Optional fix: derive the fallback from pyproject at build time or add a one-line test asserting version('agent-eval-rpc')/version == pyproject version.

🟡 LOW Doc overstates mismatch reporting: 'listing every field that moved' is capped at 25 per section — docs/multishot-golden-records.md

The doc says assertMultishotGoldenScenario throws MultishotGoldenMismatchError 'listing every field that moved'. compareJson defaults to a 25-mismatch limit (src/multishot/golden/compare.ts:22, options.limit ?? 25) and the harness never overrides it (harness.ts:112-115 and 189-194 call compareJson without options), so a widely divergent run reports at most 25 lines per compared section, not every moved field. The cap is a deliberate design documented on CompareOptions.limit, but the doc sentence overstates it. Fix: reword to 'listing each diverged field (first 25 per section)' or similar. No functional impact — the error still fires on any divergence.

🟡 LOW New export not added to requiredExports minimum set — package.json

package.json adds './multishot/golden' but scripts/verify-package-exports.mjs requiredExports (lines 89-102) was not extended, so the explicit per-subpath packed-file existence pin ('test -f' on types+import targets) skips the new entry. Coverage is not lost: verifyDistTypeRuntimeAgreement (line 976) enumerates every exports entry with types+import and both imports the js and reads the d.ts, failing on a missing file. Impact: none today; risk only if the automatic check is ever narrowed. Fix (optional, one line): add "./multishot/golden": ["import", "types"] to requ

🟡 LOW Concurrent same-version recorders share one .partial path — scripts/record-multishot-golden.ts

tempFile is derived solely from outFile, so two recorders minting the SAME version into the same --out dir race on one path: B's writeFileSync can overwrite A's temp between A's write and A's linkSync, and A then freezes B's capture while B fails EEXIST. Both captures were stability-validated in their own process, so the frozen content differs only in recordedAt metadata — integrity holds, provenance does not. Suffix the temp with process.pid or randomBytes to make each claim unique.

🟡 LOW Guard errors are classified by message prefix and can be frozen as engine outcomes — scripts/record-multishot-golden.ts

The catch rethrows only errors whose message starts with 'record-multishot-golden:'; everything else is frozen as the scenario's {kind:'error'} outcome. An engine that wraps the forbidNetwork fetch failure (e.g. new Error('request failed: ' + cause.message)) loses the prefix, so a network reach would be silently frozen as a legitimate error record instead of aborting the run. It self-corrects at check time (the checker installs no fetch stub, so the divergence surfaces then), but that violates the stated fail-loud intent. Use a branded error class or a symbol property on the guard's throw instead of a string-prefix test.

🟡 LOW One-commit stale-pin window at a3f354a hurts bisectability — src/analyst/benchmark-implementation.ts

Commit a3f354a added the ./multishot/golden subpath to package.json without repinning the lock digest; the repin landed in the immediately following commit 574bf40. Any check run at a3f354a (verify:package, test suite) fails with a dependency lock digest mismatch, so git bisect through this PR can land on a red intermediate commit. Later commits (247fab0, 47befa2) changed package.json and repinned within the same commit — that pattern should have been used here too. Head state is correct; process nit only.

🟡 LOW Tamper-detection tests flake under parallel load via esbuild service crash — src/analyst/benchmark-implementation.ts

When src/analyst/benchmark-implementation.test.ts runs in parallel with other files, the two tamper tests (test lines 161-193) intermittently fail: the checker's esbuild bundling step dies with a Go runtime crash ('The service was stopped'), producing empty stderr that fails the toContain('implementation digest mismatch') assertions. Observed 1-2 failures in 2 of 5 runs; 3/3 serial re-runs green. Not caused by this SHA change (those tests never reference the lock pin), but it undermines CI signal on this file's core invariant — the digest pin. Fix: retry the spawned checker once on non-zero-with-empty-stderr, or run these tests serially (vitest sin

🟡 LOW Matrix harness does not record engine throws as comparable outcomes, unlike the shot harness — src/multishot/golden/harness.ts

The shot check converts an engine throw into {kind:'error'} and diffs it; the matrix check lets the throw propagate raw (test at golden.test.ts:362-373 pins this). Consequence: a matrix engine that crashes mid-run reports the raw error instead of a field-level diff of what was already issued/written — the requests/judgeRequests/files comparison is skipped entirely on that path. Intentional asymmetry (MultishotMatrixGoldenRecord has no error outcome variant) and the judge-wire restore is still in finally, so no state leaks; naming it so the contract asymmetry is a chosen one, not an oversight.

🟡 LOW maskVolatileMarkdown would partially mask a compound duration and silently keep wall-clock text in the comparison — src/multishot/golden/recording.ts

Regex \*\*Duration\*\*: [\d.]+\s*(?:ms|s|m)\b matches the leading unit of a compound form: 'Duration: 2m 30s' becomes ' 30s', and the post-mask guard only checks the text right after the prefix, so the leftover ' 30s' stays in the compared Markdown and would mismatch on every run. Not reachable today: the sole renderer (src/multishot/matrix.ts:411) always emits (ms/1000).toFixed(0)}s, a single unit. Fix if desired: anchor to end-of-token, e.g. ^[\d.]+\s*(?:ms|s|m)$ on the captured value, or extend the guard to reject any digit following an elision.

🟡 LOW v1.json is cast to MultishotGoldenRecordSet without schema validation — src/multishot/golden/records/index.ts

v1 as MultishotGoldenRecordSet trusts a 7,174-line JSON file. A hand-edit that breaks the shape surfaces only indirectly as confusing mismatches or undefined deref inside a check. Mitigated by the record-set describe block (ids equal catalog order, both outcome kinds, coverage assertions) and by the reference-engine reproduction tests, so this is a debuggability nit: a small structural validator (keys present, requests arrays, outcome discriminated) at load time would convert a shape error into a named message.

_{tangletools · 2026-08-16T21:30:25Z · trace}

tangletools

✅ Approved — 13 non-blocking findings — `47befa2b`

Full multi-shot audit completed 8/8 planned shots over 22 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-08-16T21:30:25Z · immutable trace}

drewstone added 2 commits August 16, 2026 12:52

tangletools previously approved these changes Aug 16, 2026

View reviewed changes

tangletools reviewed Aug 16, 2026

View reviewed changes

drewstone dismissed tangletools’s stale review via 64aacc4 August 16, 2026 19:19

tangletools reviewed Aug 16, 2026

View reviewed changes

chore(release): 0.146.0 — freeze the multishot loop's behaviour as go…

47befa2

…lden records

tangletools reviewed Aug 16, 2026

View reviewed changes

drewstone merged commit 76969e5 into main Aug 16, 2026
2 checks passed

This was referenced Aug 17, 2026

chore(driver)!: remove the deprecated AgentDriver surface in the next major (extracted from agent-runtime#694) #618

Closed

chore(multishot): delete the loop core when gtm and tax land on runGraph (extracted from agent-runtime#694) #617

Open

Uh oh!

Conversation

drewstone commented Aug 16, 2026

What ships

Records are frozen

The records are load-bearing

Proof

Not in this PR

Uh oh!

drewstone commented Aug 16, 2026

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

✅ Auto-approved drewstone PR — 574bf400

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

✅ Auto-approved drewstone PR — 574bf400

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

🟢 Value Audit — sound

💰 Value — sound

🎯 Usefulness — sound

💰 Value Audit

🎯 Usefulness Audit

Uh oh!

drewstone commented Aug 16, 2026

Uh oh!

tangletools commented Aug 16, 2026

⚠️ Review Interrupted — 574bf400

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

🟡 Value Audit — sound-with-nits

💰 Value — sound-with-nits

🎯 Usefulness — sound

🎯 Usefulness Audit

💰 Value Audit

Uh oh!

drewstone commented Aug 16, 2026

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

🟢 Value Audit — sound

💰 Value — error

🎯 Usefulness — sound

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

🟢 Value Audit — sound

💰 Value — sound

🎯 Usefulness — error

💰 Value Audit

Uh oh!

tangletools commented Aug 16, 2026

✅ No Blockers — 574bf400

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

✅ Approved — 20 non-blocking findings — 574bf400

Uh oh!

tangletools commented Aug 16, 2026

⚠️ Review Incomplete — 64aacc41

Uh oh!

tangletools commented Aug 16, 2026

⚠️ Review Incomplete — ccae432d

Uh oh!

drewstone commented Aug 16, 2026

Uh oh!

drewstone commented Aug 16, 2026

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

🟢 Value Audit — sound

💰 Value — error

🎯 Usefulness — sound

🎯 Usefulness Audit

Uh oh!

tangletools commented Aug 16, 2026

✅ Auto-approved drewstone PR — `574bf400`

✅ Auto-approved drewstone PR — `574bf400`

⚠️ Review Interrupted — `574bf400`

✅ No Blockers — `574bf400`

✅ Approved — 20 non-blocking findings — `574bf400`

⚠️ Review Incomplete — `64aacc41`

⚠️ Review Incomplete — `ccae432d`

⚠️ Review Incomplete — `ccae432d`

✅ No Blockers — `247fab07`

✅ Approved — 27 non-blocking findings — `247fab07`

✅ No Blockers — `47befa2b`

✅ Approved — 18 non-blocking findings — `47befa2b`

⚠️ Review Incomplete — `1199f28f`

✅ No Blockers — `47befa2b`

✅ Approved — 13 non-blocking findings — `47befa2b`