fix(codex): capture custom tools and subagents by drewstone · Pull Request #32 · tangle-network/traces

drewstone · 2026-07-11T09:16:49Z

What changed

parse current Codex custom_tool_call and matching outputs without dropping malformed sessions
expose a single nested tool name while preserving the outer call metadata
capture delegated-agent starts and terminal outcomes
classify real check commands conservatively for behavioral analysis
exclude only explicitly marked process-poll calls from repeated-call findings while retaining usage totals

Why

Current Codex sessions route most tools through a custom exec item and emit delegation through sub_agent_activity. The adapter ignored both, so real sessions undercounted tool use and reported zero delegated agents.

Proof

pnpm test: 113/113 passed
pnpm typecheck: passed
pnpm build: passed
replay of session 019f5044-e222-71b3-9fc0-cbedd725179b: 116 tool calls, 33 delegated-agent starts, zero false repeated-wait findings
malformed custom input remains parseable
mutating curl forms remain ordinary execution, not checks
an unmarked domain wait repeated three times still triggers the repeated-call finding
git merge-tree --write-tree origin/main HEAD: clean

tangletools

✅ Auto-approved drewstone PR — `fe8585d8`

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-11T09:16:56Z}

tangletools

✅ Auto-approved drewstone PR — `46289473`

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-11T09:22:33Z}

tangletools

✅ Auto-approved drewstone PR — `9c558899`

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-11T09:24:29Z}

tangletools

🟡 Value Audit — sound-with-nits


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

💰 Value — sound-with-nits

Teaches the Codex adapter to see current-format sessions (custom exec tool calls, delegated-agent events) and stops false stuck-loop findings from legitimate process polls — a coherent, well-scoped telemetry fix that plugs into existing consumers; ship.

What it does: Extends CodexAdapter.parse (src/adapters/codex.ts) to: (1) parse custom_tool_call/custom_tool_call_output rollout items — current Codex wraps tool use in an exec JS sandbox calling tools.exec_command(...) — unwrapping calls that invoke exactly one nested tool so the span's tool.name is the real tool (exec_command) while the outer call type/name are preserved as traces.codex.* attri
Goals it achieves: Make traces' Codex telemetry accurate again for current Codex builds: real tool counts instead of undercounting (previously every exec-wrapped call was invisible), delegated-agent visibility where sessions previously reported zero subagents, and elimination of false stuck-loop findings from legitimate process polling without blinding the detector to genuine repeated-call loops. The `exec_command
Assessment: Good on its merits, and in the grain of the codebase. The emitted tool.Agent span with subagent_type in its content JSON plugs directly into the existing adoption pipeline (src/adoption.ts:75,151 already counts Task/Agent spans and parses subagent_type from span content) — no consumer changes needed. The parser structure, attribute conventions, and conservative regex classification mirro
Better / existing approach: None material. I searched for existing equivalents: (a) stuckLoopView in @tangle-network/agent-eval (read its dist source) accepts only minOccurrences/runId — there is no exclusion hook to reuse, so filtering spans into a second in-memory store is the minimal available mechanism and toRuntimeStore is cheap; (b) subagent capture can't reuse claude.ts's sidechain folding because Codex rollou
Model: opencode/kimi-for-coding/k2p7
Bridge attempts: 1

🎯 Usefulness — sound

Fills two real blind spots in the Codex adapter (custom_tool_call was fully ignored; delegated agents were invisible) and wires an expected-blocking exclusion into the loop detector, all following established adapter and pipeline patterns.

Integration: Fully reachable. CodexAdapter is registered at registry.ts:28, exported at index.ts:22, and selected by every CLI/SDK path. runPipelines is consumed at 4 sites (live.ts:624, evidence.ts:146, cli.ts:414, improvement.ts:621). The new traces.expected_blocking attribute flows through the standard extra→attributes path in otlp.ts:78 and is read by the pipeline filter at pipelines.ts:40. The `exec
Fit with existing patterns: Follows the established grain. Subagent handling mirrors the Claude adapter's pattern: both emit tool.Agent spans parented under the last LLM turn. The difference (Codex uses inline lifecycle events keyed by thread_id; Claude parses separate subagents/*.jsonl files) is dictated by the source formats, not a design divergence. The pipeline filtering sits at the input boundary (filter spans befor
Real-world viability: Holds up under edge cases. singleNestedToolName returns null on 0 or 2+ unique tools (multi-tool exec programs fall back to the outer name — conservative, correct). classifyNestedTool's regex targets specific known verifiers plus read-only curl; POST/PUT/PATCH/-d curls are excluded, so mutation calls are NOT misclassified. isExpectedBlockingTool only marks wait with cell_id and write_stdin w
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 1

💰 Value Audit

🟡 Two 'is this a check command' regexes now exist in different layers [maintenance] ``

codex.ts:96 verificationCommand (pnpm/npm test|typecheck|lint|build|check, vitest/jest/pytest/tsc, go test, cargo, git status/diff/show/merge-tree, gh-drew pr view/checks) semantically overlaps live.ts:186 VERIFY_RE (test|vitest|jest|pytest|go test|cargo test|tsc|typecheck|lint|biome|eslint|build|check:invariants|preflight). They serve genuinely different consumers — parse-time tool naming for external analysis vs. stream-time action classification — so merging them would couple the adapter

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 · 20260711T093725Z}

tangletools · 2026-07-11T09:53:00Z

✅ No Blockers — `9c558899`

Review health 100/100 · Reviewer score 63/100 · Confidence 80/100 · 14 findings (3 medium, 11 low)

	deepseek	kimi-code	aggregate
Readiness	63	64	63
Confidence	80	80	80
Correctness	63	64	63
Security	63	64	63
Testing	63	64	63
Architecture	63	64	63

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

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

🟠 MEDIUM Non-string custom_tool_call input crashes the whole session parse — src/adapters/codex.ts

Line 90 guards only if (!input); line 91 then calls input.matchAll(...). Verified empirically: a rollout line {type:'custom_tool_call', input: {cmd:'ls'}} (valid JSON, wrong field type) throws TypeError: input.matchAll is not a function out of CodexAdapter.parse. Callers (session-source.ts scanSessions, lines 87-92) catch parse errors and skip the session via onError, so one malformed-shape line silently drops the ENTIRE session's spans

🟠 MEDIUM Subagent completed lifecycle event not handled — src/adapters/codex.ts

The sub_agent_activity handler (lines 300-332) only processes started and interrupted kinds. If Codex emits completed events for successfully-finished subagents, those spans never receive an end_time or status: OK. The span() builder defaults end_time to start_time (otlp.ts:89), so every successful subagent appears as a zero-duration span with status OK — silently corrupting duration and status data for downstream analysts. Add a completed branch that sets toolSpan.end_time = eventTime and toolSpan.status = { code: 'OK' }, plus a test case.

🟠 MEDIUM Double toRuntimeStore call — builds two stores — src/pipelines.ts

toRuntimeStore(spans) on line 39 builds a store from ALL spans, then toRuntimeStore(loopEligible) on line 41 builds a second store from the filtered subset. This doubles span-iteration cost (2× O(n) with N InMemoryTraceStore insertions). The original store is only used for computeToolUseMetrics, the filtered store only for stuckLoopView. Could be unified: build one store, then compute tool-use from it and pass only non-blocking trace_ids to stuckLoopView — or filter spans before the single toRuntimeStore call and handle tool-use totals separately. Not a correctness bug, b

🟡 LOW Mutating curl forms mislabeled exec_command.verify — src/adapters/codex.ts

The read-only check only rejects -X/--request POST|PUT|PATCH|DELETE and -d/--data*. Verified empirically: curl -F file=@x ... (multipart POST), curl --json '{...}' ... (implies POST), curl -T ./f ... (PUT upload), and curl -X PURGE ... are all classified exec_command.verify. -XPOST (no space) is correctly NOT labeled verify. Impact is analytic-only (verification-rate inflation in the live view / tool histograms); classification is not a security boundary. Fix: extend the mutating-signal regex to cover -F/--form, --json, -T/--upload-file, or switch to an allowlist of read-only flags.

🟡 LOW No test coverage for write_stdin blocking detection path — src/adapters/codex.ts

isExpectedBlockingTool checks write_stdin with session_id in input, but the adapter test suite never exercises this path. The wait+cell_id path is tested, and the pipelines test exercises the attribute filtering indirectly, but the codex adapter itself has no test proving write_stdin+session_id produces traces.expected_blocking: true. Risk: a regex regression would go undetected.

🟡 LOW Subagent spans never closed except on 'interrupted' — src/adapters/codex.ts

Only kind === 'interrupted' sets end_time/status on the subagent span (lines 327-333); 'interacted' is ignored and no terminal-success kind is handled. Verified empirically: a started→completed sequence yields end_time == start_time (zero duration) with status OK, since span() defaults endTime to startTime. If Codex emits a completion-style kind for successful subagents, duration/error analytics for delegated agents are systematically wrong (always 0ms, never ERROR-free-closed). The PR's own test only exercises interrupted. Fix: handle the terminal-success kind (or close open subagent spans at end-of-file with the session's last timestamp).

🟡 LOW Unknown subagent kinds silently dropped with no explicit handling — src/adapters/codex.ts

The sub_agent_activity handler has no else clause for unknown kinds. The test fixture includes kind: 'interacted' which is silently consumed. This is functionally correct for progress events, but future Codex lifecycle events (e.g., failed, timed_out) would also be silently dropped with no logging or error, making adoption of new event types invisible to operators. Consider adding an explicit no-op branch or a comment enumerating known-but-unhandled kinds.

🟡 LOW hasReadOnlyCurl misses -d@file mutation pattern — src/adapters/codex.ts

The -d regex requires (?:=|\s) after the flag, but curl -d@secrets.json uses @ (read from file). A curl that reads a local file and sends it via POST with -d@path would be misclassified as read-only. Low-severity edge case in a heuristic classification system for a trace adapter, not a runtime defense.

🟡 LOW Redundant second toRuntimeStore build on every call — src/pipelines.ts

The change now builds the runtime store twice per invocation: once for all spans (line 39) and again for loop-eligible spans (line 41). toRuntimeStore is O(n) with per-span async appendRun/appendSpan, and live.ts:624 calls runPipelines on every report emission in watch mode over the full accumulated span list, so long sessions pay 2x projection cost each tick. Concrete fix: skip the second build when nothing is marked — `const loopEligible = spans.some((s) => s.attributes['traces.expected_blocking'] === true) ? spans.filter((s) => s.attributes['traces.expected_blocking']

🟡 LOW affectedRunRatio denominator silently drops all-blocking runs — src/pipelines.ts

stuckLoopView computes affectedRunRatio = affectedRuns / (await loopStore.listRuns()).length, and toRuntimeStore only appends a run for trace_ids present in the filtered span list. A trace whose every span carries 'traces.expected_blocking' === true (e.g. a degenerate codex rollout containing only wait/write_stdin function calls, no root/LLM spans) disappears from loopStore entirely, so totalRuns shrinks and the exported metric traces.signals.affected_run_ratio (file-export.ts:227) is inflated relative to pre-change behavior for the same data. Real sessions include a non-tool root span so this is an edge case, but the semantics shift is undocumented. Fix: either document that totalRuns now means 'runs with at least one loop-eligible span', or pass the full store's run count into the ratio

🟡 LOW 'interacted' sub_agent_activity fixture event has no direct assertion — tests/adapters.test.ts

The fixture includes a sub_agent_activity event with kind 'interacted' (occurred_at 09:00:06.000Z), but no assertion pins its handling. The final assertion end_time === '2026-07-11T09:00:07.000Z' still passes if an implementation wrongly closed the Agent span on 'interacted' as well, because the later 'interrupted' event overwrites end_time. The tools.length === 7 count does guard against 'interacted' creating an extra span, so the gap is narrow: the 'must not close on interacted' semantic is untestable as written. Fix: add an explicit assertion (e.g., capture span state between events, or assert end_time !== the interacted event time via a fixture where interacted occurs after interrupted would be impossible — instead assert that only 'interrupted' sets status by giving the interacted eve

🟡 LOW Negative-case test omits occurrences assertion — tests/pipelines.test.ts

The existing first test in this file asserts both findings[0].toolName AND findings[0].occurrences===3 (lines 39-40), but the new negative test at lines 81-82 asserts only toolName. A regression that miscounts occurrences (e.g. off-by-one in minOccurrences handling) would slip through. Fix: add expect(r.stuckLoops.findings[0]!.occurrences).toBe(3) for parity.

🟡 LOW No test for non-boolean truthy expected_blocking — tests/pipelines.test.ts

The filter uses strict !== true, so a non-boolean truthy value (e.g., string 'true', number 1) would bypass the exclusion and flag blocking calls as stuck loops. In practice this is unlikely because the Codex adapter only ever sets boolean true, but a defense-in-depth test would catch a regression where the attribute serialization produces a string.

🟡 LOW Service values may mislead readers about what drives exclusion — tests/pipelines.test.ts

The positive test sets root service 'codex' while the negative test uses 'other' (lines 60, 75), but the exclusion in runPipelines is purely per-span attribute-based (item.attributes['traces.expected_blocking'] !== true) — service.name is never read. A reader could infer the harness identity drives the skip. The tests are correct as written (mutation-verified); a one-line comment noting the attribute is the signal would remove the ambiguity.

_{tangletools · 2026-07-11T09:52:56Z · trace}

tangletools

✅ Approved — 14 non-blocking findings — `9c558899`

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

Full immutable report for this review: trace

Summary comment for this run: full summary

_{tangletools · 2026-07-11T09:52:56Z · immutable trace}

tangletools

✅ Auto-approved drewstone PR — `7dd9dce6`

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-11T15:38:46Z}

tangletools

🟡 Value Audit — sound-with-nits


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

💰 Value — sound-with-nits

Extends the Codex adapter to capture custom exec tool calls and subagent lifecycles that current Codex builds route everything through — fills a real visibility gap, built in the grain of the codebase.

What it does: The Codex adapter now parses custom_tool_call/custom_tool_call_output (the wrapper Codex's custom exec item emits) in addition to the legacy function_call items, extracting the real nested tool name from the JS body via a tools.XXX( regex and renaming verify-shaped calls to exec_command.verify (src/adapters/codex.ts:263-295). It captures sub_agent_activity events as tool.Agent span
Goals it achieves: (1) Stop undercounting tool use in real Codex sessions — most tools now flow through custom exec, so the old function_call-only path reported a fraction of real activity. (2) Surface delegated agents — sub_agent_activity was ignored entirely, so adoption metrics (adoption.ts:151 keys off Agent tool spans) reported zero subagents for Codex. (3) Kill false stuck-loop findings on expected blockin
Assessment: Coherent and in-grain. The adapter's job is to project native logs onto OtlpSpan[] (otlp.ts:2-4); extending function_call handling to custom_tool_call is the natural extension, and the extra escape hatch (otlp.ts:57-58) is exactly the documented channel for harness-specific attributes like traces.expected_blocking / traces.codex.*. The subagent parenting mirrors the Claude adapter's approach
Better / existing approach: Looked for reuse across three axes — none material. (a) Subagent handling: claude.ts:318 reads separate subagents/*.jsonl sidechain files, a fundamentally different mechanism from Codex's inline sub_agent_activity events; no reuse, correctly reimplemented inline. (b) Adoption integration: adoption.ts:75 already extracts subagent_type from span content, and this change feeds it cleanly. (c) V
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

Closes a real gap — the Codex adapter now parses the current rollout format (custom_tool_call + sub_agent_activity) and threads an expected-blocking signal into the stuck-loop pipeline, all along the established adapter grain.

Integration: Fully reachable. CodexAdapter is registered in src/registry.ts:28 alongside every other adapter; parse() is invoked by the CLI, live watcher, evidence, and improvement pipelines. The new traces.expected_blocking attribute is read at src/pipelines.ts:40 inside runPipelines, which all four callers invoke. No new entry point was added — the capability rides the existing parse()-shaped contract.
Fit with existing patterns: Matches the codebase grain exactly. Claude already folds subagents (src/adapters/claude.ts:318-354); Codex now does the equivalent for its event_msg-based delegation. The extra escape hatch used for traces.codex.* and traces.expected_blocking is the documented per-harness attribute mechanism (src/otlp.ts:57-58). Nested-tool extraction via regex on the JS exec wrapper is inherent to how Codex ser
Real-world viability: Conservatively scoped. singleNestedToolName returns null on zero or multiple inner tools (falls back to outer name); toolInputToString JSON-stringifies non-string input so malformed custom input stays parseable; hasReadOnlyCurl rejects any non-GET/HEAD method and any -d/-F/-T/--data/--form/--json/--upload-file flag; subagent handling dedupes duplicate 'started' events, ignores the non-terminal 'in
Model: opencode/zai-coding-plan/glm-5.2
Bridge attempts: 1

💰 Value Audit

🟡 Verification-command regex duplicated across adapter and analyst layers [maintenance] ``

src/adapters/codex.ts:102 (verificationCommand: pnpm/npm/yarn/bun test|typecheck|lint|build|check, vitest, jest, pytest, tsc, biome, eslint, go test, cargo, git status/diff/show/merge-tree) and src/live.ts:186 (VERIFY_RE: test, vitest, jest, pytest, go test, cargo test, tsc, typecheck, lint, biome, eslint, build, check:invariants, preflight) both enumerate the same concept — 'is this a verification command' — independently. They will drift (a new runner added to one is invisible to the other

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 · 20260711T154300Z}

tangletools · 2026-07-11T16:01:47Z

✅ No Blockers — `7dd9dce6`

Review health 100/100 · Reviewer score 67/100 · Confidence 80/100 · 15 findings (1 medium, 14 low)

	glm	deepseek	aggregate
Readiness	71	67	67
Confidence	80	80	80
Correctness	71	67	67
Security	71	67	67
Testing	71	67	67
Architecture	71	67	67

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

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

🟠 MEDIUM Double toRuntimeStore call duplicates span processing — src/pipelines.ts

Line 39 calls toRuntimeStore(spans) on all spans to build store for computeToolUseMetrics. Line 41 calls toRuntimeStore(loopEligible) a second time to build loopStore for stuckLoopView. Every span that passes the filter is inserted into two independent InMemoryTraceStore instances — appendRun + appendSpan executed twice per span. For large traces this is non-trivial waste. Options: (a) expose a span-filter on stuckLoopView/InMemoryTraceStore, (b) clone the first store and remove blocked spans, (c) accept the overhead and note it in a comment since both pipelines are '

🟡 LOW Nested-tool / verification classification can false-positive on text inside string literals and comments — src/adapters/codex.ts

singleNestedToolName (codex.ts:89) greps \btools\.([A-Za-z][A-Za-z0-9_]*)\s*\( over the raw JS program text with no comment/string stripping. classifyNestedTool (codex.ts:114) then runs verificationCommand (codex.ts:102) over the SAME raw text. So an exec program like tools.exec_command({ cmd: 'rm -rf /' }) // next: run pnpm test matches both the single-tool check AND the verification regex, tagging a destructive command as exec_command.verify. Symmetric false-negative: a real verify whose program mentions tools.apply_patch( in a comment is NOT classified as verify (singleNestedToolName returns null because two distinct names appear). Impact is bounded — attributes.content always preserves the verbatim program so analysts can recover — but the tool.name histogram the analysts key o

🟡 LOW Subagent wrapper span sets agent.name='codex', inconsistent with Claude adapter's 'subagent:' convention — src/adapters/codex.ts

The sub_agent_activity branch (codex.ts:315-335) builds the wrapper span with agent: SERVICE (= 'codex'), so attributes['agent.name']='codex' — identical to the parent session. Claude's sidechain parser at src/adapters/claude.ts:346 sets agent: meta.agentType ? \subagent:${meta.agentType}` : 'subagent', giving each subagent type a distinct agent.name bucket. The subagent_type IS preserved here in attributes.content (JSON) and in attributes['traces.codex.subagent_path'], so no data is lost — but downstream analysts grouping by agent.name will not see 'paper_audit' vs 'runtime_audit' as distinct agents without parsing content. Either set agent to subagent:${subagentType}` for parity with Claude, or add a one-line docstring note that subagent identity lives in content/extra for the Cod

🟡 LOW curl -G -d misclassified as non-read-only — src/adapters/codex.ts

hasReadOnlyCurl regex flags any -d/-F/-T presence as non-read-only. Curl's -G (--get) flag combined with -d sends data as query parameters — it's read-only but gets rejected. Conservative (errs toward classifying as mutating), so no functional bug, just a minor precision gap.

🟡 LOW hasReadOnlyCurl -X method regex missing /i flag (inconsistent with --request branch) — src/adapters/codex.ts

Line 108 input.match(/(?:^|\s)-X(?:=|\s*)([A-Za-z]+)\b/) has no /i flag while line 109 input.match(/(?:^|\s)--request(?:=|\s+)([A-Za-z]+)\b/i) does. curl's -X short flag is uppercase-only by spec so this is not a live bug today, but the asymmetry is a latent footgun if anyone assumes case-insensitive matching (e.g. a future Codex build that lowercases flags). Trivial: add /i to the -X regex for symmetry. No behavioral impact on the 7 curl test cases, all of which I verified pass.

🟡 LOW hasReadOnlyCurl alternation short-circuits on empty \S* match — src/adapters/codex.ts

The regex (?:^|\s)-(?:d|F|T)(?:\S*|\s+\S+) has \S* before \s+\S+ in the alternation. Since \S* always succeeds (including matching zero chars), the \s+\S+ branch is never reached. This means -d file.txt only captures -d and never sees file.txt. Functionally correct since -d alone is enough to flag the call as non-read-only, but the comment/design intent may have been to capture the argument too.

🟡 LOW singleNestedToolName regex matches tool patterns inside string literals — src/adapters/codex.ts

The regex tools.([A-Za-z][A-Za-z0-9_]*) ( scans the entire exec input string, including string literals within commands. A command like tools.exec_command({ cmd: 'echo tools.foo()' }) would find two matches (exec_command and foo), returning null — correctly failing to classify. But if the only match is bogus (e.g., a bare tools.nonexistent( in a quoted string), it incorrectly returns that name. Extremely unlikely in practice since exec inputs are generated Codex programs, not adversarial input.

🟡 LOW Attribute type fragility: only boolean true is filtered — src/pipelines.ts

The filter item.attributes['traces.expected_blocking'] !== true uses strict equality. If any adapter emits the string 'true' (a common serde artifact) instead of boolean true, the span would NOT be excluded from stuck-loop findings. The Codex adapter correctly sets boolean true via { 'traces.expected_blocking': true }, so this is not currently triggered, but future adapters could hit it. Consider !== true && !== 'true' or normalize to boolean at the adapter layer.

🟡 LOW Double toRuntimeStore conversion builds the trace store twice — src/pipelines.ts

toRuntimeStore(spans) runs first (full set, for runIds + computeToolUseMetrics), then toRuntimeStore(loopEligible) runs again over the filtered subset (for stuckLoopView). Both iterate every span and call appendRun/appendSpan on an InMemoryTraceStore, so the conversion is O(n) twice. Acceptable given the 'cheap, deterministic' framing in the module doc, but a single build that tags blocking spans (or pushing the exclusion into stuckLoopView via an attribute predicate) would halve the conversion cost and avoid two live store copies. No functional impact; pure efficiency nit.

🟡 LOW LLM token span from token_count event is not asserted — tests/adapters.test.ts

The fixture includes a token_count event with input_tokens:20, output_tokens:4 (line 209), but no assertion checks that an LLM span with llm.input_tokens=20 and llm.output_tokens=4 was emitted. The existing codex test at line ~160 may cover this, but within this new describe block the LLM span path is exercised by the fixture yet never verified. Minor: the data is there, just add expect(llm(spans)?.attributes['llm.input_tokens']).toBe(20).

🟡 LOW Subagent failed and timed_out kinds are untested — tests/adapters.test.ts

The adapter (codex.ts:346) handles three terminal error kinds: ['interrupted', 'failed', 'timed_out']. The test only exercises interrupted (thread-1). If the failed or timed_out branch were broken or removed, no test would catch it. Consider adding at least one subagent with kind:'failed' or kind:'timed_out' to verify the ERROR status and message format (subagent failed, subagent timed_out).

🟡 LOW Awkward test name grammar — tests/pipelines.test.ts

Test name 'does not call repeated blocking waits a stuck loop' is missing a verb — reads as 'does not call [something] a stuck loop'. Should be 'does not flag repeated blocking waits as a stuck loop'.

🟡 LOW Cosmetic service mismatch between session and tool spans in blocking test — tests/pipelines.test.ts

The session span uses service:'codex' but the toolCall helper hardcodes service:'claude-code' (line 21), so the 6 tool spans in this test claim claude-code while the run claims codex. The pipeline does not key loop detection off service, so the assertion holds, but for a test specifically exercising codex's expected_blocking behavior the fixture is misleading. Pass an override or parametrize the helper's service field.

🟡 LOW No boundary test for explicit traces.expected_blocking: false — tests/pipelines.test.ts

The filter in pipelines.ts:40 uses !== true, so a span with traces.expected_blocking: false (explicit false) would still be eligible for stuck-loop detection. This is correct behavior but untested — a regression could accidentally change !== true to ?? false and break this. Consider adding a boundary test.

🟡 LOW Test title is ungrammatical / missing verb — tests/pipelines.test.ts

Title reads 'does not call repeated blocking waits a stuck loop' — likely meant 'does not flag repeated blocking waits as a stuck loop'. Test description only; no functional impact, but it muddies test output for anyone reading failures.

_{tangletools · 2026-07-11T16:01:44Z · trace}

tangletools

✅ Approved — 15 non-blocking findings — `7dd9dce6`

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

Full immutable report for this review: trace

Summary comment for this run: full summary

_{tangletools · 2026-07-11T16:01:44Z · immutable trace}

fix(codex): capture custom tools and subagents

fe8585d