feat(intelligence): preserve complete customer evidence

[drewstone]

Problem

Customer sessions could lose malformed records, unreadable files, partial uploads, tool arguments, identity, cost/token aliases, and agent-surface evidence.
The package also exposed a second hand-written recommendation/proposal model that duplicated the shared agent stack without being able to apply or compare exact changes.

Solution

• Stream large Claude, Codex, Copilot, Factory, Pi, Qwen, AMP, Forge, and OpenCode sessions with bounded memory.
• Recover isolated malformed JSON and UTF-8 while retaining every valid record and an immutable location/hash receipt for every corrupt record; strict mode still fails immediately.
• Fail loudly on unreadable sessions, zero-span parses, and partial backend acceptance.
• Preserve bounded canonical tool input/output, stable session identity, source provenance, subagent/skill evidence, and standard token/cache/cost attributes.
• Reuse @tangle-network/agent-eval@0.118.3 for the telemetry vocabulary, execution accounting, deep analysts, evidence contracts, and confidence-bearing findings.
• Emit one evidence-complete investigation result and report. Remove Traces' duplicate recommendation, claim, proposal, replay, persistence, and adapter contracts; exact candidate creation/comparison/application belongs to the shared improvement packages.
• Separate measured facts from unsupported inferences. Missing prompt, skill, MCP, hook, or cost telemetry is reported as missing coverage, not guessed behavior.

Proof

• pnpm typecheck: passed.
• pnpm test: 19 files, 198/198 tests passed.
• pnpm build: passed.
• pnpm check:package: packed CLI import/binary check passed.
• git diff --check: passed.
• 100 MB corruption fixture: 203 spans and 101 tool calls retained, two corruption receipts retained, tool payloads bounded, peak RSS asserted below 128 MB under a 40 MiB V8 heap.
• Frozen 420,929,561-byte operator source: 158,538 valid rows plus one corruption receipt retained; streaming parse completed in 0.99s at 116,336 KiB peak RSS under a 64 MiB V8 heap.
• Uri read-only proof: 656 runs analyzed with no provider/upload/customer keys, no network, no billing, no actions, and identical input/output SHA-256. It surfaced 360/527 tool calls without comparable arguments, 13/656 traces without stable identity, 13/15 execution groups without measurable skill events, and 9/656 execution errors. Cost remained explicitly uncaptured at 0/656 rather than being invented.

Release

This removes public pre-1.0 SDK fields and must ship as 0.9.0.
Merge with [skip release], then dispatch the Publish workflow with release_type=minor; do not let the default main-push patch release mislabel the break.

[tangletools]

✅ Auto-approved drewstone PR — a95490ff

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T09:46:43Z}

[tangletools]

✅ Auto-approved drewstone PR — dd0186ff

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T09:52:37Z}

[tangletools]

🟡 Value Audit — sound-with-nits

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

💰 Value — sound-with-nits

Consolidates 6 duplicated per-adapter JSONL parsers into one streaming reader, streams Codex spans row-by-row, and fixes a misleading retry metric — real memory win (1.7GB→445MB) with bit-identical output; ship.

• What it does: Adds src/jsonl.ts exporting readJsonl (async generator over createReadStream+readline, skipping malformed rows), collectJsonl (array collect), and takeJsonl (bounded first-N rows). Claude/Copilot/Factory/Pi/Qwen swap parseLines(await readFile) for collectJsonl (drops the simultaneous raw-string retention). Codex goes further: locate() uses takeJsonl(path,40) for identity instead of reading the who
• Goals it achieves: (1) Cut peak memory on large sessions — Codex session listing no longer reads whole 400MB files to inspect identity (1.66GB→97MB); parse no longer holds raw string + parsed rows simultaneously (1.7GB→445MB). (2) Make the tool-use report honest: the old retryRate-as-percent-of-all-calls printed '100% retry' when every failed call was retried; the new form reports 'N/M failed calls were followed by
• Assessment: Coherent and in the grain. The duplicated parseLines lived in 6 adapters with identical skip-malformed semantics — one shared reader is the right consolidation. The streaming design (createReadStream + readline, explicit lines.close()/input.destroy() in finally so takeJsonl's break is safe) is the standard Node pattern and correctly preserves the old tolerant skip-on-bad-row behavior. The report f
• Better / existing approach: Checked agent-eval 0.116.0: it exports parseJsonlRows(text:string) from the benchmarks subpath, but it is a STRING parser (you must readFile first, holding raw+parsed in memory — exactly what this PR eliminates) and it THROWS on the first malformed row instead of skipping. Not a substitute. No pre-existing streaming JSONL reader in this repo either (grepped src/ for createReadStream/createInterfac
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

A focused memory fix that consolidates 6 duplicate parseLines into one shared streaming JSONL reader, streams Codex directly to spans, and corrects a misleading retry-percentage report — all wired into live call paths.

• Integration: Fully reachable: the 6 consuming adapters are registered in registry.ts:26 and driven by session-source.ts parseSession/scanSessions (exported at index.ts:19); renderPipelines is called from cli.ts:420 and improvement.ts:553. No dead surface — every helper has a caller.
• Fit with existing patterns: Matches the codebase's grain precisely. Each adapter previously had its own copy of the same read-file-then-split-then-parse routine; this PR collapses them into one shared jsonl.ts. The Codex true-streaming path is the natural extension for the one adapter with a proven 400MB problem. No competing pattern exists (session-index.ts:209 does key-counting, a different job).
• Real-world viability: Holds up: createReadStream+readline with crlfDelay:Infinity and a finally-block destroy is the canonical Node streaming pattern; takeJsonl breaks early and cleans up; malformed rows are skipped without aborting the trace. Codex parse() may read the file up to 3x in the rare case meta/model aren't in the first 40 lines (head + fallback scan + main loop), but the common case is head + single body pa
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

💰 Value Audit

🟡 Codex parse reads the file twice (bounded head + full stream) [maintenance] ``

src/adapters/codex.ts:199 reads takeJsonl(path,40) for first/meta/model, then src/adapters/codex.ts:234 re-streams the whole file via readJsonl to build spans. A single-pass design could buffer the first 40 rows inline during the building loop and defer root-span creation until meta/model are seen. In practice the head read is bounded to 40 lines so the extra I/O is negligible and the two-phase shape makes the bit-identical-output guarantee trivial to reason about — not worth changing, but a rev

🟡 adoption.ts still uses the old readFile+split JSONL pattern [maintenance] ``

src/adoption.ts:115-133 readLoopRuns does readFile(path,'utf8') then countSkillRunsJsonl(raw) splits on '\n' — the exact pattern this PR removed from the adapters. Now that readJsonl exists, readLoopRuns could stream via readJsonl for consistency. Not blocking: .evolve/skill-runs.jsonl files are tiny, and countSkillRunsJsonl takes a raw string deliberately (testable, sync aggregation). Note for a later consolidation pass, not this PR's scope.

---

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 · 20260713T095739Z}

[tangletools]

✅ Auto-approved drewstone PR — 4fa24975

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T10:06:01Z}

[tangletools]

🟢 Value Audit — sound

Verdict	sound
Concerns	0 (none)
Heuristic	0.0s
Duplication	0.0s
Interrogation	355.8s (2 bridge agents)
Total	355.8s

💰 Value — sound

Replaces 6 copy-pasted JSONL parsers with one streaming reader, cuts peak memory on large sessions by ~75%, and fixes a misleading '100% retry' display — clean, in-grain, ship.

• What it does: Introduces src/jsonl.ts with three exports — readJsonl (streaming async generator), collectJsonl (materialize all), takeJsonl (first N rows). Removes 6 identical parseLines functions across the claude/codex/copilot/factory/pi/qwen adapters and routes them through the shared reader. Codex discovery now reads only the first 40 parsed rows (takeJsonl) instead of slurping the whole file; Codex parse s
• Goals it achieves: (1) Cut peak parse memory on large JSONL sessions — the old readFile+split retained both the raw string and the parsed array, peaking at ~1.7GB on a 400MB Codex session; the streaming reader never holds the raw text. (2) Cut session-listing memory ~17x by reading only the header lines needed for cwd/id during discovery. (3) Fix the report's '100% retry' claim — retryRate is a ratio of error calls,
• Assessment: Coherent and well-executed. The shared reader is minimal (38 lines), correctly typed as a generic generator, tolerates malformed rows (matching the old behavior), and cleans up its stream/readline in a finally block. Routing 6 adapters through it removes genuine copy-paste duplication. The Codex streaming parse is a real architectural improvement: discovery does bounded head reads, and the main lo
• Better / existing approach: none — this is the right approach. Checked @tangle-network/agent-eval (the direct dependency) for any existing JSONL streaming utility; none exists. Two other inline JSONL parsers remain in the codebase (session-index.ts:209 summarizeJsonl and adoption.ts:89 countSkillRunsJsonl), but both take raw string input rather than a path, and summarizeJsonl specifically needs to count invalid rows (which r
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

A coherent dedup-and-stream refactor: one shared JSONL reader replaces six identical parseLines helpers, Codex streams directly into spans, and the misleading '100% retry' report now shows explicit numerators/denominators.

• Integration: Fully reachable. The new readJsonl/collectJsonl/takeJsonl in src/jsonl.ts are consumed by all 6 JSONL adapters (claude.ts:274,327; codex.ts:181,199,204,235; copilot.ts:77; factory.ts:103; pi.ts:114; qwen.ts:92), all registered in src/registry.ts:27-36 and called from cli.ts/improvement.ts/live.ts. The 4 unmigrated adapters (gemini, opencode, forge, amp) parse whole-document JSON, not line-delimite
• Fit with existing patterns: Matches the codebase grain exactly. Six copy-pasted parseLines functions (each ~10 lines, identical except the row type) collapse into one typed async generator plus two thin helpers. The interface mirrors how the rest of the code consumes things: collectJsonl returns T[] as before, takeJsonl is the bounded-head read Codex's locate/parse already needed. No competing pattern exists.
• Real-world viability: Stream cleanup is correct (try/finally in src/jsonl.ts:17-20 closes readline and destroys the stream even on early break). Malformed-row tolerance matches prior behavior. The codex parse path's triple-read (head scan -> optional meta fallback -> main loop) is bounded and the wall-time claim (2.55s unchanged) is plausible since the win is memory, not I/O. The report.ts:152 retry math (retryRate * e
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

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 · 20260713T101719Z}

[tangletools]

✅ No Blockers — 4fa24975

Review health 100/100 · Reviewer score 53/100 · Confidence 95/100 · 14 findings (14 low)

glm: Correctness 53 · Security 53 · Testing 53 · Architecture 53

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

🟡 LOW Minor-version jump on 0.x package widens transitive surface — pnpm-lock.yaml

@tangle-network/agent-eval moves 0.106.2 -> 0.116.0 (10 minor versions on a 0.x package, where minors can break the public API). Transitively, @tangle-network/agent-interface jumps 0.10.1 -> 0.22.0. The lockfile itself is correct; the risk is whether consumer code in src/adapters still compiles against the new types. That verification belongs to the other shots covering src/ and tests/ — out of scope here, flagged only as cross-shot context.

🟡 LOW Codex locate() now skips unreadable files instead of listing them with cwd:null — src/adapters/codex.ts

Old code: readFile(path,'utf8').catch(() => '') returned empty string on read error, so the session was still listed with cwd:null and filename-derived id. New code: takeJsonl throws on unreadable files, caught by try/catch which does continue — the session is excluded entirely. This is arguably more correct (can't parse an unreadable file), but it's a behavior change: a session that appeared in locate() results before may now disappear. Low impact since parse() would have failed anyway.

🟡 LOW Codex parse() reads the file 2-3x (head scan + optional fallback + main loop) — src/adapters/codex.ts

takeJsonl(path, 40) opens stream #1 (bounded 40 lines), then if !first||!meta a full readJsonl opens stream #2 (lines 204-209), then the main processing loop opens stream #3 (line 235). Old code read the file once into a buffer. For typical files (session_meta is line 1) only 2 reads occur (40-line head + full), and the head read is tiny. This is a deliberate streaming-vs-buffering trade-off (lower peak memory, 2x I/O). Acceptable for a local

🟡 LOW Fallback head-scan path (session_meta beyond line 40) is untested — src/adapters/codex.ts

The fallback for-async loop (lines 203-209) that scans the full file when first or meta is not in the first 40 lines has no test coverage. All test fixtures place session_meta as line 1. The delayed-model backfill IS tested (upgrades.test.ts: 'applies a delayed Codex model without a metadata pre-scan'), but that test has session_meta in the head, so the fallback never fires. This is a rare edge case (continuation sessions with meta late), but the fallback logic has its own break condition (first && meta) that could silently fail to find model if meta appear

🟡 LOW Performance note: codex parse() reads file up to 3x — src/jsonl.ts

Not a bug in jsonl.ts itself, but the streaming API design invites the caller pattern seen in codex.ts parse(): takeJsonl(head) reads ~40 lines, then readJsonl scans for meta fallback (full or partial), then readJsonl streams the entire file again for the main loop. For multi-MB codex rollout files this is 2-3x the I/O of the old readFile-once approach, traded for lower peak memory. The utility is correct; consider documenting that callers needing both head and full scan should consume a single generator rather than calling takeJsonl + readJsonl separately.

🟡 LOW Test gaps: ENOENT propagation, non-integer limit, fd cleanup — src/jsonl.ts

tests/jsonl.test.ts covers the happy path and negative-limit rejection but does not assert: (a) file-not-found raises ENOENT through the async generator, (b) takeJsonl rejects non-integer limits like 1.5 and NaN (both correctly throw RangeError — verified via tsx but untested), (c) no file-descriptor leak after takeJsonl's early break. These are the riskiest paths in a streaming utility. Add 3-4 assertions to tests/jsonl.test.ts.

🟡 LOW Comment claims a time-interval filter that the implementation does not enforce — src/pipelines.ts

The new comment says stuckLoopView fires 'same tool + same args ≥ N times in a short interval'. Inspected the actual implementation at node_modules/@tangle-network/agent-eval/dist/pipelines/index.js:299-327: it groups spans by ${toolName}\0${argHash}, flags any bucket with spans.length >= minOccurrences (default 3), and computes windowMs = last.startedAt - first.startedAt purely as a reported finding field. There is no maxWindowMs option and no interval gate — a run that calls the same tool with the same args 3 times across an hour is flagged identically to 3 calls within a second. The wording is inherited from upstream's own StuckLoopView docstring ('short window'), so it's consistent with the dependency, but a reader of this file could mistakenly believe a time filter exists. Impact: d

🟡 LOW 'in a short interval' stuck-loop wording is not enforced by the detector — src/report.ts

New text: 'no tool called ≥3× with identical args in a short interval'. The underlying stuckLoopView (agent-eval/dist/pipelines/index.js:299-336) only checks spans.length < minOccurrences (default 3) — there is NO time-window constraint. windowMs is the observed last-first spread, not a gate. A run calling bash {cmd:'ls'} three times over 30 minutes still gets flagged. The 'none found' result is correct, but the description of what was checked over-promises a tighter criterion than was applied. Fix: either drop 'in a short interval' (revert to the previous accurate phrasing) or pass a real intervalMs option to stuckLoopView and reflect it in the message.

🟡 LOW Pluralization grammar: '1/1 failed calls were followed' — src/report.ts

Template ${retriedFailures}/${errorCalls} failed calls were followed by another call to that tool produces '1/1 failed calls were followed...' when both values are 1, which reads as a subject-verb disagreement. Should conditionalize 'call was' vs 'calls were'. The existing test (tests/pipelines.test.ts:97) locks in the ungrammatical string, so a fix needs to update both.

🟡 LOW All assertions live in one it() block — tests/jsonl.test.ts

Six distinct behaviors (stream+skip malformed, collectJsonl parity, takeJsonl limit/0/-1) are collapsed into a single it(). When one assertion throws, vitest stops and the remaining unrun assertions are invisible. Split into separate it()s per behavior for faster failure attribution.

🟡 LOW Missing edge cases: empty file, missing file, oversized limit — tests/jsonl.test.ts

No coverage for: (1) empty file (should yield zero rows, not error), (2) non-existent path (readJsonl should surface ENOENT from createReadStream), (3) takeJsonl(path, N) where N > row count (should return all rows without error). These are the realistic failure modes for a JSONL reader; adding them would meaningfully raise confidence in the implementation.

🟡 LOW Non-integer limit branch untested — tests/jsonl.test.ts

takeJsonl guards with Number.isInteger(limit), so 1.5 or NaN should also throw 'limit must be a non-negative integer', but only the negative path (-1) is exercised. Add an explicit takeJsonl(path, 1.5) and takeJsonl(path, NaN) rejection to lock the integer check independent of the < 0 check.

🟡 LOW Temp directory never cleaned up — tests/jsonl.test.ts

mkdtemp(join(tmpdir(), 'traces-jsonl-')) creates a directory that is never removed. Across a long CI run this accumulates files in /tmp on every runner. Add afterEach(() => rm(directory, { recursive: true, force: true })) and hoist directory to describe scope, or use fs.mkdtemp + finally cleanup.

🟡 LOW Hardcoded boundary count couples test to SESSION_HEAD_LINES constant — tests/upgrades.test.ts

The test generates exactly 40 progress events (Array.from({ length: 40 }, ...)) to push turn_context past the SESSION_HEAD_LINES=40 head scan boundary. This magic number is not derived from the actual constant. If SESSION_HEAD_LINES is increased to >=43, the head scan would find the model directly and the test would pass for the wrong reason (head-scan path instead of streaming-backfill path). Consider importing SESSION_HEAD_LINES or generating SESSION_HEAD_LINES events dynamically so the test remains valid if the constant changes. Impact: low — the constant is unlikely to change, and the test is correct today.

---

_{tangletools · 2026-07-13T10:38:54Z · trace}

[tangletools]

✅ Approved — 14 non-blocking findings — 4fa24975

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

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-13T10:38:54Z · immutable trace}

[tangletools]

✅ Auto-approved drewstone PR — 8abcfd03

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T10:56:48Z}

[tangletools]

🟡 Value Audit — sound-with-nits

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

💰 Value — sound-with-nits

Adds a shared streaming JSONL reader and migrates 6 adapters off read-whole-file parsing, cutting peak memory ~4x and fixing a misleading '100% retry' report; clean and in-grain, with one deliberate behavior change worth a glance.

• What it does: Introduces src/jsonl.ts (readJsonl async generator + takeJsonl bounded reader + JsonlParseError). Migrates claude, codex, copilot, factory, pi, qwen away from readFile-then-split-then-parseLines-retain-both. Codex locate() reads only 40 parsed rows for identity discovery; Codex parse() and Claude parse() stream rows straight into spans. Claude's parseClaudeStream is refactored into createClaudeStr
• Goals it achieves: (1) Cut peak memory on large sessions by not retaining both the raw file string and the full parsed-row array — PR claims 1,695,564 KB -> 444,816 KB on a 402 MB Codex session, which is exactly what streaming line-by-line buys. (2) Speed up session listing by bounded-head discovery — 0.87 s / 1.66 GB -> 0.14 s / 97 MB. (3) Stop the misleading '100% retry' display, which arose because retryRate (a f
• Assessment: Sound and in the grain of the codebase. The shared reader is the right size (48 lines, takeJsonl composes on readJsonl), lives where every harness adapter can reach it, and replaces six near-identical private parseLines copies. The Claude refactor preserves the exported parseClaudeStream signature (still takes a readonly ClaudeEvent[]) while letting the streaming parse path reuse consumeClaudeEven
• Better / existing approach: none — this is the right approach. Searched: (a) rg for an existing generic JSONL reader in src/ (only the new src/jsonl.ts and an unrelated readline use in src/cli.ts); (b) agent-eval's exported API for a reusable stream reader (absent — its JSONL helpers are all domain-shaped around RunRecord/ReferenceReplay/Gold); (c) node_modules/@ax-llm/ax (no JSONL reader). A ~48-line local utility is correc
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Consolidates duplicated per-adapter JSONL parsing into one streaming reader that cuts peak memory ~4x on huge sessions, fixes a misleading retry-rate report by using the correct denominator, and upgrades agent-eval — all wired into existing call paths with no dead surface.

• Integration: Fully reachable. readJsonl/takeJsonl (src/jsonl.ts) are consumed by all six modified adapters, which are registered in src/registry.ts:27-35 and re-exported via src/index.ts. renderPipelines (src/report.ts:136) is called by src/cli.ts:420 and src/improvement.ts:553. parseClaudeStream remains exported and tested (tests/traces.test.ts:65).
• Fit with existing patterns: Matches the codebase grain exactly: replaces a verbatim-copy parseLines helper that existed in every JSONL adapter with one shared streaming reader. No competing or established alternative pattern exists in the repo.
• Real-world viability: Streaming uses canonical Node createReadStream+createInterface with proper finally cleanup; empty/missing/malformed inputs each have explicit tests (tests/jsonl.test.ts, tests/adapters.test.ts:48-67). A 100MB/128MB-heap child-process test (tests/adapters.test.ts) directly proves the memory claim. The retry-rate fix uses the correct denominator, verified against agent-eval source (retryRate = retri
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

💰 Value Audit

🟡 Malformed-line tolerance was deliberately removed — verify it holds against real Copilot files [maintenance] ``

Every prior parseLines silently skipped bad lines (src/adapters/copilot.ts had an explicit comment: 'GitHub's writer has shipped lines with raw separators — skip tolerantly'). The new readJsonl throws JsonlParseError on the first unparseable line (src/jsonl.ts:29), so one bad line now aborts the whole session parse, yielding partial spans up to that line then throwing. This is a deliberate, tested choice (commit 8abcfd0 'fail loudly while streaming JSONL'; tests/jsonl.test.ts:22 asserts the type

---

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 · 20260713T110251Z}

[tangletools]

❌ Needs Work — 8abcfd03

Review health 100/100 · Reviewer score 24/100 · Confidence 95/100 · 14 findings (1 high, 1 medium, 12 low)

glm: Correctness 24 · Security 24 · Testing 24 · Architecture 24

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

Blocking

🔴 HIGH 100MB heap test fails deterministically under vitest (tsx subprocess SIGABRT) — tests/adapters.test.ts

The test spawns spawnSync(process.execPath, ['--max-old-space-size=128', '--import', 'tsx', '--input-type=module', '--eval', childSource], ...) from inside a vitest worker. Verified failure 3/3 isolated runs: child.status=null, child.signal=SIGABRT, stderr contains 'pthread_create: Resource temporarily unavailable'. Reproduced with a minimal probe (tsx --eval writing a constant) — same SIGABRT under vitest, but succeeds in 527ms when run as a standalone node script.mjs outside vitest. The vitest parent already loads tsx + tinypool worker threads; the child tsx instance tries to spawn its own esbuild service threads and hits RLIMIT_NPROC. Impact: this test will fail CI on runners with tight ulimit -u (typical shared/containerized CI). Even when it does run, a 30s wall-clock + 100MB fi

Other

🟠 MEDIUM codex locate() silently drops sessions with malformed JSON in the first 40 lines — src/adapters/codex.ts

The new head = await takeJsonl<CodexLine>(path, SESSION_HEAD_LINES) throws JsonlParseError on the first malformed line; the wrapping catch { continue } then skips the session entirely. OLD code did readFile().catch(()=>'').split('\n',40) with a per-line try/catch around JSON.parse that skipped bad lines and kept scanning — so a session with one corrupt line in the head still appeared in the listing (with best-effort cwd/id). NEW behavior: the session is invisible and the user gets no error explaining why. This is inconsistent with the PR's 'fail loud' intent (parse() does throw with location; locate() hides the session). The cwd-recovery path exists precisely for continuation sessions whose head is unusual, so this regression bites exactly the edge case the head-scan was added for.

🟡 LOW no test covers empty-file or meta-late-in-file edge cases for the streaming adapters — src/adapters/codex.ts

The new streaming + rescan logic in codex parse() has three branches (meta-in-head, rescan-for-meta, empty file) that are exercised only by the positive cwd-recovery test. An empty rollout file (0 events) returns [root] with root.end_time === root.start_time — reasonable, but unverified. A session where session_meta appears after line 40 triggers a second full read of the file head-to-meta before the main loop re-reads head-to-end; the logic is correct but the doubled I/O on very-large continuation sessions is uncharacterized. A one-line test for each would lock in the contract.

🟡 LOW factory tool_result backfill doesn't set end_time (tool spans show zero duration) — src/adapters/factory.ts

When a tool_result block arrives, only t.status is set — t.end_time is never assigned, so the tool span keeps end_time === start_time (from the span() factory default). Every other adapter in this shot sets end_time on result backfill: claude.ts backfillResult (claude.ts:231), qwen.ts:183, pi.ts:207, codex.ts:305, copilot.ts:145. This is pre-existing (the old factory.ts had the same omission) but it lives in the code path being refactored, so it's a cheap one-line fix to align factory with its siblings: add t.end_time = ts before the status assignment.

🟡 LOW No explicit test asserting fd cleanup on early termination — src/jsonl.ts

takeJsonl breaks out of readJsonl once rows.length===limit; the generator's finally closes readline + destroys the stream. This works (verified manually: takeJsonl(p,5) on 100 rows returns 5 cleanly), but the test suite never asserts the cleanup path on a multi-row file — takeJsonl(path,1) on a 2-row file implicitly exercises it, yet there's no assertion that the underlying ReadStream hit 'close'. Given this primitive backs every adapter's parse, a regression in the finally block would silently leak fds. Suggest adding a test that listens to input 'close' after a partial consume.

🟡 LOW Whitespace-only lines are not skipped and hard-fail the parse — src/jsonl.ts

The blank-line guard is if (!line) continue, which only matches the empty string. A line containing spaces/tabs (e.g. " ") is truthy, so it reaches JSON.parse(' '), which throws, and the whole stream aborts with JsonlParseError. Verified: a file with {"id":1}\n \n{"id":2}\n yields only {id:1} then errors at line 2. Real JSONL producers/editors sometimes emit whitespace-padded trailing lines. Strict JSONL forbids them so this is defensible, but for a trace-ingestion tool robustness favors if (!line.trim()) continue. Low impact because well-formed JSONL never hits this.

🟡 LOW Variable name 'retriedFailures' overstates the metric semantics — src/report.ts

retryRate counts error calls that are followed by ANY next call to the same tool (arr[i+1] exists), regardless of whether that follow-up succeeded. The displayed text correctly says 'followed by another call to that tool', but the internal name 'retriedFailures' implies successful retries. Consider 'followedUpFailures' or 'reAttemptedErrors' for accuracy. Display output is correct; naming-only nit.

🟡 LOW retriedFailures computed unconditionally but only used when errorCalls > 0 — src/report.ts

Line 152 runs Math.round(m.retryRate * errorCalls) before the errorCalls>0 guard on line 153. When errorCalls===0 (no errors), retryRate is also 0 so the result is 0 and unused — harmless, but the computation is dead in that path. Optionally move the retriedFailures line inside the errorCalls>0 branch or compute failureFollowUp inline. No behavioral impact; cosmetic only.

🟡 LOW Asymmetric spanDigest coverage in parametrized conversation-capture test — tests/adapters.test.ts

Inside the parametrized loop over codex/pi/factory cases, only c.name === 'pi' gets a spanDigest lock; codex and factory do not. The same test file locks copilot/qwen/factory/claude in their dedicated describe blocks. The inconsistency means the pi case gets full-shape regression coverage here while the codex/factory cases in this block rely only on the three field-level assertions. Either lock all three cases or none — the current if (c.name === 'pi') reads as an accidental leftover.

🟡 LOW Shared tmpdir never cleaned up; 100MB fixture leaks per run — tests/adapters.test.ts

const dir = mkdtempSync(join(tmpdir(), 'tt-adapters-')) is module-scoped and never removed. Each test process leaves behind its fixtures, including the 100MB large-ignored-session.jsonl. On long-lived dev machines or CI runners that re-use tmp, this accumulates. Fix: add a afterAll(() => rmSync(dir, { recursive: true, force: true })) (imported from node:fs).

🟡 LOW spanDigest assertions use unauditable 64-char magic hashes — tests/adapters.test.ts

Four tests lock the entire span array to a SHA-256 (e.g. copilot: '94cfe096...'). A reviewer cannot tell whether the locked hash encodes the intended output or merely whatever the code produced the day it was written — there is no golden reference or annotated fixture to compare against. Any future additive change (a new attribute, a renamed field) will break the test with no signal about which field moved. The structural assertions on the following lines already cover the important fields (token counts, tool names, status codes); the digest adds regression coverage at the cost of auditability. Fix: either keep the structural assertions and drop the digest, or commit a tests/fixtures/<adapter>.expected.json golden next to the test so a diff is human-readable.

🟡 LOW No coverage for early-break stream cleanup or empty file — tests/jsonl.test.ts

The test exercises takeJsonl (which breaks out of the for await) but does not explicitly assert the underlying file descriptor is released afterward (e.g. via checking the stream's destroyed state or that a second read succeeds cleanly). Also no test for a 0-byte file or a file containing only blank lines. These are edge cases that exercise the finally { lines.close(); input.destroy() } path and the if (!line) continue skip loop. Not blocking — current behavior is correct — but adding these would harden the primitive given its 6 consumers.

🟡 LOW Temp directories never cleaned up — tests/jsonl.test.ts

All three tests call mkdtemp(join(tmpdir(), 'traces-jsonl-')) or derive a path under tmpdir but never rm the directory. Repeated test runs accumulate files in the OS tmpdir until it is reaped. Low impact since tmpdir is OS-managed, but adding an afterEach/try-finally with rm(directory, { recursive: true, force: true }) would be cleaner hygiene and match the streaming module's own resource-cleanup discipline (the finally block closing the read stream in src/jsonl.ts:33-36).

🟡 LOW renderPipelines retry path not covered for zero-failure and zero-retry cases — tests/pipelines.test.ts

The new test only covers the case where errorCalls>0 AND retriedFailures>0. Two related branches of src/report.ts:148-158 are not asserted through the rendered string: (1) errorCalls===0 → failureFollowUp='' (the empty-suffix path); (2) a failed call NOT followed by another call to the same tool (retriedFailures=0, retryRate=0). The older tests assert r.toolUse fields directly but never call renderPipelines, so the rendered wording for those branches is unverified. Impact: a future regression that broke the empty-suffix formatting (e.g. producing '0/0 failed calls were followed...') would not be caught. Fix: add one spans fixture with a single ERROR bash and no subsequent bash call, and assert the rendered line contains '1/1 failed' WITHOUT the 'were followed by another call' suffix.

---

_{tangletools · 2026-07-13T11:34:56Z · trace}

[tangletools]

❌ 1 Blocking Finding — 8abcfd03

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

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-13T11:34:56Z · immutable trace}

[tangletools]

✅ Auto-approved drewstone PR — d5116513

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T13:05:03Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	4 (1 low, 3 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	272.3s (2 bridge agents)
Total	272.3s

💰 Value — sound-with-nits

A coherent streaming + tool-I/O refactor: parsers no longer buffer multi-hundred-MB JSONL files, tool args/results become first-class OpenInference attributes, silent error-swallowing is replaced by typed errors that don't leak raw line content; minor concerns only.

• What it does: Adds a streaming line reader (src/jsonl.ts) and a single-doc JSON reader (src/json.ts) with typed, secret-scrubbed errors; rewrites every JSONL adapter (Claude, Codex, Copilot, Factory, Pi, Qwen) to consume rows lazily via for-await; introduces src/adapters/tool-io.ts so tool input/output lands on proper OpenInference keys (input.value/output.value, mime, sha256, byte size, truncation flag) instea
• Goals it achieves: (1) Peak memory on a 402 MB Codex session drops from ~1.7 GB to ~445 MB during parse and from ~1.6 GB to ~95 MB during locate — measured, with identical 74,753-span output and SHA-256. (2) Tool I/O becomes inspectable and redactable as first-class values (OpenInference shape) rather than getting lost in the prose content blob. (3) Parse failures stop being silently swallowed — a malformed line o
• Assessment: Good change, in the grain of the codebase. The cascade is coherent, not arbitrary scope creep: once you stream, you cannot JSON.parse(line) inline with try/catch swallowing, so you need a shared reader; the shared reader needs typed errors that don't leak raw line content; the new tool I/O attributes need their own redaction path; and the new outbound shape changes what 'same session' means for up
• Better / existing approach: Searched node_modules of @tangle-network/agent-eval 0.103 (the version before the bump): agent-eval DOES export a readJsonl (dist/index.js:8430) — but it is readFileSync(path,'utf8').split('\n').map(JSON.parse), i.e. the exact buffered pattern causing the 1.7 GB peak. The local src/jsonl.ts is therefore NOT a duplicate; it is the streaming replacement for a substrate helper that does not stream.
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

A well-wired consolidation that replaces 6 duplicate per-adapter JSONL parsers with one shared streaming reader, cutting Codex session-listing memory ~17x and fixing a misleading retry-percentage report.

• Integration: Fully reachable. readJsonl/takeJsonl (src/jsonl.ts:24,56) are consumed by 6 adapters (claude.ts:311,377; codex.ts:231,246,251,297; copilot.ts:94; factory.ts:128; pi.ts:132; qwen.ts:107). readJsonFile/isMissingPathError/isMissingJsonSource (src/json.ts) back amp/claude/opencode/gemini/factory/forge. tool-io.ts (toolIoAttributes/recordToolOutput/normalizeToolIoAttributes) is used by every adapter pl
• Fit with existing patterns: Fits the established pattern precisely. The registry already collapses many harnesses onto shared adapters; this PR applies the same consolidation to JSONL parsing — removing 6 copies of parseLines (claude.ts old lines 57-68, codex.ts old parseLines, etc.) in favor of one streaming primitive. The tool-io module unifies what was previously per-adapter JSON.stringify of tool input/output into a shar
• Real-world viability: Holds up on real paths. readJsonl uses createReadStream with a finally-destroy (jsonl.ts:51-53), handles \r\n, handles a trailing line without newline, and skips blank lines. The deliberate fail-loud JsonlParseError (jsonl.ts:3-13) replaces silent line-swallowing; the team iterated on this across the branch (commits 8abcfd0 'fail loudly', d511651 'prevent silent session data loss'), so the error p
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: console debug added src/cli.ts

• console.log('No sessions found to upload.')

🎯 Usefulness Audit

🟡 Library surface exports readJsonFile but not readJsonl/takeJsonl or tool-io helpers [ergonomics] ``

index.ts:18 exports readJsonFile/JsonSourceError and :19 exports JsonlParseError, but the broadly-useful readJsonl/takeJsonl streaming primitives and toolIoAttributes/recordToolOutput are NOT re-exported — even though index.ts:33-36 explicitly invites custom-adapter authors via the conversation helpers. Anyone writing a custom adapter (the documented extension path) would want the same streaming reader and tool-I/O attribute contract the built-ins use. Minor inconsistency; not a blocker since bu

💰 Value Audit

🟡 Streaming readJsonl duplicates a substrate helper by name with different semantics [maintenance] ``

@tangle-network/agent-eval already exports readJsonl (verified in node_modules/.../@tangle-network/agent-eval/dist/index.js:8430 in v0.103; still present in the 0.116 line), but its implementation is the buffered readFileSync(...).split('\n').map(JSON.parse). src/jsonl.ts:24 ships a same-named async generator with streaming semantics. Two functions named readJsonl with different sync/async shapes and different memory profiles is a future-confusion trap — a maintainer who imports the wrong

🟡 PR title understates scope: upload dedup + redaction rewrite is bundled with the streaming fix [proportion] ``

The title says 'stream JSONL trace analysis', but the diff also rewrites upload-state.ts (sessionHash → contentHash + new outboundHash with UploadIdentity), redact.ts (scrub input.value/output.value/status.message, not just content), and upload.ts (prepareOutbound stage, PartialUploadError, deferred uploadedAt-stamping). These are legitimate consequences of introducing the tool-io attributes — the old 4-field structural hash would miss tool-I/O changes and the old redactor pass would miss the ne

---

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 · 20260713T133406Z}

[tangletools]

✅ Auto-approved drewstone PR — 9515207e

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T14:04:44Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	4 (1 low, 3 weak-concern)
Heuristic	0.1s
Duplication	0.0s
Interrogation	305.9s (2 bridge agents)
Total	306.0s

💰 Value — sound-with-nits

Centralizes streaming JSONL + bounded corruption recovery and threads it through every adapter so isolated malformed records and non-ENOENT errors stop dropping sessions silently; well-built, in-grain, no existing equivalent to reuse.

• What it does: Adds a streaming JSONL reader (src/jsonl.ts) with two modes — strict (throws a typed JsonlParseError carrying byte offset/length/SHA-256) and recover (skips the bad record, emits a location-only receipt via onCorruption). Adds src/integrity.ts: a per-SessionRef corruption accumulator with hard bounds (≤128 receipts, ≤64 KiB serialized) that throws SessionCorruptionLimitError past the limit and sta
• Goals it achieves: (1) Stop silent session data loss — a single bad line, an unreadable subagent file, or a permission error no longer drops the whole session or is swallowed as 'no sessions'. (2) Stream multi-hundred-MB rollouts without OOM (the cited 105 MB / 415 MB regressions). (3) Make degradation visible and auditable: location+SHA-256 receipts on the root span and in the report, raw malformed bytes never reta
• Assessment: Coherent and in the grain of the codebase. The right seam: two new modules (jsonl.ts for the streaming primitive, integrity.ts for the per-session receipt accounting) consumed by every adapter through one helper (sessionJsonlOptions). Strict-vs-recover split preserves whole-file validation as an opt-in while making the default resilient — the prior parseLines catch-all (`try { JSON.parse } catch {
• Better / existing approach: none — this is the right approach. Searched node_modules/@tangle-network/agent-eval/dist for readJsonl|streamJsonl|parseJsonl|JsonlReader|createReadStream|onCorruption — no existing JSONL streamer. The only adjacent primitive is OtlpFileTraceStore (traces.d.ts:453–539), which pins the whole file in a Buffer for already-normalized OTLP and serves analyst search/view APIs; wrong layer for streaming
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

Streaming JSONL + bounded corruption recovery + structured OpenInference tool I/O land cleanly on the main locate→parse spine, reach every adapter uniformly, and correctly expand the redaction surface to match the new data capture surface.

• Integration: All new surface is reachable. readJsonl/takeJsonl (src/jsonl.ts) replace the old one-shot collectJsonl (now fully removed — grep returns no references) across every adapter (claude, codex, copilot, factory, pi, qwen, amp, opencode). readJsonFile/listJsonFiles/isMissingPathError/isMissingJsonSource (src/json.ts) replace raw JSON.parse(readFile(...)) + swallow-catch in amp, claude su
• Fit with existing patterns: Fits the established grain precisely. The old pattern was: each adapter did JSON.parse(await readFile(...)) inside a try { ... } catch { return [] } that silently swallowed EVERYTHING — read errors, parse errors, missing dirs — indistinguishably. The PR replaces that with two typed primitives (readJsonFile for single objects, streaming readJsonl for line streams) plus a single `isMissingPa
• Real-world viability: The streaming + bounded-recovery design holds up under the realistic failure modes the PR targets. readJsonl (jsonl.ts:98-139) is a true streaming generator over createReadStream with manual LF scanning and cross-chunk fragment reassembly — memory stays bounded regardless of file size (the 105MB / 415MB regression numbers in the PR body corroborate this). Corruption handling is isolated per-li
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: console debug added src/cli.ts

• console.log('No sessions found to upload.')

🎯 Usefulness Audit

🟡 strict corruption mode is SDK-only; no CLI flag [ergonomics] ``

ParseOptions.corruptionMode: 'strict' is threaded from ScanOptions through scanSessions (session-source.ts:106) into sessionJsonlOptions (integrity.ts:78), but the CLI (src/cli.ts) never sets it — every CLI invocation runs in the default recover mode, and there is no --strict flag in parseArgs. The PR body explicitly frames recover-as-default + strict-for-validation as the intended contract, and recover IS the right default for operator tooling, so this is not a blocker. But an ope

💰 Value Audit

🟡 Corruption receipt dedup keyed by SessionRef object identity [maintenance] ``

src/integrity.ts:15 keys the receipt-seen cache on a WeakMap<SessionRef, ReceiptState>. That is fine for the single parseSession() pass (src/session-source.ts:28 calls stampSessionIntegrity once per ref), but a future caller that constructs two SessionRef objects for the same underlying file would lose dedup and re-record receipts against both. Not a blocker for the current single-parse spine; worth a one-line comment at the WeakMap declaring the object-identity assumption so a later caller does

🟡 stampSessionIntegrity mutates receipts in place after trace id is known [maintenance] ``

src/integrity.ts:101–114 overwrites harness and sessionId on each corruption receipt after the root span's trace_id is resolved, depending on parse having completed before stamping. The ordering is correct today because parseSession() (src/session-source.ts:26–28) calls adapter.parse → stampSessionIntegrity in that order. The coupling is implicit; a future adapter that stamped integrity mid-parse would get half-enriched receipts. A one-line note in the JSDoc that stamp must run post-parse wo

---

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 · 20260713T141142Z}

[tangletools]

✅ Auto-approved drewstone PR — f84d330a

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T14:48:09Z}

[tangletools]

✅ Auto-approved drewstone PR — f84d330a

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T14:48:11Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	2 (1 low, 1 weak-concern)
Heuristic	0.1s
Duplication	0.0s
Interrogation	456.8s (2 bridge agents)
Total	456.9s

💰 Value — sound-with-nits

A coherent, well-architected shift from read-everything-then-skip-the-bad-line to streaming JSONL with per-record corruption receipts + OpenInference tool I/O; one minor DRY nit inside the change itself.

• What it does: Replaces every adapter's 'readFile → JSON.parse the whole file → swallow errors' pattern with a single streaming readJsonl (src/jsonl.ts) that yields valid records and emits one immutable source.corruption.receipt child span (location + SHA-256, never raw bytes) per malformed line, with strict mode that throws a typed JsonlParseError. Adds readJsonFile (src/json.ts) with typed errors, an int
• Goals it achieves: (1) No silent data loss — every corrupt JSONL record is accounted for as evidence rather than dropped, while every valid record still flows through. (2) Bounded memory — stream files instead of holding parsed arrays (regression test pins a 100MB file under 128MB RSS). (3) Fail loudly on unreadable sessions / zero-span parses / partial backend acceptance instead of returning []. (4) Capture tool I/
• Assessment: Sound and in the grain of the codebase. The new primitives are small, single-purpose, and exported from src/index.ts for SDK consumers. The spine stays in session-source.ts (parseSession calls stampSessionIntegrity once), adapters stay per-harness, and the substrate (agent-eval) provides the heavy lifting (contentHash, OtlpFileTraceStore, analysts). The receipt design is the right level of evidenc
• Better / existing approach: Checked for an existing JSONL reader in @tangle-network/agent-eval — it ships LockedJsonlAppender (write-side only) and canonicalJson/contentHash, but NO streaming reader. Searched src/ for createReadStream/readline/readJsonl usage — only the new src/jsonl.ts. So the reader is genuinely new capability, not a reinvention. The permissive canonicalJson in tool-io.ts differs deliberately from agent-ev
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Streaming JSONL readers with per-record corruption receipts, bounded tool-I/O capture, and Codex provenance are wired into the single parseSession chokepoint reached by every read path — coherent, complete, no dead surface.

• Integration: Fully reachable. parseSession (src/session-source.ts:21) is the one funnel for cli/upload/collect/live/evidence/session-index and stamps integrity on every path. All 6 JSONL adapters consume readJsonl/sessionJsonlOptions; all JSON adapters use readJsonFile; all 10 adapters plus redact+upload consume the tool-I/O helpers; Codex provenance flows to the report's SelectedSessionSource table. Exported
• Fit with existing patterns: Extends the established OTLP span+attribute model rather than competing with it: corruption evidence is emitted as source.corruption.receipt child spans (same span() primitive), tool I/O uses OpenInference input.value/output.value that runtime-store.ts:62 already consumes, and post-redaction digest recompute (normalizeToolIoAttributes) composes inside the existing redactSpans pipeline. The streami
• Real-world viability: Robust beyond the happy path: the reader handles cross-chunk line splits, CR-LF, missing trailing newline, invalid UTF-8 pre-decode, strict-vs-recover, and stream teardown in finally. Receipts are idempotent (dedup by identity tuple), upload dedup keys on post-privacy-normalization hash, PartialUploadError fails loudly on partial backend acceptance, EmptySessionError catches zero-span parses. Test
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: console debug added src/cli.ts

• console.log('No sessions found to upload.')

💰 Value Audit

🟡 Placeholder-trace_id → discover → remap pattern duplicated across 4 adapters [maintenance] ``

Claude (src/adapters/claude.ts:297-332), Factory (src/adapters/factory.ts:118-152), Pi (src/adapters/pi.ts:123-150), and Qwen (src/adapters/qwen.ts:98-203) each repeat the same ~10-line shape: track sourceTraceId=ref.sessionId + sourceRootId, accumulate spans under it during the stream, track firstTimestamp/lastTimestamp/sawEvent, then after the stream iterate spans to remap trace_id + parent_span_id and prepend a root. A small helper like `remapSessionTraceId(spans, sourceRootId, discoveredTrac

---

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 · 20260713T145814Z}

[tangletools]

🟢 Value Audit — sound

Verdict	sound
Concerns	3 (1 low, 2 weak-concern)
Heuristic	0.1s
Duplication	0.0s
Interrogation	442.9s (2 bridge agents)
Total	443.0s

💰 Value — sound

Replaces whole-file JSON.parse + silent line-dropping with a streaming JSONL reader that recovers every valid record, fingerprints each corrupt one with byte-exact receipts, and threads bounded integrity provenance + post-redaction metadata normalization through every adapter and the upload spine —

• What it does: Replaces the prior pattern (readFile whole file → split('\n') → JSON.parse with try/catch that silently dropped malformed lines, e.g. src/adapters/copilot.ts:48-58 on origin/main) with: (1) a streaming async-generator JSONL reader (src/jsonl.ts:98-139) that yields records one at a time, never holding the source file in memory; (2) byte-exact corruption handling with two modes — 'strict' (throws Js
• Goals it achieves: (1) No silent data loss: every valid JSONL record is parsed regardless of how many corrupt records surround it; the 130-corruption regression is the proof case. (2) Bounded memory: large sessions no longer sit fully parsed in RAM. (3) Loud failures for genuinely unreadable sessions (vs. the old catch { return [] } which silently made unreadable sessions look like zero-session cwd mismatches) — s
• Assessment: Sound. The change is correctly scoped and in the grain of the codebase. The codebase already funnels every OTLP-producing path through scanSessions → parseSession (src/session-source.ts:21-34), so stampSessionIntegrity hooks the one right place. Each adapter keeps its own line-type parser but routes I/O through four shared helpers (readJsonl, readJsonFile+isMissingPathError, toolIoAttributes, reco
• Better / existing approach: none — this is the right approach. Searched the substrate for an existing equivalent before concluding: rg 'readJsonl|streamJsonl|JsonlReader|readJsonFile' node_modules/@tangle-network/agent-eval/dist/index.d.ts → no exports; rg 'TOOL_IO_VALUE|toolIoAttributes|input\.value.*output\.value' node_modules/@tangle-network/agent-eval/dist/traces.d.ts → the substrate CONSUMES input.value/output.value
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Streaming JSONL refactor eliminates silent data loss across all harness adapters via one shared primitive, with corruption receipts and fail-loud error paths — fully wired into the existing scan/parse/upload spine and verified to hold under corrupt-input and invalid-UTF-8 edge cases.

• Integration: Fully reachable. The new streaming primitives (readJsonl/takeJsonl) are consumed by 6 JSONL adapters; readJsonFile/listJsonFiles/isMissingPathError/isMissingJsonSource by the remaining JSON-shaped adapters (amp, opencode, gemini, forge). sessionJsonlOptions + stampSessionIntegrity ride the central parseSession spine (session-source.ts:26-28), which is consumed by 6 callers (upload, collect, sessio
• Fit with existing patterns: Consolidates rather than competes. Before, each adapter owned a whole-file readFile+JSON.parse (claude's old parseLines, amp's old JSON.parse(await readFile)). The new readJsonl async generator is the single streaming primitive all JSONL adapters share; corruption receipts project onto the existing OtlpSpan model as source.corruption.receipt child spans; tool I/O uses OpenInference input.value/out
• Real-world viability: Holds past the happy path. Verified directly: 130 interleaved corrupt records yield 130 unique SHA-256 receipts with both bracketing valid records retained; an invalid-UTF-8 line is detected and recovered around; strict mode throws at the first corrupt line (line 2); second stamping is idempotent (3→3 spans, no duplicate children); root integrity metadata is 255 bytes (<256 bound). locate/parse no
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: console debug added src/cli.ts

• console.log('No sessions found to upload.')

💰 Value Audit

🟡 analyze.ts ceiling rename+raise is unrelated to the silent-data-loss fix and undescribed in the PR body [maintenance] ``

src/analyze.ts:66-78 renames DETERMINISTIC_VIEW_CEILING (256MB) → GENERATED_TRACE_FILE_CEILING (512MB) AND now also sets maxFileBytes on both OtlpFileTraceStore instances. This is a 2x cap raise riding on a fix PR. The recover-mode change should make sessions SMALLER on average (corrupt lines that were silently dropped before now still parse the valid ones; the only growth is N small receipt child spans). So the cap raise isn't demanded by this PR's goal — it's a separate 'let genuinely huge s

🟡 JsonSourceError mutates the underlying SyntaxError cause in place [maintenance] ``

src/json.ts:28-34 (pathSafeParseCause) rewrites cause.message and cause.stack on the original SyntaxError rather than wrapping it. This is mildly surprising for any caller that held a reference to the original parse error, but in practice the SyntaxError is fresh from JSON.parse and only the propagated JsonSourceError is observable. Worth a comment or a wrap-in-new-Error revision eventually; not a blocker.

---

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 · 20260713T151000Z}

[tangletools]

❌ Needs Work — f84d330a

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 26 findings (2 high, 3 medium, 21 low)

glm: Correctness 0 · Security 0 · Testing 0 · Architecture 0

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

Blocking

🔴 HIGH scanSessions default-onError change silently turns one bad session into total pipeline failure for callers that pass opts through — src/session-source.ts

Before: opts.onError?.(err) — errors were always swallowed and the scan continued (the file's documented 'never aborting' contract). After: if (!opts.onError) throw err for both locate (line 97) and parse (line 108). The in-tree callers that pass user-supplied opts straight through — planUpload (upload.ts:68), collectSessions (collect.ts:31), collectSessionIndex (session-index.ts:335), collectPolicyEvidence (evidence.ts:197), streamSessions (live.ts:587 unless caller set onError) — now abort the ENTIRE batch/upload/index/evidence build on the first empty or

🔴 HIGH 100 MB memory-bounded parsing test SIGABRTs/ETIMEDOUTs under vitest default pool — fails on pnpm test — tests/adapters.test.ts

Reproduced twice: full-file vitest run tests/adapters.test.ts fails this single test with child.status=null. First run hit the spawnSync 30s timeout (ETIMEDOUT). Subsequent runs die in ~1.4s with child.signal='SIGABRT', child.error=undefined, child.stderr='' — Node aborts during exit after stdout has flushed. Bisect: the same spawn args (--max-old-space-size=40 --max-semi-space-size=1 --import tsx --input-type=module --eval ...) succeed in 945ms under standalone node /tmp/investigate.mjs from the worktree (child.status=0, spanCount=203). Importing ClaudeAdapter alone (without parsing) under the 40MB heap also SIGABRTs at exit inside vitest, but exits cleanly with the default heap — so the abort is heap-pressure during module teardown when the child inherits vitest-polluted env. Delet

Other

🟠 MEDIUM Codex parse reads the JSONL file up to 3 times — src/adapters/codex.ts

parse() calls takeJsonl(path, 40) [read 1], then if meta is missing falls back to readJsonl(path) with an early break [read 2], and finally always runs a full readJsonl(path) main loop [read 3]. Best case is 2 full passes (head + main loop). For large codex rollout files (the 100MB test demonstrates real-world sizes), this triples I/O and slows ingestion. Fix: buffer the head lines and replay them into the main loop, or carry meta/first/model forward from the head scan and skip the fallback entirely by reading the head lazily into the main loop.

🟠 MEDIUM parseSession throws on empty spans, breaking direct CLI invocations and rendering downstream filters dead — src/session-source.ts

if (spans.length === 0) throw new EmptySessionError(ref.path) turns a previously-recoverable empty result into a hard error. Direct callers in cli.ts are not wrapped: collectSpans --session path (cli.ts:296), the discover loop (cli.ts:314), collectSessionRows (cli.ts:343, 348), streamExplicitSession (cli.ts:641). Two consequences: (a) traces inspect --session path/to/blank.jsonl and similar now crash instead of returning empty output; (b) collectSessionRows(...).filter(row => row.spans.length > 0) at cli.ts:354 is now dead code because parseSession never returns an empty array. The intent (reject empty as data loss) is reasonable for the scan path, but parseSession is a shared primitive also used for single-session inspection — making it throw conflates 'parser found nothing' with

🟠 MEDIUM Implicit one-time re-upload of every prior session after upgrade (no migration note) — src/upload.ts

The hash algorithm changed fundamentally: old state entries were written under the structural-only sessionHash (upload-state.ts:35 prior version), while new code writes under outboundHash(events, identity). Every existing record in ~/.local/state/tangle-traces/uploaded.json will miss on the first run after upgrade and every prior session will be re-sent to the backend — potentially a large surge for active users. Not a correctness bug (re-upload is idempotent via the server key), but the PR description should call this out as a migration side-effect, and operators may want to delete the state file to avoid noise. No version bump on the state file schema either.

🟡 LOW Claude foldSubagents meta parse errors now abort the entire session parse — src/adapters/claude.ts

The catch now uses if (!isMissingJsonSource(error)) throw error, which only suppresses ENOENT. A corrupt .meta.json (valid file, invalid JSON) now throws JsonSourceError(kind='parse') and kills the whole session parse — previously the subagent was silently orphaned under root and the session was still emitted. This is a deliberate strictness change (tested at line 459), but it means one bad sidecar aborts a session that used to degrade gracefully. If this is intentional (surface corruption), consider adding a corruption-receipt path for sidecar JSON instead of aborting, to match how JSONL corruption is handled.

🟡 LOW setAgentSessionIds extracts ALL UUIDs from input — false-positive risk for send_input/wait_agent — src/adapters/codex.ts

For non-spawn agent operations (send_input, wait_agent, etc.), sessionIds(input) runs CODEX_SESSION_ID globally on the entire input string. If the message body or any other field contains an unrelated UUID (request ID, build ID, etc.), it gets tagged as traces.codex.agent_session_ids and potentially traces.child_session_ids. The test only covers the happy path (single UUID in target field). Consider restricting extraction to the target/agents parameter via structured parsing rather than whole-text regex.

🟡 LOW Opencode tool parts in user messages silently change parent from LLM to root — src/adapters/opencode.ts

Behavior change: user messages no longer emit an LLM span (correct semantic fix). Tool parts in user messages now get parentSpanId: isUser ? rootId : llmId. Previously all tools were parented to an (incorrectly emitted) LLM span for the user message. The new behavior is more correct, but downstream consumers that relied on tools always having an LLM parent may see orphaned tool spans under root for user-message tool results. Tested implicitly but no explicit test for tool-in-user-message parentage.

🟡 LOW No dedicated unit tests for integrity.ts pure helpers — src/integrity.ts

Coverage is entirely via integration tests in tests/adapters.test.ts and tests/upload.test.ts. The pure helpers (receiptIdentity, receiptKey, corruptionDigest, receiptSpanId, stateFor) have no direct unit tests for edge cases: empty corruptions array passed to corruptionDigest, WeakMap state lifetime across multiple SessionRef instances, receiptKey collision behavior when sourcePath contains JSON-special characters, or stateFor rebuild when the same ref's corruptions array is replaced. Integration tests cover the happy paths well (single corruption, 130 corruptions, strict round-trip, upload provenance) but a focused integrity.test.ts would lock down the invariants of these exported helpers independently.

🟡 LOW SessionIntegrity.status is a single-value enum — src/integrity.ts

status is typed as the literal 'degraded_not_lossless' and only ever assigned that value (recordSessionCorruption line 50, sessionIntegrityAttributes line 67). The type and ATTR.SESSION_INTEGRITY attribute suggest future expansion (e.g., a 'complete' or 'verified_lossless' state), but the code currently has no path that produces any other status. Not a bug — flagging in case the intent was to also emit positive integrity signals, which would require a setter path beyond the corruption-recovery flow.

🟡 LOW stampSessionIntegrity destructively mutates stored receipts redundantly — src/integrity.ts

Lines 95-98 overwrite receipt.harness and receipt.sessionId on every receipt already stored by recordSessionCorruption. recordSessionCorruption (line 42) already stamped these via { ...receipt, harness: ref.harness, sessionId: ref.sessionId }. The re-stamp is partially redundant for harness (never changes) and only meaningful for sessionId when parent.trace_id differs from ref.sessionId (the codex head-scan case). Idempotent in practice and harmless, but creates an ordering coupling: callers MUST invoke stampSessionIntegrity after parse to normalize sessionId to

🟡 LOW Empty-byTool fallback path is untested — src/report.ts

Lines 248-253 branch on toolStats.length > 0; when byTool is empty the code falls back to Math.round(m.duplicateRatem.totalCalls) / Math.round(m.errorRatem.totalCalls). tests/report.test.ts:138-141 only exercises the populated-byTool path. computeToolUseMetrics always returns a byTool record in practice (verified in baseline-DsNteOgR.d.ts:13-24), so the fallback is defensive dead-ish code, but if it ever fires the retriedFailures = Math.round(m.retryRate*errorCalls) computation compounds two rounding steps and the display string 'X% of failed calls retried (Y/Z)' would carry that error. Add a one-line test fixture with byTool:{} to lock the fallback's format.

🟡 LOW corruptionDigest rendered raw without tableCell sanitization — src/report.ts

Line 157 interpolates source.corruptionDigest directly into backticks without tableCell. The line is a list item (not a table row) so pipe-escaping is not needed, but the value comes from span attribute ATTR.CORRUPTION_DIGEST (cli.ts:266) and could in principle contain a backtick that breaks the inline code span. Low impact since digests are conventionally hex/sha-prefixed. Mention only for consistency with the tableCell pattern used for every other untrusted string in this block.

🟡 LOW tableCell does not escape backticks; subject cell is not backtick-wrapped — src/report.ts

tableCell (line 67-69) escapes pipes and collapses whitespace but not backticks. sessionId/path cells are wrapped in backticks, so an embedded backtick in a path would break the inline code span. More notably the subject cell at line 120 (${tableCell(source.subject) || '(no prompt captured)'}) is NOT wrapped in backticks at all, so a user's first prompt line containing markdown-special chars (* _ # [) renders as live markdown inside the table cell. Pipes are escaped (table won't break), so impact is cosmetic, but it is the one cell that holds untrusted free-form text. Ei

🟡 LOW Dead defensive guard in stampSessionIntegration caller path — src/session-source.ts

parseSession calls stampSessionIntegrity(ref, spans) only after if (spans.length === 0) throw. Inside integrity.ts, stampSessionIntegrity does const parent = spans.find(...) ?? spans[0]; if (!parent) return — that if (!parent) return is now unreachable because spans is guaranteed non-empty here. Not harmful, but it implies the API contract isn't encoded in the type (spans: OtlpSpan[] vs NonEmptyArray). Consider a NonEmptySpan type for parseSession's return so the dead guard can be deleted and the contract is enforced at the type level.

🟡 LOW locateSessions cwd-bypass for integrity-stamped refs is not exercised by any real adapter — src/session-source.ts

if (!cwdMatchesSelection(ref.cwd, cwdSelections) && !(ref.cwd === null && ref.integrity)) continue — the second clause allows a degraded session whose cwd couldn't be recovered to bypass the cwd filter. shared.test.ts:105-134 proves the branch works when a synthetic adapter pre-populates ref.integrity at locate time, but a grep of src/ shows no real adapter currently sets integrity during locate (only recordSessionCorruption in integrity.ts:50 writes it, and only during parse). The branch is correct as a forward-looking contract but currently dead in production. Either document the contract on HarnessTraceAdapter.locate ('may set integrity to mark pre-known degradation') or note in the comment that the branch is anticipating future adapter behavior so reviewers don't read it as a bug.

🟡 LOW Stale-state edge: previous?.uploadedAt ?? uploadedAt keeps invalid string timestamps — src/upload.ts

If a corrupted uploaded.json has an entry with uploadedAt: '' or a non-ISO string, ?? does not replace it (only null/undefined). The resulting eventsAt(eventTimestamp) produces events with a bogus tangle.uploaded_at attribute used in the first hash probe. It happens to be harmless (hash won't match → fallback to current uploadedAt → upload with correct timestamp), but worth tightening: validate previous.uploadedAt via parseIsoToEpochMs(...) or Date.parse(...) and fall back to uploadedAt on failure.

🟡 LOW PartialUploadError lacks cause; retry semantics after a partial accept are unspecified — src/upload.ts

On res.accepted !== events.length the code throws new PartialUploadError(key, res.accepted, events.length) without surfacing any backend response detail. More importantly: the backend HAS the partially-accepted spans under the same idempotency key, so on retry the server may return accepted: 0 (already seen) and trigger ANOTHER PartialUploadError, looping until the user gives up. Worth either (a) documenting the expected retry behavior in the ExecuteOptions JSDoc, or (b) treating accepted === 0 on retry as success when the idempotency key matches a prior send.

🟡 LOW UploadItem.hash semantics silently changed (structural -> full content); field now redundant — src/upload.ts

planUpload still computes hash = sessionHash(spans) (upload.ts:70) and exposes it on UploadItem. The comment was updated to 'Source-span hash for inspection. Final outbound hashing happens in executeUpload.' but the underlying sessionHash itself changed from a 4-field structural digest to a full content hash (upload-state.ts:35). External callers (or future maintainers) who thought they knew what this field means will see different values. Since dedup no longer consumes this field, prefer either dropping it from the public interface or renaming it to sourceContentHash to make the new semantics explicit.

🟡 LOW UploadItem.isNew is hard-coded to true and misleading — src/upload.ts

The old isNew: !alreadyUploaded(...) was the field's contract; it is now isNew: true unconditionally, with only a docstring ('Candidate marker retained for API compatibility; final freshness depends on execute options'). Callers reading item.isNew to decide whether to display/process a session will get wrong answers. Grep confirms no internal consumers read it (src/cli.ts uses plan.items.length instead), but it is exported via src/index.ts. Recommend either removing the field or renaming to candidate: true so the meaning matches the value.

🟡 LOW prepareOutbound runs the external redactor over sessions that will be dedup-skipped — src/upload.ts

The redactor branch (if (opts.redactor && !opts.stripContent)) maps over ALL plan.items BEFORE the per-item dedup check at line 243. The old code only applied the redactor to newItems (post-source-hash dedup). For users with many already-uploaded sessions and an expensive ML scrubber, this is a real regression — every re-run pays the redactor cost on sessions that will be discarded. Either move the dedup probe above the redactor pass (compute a cheap pre-redactor outbound hash first as a fast filter), or document the cost.

🟡 LOW Hardcoded traceShapeDigest snapshots break on any unrelated attribute addition — tests/adapters.test.ts

traceShapeDigest excludes input/output attributes but is still a literal sha256 of canonicalized spans for copilot/qwen/factory/claude/pi. Any unrelated change to the adapter's attribute set (adding a new 'traces.codex.*' label, changing status message wording, reordering yields) breaks the snapshot with no semantic signal. The function name suggests shape-only but it actually encodes content+ordering. Acceptable as a deliberate snapshot guard, but should be documented as such (e.g., a comment naming what change-class the snapshot guards against) so a future editor doesn't waste an hour bisecting a hash mismatch.

🟡 LOW afterAll cleanup may mask per-test failures on shared dir — tests/adapters.test.ts

const dir = mkdtempSync(...); afterAll(() => rmSync(dir, { recursive: true, force: true })) is fine, but tests inside the file write fixture files into dir using deterministic names (e.g., ${harness}-malformed-session.jsonl). If a future test reuses one of those names or fails to overwrite cleanly, residue from a prior test run could mask a regression. Low risk today because each test writes before reading, but worth a one-line note in the file's conventions.

🟡 LOW Dry-run output paths are pid-based, not per-test unique — tests/upload.test.ts

tt-keep-${process.pid}.jsonl and tt-strip-${process.pid}.jsonl live in tmpdir() and are overwritten on each run. Pre-existing pattern, not introduced here, but it means concurrent vitest workers sharing a pid (rare) or reruns within one process silently clobber. mkdtempSync would be cleaner. No correctness impact on the assertions, which read the files immediately after writing.

🟡 LOW No coverage for redactor + stripContent combination — tests/upload.test.ts

prepareOutbound (src/upload.ts:217-225) explicitly makes stripContent win when both opts.redactor and opts.stripContent are set (the redactor branch is gated by !opts.stripContent). The test suite never exercises the both-set case to assert that precedence. Cheap to add: pass both and assert the output equals the strip-only output. Behavior is correct by code inspection; just unverified.

🟡 LOW Partial-error test only covers single-item, all-rejected case — tests/upload.test.ts

Backend returns events.length - 1 for an item with exactly one span, so accepted=0/expected=1. The state-empty assertion confirms no saveState, but the test does not exercise (a) a multi-item plan where item 1 fully accepts and item 2 partially fails (would item 1's state have been written? Answer by code reading: no, because state writes happen only after the full items loop succeeds — src/upload.ts:296-314), nor (b) accepted>0 but <expected within one session. Both are interesting boundary conditions of PartialUploadError; documenting as a coverage nit, not a defect.

---

_{tangletools · 2026-07-13T15:42:19Z · trace}

[tangletools]

❌ 2 Blocking Findings — f84d330a

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

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-13T15:42:19Z · immutable trace}

[tangletools]

✅ No Blockers — f84d330a

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 26 findings (3 medium, 23 low)

glm: Correctness 0 · Security 0 · Testing 0 · Architecture 0

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

🟠 MEDIUM Batch callers inherit abort-on-first-empty-session without onError — src/session-source.ts

scanSessions now throws on the first empty/corrupt session when no onError is supplied (line 108: if (!opts.onError) throw err). In-tree batch consumers planUpload (upload.ts:68), collectSessions (collect.ts:31), collectPolicyEvidence (evidence.ts:197), and collectSessionIndex (session-index.ts:335) all call scanSessions(opts) without onError. A single truncated/empty session file on disk will now abort an upload/collect/index of hundreds of sessions — a regression from the prior silent-skip. collect.ts/evidence.ts/session-index.ts are NOT in this PR's diff. Fix: either thread an onError into those call sites that logs-and-continues (preserving batch completion), or docume

🟠 MEDIUM Idempotency key now includes volatile timestamp, weakening cross-run retry-safety — src/upload.ts

idempotencyKey = ${key}:${hash} where hash = outboundHash(events, identity). events carry ATTR.UPLOADED_AT on the root span (injected at line 238), so the hash is timestamp-sensitive. Old code used item.hash = sessionHash(spans) with no timestamp → identical key across retries. New code: after a mid-batch PartialUploadError (state save is deferred, line 315), the retry runs with a different uploadedAt → different events → different hash → different idempotency key, so the server's idempotency-key dedup will NOT fire for sessions already accepted in the failed batch. The lin

🟠 MEDIUM Tests write to real ~/.local/state/tangle-traces/uploaded.json — no XDG_STATE_HOME isolation — tests/upload.test.ts

T5 (line 210, 219, 232, 234) and T7 (line 290) call executeUpload with a custom backend but WITHOUT dryRun:true. executeUpload (src/upload.ts:315) calls saveState(state) on success, which writes to stateFile() = ~/.local/state/tangle-traces/uploaded.json. Confirmed empirically: my test run left { 'claude-code:sess1': { hash: 'd85fe4a3...', spanCount: 131, uploadedAt: '2026-07-13T15:44:05Z' } } in the real file. T6 is safe (throws before saveState). The sibling tests/sdk.test.ts:21-24 already follows the correct pattern: beforeAll(() => { process.env.XDG_STATE_HO

🟡 LOW Codex parse() reads the rollout file up to 3x (locate head + parse head + fallback meta scan + main loop) — src/adapters/codex.ts

parse() calls takeJsonl(path, 40) for the head (line 255), then if session_meta is absent from the first 40 lines, does a full readJsonl scan to find it (line 260), then a second full readJsonl for the main event loop (line 306). locate() already did its own head scan (line 235). Memory stays bounded (each stream is destroyed in r

🟡 LOW Opencode user turns no longer emit an LLM span — behavioral change in span structure — src/adapters/opencode.ts

Previously every message (user or assistant) got an LLM span; user messages with text also got a user.prompt span. Now user messages get ONLY a user.prompt span (no LLM span), and tool parts in user turns parent to rootId instead of a user-LLM span. This is semantically more correct (a user message is not an LLM call) and is tested (line 1235: expect(spans.filter(name === 'message.user')).toHaveLength(0)). Flagging as a behavioral change because any downstream analyst or golden-file digest that counted LLM spans per opencode session will see different numbers. Not a bug — intentional and tested — but worth noting in a changelog.

🟡 LOW No programmatic cap on corruption receipt span count — src/integrity.ts

stampSessionIntegrity emits one child span per corruption receipt with no upper bound. Tests cover 130 corruptions (adapters.test.ts:341), but a heavily corrupted production session (thousands of bad lines) would produce thousands of receipt spans, unboundedly growing the OTLP output. Each receipt span is individually size-bounded (~16KB test assertion at adapters.test.ts:390), but the count is uncapped. Consider a configurable ceiling with a summary receipt for overflow, or document the expected maximum.

🟡 LOW sourcePath emitted in receipt attributes without local redaction at this layer — src/integrity.ts

The receipt span includes traces.session.corruption.source_path verbatim from the receipt (line 122). File paths can contain usernames, project names, or directory structures considered sensitive. This module relies entirely on the downstream redaction pipeline (redactSpans called in upload.test.ts:286) to scrub paths. If a consumer calls stampSessionIntegrity and serializes spans outside the upload path (e.g., writeOtlpFile directly), source_path leaks unredacted. Not a bug in the current upload flow, but a defense-in-depth gap worth documenting.

🟡 LOW Leading UTF-8 BOM (\uFEFF) on line 1 is treated as corruption — src/jsonl.ts

A file starting with EF BB BF then valid JSON fails JSON.parse because the BOM becomes part of the first line's bytes. Recover mode receipts line 1 and proceeds; strict mode rejects the entire file at line 1. BOM-prefixed JSONL is common from Windows editors and Out-File on PowerShell. JSON spec technically forbids BOM, so this is defensible, but a one-line strip of a single leading 0xEF 0xBB 0xBF on lineNumber===1 would avoid spurious corruption receipts in real-world traces. Confirmed via runtime probe: BOM file yields 0 rows + 1 receipt in recover mode.

🟡 LOW No max-line-length guard; pathological input buffers whole file in memory — src/jsonl.ts

fragments[] accumulates one subarray view per chunk when no LF arrives. For a multi-GB file with no newlines (or a corrupted multi-hundred-MB line), Buffer.concat(fragments) materializes the entire line before parse/hash. Normal JSONL is fine (records are short), but a hostile or truncated file could OOM the process before the line is even parsed. Consider a hard cap (e.g., 64 MiB per line) that routes to handleCorruption when exceeded, preserving the streaming contract.

🟡 LOW fragments = [] allocates a fresh array per record — src/jsonl.ts

Reset is fragments = [] rather than fragments.length = 0. For high-throughput parsing (millions of lines) this creates one extra array object per line for GC. Micro-optimization, not a correctness issue — flag only because the hot loop is the primary parse path for every adapter.

🟡 LOW 'omitted' path not covered by a multi-source test — src/report.ts

The new tests cover single-source omission math (130 total, 1 supplied -> '129 additional receipts omitted'). The double-break cap interaction across multiple sources (each pre-sliced to 100 by cli.ts:277, then re-capped to 100 TOTAL by the report loop at lines 140/143) has no test. The math is correct by inspection (omitted = totalCorruptionCount - corruptions.length, both consistent post-cap), but a 2-source / 250-receipt test would lock it in against future regressions of the break order.

🟡 LOW Stale 'in a short interval' suffix added without verifying detector window semantics — src/report.ts

The empty-state copy changed from 'no tool called >=3x with identical args' to '... identical args in a short interval'. This is a docstring-only change in the report, but StuckLoopFinding carries an explicit windowMs (used at line 241: f.windowMs/1000). 'Short interval' is now accurate, but the threshold value is not surfaced anywhere — a reader can't tell what 'short' means. Consider either naming the window (e.g. 'within <1s') or leaving the qualifier out; the current wording is vague.

🟡 LOW sha256 and corruptionDigest bypass tableCell escaping — src/report.ts

Receipt rows render `${receipt.sha256}` and the per-source digest line renders `${source.corruptionDigest}` without going through tableCell. These are hex (sha256:hex for digest) so they are safe today, but it's an inconsistency vs the rest of the table — if the digest scheme ever carries a non-hex suffix (e.g. a path), it would break the table. Cheap to route through tableCell for uniformity.

🟡 LOW tableCell does not escape backticks; subject/path/sessionId with a backtick breaks the inline-code wrapper — src/report.ts

tableCell only does value.replace(/\s+/g,' ').replaceAll('|','\|').trim(). Cells like sessionId, parentSessionId, path, and (for receipts) sourcePath are wrapped in backticks: \${tableCell(source.sessionId)}`. A backtick inside the value closes the inline-code span early and corrupts the markdown table. Session IDs are UUIDs (safe), but source paths and the receipt sourcePath come straight from disk and can contain backticks on some filesystems. Fix: also escape backticks, e.g. .replaceAll('','\`'), or drop the backtick wrapper for path/subject columns.

🟡 LOW scanSessions forwards only corruptionMode, not the full ParseOptions — src/session-source.ts

ScanOptions extends ParseOptions (line 68), but scanSessions only threads opts.corruptionMode into parseSession at line 106 ({ corruptionMode: opts.corruptionMode }), not the full ParseOptions object. If ParseOptions gains fields later, they silently won't propagate through the scan path. Minor maintenance footgun; consider parseSession(adapter, ref, { corruptionMode: opts.corruptionMode }) staying explicit (current) vs. parseSession(adapter, ref, opts) to forward everything.

🟡 LOW stampSessionIntegrity mutates spans before repo attr resolution — src/session-source.ts

stampSessionIntegrity (line 28) pushes synthetic corruption-receipt spans into the array before resolveSessionRepoAttrs/stampRepoAttrs run (lines 29-32). The synthetic spans then receive repo attrs and are seen by the workdir-repo stamper. In practice harmless — receipt spans carry no cwd and anchor to the parent — but the ordering couples integrity emission to repo-attr resolution implicitly. A comment noting the synthetic spans are safe for downstream stampers would clarify intent.

🟡 LOW No test covers the two-pass timestamp recompute or retry-after-failure paths — src/upload.ts

tests/upload.test.ts covers: unchanged-session skip (line 215-217), privacy-mode separation (219-241), PartialUploadError non-recording (243-263), stripContent (176-192). Missing: (a) a plan where state has a previous record AND the spans differ — exercising the line 245-249 second-pass recompute with the new uploadedAt; (b) a retry after PartialUploadError proving the idempotency-key stability (or documenting the intentional instability). These are the subtlest new code paths and the highest regression risk.

🟡 LOW Two-pass timestamp logic in prepareOutbound has no explanatory comment — src/upload.ts

Lines 240-249 compute events+hash twice: first with previous?.uploadedAt (to test stable re-upload → skip), then recompute with uploadedAt if content changed. This non-obvious dance will tempt a future maintainer to 'simplify' it (e.g., always use uploadedAt), which would silently break the unchanged-session skip (every run would get a new timestamp → new hash → re-upload). Add a 2-line comment stating: first pass tests byte-identity against the stored record; second pass promotes the timestamp only when content genuinely changed.

🟡 LOW UploadItem.isNew is now unconditionally true — misleading public API — src/upload.ts

UploadItem is re-exported via src/index.ts:76 (export * from './upload.js'). The field was previously set to !alreadyUploaded(...) in planUpload and the old executeUpload consumed it via plan.items.filter(i => i.isNew). Now it is hardcoded true. The JSDoc ('Candidate marker retained for API compatibility; final freshness depends on execute options') warns internally, but external SDK consumers using the same filter pattern the library itself used will silently get all items. Consider marking @deprecated in the JSDoc and/or scheduling removal.

🟡 LOW 100 MB spawnSync test fails ungracefully on EAGAIN — tests/adapters.test.ts

spawnSync(process.execPath, …) returns {status: null, error: Error('EAGAIN')} when the process cannot be forked (RLIMIT_NPROC, container thread caps, CI sandbox). The assertion expect(child.status, …).toBe(0) then reports expected null to be 0 with no skip path. In this review environment the test fails with exactly that. The test should detect child.error?.code === 'EAGAIN' (and/or ENOMEM) and it.skip with a clear reason, rather than reporting a false failure unrelated to the adapter under test.

🟡 LOW Hardcoded trace-shape SHA-256 digests couple tests to exact adapter output — tests/adapters.test.ts

Lines 558, 592, 620, 666, 719 assert literal 64-hex-char digests over the (canonicalized, input/output-stripped) span array. Adding any new attribute — even an opt-in telemetry flag — turns 5 green tests red. The shape-lock intent is reasonable, but the cost is high maintenance and noisy failures that don't correspond to behavior breaks. Consider asserting on structural invariants (span names, kinds, parent links) instead, or pin the digest only in a dedicated 'snapshot' test isolated from the behavior tests.

🟡 LOW Inconsistent env-restore style — withEnv helper not used uniformly — tests/adapters.test.ts

The PR introduces a clean withEnv helper (lines 123-132) and uses it in the new tests, but two sibling pre-existing blocks (lines 1333-1341 for CODEX_HOME, lines 1403-1411 for HOME) still inline the same prev = process.env.X / try / finally pattern. The inconsistency is minor but means a future reader cannot grep for one pattern. Folding the two remaining blocks into withEnv would also shorten the diff and remove a sm

🟡 LOW Memory-bounded streaming test only covers Claude; Codex (the triple-read adapter) has no RSS assertion — tests/adapters.test.ts

The 100MB/128MB-RSS test (line 196-278) exercises ClaudeAdapter only. CodexAdapter — which has the most complex multi-read path (head + fallback + main loop) and the most event types — has no equivalent memory-bound assertion. Given Codex rolls out files under ~/.codex/sessions/YYYY/MM/DD/ that can grow large, a Codex large-file test would guard against a future regression where a read path accidentally buffers. The streaming primitive (readJsonl) is shared so risk is low, but the test coverage is asymmetric.

🟡 LOW Temp directories leak — never cleaned up — tests/adapters.test.ts

Lines 1220, 1245, 1260, 1269, 1287, 1325, 1345, 1372, 1399, 1415 each call mkdtempSync(join(tmpdir(), 'tt-…-')) but never schedule cleanup. Only the module-level dir (line 29) gets afterAll(() => rmSync(dir, …)). I confirmed the leak by listing /tmp/tt-codex-cont-*, /tmp/tt-oc-*, etc. — hundreds of stale dirs from prior runs. Each test run leaves ~10 more. Fix: either route these through the shared dir (e.g. mkdtempSync(join(dir, 'oc-'))) or wrap each in its own try/finally with rmSync.

🟡 LOW T2 doesn't assert traces.output.truncated === true despite value exceeding 16KB limit — tests/upload.test.ts

T2 constructs output = ghp_aaa…aaa: + 'x'.repeat(20_000) (~20KB, over TOOL_IO_VALUE_MAX_BYTES=16*1024). After redaction the token is replaced but ~20K chars remain, so normalizeToolIoAttributes must set traces.output.truncated=true. The test asserts the [truncated] suffix marker (line 124) and that bytes/sha256 match the post-truncation value, but never asserts the truncated boolean itself. T1 symmetrically asserts truncated===false for the small case (line 110). Add expect(attributes['traces.output.truncated']).toBe(true) for completeness — it's the attribute d

🟡 LOW UploadResult.redactionCount aggregation and byRule propagation untested — tests/upload.test.ts

executeUpload returns redactionCount = items.reduce((n, {item}) => n + item.redaction.redactionCount, 0) (src/upload.ts:259). All uploadItem helpers hardcode redaction.redactionCount=0 and byRule={}, so the aggregation path is never exercised with non-zero counts. sessionMeta also surfaces ATTR.REDACTION_COUNT on the root span (src/upload.ts:102). A test with redactionCount>0 across multiple items would verify the sum and the root-span attribute.

---

_{tangletools · 2026-07-13T15:52:23Z · trace}

Superseded by re-review — no blocking findings on latest commit.

[tangletools]

✅ Approved — 26 non-blocking findings — f84d330a

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

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-13T15:52:23Z · immutable trace}

[tangletools]

✅ Auto-approved drewstone PR — bbc9fdda

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-13T16:32:33Z}

[tangletools]

🟢 Value Audit — sound

Verdict	sound
Concerns	3 (2 low, 1 weak-concern)
Heuristic	0.1s
Duplication	0.0s
Interrogation	273.8s (2 bridge agents)
Total	273.9s

💰 Value — sound

Replaces whole-file session reads with streaming JSONL + corruption-receipt recovery across 6 adapters and tightens report honesty — coherent, in-grain, no existing equivalent to duplicate.

• What it does: Converts all JSONL-based harness adapters (Claude, Codex, Copilot, Factory, Pi, Qwen) from readFile+split-the-whole-file into a streaming async-generator readJsonl with two corruption modes: strict (throw on first bad record) and recover (skip the record, emit an immutable source.corruption.receipt child span carrying source path, line number, exact byte offset/length, and SHA-256 — but never the
• Goals it achieves: Eliminate four classes of silent data loss the prior code had: (1) malformed JSONL lines silently skipped via catch{}, (2) unreadable discovered sessions silently returned [], (3) zero-span parses silently filtered, (4) partial backend acceptance silently ignored. Secondary goals: make corruption auditable (location+hash receipts) without retaining raw malformed bytes in traces/uploads; make tool
• Assessment: Sound. The four new modules (jsonl.ts, json.ts, integrity.ts, tool-io.ts) are each single-purpose and consumed uniformly across the adapters that need them — the factoring matches the existing adapter-module conventions. The streaming reader is the correct Node.js primitive (createReadStream + buffer accumulation for split chunks) and the receipt design is well-considered: fingerprint-only (sha256
• Better / existing approach: Checked agent-eval 0.116.0 (npm-packed and inspected dist/index.js): its internal readJsonl is readFileSync(path,'utf8').split('\n') — the exact whole-file antipattern this PR fixes, and it is NOT exported (no readJsonl in any .d.ts). So there is no existing substrate equivalent to reuse. The PR correctly builds the streaming+receipt infrastructure locally and correctly reuses agent-eval's expor
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — error

usefulness agent produced no parseable value-audit JSON.

• Model: opencode/deepseek/deepseek-v4-pro
• Bridge attempts: 3
• Bridge error: opencode/zai-coding-plan/glm-5.2: bridge stream ended without value-audit content; opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content; opencode/deepseek/deepseek-v4-pro: bridge stream ended without value-audit content

🔎 Heuristic Signals

🟡 Cruft: console debug added src/cli.ts

• console.log(

🟡 Cruft: magic number added src/report.ts

•  lines.push(`  - \`${f.toolName}\` ×${f.occurrences} with identical args across ${(f.windowMs / 1000).toFixed(1)}s`)
  

💰 Value Audit

🟡 Streaming JSONL + corruption-receipt pattern is a substrate-upstream candidate [maintenance] ``

The jsonl.ts streaming reader with sha256 corruption receipts solves a problem every agent-eval consumer will face when reading large session files (the PR's own proof: 420MB / 158K-line operator session at 116MB peak RSS). agent-eval's own internal readJsonl (inspected in @tangle-network/agent-eval@0.116.0 dist/index.js) still uses readFileSync+split — the antipattern. Once this pattern is proven in traces (the PR ships regression tests at tests/jsonl.test.ts:55-80 and a 130-corruption regressi

---

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 · 20260713T181115Z}

[tangletools]

✅ No Blockers — bbc9fdda

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 26 findings (3 medium, 23 low)

glm: Correctness 0 · Security 0 · Testing 0 · Architecture 0

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

🟠 MEDIUM Codex parse() scans the JSONL file 2–3 times — src/adapters/codex.ts

parse() does: (1) takeJsonl(head, 40 lines) at line 255, (2) a fallback full-file readJsonl when meta isn't in head (lines 259-266), then (3) the main full-file readJsonl loop (line 306). For continuation sessions where session_meta isn't in the first 40 lines, this means 3 full passes over a file that can be 100MB+. The other refactored adapters (claude/pi/qwen/factory) avoid this by using sourceTraceId as a placeholder then rewriting trac

🟠 MEDIUM scanSessions throw-by-default breaks batch callers that don't set onError — src/session-source.ts

The contract changed from silent-swallow (opts.onError?.(err); continue) to throw-by-default (if (!opts.onError) throw err). This is documented and tested (shared.test.ts:70-86), but planUpload (upload.ts:68), collectSessions (collect.ts:31), collectSessionIndex (session-index.ts:335), and collectPolicyEvidence (evidence.ts:197) all thread ScanOptions straight through to scanSessions without injecting onError. cmdUpload (cli.ts:750) calls planUpload without onError, so traces upload will now abort the entire batch on the first corrupt/empty session file instead of skipping it. Impact: a single malformed session in the time window blocks all uploads. Fix: either (a) have these batch wrappers inject a default onError that logs-and-continues, or (b) confirm the fail-loud behavior is int

🟠 MEDIUM 100MB RSS test spawnSync timeout is too tight for CI reliability — tests/adapters.test.ts

spawnSync uses timeout: 30_000. The child process must start node, load the tsx loader (--import tsx), compile the ClaudeAdapter, stream-parse 101 records of ~1MB each with a 40MB heap cap (--max-old-space-size=40), and emit results. In this environment the child exceeded 30s, producing ETIMEDOUT (child.status === null). The test then fails at expect(child.status).toBe(0). The 100MB file is written synchronously before spawnSync (101 x writeSync of ~1MB), consuming additional wall time. Fix: increase timeout to 60-90s, or reduce record count (e.g. 21 records at 5MB each still proves bounded memory), or pre-write the file in beforeAll.

🟡 LOW amp.ts tool_use block in a user message gets a dangling parent span id — src/adapters/amp.ts

For user messages the adapter emits only a user.prompt span (no LLM span), but the tool_use block handler at line 170 unconditionally sets parentSpanId: llmId. If a tool_use block ever appears in a user message (unusual but not schema-forbidden), the resulting tool span's parent_span_id refers to an LLM span id that was never emitted, producing a dangling reference in the trace tree. Pre-existing — the diff only changed content→extra:toolIoAttributes, not the parent logic. The opencode adapter handles this correctly with parentSpanId: isUser ? rootId : llmId ([line 194](https://github.com/tangle-network/traces/blob/bbc9fdda72a22d5e57175a058235f9598e25ee16/src/adapters/amp.ts

🟡 LOW Codex sessionRole operator-precedence relies on || short-circuit — src/adapters/codex.ts

parentSessionId || meta?.payload?.thread_source === 'subagent' ? 'child' : 'operator' parses as (parentSessionId || (thread_source === 'subagent')) ? 'child' : 'operator'. Correct today, but fragile — a future editor could misread the precedence. An explicit grouping parens or Boolean(...) would prevent regressions. Not a bug, just a readability nit on a load-bearing expression.

🟡 LOW forge.ts recordToolOutput fallback captures is_error flag inside output.value — src/adapters/forge.ts

recordToolOutput(t, entry.tool.output?.values ?? entry.tool.output) — when the output object has no .values field, the fallback captures the entire {is_error: bool} envelope as output.value. The is_error signal is already recorded on the span status (line 183), so including it in output.value is redundant and slightly misleading for analysts reading the captured tool output. Prefer entry.tool.output?.values only, or strip the envelope before falling back.

🟡 LOW Watch label [full-session, not time-bounded] juxtaposed with (Ns) reads as contradictory — src/cli.ts

The onLoop line emits ${ts} REPEATED-GROUP [full-session, not time-bounded] ... (${(l.windowMs / 1000).toFixed(0)}s). not time-bounded refers to the grouping scope (entire session, no sliding window), while windowMs is the wall-clock span of the group itself (first to last occurrence). Both are accurate but a reader sees 'not time bounded' next to a seconds figure. Clarify by relabeling the seconds, e.g. (span: Ns) or (group span Ns).

🟡 LOW cmdUpload dry-run no longer surfaces 'all already uploaded' state distinctly — src/cli.ts

Previous flow printed Nothing new to upload. when newItems was empty (post-dedup at plan time). Now candidates.length === 0 only fires when no sessions exist in the window; a window where every session is already-uploaded falls through to dry-run printing dry run: 0 session(s), N redaction(s). Minor UX regression — the user has to infer 'all already uploaded' from the zero count rather than getting the dedicated message. The real upload path still reports skippedSessions correctly at line 800, so this only affects the dry-run preview branch.

🟡 LOW cmdUpload redaction total overstates uploaded redactions when sessions are skipped — src/cli.ts

totalRedactions is summed from candidates (plan.items) BEFORE executeUpload dedup. The line PII/secrets redacted: ${totalRedactions} is printed before the dry-run/confirm/upload split, so when some sessions are skipped as already-uploaded (res.skippedSessions > 0), the displayed number includes redactions on sessions that never leave the machine. Not a leak — pure display inflation. Fix: re-derive from the post-dedup redactionCount returned by executeUpload (res.redactionCount) for the final summary, or label the pre-dedup number as (candidates).

🟡 LOW receiptIdentity recomputed up to 3x per receipt on large corrupt files — src/integrity.ts

The same JSON.stringify(tuple) is computed in receiptKey (recordSessionCorruption), corruptionDigest, and receiptSpanId. For the tested 130-corruption case this is negligible, but a pathologically corrupt file would re-hash the same tuple repeatedly. Memoize the key on the receipt object or compute once per call site if million-receipt files are plausible.

🟡 LOW receiptSpanId produces non-OTLP-compliant span IDs — src/integrity.ts

Format is ${parentSpanId}:corruption:${64-hex-digest} — well beyond OTLP's 8-byte/16-hex span ID contract. The codebase types span_id as plain string throughout (otlp.ts:27) and toOpenInferenceSpan passes it through unchanged, so strict OTLP collectors (e.g. OTel collector with ID validation) would reject these spans. Consistent with the rest of the codebase's loose IDs, so not a regression — just flagging the interop risk if these artifacts ever land on a validating consumer.

🟡 LOW stampSessionIntegrity mutates receipt.harness/sessionId as a hidden side effect — src/integrity.ts

Lines 95-98 overwrite receipt.harness and receipt.sessionId on every corruption in place. The harness assignment is always a no-op (recordSessionCorruption already set it to ref.harness at line 42); the sessionId assignment replaces ref.sessionId with parent.trace_id. When the two differ this silently masks the inconsistency instead of surfacing it. The function name ('stamp') suggests span emission, not receipt mutation. Either drop the harness line (dead) and document the sessionId canonicalization, or move the rewrite into recordSessionCorruption where the receipt

🟡 LOW JSON.parse result cast to T without runtime shape validation — src/jsonl.ts

return JSON.parse(jsonBytes.toString('utf8')) as T performs an unchecked type assertion. Valid JSON that does not conform to T's shape (e.g. missing required fields, wrong types) passes through silently and fails downstream with a confusing error far from the parse site. This is standard TS practice for generic JSON readers, and callers (codex.ts, claude.ts, etc.) do their own field-level access with optional chaining, so the blast radius is limited. If stricter contracts are desired, a zod/schema validator hook could be added to JsonlReadOptions. No correctness bug observed in current callers.

🟡 LOW byteOffset doc comment wording is misleading — src/jsonl.ts

Comment says 'Zero-based byte offset of the record, excluding prior line delimiters.' The actual value is the absolute byte position in the file (including all prior LF bytes). Tests confirm: for line 3 after '{"id":1}\n\n', byteOffset = Buffer.byteLength('{"id":1}\n\n') = 10, which includes the two prior LFs. 'Excluding prior line delimiters' reads as if LFs are subtracted from the offset — they are not. Suggested rewrite: 'Absolute zero-based byte offset of the first byte of this record in the source file.'

🟡 LOW takeJsonl skips recover-mode options validation when limit === 0 — src/jsonl.ts

if (limit === 0) return rows exits before readJsonl's onCorruption callback check runs. A JS caller passing takeJsonl(path, 0, { mode: 'recover' } as never) gets no error instead of the TypeError that readJsonl would throw. Harmless since no file is read at limit 0, but the validation contract is inconsistent: non-zero limits validate options, zero limits do not. Moving the limit===0 early return below the for-await loop, or validating options before the early return, would make the contract uniform.

🟡 LOW Corruption receipt table biases toward earliest sessions when total exceeds limit — src/report.ts

The display loop fills the corruptions array FIFO across sources in iteration order. When totalCorruptionCount > CORRUPTION_RECEIPT_DISPLAY_LIMIT (100), only the earliest sessions' receipts appear in the detailed table; later sessions appear only in the bullet summary with a count. A reader scanning the table could miss that later sessions also had receipts. Mitigated by the bullet list above the table and the accurate omitted count. Not blocking. If fairness matters, round-robin across sources or show the highest-count session first.

🟡 LOW Unreachable fallback branches in renderPipelines — src/report.ts

The toolStats.length > 0 ? ... : Math.round(m.duplicateRate * m.totalCalls) ternary (and the parallel one for errorCalls) always takes the truthy branch: the loop above does if (m.totalCalls === 0) continue, and agent-eval's computeToolUseMetrics populates byTool whenever there are tool spans (totalCalls>0). The fallback is dead code. Not a bug — the byTool-sum and rate*total paths yield the same value — but it implies an inconsistency that does not exist. Fix: drop the ternary and use Object.values(m.byTool).reduce(...) directly, or add a comment that the fallback defends against a future agent-eval version that returns sparse byTool.

🟡 LOW tableCell does not escape backticks inside inline code spans — src/report.ts

tableCell escapes pipes and collapses whitespace but not backticks. sessionId, path, parentSessionId, corruptionDigest, sha256, and sourcePath are all rendered inside ... inline code. If any contained a backtick it would break the markdown. In practice these are UUIDs/hex hashes/POSIX paths without backticks, so this is theoretical. Low impact; flag only because it's the one escaping gap in an otherwise careful markdown-table renderer.

🟡 LOW locateSessions cwd-filter bypass for degraded sessions is codex-adapter-specific — src/session-source.ts

The condition !(ref.cwd === null && ref.integrity) retains sessions whose cwd was lost to corruption. Tracing all adapters, only CodexAdapter.locate (codex.ts:235) populates ref.integrity during locate (via takeJsonl → sessionJsonlOptions → recordSessionCorruption). Other adapters (claude, gemini, qwen, etc.) call sessionJsonlOptions only in parse, so ref.integrity is always undefined at locate time and the bypass never fires for them. This means corrupt claude/gemini sessions that lose their cwd-bearing first line are still filtered out — the data-loss fix only covers codex. Not a regression (behavior is identical to base for non-codex adapters), but the JSDoc for locateSessions doesn't scope the behavior to codex, which could mislead future readers.

🟡 LOW parseSession return contract changed from always-resolves to may-throw EmptySessionError — src/session-source.ts

parseSession now throws EmptySessionError when adapter.parse returns zero spans. Five direct call sites in cli.ts (lines 296, 314, 343, 348, 644) call parseSession without a try/catch and previously received an empty array for empty sessions. cmdConvert (cli.ts:326) even checks if (spans.length === 0) throw new Error(...) — now unreachable for the empty case because parseSession throws first with a different error class and message. Not a bug in session-source.ts itself (the fail-loud design is correct for data-loss prevention), but the cli.ts callers have a now-dead branch and unguarded throws. Verify these paths handle EmptySessionError appropriately.

🟡 LOW PartialUploadError uses strict !== instead of < for accepted count — src/upload.ts

The guard if (res.accepted !== events.length) throw new PartialUploadError(...) treats accepted > events.length as a failure even though that is not a data-loss condition (the backend took everything we sent, possibly counting prior idempotent submissions). The actual failure mode is accepted < events.length. Use res.accepted < events.length to avoid a spurious throw if a backend ever returns a surplus count (e.g., when it folds in deduped spans from an earlier idempotent retry). Low impact because the hosted client today returns exactly events.length on success.

🟡 LOW UploadItem.hash computed but no longer used for dedup — dead-ish load-bearing-looking field — src/upload.ts

planUpload still calls sessionHash(spans) and stores it on UploadItem.hash, but executeUpload/prepareOutbound never read item.hash for dedup — they compute outboundHash(events, identity) over the final transformed events. The field is now 'for inspection' per JSDoc (line 50), but its name and prominent placement invite callers to assume it is the dedup key. Costs a full contentHash pass over every session's spans on every planUpload, even though the result is unused. Either remove it (along with isNew) or rename to sourceHash to make the detachment from dedup unambiguous.

🟡 LOW UploadItem.isNew always true — public API field silently changed semantics — src/upload.ts

planUpload now hard-codes isNew: true for every item (was !alreadyUploaded(...)). The UploadItem interface is exported via src/index.ts:76, so external consumers that filter plan.items by isNew (expecting only-actually-new sessions) will now include every candidate and silently do redundant work, or worse, surface a misleading 'N new sessions' count to users. Internal cmdUpload (cli.ts:760) does NOT use isNew — it relies on executeUpload's skippedSessions — so the CLI is unaffected. JSDoc on line 52 says 'Candidate marker retained for API compatibility; final freshness depends on execute options,' which documents the change but does not prevent misuse. Either drop the field (breaking

🟡 LOW redactionCount undercounts when stripContent or external redactor applied — src/upload.ts

redactionCount = items.reduce((n, { item }) => n + item.redaction.redactionCount, 0) only sums the regex-pass counts captured in planUpload. When stripContent drops content/tool-values, or when opts.redactor mutates fields, additional redactions happen that are not reflected in the reported number. The CLI surfaces this number to the user (cli.ts:800 'N redaction(s)'). Not a regression (old code had the same undercount), but the expanded scope of stripSpanContent (now also drops input.value/output.value) makes the undercount larger. Consider also counting stripped fields, or relabel the output as 'regex redactions'.

🟡 LOW Golden SHA-256 digests create tight coupling to adapter output shape — tests/adapters.test.ts

traceShapeDigest produces a structural hash by filtering input/output.* and traces.(input|output).* attributes, then SHA-256-ing the canonicalized span array. Five tests hardcode expected digests (copilot, qwen, factory, claude, pi). Any benign attribute addition, rename, timestamp format change, or span reorder breaks these tests even when behavior is unchanged. The digest filter also strips content from TOOL spans but not from LLM spans, which is an asymmetric design choice. This is a deliberate golden-master strategy but creates maintenance friction — consider snapshot testing with diff output instead of bare hash comparison for easier debugging.

🟡 LOW Variable shadowing: local const tool shadows module-level tool() helper — tests/adapters.test.ts

Line 1167 declares const tool = spans.find(...) which shadows the module-level const tool = (s: OtlpSpan[]) => s.find(...) helper at line 36. Functionally correct (finds the span inline), but inconsistent with every other test in the file that calls tool(spans) as a function. A future maintainer adding a test inside this it.each that tries tool(spans) would get a TypeError. Fix: rename to toolSpan or call tool(spans).

---

_{tangletools · 2026-07-13T18:58:47Z · trace}

[tangletools]

✅ Approved — 26 non-blocking findings — bbc9fdda

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

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-13T18:58:47Z · immutable trace}

[tangletools]

✅ Auto-approved drewstone PR — 1da788dd

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

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-14T04:26:21Z}

[tangletools]

⚠️ Review Interrupted — 1da788dd

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 · #35 · model: kimi-for-coding · updated 2026-07-14T04:31:40Z}