Skip to content

feat(supervise): agent graphs — runGraph, observable edge ledger, prompt registry - #704

Merged
drewstone merged 8 commits into
mainfrom
feat/agent-graph-694
Aug 2, 2026
Merged

feat(supervise): agent graphs — runGraph, observable edge ledger, prompt registry#704
drewstone merged 8 commits into
mainfrom
feat/agent-graph-694

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Closes the P0 of #694: loops between agents become plain data, and every edge tells the truth.

What

  • runGraph / AgentGraph (src/runtime/supervise/graph.ts) — nodes are AgentProfiles, edges are typed delegates/analyzes rows, a loop is a cyclic graph. Composes supervise() directly; no second scheduler. A working 2-node driver↔worker topology is ≤20 LOC of data (see tests/kernel/graph.test.ts).
  • Edge ledger — every traversal is recorded (delivered | stripped | empty | unpropagated, byte counts) in memory on the result AND as edge events in the run journal. Motivating incident: a filter silently replaced 1,700 chars of steering with 241 chars of boilerplate for three rounds and no artifact said so.
  • Prompt registry (src/runtime/supervise/prompt-registry.ts) — versioned directives as data (<surface>/v<n>), immutable versions, unknown handles and empty directives fail loud. Every edge is a DSPy/GEPA optimization target. The supervisor policy prompt now lives here too.
  • Ledger truthfulness, adversarially audited — three confirmed bugs fixed at the root and locked in by probes proven load-bearing (each fails under a micro-revert of its fix):
    • an analyst returning undefined (or findings with nested undefined) vanished the event entirely — no row, no journal line, no error; finding events are now producer-canonicalized to finite RFC 8785 JSON, and a non-serializable payload becomes a record of that fact;
    • an exhausted analyzes cap wrongly raised GraphEdgeCapError; only delegates caps close the spawn cycle — analyzes exhaustion is observable, never fatal;
    • a spawn refused after the worker factory ran was ledgered delivered; spawn rows are provisional until the agent.spawn hook binds a live worker, and unbound rows flush as unpropagated with 0 bytes.
  • W3C trace propagationTRACEPARENT is read first with dual-write of the legacy TRACE_ID/PARENT_SPAN_ID pair; OTLP export ids derive via deriveHexId from @tangle-network/agent-trace-contract 1.0.2 (replaces padTraceId), so runtime spans join the same distributed trace as VB, cli-bridge, and sandbox workers.
  • Consolidation — node pinning (a driver cannot smuggle capabilities into a worker it did not define), conserved budget + mandatory deliverable for termination, oracles/analysts refused as nodes (they are environment), dead fell-back vocabulary removed.

Verification

  • Full suite: 2310 passed | 6 skipped (199 files), post-merge with latest main.
  • tsc --noEmit (src + examples), biome check (545 files), tsdown build, docs:freshness — all clean; docs/api regenerated.
  • Version gate: check:version-bump green — manifest change (new dependency) paid for by 0.120.0 → 0.121.0, CHANGELOG entry included.
  • Ledger probes: each of the three truthfulness tests fails under a targeted revert of its fix and passes with it (verified both directions).

…dge ledger, prompt registry (#694)

Nodes are AgentProfiles, edges are typed data carrying registry-resolved
directives, a loop is a cyclic graph. runGraph composes supervise() — no
second scheduler.

- src/runtime/supervise/graph.ts: AgentGraph/runGraph; every delegates/
  analyzes traversal lands in an edge ledger (delivered | stripped |
  empty | unpropagated, byte counts) in memory AND as journal 'edge'
  events; node pinning (a driver cannot smuggle capabilities into a
  worker it did not define); conserved budget + mandatory deliverable;
  delegates caps fail loud via GraphEdgeCapError, analyzes caps are
  observability-only.
- src/runtime/supervise/prompt-registry.ts: versioned directives as data
  (<surface>/v<n>), immutable versions, no silent fallback — every edge
  is a DSPy/GEPA optimization target.
- Finding events are producer-canonicalized to finite RFC 8785 JSON
  (nested undefined stripped, non-serializable payloads recorded as
  such) so a digesting subscriber can never vanish an event.
- Spawn ledger rows are provisional until the agent.spawn hook binds a
  live worker; unbound rows flush as unpropagated with 0 bytes.
- W3C trace propagation: TRACEPARENT read first with dual-write legacy
  TRACE_ID/PARENT_SPAN_ID; otel-export ids via deriveHexId from
  @tangle-network/agent-trace-contract (replaces padTraceId).
- Supervisor policy prompt collapsed into the registry; tests: 2292
  passing incl. adversarial ledger-truthfulness probes.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — cf194363

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-08-02T01:08:14Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — cf194363

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-08-02T01:27:11Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

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

💰 Value — sound

Adds a data-driven agent-graph layer that composes over the existing supervise() core (not a second scheduler), with an observable edge ledger and versioned prompt registry — coherent, in-grain, no existing equivalent found.

  • What it does: Introduces runGraph/AgentGraph (src/runtime/supervise/graph.ts:376) where nodes are canonical AgentProfiles and edges are typed delegates/analyzes rows carrying versioned directives; a loop is a cyclic graph. It composes directly over supervise() (graph.ts:721 calls it; it wraps only makeWorkerAgent for node-pinning and onCoordinationEvent for observation). Every edge traversal is re
  • Goals it achieves: (1) Make agent loops (delegation + analysis between agents) plain data — authorable in ~20 LOC, swappable without code changes. (2) Make every edge observable with byte counts — the motivating incident was a filter silently replacing 1,700 chars of steering with 241 chars of boilerplate across three rounds with no artifact. (3) Make every directive a versioned optimization target (DSPy/GEPA), so e
  • Assessment: Strong, in-grain change. The central design decision — runGraph as an interpretation layer over supervise() rather than a parallel scheduler — is verified at graph.ts:721 and is correct: it reuses the supervisorAgent/driverAgent machinery, conserved-pool budget, deliverable-gated settlement, and makeWorkerAgent seam. The prompt-registry replaces genuinely hardcoded builder-function prose (conf
  • Better / existing approach: none — this is the right approach. Searched for existing graph/topology machinery in the supervise path (git grep on main at 5255bc9 for graph|topology|node.edge across src/**/.ts) — none; the word 'topology' in src/runtime/index.ts refers to the per-round driver topology, a different (run-loop) layer. Searched for an existing prompt registry — none; prompts were inline builder functions. Confir
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Agent-graph layer composes supervise() as plain data with an observable edge ledger and a prompt registry that already unifies the two supervisor front doors — fits the codebase grain, is wired into live paths today, and the only caller-less piece (runGraph itself) is the explicitly-intended future

  • Integration: Strongly reachable. runGraph is exported from the public surface (src/runtime/index.ts:638) and composes supervise() directly (graph.ts:720-744 calls supervise(rootProfile, graphTask, {...}) — it is an interpretation layer, NOT a second scheduler, exactly as documented). More importantly, the prompt registry is ALREADY LIVE on every supervised run, not waiting for runGraph: supervisorPolicyPrompt
  • Fit with existing patterns: Fits the grain cleanly. New SpawnEvent kinds 'edge' and 'trace-unpropagated' (types.ts:920-962) are handled identically to existing informational events in outsideCursorNamespace, replaySpawnTree, and materializeTreeView (spawn-journal.ts:484-556,720) — same skip-on-replay pattern as 'waiting'/'metered'/'materialized'/'execution-bound'. runGraph does not compete with supervise(); it wraps it, reus
  • Real-world viability: Holds up under adversarial conditions. The three ledger-truthfulness bugs are each locked by a probe proven to fail under a micro-revert: undefined findings (canonicalFindingEvent strips the key so the RFC 8785 digest cannot throw), nested-undefined payloads (JSON round-trip in the producer, with a non-canonical fallback for cycles/BigInt/bare functions), and the delegates-vs-analyzes cap distinct
  • 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 · 20260802T012920Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — cf194363

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 60 findings (14 medium, 46 low)

glm deepseek deepseek-flash aggregate
Readiness 14 13 0 0
Confidence 95 95 95 95
Correctness 14 13 0 0
Security 14 13 0 0
Testing 14 13 0 0
Architecture 14 13 0 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 35 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 35 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 35 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Zero test coverage for the new analyst-routing feature — src/mcp/tools/coordination.ts

Searched all *.test.ts for AnalyzeOnSettleRoute, normalizeAnalyzeOnSettle, deliverRoutedFinding, canonicalFindingEvent, nonCanonicalFindings, route.to, route.over, route.directive — no matches. The only analyzeOnSettle test (coordination.test.ts:1112-1177) exercises the bare-string form ('completeness'); nothing covers the route form, the over-filter, routing delivery to a named live worker, directive wrapping, the unknown-worker failed-steer record, or canonicalFindingEvent's three branches (undefined-strip, JSON round-trip, non-canonical fallback). canonicalFindingEvent now guards three publish sites (settle-hook line 982, online-detector [line 1370](https:/

🟠 MEDIUM deliverRoutedFinding silently drops authorization/refusal failures — contradicts its 'never a silent drop' docstring — src/mcp/tools/coordination.ts

The try/catch at lines 940-953 wraps authorizeInstruction + recordInstruction + attemptDelivery and swallows any throw. The catch comment (951-952) claims 'The failed delivery is already recorded on the bus (a steer with delivered:false or an authorization refusal)', and the docstring (908-910) promises 'observable, never a silent drop'. Both are FALSE for the authorize/record path: authorizeInstruction (line 941) throws at 1066-1069 when authorizeDownMessage is configured and workerIdentity is undefined, and the user's autho

🟠 MEDIUM deliverRoutedFinding silently drops delivery when authorizeInstruction throws — src/mcp/tools/coordination.ts

When authorizeInstruction('steer', targetId, text, false) throws (e.g. opts.authorizeDownMessage is set but the target worker lacks a durable identity), the catch at line 950 swallows the error. No bus event is published — no steer, no instruction, nothing. The comment on line 951 claims 'The failed delivery is already recorded on the bus' but this is only true for failures AFTER recordInstruction is called ([line 942](https://github.com/tangle-network/agent-runtime/blob/cf194363f17511e06bf7d841f95a53d09a84ff06/src/mcp/t

🟠 MEDIUM Pass-through guard drops the old dash-strip normalization: UUID-form ids are silently hashed, breaking cross-version join — src/otel-export.ts

Old padSpanId/padTraceId explicitly did id.replace(/-/g,''), so a dashed/UUID id produced a valid, joinable 16/32-hex wire id. New code only passes through ids that isW3CSpanId/isW3CTraceId accept (strict /^[0-9a-f]{16|32}$/, no dashes). A UUID trace id like a3ce929d-0e0e-4736-a3ce-929d0e0e4736 now hashes via deriveHexId instead of passing through dash-stripped — verified: old output a3ce929d0e0e4736a3ce929d0e0e4736 is a valid W3C id, new output is a35d451a0b5d4194a4d02570f05a7a12. Callers feed caller-supplied ids (e.g. meta.traceId in src/intelligence/index.ts:614,724), which are commonly UUIDs. Impact: spans no longer join a parent that used the dash-stripped UUID form — a silent break at the upgrade boundary (old emitter ↔ new emitter) and against external systems

🟠 MEDIUM define-leaderboard: continuation text changed from '' to 'retry' — src/runtime/define-leaderboard.ts

The migration from naiveDriver to steeringDriver changes the continuation string from empty ('') to 'retry'. Under the identity applyContinuation (task) => task this is functionally equivalent — the continuation is never folded into the task. However, this is a semantic delta: if applyContinuation is ever changed to actually fold continuation text, 'retry' in the task prompt would mean something different from ''. The old '' continuation was intentionally a no-op; the new 'retry' is not, even though it has the same effect today. To keep the exact old behavior use continuation: ''.

🟠 MEDIUM Analyzes edges silently die when node.id != node.profile.name — src/runtime/supervise/graph.ts

Routes are built from NODE ids (over: edge.over, to: edge.to, lines 515-524), but the coordination layer matches routes against the settled/live worker's PROFILE NAME and label (workerRouteNames / liveWorkerIdNamed, coordination.ts:887-903), which is node.profile.name on the pinned profile. validateGraph (lines 248-259) validates node ids and profile schemas independently and never requires node.id === profile.name. When they diverge, an analyzes edge's over filter never matches the settled worker's name => the analy

🟠 MEDIUM Duplicate analyst id across analyzes edges collapses the ledger maps — src/runtime/supervise/graph.ts

driverAnalyzesByAnalyst and routedAnalyzesByAnalyst are keyed by analyst id only (lines 582-586). Two analyzes edges sharing an analyst (e.g. convergence over builder->fixer and convergence over coder->reviewer) collapse to one map entry: onCoordinationEvent (lines 636, 658) always resolves the LAST-registered edge, so the first edge's traversals are either dropped (when the resolved edge's over excludes the source node, [line 641](https://github.com/tangle-network/agent-runtime/blob/cf194363f17511e06bf7d841f95a53d09a84ff

🟠 MEDIUM GraphEdgeCapError misattributes aborted/budget-exhausted run endings — src/runtime/supervise/graph.ts

At lines 768-771, any no-winner result with exhaustedDelegates.size > 0 throws GraphEdgeCapError, regardless of the terminal reason. A run aborted via opts.signal, or that exhausts the conserved budget, returns no-winner('aborted'/'budget-exhausted') (types.ts:1100-1102); if a delegates cap was previously exhausted (the driver kept re-spawning after a refusal), this path throws instead of returning the lifecycle result, and the error message claims 'the cap (the cyclic-graph backstop), not the task, ended this run' — factually wrong when the abort or budget ended it. This also converts a caller-initiated abort into an uncaught exception. Fix: only throw

🟠 MEDIUM Caller seam-env override wins for TRACE_ID/PARENT_SPAN_ID but TRACEPARENT still leaks the recorder's ids — precedence contract asserted by the test is violated — tests/kernel/supervise-worker-trace.test.ts

The test at lines 264-286 documents the precedence contract ('the supervisor never overrides it') and asserts only TRACE_ID/PARENT_SPAN_ID equal the caller's values. It never asserts TRACEPARENT. With the new dual-write, the contract is broken: traceContextToEnv (src/mcp/trace-propagation.ts:136-146) writes TRACEPARENT whenever parentSpanId exists, and the cli executor merges env as { ...process.env, ...args.traceEnv, ...(args.seam.env ?? {}) } (src/runtime/supervise/runtime.ts:1053). seam.env overrides the legacy pair only; TRACEPARENT (from traceEnv, merged before seam.env) keeps the recorder's W3C ids. A child under a caller-supplied TR

🟠 MEDIUM TRACEPARENT precedence untested in the caller-override case — stamped W3C wire leaks past a caller's legacy-only declaration — tests/kernel/supervise-worker-trace.test.ts

This PR adds TRACEPARENT to InheritedEnv (line 50) and PRINT_TRACE_ENV (line 59), extending the feature surface to the W3C wire. The precedence test at line 264 asserts env.TRACE_ID and env.PARENT_SPAN_ID are caller-owned, but does NOT assert env.TRACEPARENT. Verified empirically: traceContextToEnv() writes TRACEPARENT from the recorder context (trace-propagatio

🟠 MEDIUM probeArgs missing TRACEPARENT — front-door dual-write untested — tests/kernel/supervise-worker-trace.test.ts

The probeArgs helper at line 472-480 prints only TRACE_ID and PARENT_SPAN_ID into the output file, omitting TRACEPARENT. The main PRINT_TRACE_ENV constant at line 54-60 includes all three. Consequently, the supervise({ backend, otel }) front-door test at line 504 ('hands the front door worker the run trace id and the root span id') never as

🟠 MEDIUM Dual-write 'same trace' invariant only tested for pre-hex ids, not the human-id derivation path — tests/mcp/trace-propagation.test.ts

The roundtrip test uses parent ids that are already valid W3C hex (a3ce929d0e0e4736a3ce929d0e0e4736 / 00f067aa0ba902b7), so traceContextToEnv keeps them verbatim and both readers trivially return identical strings. The module's core claim — 'a human id is derived through the contract's deriveHexId ... so both spellings name the SAME trace' (src/mcp/trace-propagation.ts:130-134) — is never exercised. With a human id like 'my-trace', the legacy reader returns traceId='my-trace' while the W3C reader returns deriveHexId('my-trace',16)='e2adbfc2f480c903f35289a43aef5a89' (verified against the installed contract) — different strings that only converge later at export via padTraceId. The test as written would FAIL if parent.traceId were human-readable, so it deliberately (or accidentally) sidestep

🟠 MEDIUM Severed-hop span stamping (tangle.trace.unpropagated) has no test coverage — tests/mcp/trace-propagation.test.ts

The new unpropagated flag's only consumer is the stamping branch in createPropagatingTraceEmitter (src/mcp/trace-propagation.ts:108-118) which adds { key: 'tangle.trace.unpropagated', value: { boolValue: true } } to every span under a severed context. The sole emitter test (line 97) deletes OTEL_EXPORTER_OTLP_ENDPOINT, so createOtelExporter() returns undefined, emit() early-returns on if (!exporter), and no span is ever built — the stamping branch is dead in the test suite. The unpropagated marker is asserted only at the read/context level ([line 85](https://github.com/tangle-network/agent-runtime/blob/cf194363f17511e06bf7d841f95a53d09a84ff06/tests/mcp/

🟠 MEDIUM createPropagatingTraceEmitter emit pipeline entirely untested — tests/mcp/trace-propagation.test.ts

The test at line 97 only verifies the emitter object exists and exporter is undefined. The full emit path — buffering events per runId, flushing on 'loop.ended', building OTLP spans via buildLoopOtelSpans, calling exporter.exportSpan, and stamping the 'tangle.trace.unpropagated' attribute when ctx.unpropagated is true — is completely untested. This is the load-bearing observable behavior of the module. Without it, a regression in the unpropagated attribute stamp (trace-propagation.ts:109-118) or the buffer/flush logic ([lines 97-103](https://github.com/tangle-network/agent-runtime/blob/cf194363f17511e06bf7d841f95a53d09a84ff06/tests/mcp/trace-propagation.t

🟡 LOW Cross-page anchor runtime.md#kind-12 resolves to the wrong interface's property under GitHub rendering — docs/api/mcp.md

The 'Inherited from' link [kind](runtime.md#kind-12) intends LoopSandboxPlacement.kind, but typedoc-plugin-markdown 4.12.0's per-page anchor counter disagrees with GitHub's heading-order slugger: simulating GitHub's algorithm over head runtime.md assigns kind-13 to LoopSandboxPlacement.kind (line 17128) and kind-12 to LoopPlanDescription.kind (line 16893), so the link lands on the wrong symbol (no 404, wrong target). This is a PRE-EXISTING generator offset, not a PR regression: base runtime.md had 12 kind headings before the target and the base l

🟡 LOW AnalystFindingEvent.findings changed from required to optional — breaking for callers without null guards — docs/api/runtime.md

The findings property changed from readonly findings: unknown to readonly findings?: unknown. Any TypeScript caller that accessed event.findings.someProp without optional chaining will now get a compile error or runtime undefined. The doc says findings are ABSENT when the analyst returned undefined (no findings), which is rational, but this is a contract change existing subscribers may not handle. Verify that all consumers of AnalystFindingEvent (coordination subscribers, bus handlers, CLI code patterns) handle the optional case.

🟡 LOW Dumb steering onPass continuation is registered but never delivered by plan() — docs/api/runtime.md

steeringDriver.plan() (src/runtime/steering-drivers.ts:110) short-circuits to [] when last.verdict.valid===true, so the dumb directive's onPass string is never actually issued as a continuation — yet the rendered doc says 'one of two fixed continuations keyed on the verdict's boolean' and dumbContinuationPassPrompt is documented as 'The pass branch of the dumb steering control.' The onPass branch is effectively dead in the loop path (it only matters for decide()'s pick-winner). This is consistent with the legacy dumbDriver doc ('issues onPass or onFail accordingly') so it is NOT a regression introduced by this PR — but the rendered prose overstates onPass's role. Fix: tighten the SteeringDirectiveData dumb-arm doc to note onPass is the terminal/keep-going branch (seldom reached because a v

🟡 LOW Orphaned legacy JSDoc block above naiveDriver/dumbDriver deprecation — docs/api/runtime.md

src/runtime/steering-drivers.ts:166-180 and :211-228 each carry TWO stacked /** */ blocks — the legacy descriptive block (lines 166-176 / 211-224) sits orphaned above the @deprecated block that actually renders. TypeDoc emits only the deprecated block (which is what docs/api/runtime.md shows), so the rendered output is correct, but the source-side duplication is a latent trap for the next contributor editing the description. Cosmetic source fix: delete the orphaned upper block on each function; no doc regeneration needed.

🟡 LOW New event-kind skip paths lack direct regression tests — src/durable/spawn-journal.ts

The three new branches (replaySpawnTree skip at :557-558, materializeTreeView settlement exclusion at :724-725, outsideCursorNamespace at :488-489) are covered only at the journaling side: tests/kernel/graph.test.ts asserts edge events land in the journal, tests/kernel/supervise-worker-trace.test.ts asserts trace-unpropagated is appended, but no test feeds either kind through replaySpawnTree, materializeTreeView, or loadSpawnForest. A future regression that re-includes these kinds (e.g. requireNode on graph: ids, or a seq-namespace collision tripping assertSeqUnique) would be caught only by a resume of a graph-run journal in production. Add one unit test that materializes/replays a mixed tree containing edge + trace-unpropagated events alongside real spawns/settlements.

🟡 LOW No test covers replay/view skipping of edge and trace-unpropagated events — src/durable/spawn-journal.ts

The three new skip paths (outsideCursorNamespace:488-489, replaySpawnTree:557-558, materializeTreeView:724-725) are load-bearing — without them replay throws on outRef===undefined and materializeTreeView corrupts node status via the else-branch at line 773. Yet tests/runtime/spawn-journal-replay-identity.test.ts exercises the analogous skips for materialized/execution-bound (lines 34-79) but adds no case appending kind:'edge' or kind:'trace-unpropagated' and asserting replay length and node statuses are unchanged. Since graph.ts

🟡 LOW A throw in one route's analysis or delivery aborts all subsequent routes for the same settlement — src/mcp/tools/coordination.ts

The applicable-routes loop (976-989) has no per-route error isolation. If opts.analysts.run(route.kind, trace) throws for route N (e.g., a misconfigured/typo'd kind id), routes N+1..M never fire and their findings never reach the bus. deliverRoutedFinding's internal catch isolates auth/delivery throws, but the unknown-worker bus.publish (923) and the finding publish (980) are unguarded — a subscriber throw there aborts remaining routes. pendingSettlement is already cleared (961) so the settlement is retained, but later routes are permanently skipped (cursor advances on next drain). Fix: wrap each route iteration in try/catch and continue, so one bad lens cannot starve the others.

🟡 LOW New fallback and failure branches are untested — src/mcp/tools/coordination.ts

graph.test.ts covers only the top-level findings: undefined case — whose claimed pre-fix failure mode does not actually exist (see first finding). Untested: canonicalFindingEvent's nonCanonicalFindings fallback for cyclic and BigInt payloads (the real throw path), nested-undefined → drop/null semantics, the deliverRoutedFinding unknown-worker branch (line 922), the auth-refusal swallow (line 950), and label/profile-name disambiguation. Recommend unit tests for the cycle and BigInt fallback and a routed delivery to a settled/none

🟡 LOW No test coverage for new routing, canonicalization, and delivery functions — src/mcp/tools/coordination.ts

canonicalFindingEvent, deliverRoutedFinding, safeJsonText, liveWorkerIdNamed, workerRouteNames, and normalizeAnalyzeOnSettle have zero test coverage. The existing coordination-driver test only verifies that analyzeOnSettle: ['progress'] (bare string form) constructs. No test exercises the route form (AnalyzeOnSettleRoute with to, directive, over), the filtering logic in flushPendingSettlement, the canonicalization of undefined/cyclic/BigInt payloads, or the delivery fallback (unknown-worker publish). The trace-propagation.ts tests are thorough; the new coordination functions need counterparts.

🟡 LOW Routed-delivery authorization refusals are swallowed with NO steer record — contradicting the 'never a silent drop' docstring — src/mcp/tools/coordination.ts

Lines 191-193 and 908-910 promise every routed failure is recorded as a steer with delivered:false, 'never a silent drop', and the catch comment (951-952) lists 'an authorization refusal' as already-on-the-bus. Grounded: if opts.authorizeDownMessage throws (refusal) or the live target node has no durable identity, authorizeInstruction throws BEFORE recordInstruction/attemptDelivery run; the empty catch swallows it and NO instruction receipt, delivery-attempt, or steer event is ever published. Since graph.ts:657-668 ledgers a routed analyzes traversal only on the analyst-bearing steer, the edge ledger has no row for that traversal either — an omission th

🟡 LOW canonicalFindingEvent rationale is factually wrong: the event-id digest tolerates undefined; cycles/BigInt are the real throwers — src/mcp/tools/coordination.ts

The docstring (lines 147-160) and the graph test (tests/kernel/graph.test.ts:538-540) claim the RFC 8785 'coordination-event id' digest throws on ANY undefined value, nested included, so the producer must strip undefined. Grounded: coordinationEventId (supervise.ts:1148) → canonicalCandidateDigest → contentAddress → stableStringify (content-address.ts:9-15), which FILTERS undefined object values and maps top-level undefined to 'null' — it never throws on undefined. The durable coordination-log subscriber (coordination-log.ts:127) uses plain JSON.stringify, which also drops undefined without throwing. Verified by executing both: findings: undefined and n

🟡 LOW liveWorkerIdNamed label fallback is ambiguous when labels repeat (default label is 'worker') — src/mcp/tools/coordination.ts

spawn_agent defaults the label to 'worker' (line 1512) and AgentProfile.name is optional, so several live workers can share a name the route targets. liveWorkerIdNamed does live.find(...)? .id ?? live.find(label)...? .id — first match wins with no disambiguation or error. A route to: 'worker' (or any duplicated profile name) silently delivers analyst findings to an arbitrary one of the matching live workers. Profile-name-first ordering makes the graph's pinned-node case (profile.name = node id) safe, but the label fallback and duplicated profile names are a real correctness foot-gun. Fix: match unique name only (error on ambiguity) or require the caller t

🟡 LOW profileNameByWorker grows unbounded — entries added at spawn, never evicted at settle — src/mcp/tools/coordination.ts

profileNameByWorker.set at line 1552-1554 runs on every successful spawn; there is no corresponding delete in commitSettled (862-871) or unwatchWorker. The comment at 703-706 says 'dropped never (a settled source can still be matched by over after it is gone)' — but workerRouteNames/over-matching only runs during flushPendingSettlement for the CURRENTLY settling worker, which by definition has not yet been removed from any structure. After settlement, the entry is dead weight. For a long-lived coordination manager with many spawns this leaks one string entry per worker indefinitely. Low impact in practice (spawns are budget-bounded), but the 'dropped ne

🟡 LOW profileNameByWorker not restored on resume, limiting route matching — src/mcp/tools/coordination.ts

The profileNameByWorker map is populated at spawn time (line 1552-1554) but is never seeded from resumed scope data. On a process restart, all previously spawned workers' profile names are lost from this map — only their labels survive through nodeForWorker().label. This means an AnalyzeOnSettleRoute.over filter that names a worker by profile name will NOT match any resumed settled worker; only label-based filters work. The settled worker's profile name IS available from the resumed scope's node identity (e.g. resumedNode.identity?.profile?.name), it is just not fed back into profileNameByWorker. Severity low because label-based matching still

🟡 LOW Cross-version 'child running either version joins the same trace' promise is false for human-readable ids — src/mcp/trace-propagation.ts

The module doc (lines 8-11) and worker-trace.ts:10-16 promise a legacy-version child joins the same trace as a new parent for one release. For a non-W3C (human) trace id, traceContextToEnv writes TRACEPARENT with deriveHexId(traceId) but also writes TRACE_ID verbatim. A LEGACY child reads TRACE_ID and its own old emitter pads it with the pre-PR slice-and-pad (otel-export.ts old padTraceId: 'trace-id-123' → 'trace-id-123000...'), producing a DIFFERENT OTel trace id than the new parent's deriveHexId('trace-id-123'). So an old child spawned by a new parent does NOT join the parent's trace for human ids — the promise holds only when ids are already W3C hex (which

🟡 LOW parseTraceparent discards sampling flags; traceContextToEnv hardcodes flags to 01 — src/mcp/trace-propagation.ts

parseTraceparent (45-53) extracts only traceId and parentSpanId, dropping the 2-hex flags byte. traceContextToEnv (144) always writes '00-...-01'. A parent that deliberately set flags=00 (not-sampled) has its decision overridden on re-emission, so a child of a not-sampled trace would be marked sampled. The TraceContext type carries no flags field, so this is a fidelity ceiling of the current design, not a regression. createPropagatingTraceEmitter also ignores sampling (exports whenever the endpoint is set), so no current behavior breaks. Worth a TODO or a flags field on TraceContext if sampling fidelity becomes a goal.

🟡 LOW traceContextToEnv always rewrites the sampled flag to 01, losing the upstream sampling decision — src/mcp/trace-propagation.ts

parseTraceparent (line 45) discards the trace-flags field (only validates it as 2 hex), and TraceContext carries no sampled flag, so a parent that inherited an UNSAMPLED (00) traceparent propagates it to every child as -01 (sampled). Sampling semantics are advisory, but a fleet that sets flags 00 to drop cold traces will silently re-sample at each agent-runtime hop. Related grammar strictness: the regex accepts any version byte (W3C reserves ff and says unknown versions must not be interpreted) and rejects valid traceparents with trailing fields after flags (spec says trailing fields are ignored). All paths fail closed (degrade to a severed root) rather th

🟡 LOW Backwards-incompatible wire-id change for non-W3C inputs is intentional but unflagged as a migration — src/otel-export.ts

Any consumer that persisted the OLD sliced/padded ids (e.g. a UUID '12345678-...' -> '1234567812341230', or a run id 'vb-web-grounded-...' -> 'vbwebgrounded2026...') will see DIFFERENT span/trace ids after this change, because non-W3C inputs now route to deriveHexId instead of slice-and-pad. This is the intended fix (the old ids were invalid W3C hex and leaked raw input), and the CHANGELOG documents the dependency swap. No action required for merge — flagging only so anyone with stored traces keyed on the old ids knows the wire identity moves on this commit. The new ids are strictly more correct (valid hex, deterministic cross-process, no raw-input leakage).

🟡 LOW Hyphenated hex strings change behavior (by design) — src/otel-export.ts

Old: 'a3ce-929d-0e0e-4736' → cleaned to 'a3ce929d0e0e4736' (valid 16-hex, passed through). New: isW3CSpanId returns false (hyphens not W3C-valid) → hash-derived to a completely different ID. For padTraceId: 'a3ce929d0e0e4736a3ce929d0e0e47-36a3ce' → old cleaning yields 32 hex chars, new derivation yields a hash. Any external integration piping hyphenated parent-span/trace IDs into this module will see trace-joining break. This is the documented intent (only clean W3C ids pass through) and the old slice-and-pad was lossy/truncated, but callers should be aware.

🟡 LOW Non-W3C uppercase hex treated as derivation, not pass-through — src/otel-export.ts

isW3CSpanId/isW3CTraceId require lowercase hex (per W3C trace-context spec). An ID like 'A3CE929D0E0E4736' (uppercase 16-hex, accepted by some OTel SDKs) returns false → hash-derived instead of passed through. While W3C mandates lowercase, OTel SDKs do normalize case on wire, so an external system sending uppercase parent IDs through a traceparent header gets a different wire ID than the one it owns. Low impact: generateSpanId always produces lowercase, and standard OTel SDKs emit lowercase.

🟡 LOW padSpanId/padTraceId not exported, untestable in isolation — src/otel-export.ts

Both functions are module-private and tested only indirectly through loopEventToOtelSpan/flatOtelSpan/buildLoopOtelSpans. Edge cases (empty string → reserved all-zero id avoidance, very long input, non-string input) are only exercised through callers that guard with truthiness ternary checks. Coverage is adequate since all call sites protect against falsy inputs, but a direct unit test would make the contract of 'never produce all-zero, never throw, always 16/32 hex chars' self-documenting.

🟡 LOW steeringDriver: no direct tests for the new interpreter function — src/runtime/steering-drivers.ts

steeringDriver is the new canonical interpreter for SteeringDirectiveData. It is tested indirectly through the deprecated naiveDriver and dumbDriver wrappers (steering-drivers.test.ts, 126 lines), which delegate to it. But there are no direct tests exercising the naive/dumb SteeringDirectiveData shapes through steeringDriver itself — e.g., a naive directive with continuation: '' and maxTraversals: 1, or a dumb directive with onPass/onFail edge cases. The deprecated wrappers will be removed, and when they are, the tests leave with them unless new tests target steeringDriver directly.

🟡 LOW Duplicate routed analyzes edges with same analyst kind silently overwrite in lookup map — src/runtime/supervise/graph.ts

The loop for (const edge of analyzes) { (edge.to === root.id ? driverAnalyzesByAnalyst : routedAnalyzesByAnalyst).set(edge.analyst, edge) } uses analyst kind as the map key. Two routed (non-root-destined) analyzes edges sharing the same analyst kind (e.g. both kind:'foo' with different to destinations) cause the second to overwrite the first. When steer events carrying analyst:'foo' arrive, ALL are attributed to the last edge — the first edge's traversals are never ledgered and its cap is never enforced. validateGraph checks for duplicate delegates-to-worker targets but has NO equivalent check for duplicate routed-analyst kinds. Impact: ledger inaccuracy and unenforced cap in an unusual but authorable topology. Fix: add a validation check that rejects two routed analyzes edges wi

🟡 LOW Spawn row journaled 'delivered' immediately when spawnContext lacks assignmentId — no rewrite if makeLeaf throws — src/runtime/supervise/graph.ts

When spawnContext?.assignmentId === undefined, the delegates traversal row is journaled immediately via appendJournal(row, ...) with outcome 'delivered'. If makeLeaf(pinned, spawnContext) subsequently throws synchronously, the 'delivered' row persists in the journal even though no worker went live. The pending-rewrite loop at the end of start() only processes pendingByAssignment entries (the assignmentId-present path). In practice the coordination layer always provides assignmentId, so this path is unreachable through normal supervise(), but the MakeWorkerAgent type permits context?: WorkerSpawnContext (undefined). Fix: always defer journaling (push to a universal pending list) or rewrite orphaned immediate-journal rows in the cleanup loop.

🟡 LOW runGraph: sync validation errors on async-returning function — src/runtime/supervise/graph.ts

runGraph returns Promise but validateGraph and the backend/makeWorkerAgent check throw synchronously before the async IIFE starts. A caller doing runGraph(g, opts).catch(handler) will miss synchronous validation errors — they need a try/catch around the call. The comments document this as matching supervise()'s contract, but it's a footgun for callers who see Promise<GraphResult> and assume all errors surface via the promise.

🟡 LOW strippedByDigest is unbounded and routed analyst deliveries never ledger 'stripped' — src/runtime/supervise/graph.ts

strippedByDigest grows one entry per uniquely-narrowed instruction and is never evicted within a run (memory bound by driver turns, but unbounded in principle). Separately, the analyst branch of the steer handler (line 656-663) does not consult strippedByDigest, so a ROUTED analyst delivery whose directive+findings text authorization narrows is ledgered 'delivered' with the delivered bytes, never 'stripped' — a minor gap in the filter-observation contract that only delegates steers honor. Fix: consult the digest map in the analyst branch too, and bound the map (or key by receiptId).

🟡 LOW strippedByDigest map grows unbounded across long runs — src/runtime/supervise/graph.ts

The authorizeMessage wrapper records every narrowed instruction by its output digest: strippedByDigest.set(canonicalCandidateDigest(decision.instruction), { composedBytes }). This map is never pruned. For a long-running graph with many steers, it accumulates one entry per distinct stripped instruction. Each entry is small (~100 bytes), so practical impact is negligible, but it is technically an unbounded leak. Additionally, if two different composed instructions strip to the same output, the second overwrites the first's composedBytes. Fix: cap the map size or key by a composite of composed+stripped digests.

🟡 LOW strippedByDigest map unbounded within a run — src/runtime/supervise/graph.ts

The strippedByDigest map accumulates entries for every narrowed steer during a run. It is never pruned. For a normal run this is bounded by the number of steer events, but a degenerate driver issuing many distinct narrowings could build a large map. The digest keys are SHA256 strings (~64 chars each) so memory pressure is low, but the map is never cleared.

🟡 LOW trace-unpropagated journal append serialized into spawn critical path (extra fsync for file journals) — src/runtime/supervise/scope.ts

The trace-unpropagated appendEvent is chained via .then() onto spawnCommitted, which materializationCommitted and downstream spawn flow await. For the in-memory journal this is negligible, but FileSpawnJournal.appendEvent fsyncs per call — this doubles the fsync count on the spawn critical path for every traced spawn on a non-propagating backend (bridge/cli-worktree/router). Not a correctness bug (ordering is correct: spawned → trace-unpropagated → materialized), but a latency regression for durable runs with tracing. Fix: fire the trace-unpropagated append in parallel (it annotates the spawn event, doesn't gate materialization ordering).

🟡 LOW trace-unpropagated journal write failure fails a successful spawn — src/runtime/supervise/scope.ts

The trace-unpropagated append is chained into spawnCommitted.then (lines 738-753), which feeds materializationCommitted (line 754) -> executionReady in runChild (scope.ts:1779). An informational telemetry record that fails to write (e.g. FileSpawnJournal fsync error) rejects the chain, turning a spawn that otherwise succeeded into a typed-down child with a refunded reservation — failure amplification of observability over execution. Recommendation: attach the trace-unpropagated write to the same journalWrites-style settle-a

🟡 LOW workerTraceUnpropagated: no unit test for declaration or journaling — src/runtime/supervise/supervise.ts

The workerTraceUnpropagatedDeclaration function (supervise.ts:177-191) classifies backend trace propagation and the scope.ts change (scope.ts:1490-1505) journals trace-unpropagated events. Neither has a unit test. The feature's correctness relies on the WORKER_TRACE_PROPAGATION census being accurate and the journaling code not interfering with the spawn event chain. The trajectory report correctly filters these events (trajectory.ts:78-79), but the end-to-end trace-unpropagated path is unverified.

🟡 LOW Ambient TRACEPARENT is not scrubbed from process.env, so the 'stamps nothing' assertions can leak a real id and flake — tests/kernel/supervise-worker-trace.test.ts

beforeEach deletes TRACE_ID and PARENT_SPAN_ID but not TRACEPARENT (lines 184-196). The two untraced cases assert the child observed { TRACE_ID: null, PARENT_SPAN_ID: null, TRACEPARENT: null } (lines 301-305 and 318-322). The cli child env is { ...process.env, ...traceEnv, ...seam.env } (runtime.ts:1053), so an ambient TRACEPARENT exported by the process that launches the test runner leaks into the child when the run is untraced (traceEnv is {}). Since this repo's CI/tooling runs under a

🟡 LOW First test times out (20s) under vitest default parallelism — subprocess-spawn contention — tests/kernel/supervise-worker-trace.test.ts

When all three shot files run together, this test hit vitest's 20s default timeout (it spawns a real node -e subprocess). Passes in 30ms in isolation and in 187ms under --no-file-parallelism. Pre-existing characteristic of the subprocess-spawning suite, not introduced by this PR's TRACEPARENT addition, but the added assertion makes the test one-statement heavier. Consider raising testTimeout for this describe block or marking it serial.

🟡 LOW Severed-hop journal tests exercise the mechanism but never the production declaration derivation — the backend census is untested — tests/kernel/supervise-worker-trace.test.ts

Both severed-hop tests pass workerTraceUnpropagated: { backend: 'bridge', reason: 'no-env-channel' } manually to the raw createSupervisor().run path while the actual executor is cli (a PROPAGATING backend per WORKER_TRACE_PROPAGATION, worker-trace.ts:76-84) — so the journaled 'trace-unpropagated' record does not describe a real severed hop (the worker did receive the context). The production derivation workerTraceUnpropagatedDeclaration (supervise.ts:187-196, incl. the router/'no-worker-process' vs bridge/'no-env-channel' mapping) and its wiring (supervise.ts:1389-1391, 1684-1687: only when options.backend AND a recorder resolve) are never reached: the only supervise({backend, otel}) tests in the file use backend cli ([line 489](https://github.com/tangle-network/agent-runtime/blob/

🟡 LOW Emitter span-attribute stamping for unpropagated contexts is untested — tests/mcp/trace-propagation.test.ts

The source's createPropagatingTraceEmitter (src/mcp/trace-propagation.ts:108-118) stamps tangle.trace.unpropagated=true on every exported span when ctx.unpropagated===true. This is the feature that makes a severed hop 'queryable' per the module doc. No test in this file (or any other test file grepped) verifies the attribute actually appears on exported spans. The existing emitter test (line 97) only checks context passthrough and exporter absence. Fix: inject a mock exporter, emit a loop.started..loop.ended cycle with an unpropagated context, and assert the exported spans contain {key:'tangle.trace.unpropagated',value:{boolValue:true}}.

🟡 LOW Incomplete assertion in malformed TRACEPARENT fallback test — tests/mcp/trace-propagation.test.ts

The test 'a malformed TRACEPARENT fails closed to the legacy pair' only asserts ctx.traceId === 'legacy-trace'. It does not assert that parentSpanId is undefined (PARENT_SPAN_ID was explicitly deleted at line 50) or that unpropagated is undefined (the legacy path was used, not a fresh mint). Missing these assertions means a bug that leaks undefined parentSpanId or sets unpropagated on the legacy path would not be caught.

🟡 LOW Mixed hex/non-hex id combinations in traceContextToEnv untested — tests/mcp/trace-propagation.test.ts

traceContextToEnv tests both all-non-hex (line 109) and all-hex (line 118) id inputs, but not the mixed case where traceId is already hex but parentSpanId is human-readable (or vice versa). The source code handles each independently via isW3CTraceId/isW3CSpanId (trace-propagation.ts:140-143), so the mixed path is logically covered by the independent branches, but an integration-style test would catch a copy-paste error where the wrong id is used in the wrong position of the TRACEPARENT string.

🟡 LOW No assertion that a propagated TRACEPARENT context leaves unpropagated undefined — tests/mcp/trace-propagation.test.ts

test 1 asserts unpropagated===undefined for the legacy pair, and test 5 asserts unpropagated===true for the fresh mint, but the TRACEPARENT-win path (line 37-45) never asserts unpropagated is undefined. A regression that accidentally set unpropagated=true on a successfully parsed traceparent would flip every child span into a 'severed hop' without any test failing. Add expect(ctx.unpropagated).toBeUndefined() to the TRACEPARENT-wins test.

🟡 LOW Round-trip test only covers already-hex ids, not the human-id divergence case — tests/mcp/trace-propagation.test.ts

The test at line 56 uses traceId='a3ce929d0e0e4736a3ce929d0e0e4736' (already valid W3C hex), so traceContextToEnv writes the SAME string to both TRACE_ID and TRACEPARENT. The interesting case is a human id like 'my-trace': traceContextToEnv writes TRACE_ID='my-trace' (verbatim) but TRACEPARENT='00-<deriveHexId(my-trace,16)>-...-01' (derived hex). A W3C-only child reads the hex; a legacy-only child reads the human string — different TraceContext.traceId values. The test name claims 'either convention joins the same trace' but only validates the trivial case where no divergence exists. The traceContextToEnv-writes-BOTH-conventions test ([line 109](https://g

🟡 LOW parseTraceparent valid-path not directly unit-tested — tests/mcp/trace-propagation.test.ts

The parseTraceparent unit test (line 88) covers undefined, empty, non-hex, and all-zeros — all invalid cases. It never calls parseTraceparent with a valid traceparent to confirm it returns {traceId, parentSpanId} with lowercased hex. The valid path is only exercised indirectly through readTraceContextFromEnv in the precedence test (line 37). A direct assertion like expect(parseTraceparent('00-AB-...-01')).toEqual({traceId:'ab-...', parentSpanId:'...'}) would lock the contract and verify the toLowerCase() normalization at

🟡 LOW traceContextToEnv TRACEPARENT assertion is shape-only, not value-exact — tests/mcp/trace-propagation.test.ts

'traceContextToEnv writes BOTH conventions' asserts env.TRACEPARENT only against /^00-[0-9a-f]{32}-[0-9a-f]{16}-01$/. Any 32-hex trace id + 16-hex span id passes, so a wrong-but-valid-hex derivation (e.g. swapped byte widths or hashing the wrong input) would be accepted. Pin the exact value for known inputs (deriveHexId('my-trace',16)/deriveHexId('my-span',8)) to lock the derivation, mirroring the verbatim-hex test at line 124 which already does exact matching.

🟡 LOW Env-sensitive pre-existing test: 'throws without an api key' fails when TANGLE_API_KEY is set — tests/otel-export.test.ts

Reproduced: with TANGLE_API_KEY set in the environment, this test resolves { ok:false, status:426 } instead of rejecting (the apiKey guard at src/otel-export.ts reads process.env.TANGLE_API_KEY before throwing). Identical test exists at base line 595, so this is NOT introduced by this PR's diff. It makes the file non-hermetic in any shell/CI that exports the key. Fix (out of shot scope): unset process.env.TANGLE_API_KEY at test start, not only in afterEach.

🟡 LOW Test title overstates cross-process determinism that the test does not exercise — tests/otel-export.test.ts

The title says 'in every process' but the body only calls loopEventToOtelSpan twice in the SAME process and asserts a.traceId === b.traceId. Cross-process determinism is a property of deriveHexId itself (a pure function of UTF-8 bytes) and is the contract package's job to test, not this repo's. Minor: either drop 'in every process' from the title, or add a child_process spawn if you want to genuinely exercise it. The assertion that IS made (same runId → same traceId within one process) is correct and valuable.

🟡 LOW asContractSpan helper uses wrong field names and hides it behind as nevertests/otel-export.test.ts

The helper emits { start_time_unix_nano, end_time_unix_nano } but ContractSpan (agent-trace-contract/dist/span.d.ts:66) declares start_time / end_time (ISO 8601) — and omits parent_span_id, status, attributes. The as never cast on line 615 silences the type mismatch instead of admitting it. The test still proves its narrow claim (no non-hex-id finding for the derived trace_id) because validateTraceSpans is designed to accept arbitrary input, but the name asContractSpan is misleading since the shape is NOT a valid ContractSpan. Fix: cast as unknown as readonly ContractSpan[] and either rename the helper to asIdOnlySpan or fill the real fields, so a future

🟡 LOW as never cast bypasses type safety in validator integration test — tests/otel-export.test.ts

validateTraceSpans([asContractSpan(span)] as never) and validateTraceSpans([...] as never) at lines 615/619 use as never to work around a type mismatch between the contract Span type and OtelSpan. The cast suppresses type checking — if validateTraceSpans' signature or shape requirements change, these tests silently pass with the wrong shape. Consider importing the contract's Span type or adding a comment explaining why the cast is safe.

🟡 LOW attrMap redefinition shadows top-level helper with different semantics — tests/otel-export.test.ts

The const attrMap at line 569 shadows the function attrMap defined at line 18. The function unwraps OtelAttribute values (returning string|number|boolean|undefined), while the const returns raw { stringValue?, intValue?, ...} objects. A reader skimming the assertions at lines 570-572 may incorrectly assume the function semantics. Rename to rawAttrs or otelAttrs.


tangletools · 2026-08-02T02:29:18Z · trace

tangletools
tangletools previously approved these changes Aug 2, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 60 non-blocking findings — cf194363

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

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-02T02:29:18Z · immutable trace

…g failures always ledgered, graph identity invariant, cap-error attribution, W3C precedence, UUID id joins

- deliverRoutedFinding: an authorize/record throw now publishes a failed
  steer (delivered:false, runtime-error) before being swallowed; the one
  bus-refuses-the-attempt-record sub-path is documented, not overclaimed.
- validateGraph requires node.profile.name === node.id (analyst routing
  matches on profile name) and refuses two analyzes edges sharing one
  analyst id (traversals are ledgered by analyst).
- GraphEdgeCapError only blames the cap when the cap ended the run:
  aborted/budget-exhausted no-winners return with the exhaustion
  observable in exhaustedEdges.
- mergeTraceEnv: a seam.env overriding TRACE_ID/PARENT_SPAN_ID without
  its own TRACEPARENT gets TRACEPARENT rebuilt from the caller's ids —
  the recorder's wire identity never survives a caller override.
- padSpanId/padTraceId strip dashes before the W3C check so UUID-form
  ids export the same joinable wire id earlier releases produced;
  traceContextToEnv single-sources through them.
- define-leaderboard continuation restored to '' (exact no-op).
- canonicalFindingEvent exported + 3-branch unit tests; human-id
  dual-convention convergence test; 2324 tests green.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — bcb23d74

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-08-02T03:08:08Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Concerns 1 (1 low)
Heuristic 0.0s
Duplication 0.0s
Interrogation 88.4s (2 bridge agents)
Total 88.4s

💰 Value — sound

Adds a plain-data agent-graph layer (runGraph) over the existing supervise() core with a fully observable edge ledger and a versioned prompt registry — coherent, in-grain, no second scheduler and no existing equivalent to duplicate.

  • What it does: Three coupled capabilities. (1) runGraph/AgentGraph (src/runtime/supervise/graph.ts) lets a multi-agent topology be authored as plain data: nodes are canonical AgentProfiles, edges are typed delegates/analyzes rows carrying versioned PromptHandle directives; it composes supervise() directly via its existing makeWorkerAgent/onCoordinationEvent/analyzeOnSettle/journal seams — it
  • Goals it achieves: Make agent loops plain, authorable, optimizable data; make every inter-agent edge's behavior a queryable fact (the motivating incident was a filter silently replacing ~1,700 chars of steering with 241 chars of boilerplate across three rounds with no artifact); make directives versioned optimization targets instead of hardcoded prose; and join the standard W3C trace wire so edges are joinable acros
  • Assessment: Good change, built in the grain of the codebase. The central design decision — composing supervise() rather than writing a parallel scheduler — is correct and visible: the graph layer adds only node-pinning, directive delivery, and ledger observation AROUND the existing execution core (graph.ts:11-16, 746-770). It uses supervise()'s designed extension points (makeWorkerAgent wrapper for node
  • Better / existing approach: none — this is the right approach. Searched for an existing topology/orchestration primitive to extend: (a) runAgentRounds/run-loop.ts is a different substrate — per-round driver.plan() fan-out refinement with a validator, not durable spawn/settle coordination with spawn_agent/await_event/steer/analyst-on-settle verbs — and the author deliberately names it as NOT the substrate (graph.t
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A coherent agent-graph layer that composes the existing supervise() execution core (not a second scheduler), with its prompt-registry consolidation, steering-driver refactor, and trace-propagation changes already wired to live production callers.

  • Integration: Every non-experimental surface is wired to a real caller. steeringDriver is consumed by define-leaderboard.ts:522 (the optimization suite). supervisorPolicyPrompt consolidates the two previously-contradictory supervisor defaults and is consumed by BOTH front doors — authoring.ts:28,110 (supervisorInstructions) and supervisor-agent.ts:45,59 (defaultSupervisorPrompt). The W3C trace work lands on liv
  • Fit with existing patterns: Fits the codebase's grain rather than competing. runGraph composes supervise() directly (graph.ts:747) — the same execution core, makeWorkerAgent seam, conserved-pool budget, and deliverable gate — exactly the pattern delegate.ts uses as a thin front door. It does NOT duplicate delegate() (which has the supervisor author whatever worker the intent needs) or conversation/ (peer turn-taking, a diffe
  • Real-world viability: Built around a real failure mode (the cited incident: a filter silently replaced 1,700 chars of steering with 241 chars of boilerplate for three rounds with no artifact saying so). The edge ledger records every traversal with outcome (delivered|stripped|empty|unpropagated) and byte counts, and the three truthfulness probes address genuine error paths: undefined/nested-undefined findings are produc
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: commented out code tests/mcp/trace-propagation.test.ts

  • // export both become the SAME wire id — one trace, either convention.

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 · 20260802T043509Z

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Incomplete — bcb23d74

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

Trigger a fresh review on the current PR head.

tangletools · 2026-08-02T05:20:32Z

@drewstone

Copy link
Copy Markdown
Contributor Author

@tangletools review

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — bcb23d74

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-08-02T05:27:17Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Concerns 1 (1 low)
Heuristic 0.0s
Duplication 0.0s
Interrogation 93.7s (2 bridge agents)
Total 93.7s

💰 Value — sound

A well-architected declarative topology layer over supervise() with observable edge ledgers and a prompt registry that consolidates contradictory defaults — no duplication, no better approach found.

  • What it does: Introduces three net-new capabilities, all composing the existing supervise() core rather than replacing it: (1) runGraph/AgentGraph (graph.ts:819 lines) — agent topologies as plain data: nodes are canonical AgentProfiles, edges are typed delegates/analyzes carrying versioned PromptHandle directives. runGraph calls supervise() directly (graph.ts:747), wrapping makeWorkerAgent to pin worker nod
  • Goals it achieves: Three concrete goals, all readable from the code: (1) Make agent-to-agent edges observable — the motivating incident (graph.ts:22-25, a filter silently replacing 1,700 chars of steering with 241 chars of boilerplate for three rounds with no artifact) is directly addressed by the ledger's stripped outcome that captures composed-vs-delivered byte deltas at authorization time (graph.ts:593-606). Po
  • Assessment: Sound on its merits and built in the grain of the codebase. The central architectural decision is correct: runGraph is an interpretation layer that composes supervise() via the existing makeWorkerAgent seam and composeRuntimeHooks, adding node pinning + edge observation without duplicating the scheduler, budget, deliverable gate, or spawn-tree machinery. The edge ledger is a DERIVED view (populate
  • Better / existing approach: none — this is the right approach. I searched for existing equivalents before concluding: (1) grep for runGraph|AgentGraph|agent.?graph|graph\.ts across src/ — no prior topology concept exists; the recursive driver-executor (coordination-driver.ts:15-17 'an agent drives an agent that drives an agent') supports RUNTIME recursion but not DECLARATIVE topology with pinned nodes and observable edges,
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A coherent agent-graph layer that composes supervise() (not a second scheduler), unifies two contradictory supervisor prompts into one registry, and makes every inter-agent edge observable — built in the grain of the codebase with no competing equivalent.

  • Integration: Fully wired. runGraph composes supervise() directly (graph.ts:747) and is publicly exported (runtime/index.ts:639). The prompt registry is already consumed by EXISTING builders (authoring.ts:28,110 imports supervisorPolicyPrompt; supervisor-agent.ts:59 sets defaultSupervisorPrompt = supervisorPolicyPrompt.text), so it is live source-of-truth, not dead surface. canonicalFindingEvent runs at every f
  • Fit with existing patterns: Fits cleanly; no competing pattern. There is no existing 'topology-as-declarative-data' abstraction: superviseSurface specializes supervise for graded AgenticSurface tasks (a different axis — eval surfaces, not topology-as-data); delegate() is a one-shot verb; personify combinators (fanout/pipeline/panel) are eval/benchmark shapes; runAgentRounds is the round-synchronous kernel. runGraph is genuin
  • Real-world viability: Robust beyond the happy path. The three ledger-truthfulness bugs are fixed at the root with probes proven load-bearing (graph.test.ts:585-742 asserts each behavior the pre-fix bug broke). Error paths are recorded, not swallowed: deliverRoutedFinding publishes a failed steer (delivered:false) on every failure path before catching (coordination.ts deliverRoutedFinding); a spawn refused after the fac
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: commented out code tests/mcp/trace-propagation.test.ts

  • // export both become the SAME wire id — one trace, either convention.

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 · 20260802T054153Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — bcb23d74

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 51 findings (6 medium, 45 low)

glm deepseek deepseek-flash aggregate
Readiness 37 58 0 0
Confidence 95 95 95 95
Correctness 37 58 0 0
Security 37 58 0 0
Testing 37 58 0 0
Architecture 37 58 0 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 39 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 39 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 39 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Routed delivery of absent findings sends literal "undefined" to the destination worker — src/mcp/tools/coordination.ts

When an analyst lens returns undefined, canonicalFindingEvent correctly omits the findings key (coordination.ts:162-164) and the graph layer treats absent findings as zero bytes ('empty', graph.ts:668-669, test asserts 'never the text "undefined"' at graph.test.ts:629-631). But flushPendingSettlement still calls deliverRoutedFinding(route, findings) whenever route.to !== undefined, and safeJsonText(undefined) evaluates JSON.stringify(undefined) ?? String(undefined)undefined ?? "undefined" → the literal string "undefined" (coordination.ts:2080-2087). The steer instruction text therefore becomes directive\n\nundefined, the destination worker receives a meaningless payload, and a routed-edge traversal with an undefined analyst is ledgered 'delivered' counting the "und

🟠 MEDIUM authorizeDownMessage cannot distinguish analyst-routed delivery from driver steer — src/mcp/tools/coordination.ts

deliverRoutedFinding calls authorizeInstruction('steer', targetId, text, false) with the same DownMessageAuthorizationInput shape a driver steer uses. The analyst origin is attached to the bus event AFTER authorization (line 1198 via origin?.analyst), never to the authorization input (authorizeInstruction at line 1088 builds the auth input without any origin field). Before this PR, analyst findings only reached the driver via the bus — the driver reviewed and chose whether to act. Now an analyst's output is delivered directly

🟠 MEDIUM Doc claim 'UUID-form id passes dash-stripped' is false for span ids — genuine UUIDs are derived, not preserved — src/otel-export.ts

The docstring (lines 748-750) promises a 'DASHED hex id (a UUID-form id) passes through dash-stripped: that is the exact wire id every earlier release exported for it'. True for trace ids and for dashed span ids whose dashless form is exactly 16 hex, but a genuine UUID passed as a SPAN id has 32 dashless hex chars, which is not a valid W3C span id, so padSpanId falls through to deriveHexId. Computed against the pinned contract (agent-trace-contract@1.0.2): old padSpanId('a3ce929d-0e0e-4736-a3ce-929d0e0e4736') = 'a3ce929d0e0e4736' (old slice(0,16)), new = '84ac537cb7920d70' (hash). Cross-version joins for UUID-form span ids silently break exactly where the comment cl

🟠 MEDIUM GraphEdgeCapError misattributes endings for the all-children-down lifecycle arm — src/runtime/supervise/graph.ts

The lifecycle exemption only covers reason==='aborted'||reason==='budget-exhausted', but types.ts:1092-1101 and supervisor.ts:334 classify 'all-children-down' as a lifecycle arm too. When a run ends no-winner:'all-children-down' (breaker tripped / every worker down) while a delegates cap was also exhausted along the way, runGraph throws GraphEdgeCapError claiming "the cap, not the task, ended this run" — a factually wrong attribution, and the documented lifecycle arm converts from a normal SupervisedResult to a thrown error. Evidence: the misattribution probe at graph.test.ts:367-412 only exercises 'aborted', leaving 'all-children-down' uncovered. Fix: include 'all-children-down' in the lifecycleEnded exemption (or reclassify per supervisor.ts), keeping exhaustion observable via exhaustedE

🟠 MEDIUM Orphaned provisional ledger row on overlapping keyed spawns — src/runtime/supervise/graph.ts

pendingByAssignment is a Map keyed by assignmentId (key:<k> for keyed spawns). The factory runs BEFORE scope's duplicate-key check (scope.ts:514 then 522), so a refused duplicate-key spawn records a provisional 'delivered' row keyed by the same assignmentId. If a SECOND duplicate-key spawn of the same key occurs while the first worker is still live, line 517 overwrites the map entry and the earlier row is orphaned: it never gets the agent.spawn-hook bind, is no longer reachable by the settlement rewrite loop (779-790, which iterates only pendingByAssignment.values()), and stays in the frozen ledger as 'delivered' with its composed byte count — a phantom del

🟠 MEDIUM graphWorker shallow-clones node profile; nested objects are reference-shared with the original graph node — src/runtime/supervise/graph.ts

The pinned profile is built as { ...node.profile, prompt: { ... } }. The prompt.instructions array is properly copied, but every other nested field (tools, mcp, resources, hooks, subagents, permissions, modes, model) is reference-shared with the original node.profile stored in graph.nodes. If any downstream executor mutates a nested field — e.g. adding a key to profile.tools — the mutation propagates back to the original graph definition, potentially affecting subsequent runs that reuse the same AgentGraph object. In practice profiles are treated as immutable in this codebase, so the risk is low, but a defensive deep-freeze or deep-clone of at least tools/mcp/resources would make the invariant explicit.

🟡 LOW padSpanId doc falsely claims UUID-form ids pass through dash-stripped — docs/api/index.md

The prose says 'A DASHED hex id (a UUID-form id) passes through dash-stripped: that is the exact wire id every earlier release exported for it, so cross-version joins survive.' The code (src/otel-export.ts padSpanId) is: if (isW3CSpanId(id)) return id; const dashless = id.replace(/-/g,''); return isW3CSpanId(dashless) ? dashless : deriveHexId(id, 8). agent-trace-contract's isW3CSpanId requires exactly 16 lowercase hex (dist/ids.js: SPAN_ID_PATTERN = /^[0-9a-f]{16}$/); a UUID is 32 hex, so a UUID span id does NOT pass through dash-stripped — it is DERIVED via deriveHexId(id, 8). The 'exact wire id every earlier release exported' claim also fails for UUID span inputs: the old implementation exported cleaned.slice(0,16).padEnd(16,'0') (first 16 hex), which differs from the derived id, so

🟡 LOW steer.analyst doc says 'when this steer DELIVERED' but field is set on failed routed deliveries too — docs/api/index.md

Prose: 'Present when this steer DELIVERED an analyst's routed findings (an analyzes-edge traversal), naming the lens — absent on an ordinary driver-authored steer.' In src/mcp/tools/coordination.ts, deliverRoutedFinding publishes analyst: route.kind on routed steers regardless of delivery outcome: the no-live-destination branch publishes a steer with delivered: false, outcome: 'unknown-worker' and analyst set; the authorization-failure branch sends a steer with delivered: false, outcome: 'runtime-error' and analyst set; and graph.ts:683-693 relies on event.analyst !== undefined to ledger a routed analyzes traversal as 'unpropagated' when down.delivered is false. So analyst is the routed-steer discriminator, present on delivered AND failed deliveries. The parenthetical is right; t

🟡 LOW AgentGraph listed as Undocumented in primitive-catalog — docs/api/primitive-catalog.md

The AgentGraph type is exported and central to the new runGraph() API, but lacks a TSDoc comment at its declaration line (src/runtime/supervise/graph.ts:104). The primitive catalog correctly flags it in the 'Undocumented supporting types' section. The type-level docs in runtime.md are present (properties are documented) but the top-level descriptive TSDoc is missing. Add a TSDoc comment at the interface declaration so it moves from the 'undocumented' list to a catalog table row with a summary.

🟡 LOW SteeringDirectiveData 'attachable to a graph edge' overstates current capability — docs/api/runtime.md

Prose: 'Because this is JSON-able data, it is versionable in a prompt registry and attachable to a graph edge — the same policy that used to exist only as a builder FUNCTION.' The registry-seeding part is implemented (naiveContinuationPrompt/dumbContinuationPassPrompt/dumbContinuationFailPrompt are registered by kernelPromptRegistry), but no graph code path consumes a SteeringDirectiveData: GraphEdge.directive is a PromptHandle whose resolved text is appended to the worker profile's prompt.instructions (graph.ts:500-536) and is never interpreted as a steering policy; the delegates edge carries its own maxTraversals, and nothing reads the seeded delegates/naive-continuation surfaces at graph-run time. The sentence presents design intent as current capability. Suggest qualifying as 'the poli

🟡 LOW naiveDriver/dumbDriver rendered docs lost their entire explanatory body — docs/api/runtime.md

The rendered sections for naiveDriver() (runtime.md:22762-22792) and dumbDriver() (runtime.md:22796+) now show ONLY 'Thin compatibility wrapper over steeringDriver for the X control.' plus the Deprecated block. The previous docs carried the multi-paragraph explanation of plan() semantics, the leak-free firewall ('MUST NOT read .notes or .scores'), and the naive->dumb->refine measurement axis (the headline experimental design). Root cause is in source steering-drivers.ts:166-180 and :211-228, where TWO stacked /** */ blocks sit on one declaration and the doc generator keeps only the last (the deprecation notice), discarding the descriptive block above it. Impact: a user maintaining existing callers can no longer learn from the rendered API reference what these public functions do or why the

🟡 LOW No direct test that replaySpawnTree/materializeTreeView skip the new informational kinds — src/durable/spawn-journal.ts

Producer-side tests exist (graph.test.ts:221/464/635/693 assert edge events are journaled; supervise-worker-trace.test.ts:348 asserts trace-unpropagated is appended), but no test constructs a journal containing these kinds and asserts replaySpawnTree returns the same Settled[] and materializeTreeView produces the same node set as a journal without them. A regression that, e.g., removes line 557 or 725 would not be caught. Low severity because (a) the same coverage gap exists for the older materialized/execution-bound kinds and (b) the types.ts docstrings state the skip contract explicitly. Fix: add a 10-line test mixing an edge and a `trace-unpr

🟡 LOW No test coverage for new edge/trace-unpropagated event kinds — src/durable/spawn-journal.ts

The three modified functions (outsideCursorNamespace, replaySpawnTree, materializeTreeView) have no test cases exercising the edge or trace-unpropagated event kinds. The existing test file tests/runtime/spawn-journal-replay-identity.test.ts covers spawned, settled, cancelled, materialized, and execution-bound but not the two new kinds. While these are informational/non-structural events and the risk is low, a regression test confirming they are correctly excluded from settlement output and tree node status would close the gap.

🟡 LOW Replay/materialize skip branches for edge/trace-unpropagated have no direct regression test — src/durable/spawn-journal.ts

The fix's highest-stakes branch is the replaySpawnTree skip (if (ev.kind === 'edge') continue / trace-unpropagated), because pre-fix an edge event (no outRef, no status) fell through to if (ev.outRef === undefined) throw and crashed durable resume of a graph run; materializeTreeView pre-fix would set a live spawned node to 'cancelled' for bound trace-unpropagated rows and throw 'settle/cancel with no prior spawn' for unbound graph:<node> edge ids. Coverage is only indirect: graph.test.ts and supervise-worker-trace.test.ts exercise the assertSeqUnique guard path live (counterfactually 10/24 + 1 tests fail at base), but no test feeds a mixed edge+settlement journal into replaySpawnTree or materializeTreeView. Recommendation: add a unit test that journals spawned+edge+settle

🟡 LOW Previously private generic-named helpers become public API surface — src/index.ts

padSpanId/padTraceId are short, generic identifiers now re-exported from the package root (src/index.ts:191-192). They are pure wire-normalization utilities, so the public-surface risk is low, but there is no release note or migration note flagging the expansion beyond the existing API block, and consumers importing both this package and another that happens to export padSpanId would collide. Evidence: both are defined as export function in src/otel-export.ts:757/766 with internal usage in src/mcp/trace-propagation.ts:141; no CHANGELOG entry in this PR mentions the new exports (CHANGELOG.md diff covers other items). Fix (optional): document the new exports in the API block's header comment or changelog; consider namespacing if the generic names are expected to be used widely.

🟡 LOW Analyst lens throw rejects the settle flush and drops remaining routes' findings — src/mcp/tools/coordination.ts

The new multi-route loop awaits opts.analysts.run(route.kind, trace) and bus.publish(finding) unguarded (coordination.ts:1007-1019). A throwing lens (or a bus that refuses the finding record) rejects flushPendingSettlementdrainSettlementawait_event errors AFTER the settled event was already published/committed, and every later route for that worker never publishes its finding. This is inconsistent with the file's own explicit posture that a routed finding 'must never take down the settle path' (the deliverRoutedFinding swallow-everything design, coordination.ts:905-986) and is a NEW exposure: with multiple routes, one bad lens discards the other lenses' findings for that settlement. The single-lens form pre-existed, but the PR's documented never-swallowed guarantee should

🟡 LOW Unknown-worker routed steer mints a receiptId with no matching instruction/attempt record — src/mcp/tools/coordination.ts

The no-live-destination path publishes a steer event with a fresh randomUUID() receiptId and outcome: 'unknown-worker' (coordination.ts:926-941) without ever recording the corresponding instruction receipt or delivery-attempt marker. DownMessageEvent documents that 'receiptId and instructionDigest link it to the pre-delivery authorization receipt and attempt marker' (coordination.ts:232-242); a consumer correlating steer outcomes to delivery attempts will find no match for this record. Every other failed-steer path in this file records the attempt first (attemptDelivery → recordDeliveryAttempt). Low impact (record-only, never pulled) but a contract divergence introduced by this PR. Fix: also publish a delivery-attempt (or reuse the ordinary attemptDelivery shape with a nil

🟡 LOW isLive used before its const declaration in lexical order (TDZ dependency) — src/mcp/tools/coordination.ts

liveWorkerIdNamed (line 897) calls isLive(node.status) but isLive is defined with const at line 1308 — 410 lines later. The function is only called from async paths (deliverRoutedFinding → flushPendingSettlement → drainSettlement/drainResolved), so at runtime isLive is already initialized. However, the ordering is misleading for readers and would throw ReferenceError if anyone refactors to call liveWorkerIdNamed synchronously at definition time. Move isLive before liveWorkerIdNamed or hoist it.

🟡 LOW liveWorkerIdNamed delivers nondeterministically when two live workers share a profile name — src/mcp/tools/coordination.ts

liveWorkerIdNamed returns the FIRST live node whose profileNameByWorker entry or label equals the destination (coordination.ts:896-903). The docstring asserts the profile name is 'the stable node identity a graph pins' (coordination.ts:186-194), but nothing here enforces uniqueness among concurrently live workers: two live 'researcher' workers make route.to: 'researcher' deliver to whichever appears first in scope.view.nodes order. The graph layer enforces profile.name === node.id uniqueness at graph build time (graph.ts validateGraph), but createCoordinationTools is public and bound to a caller-owned scope, so a standalone caller can hit ambiguous routing. Fix: fail closed (unknown-worker) or refuse the route when more than one live worker matches.

🟡 LOW liveWorkerIdNamed returns arbitrary worker when multiple share a profile name — src/mcp/tools/coordination.ts

liveWorkerIdNamed uses Array.find on opts.scope.view.nodes filtered by isLive. When multiple live workers share the same authored profile name (common in parallel fan-out where the same profile is spawned N times), find returns the first in node-array order. A route { kind: 'reviewer', to: 'reviewer' } in a graph with 3 live 'reviewer' workers always delivers to the first-spawned one — nondeterministic from the driver's perspective. The docstring says 'by profile name first, label second' but does not address same-name ambiguity. This could deliver findings to the wrong sibling. Consider: document this as first-match, or add a disambiguation/error when the destination matches more than one live worker.

🟡 LOW safeJsonText(undefined) returns string 'undefined' for routed delivery of no-findings — src/mcp/tools/coordination.ts

When an analyst returns undefined, canonicalFindingEvent strips the findings key from the bus event (correct), but deliverRoutedFinding passes the raw undefined to safeJsonText, which produces the literal string 'undefined' via 'String(undefined)'. The routed worker receives a steer whose instruction text contains the string 'undefined' — harmless but confusing. Could check value === undefined and return '' or a nullish representation instead.

🟡 LOW TRACEPARENT flags hardcoded to 01 (sampled), discarding parent sampling decision — src/mcp/trace-propagation.ts

traceContextToEnv writes TRACEPARENT with flags '01' (sampled). parseTraceparent extracts trace-id and span-id but discards the flags byte (TraceContext has no flags field). When a parent passes flags '00' (not sampled), the child receives '01', flipping the sampling decision downstream. This is a pre-existing type limitation (TraceContext never carried flags), but this PR introduces the TRACEPARENT write that makes it observable on the wire. Low severity because the default-sampled behavior is reasonable for a context being actively exported, and the existing tests expect '01'. Consider adding an optional flags?: string to TraceContext if sampling fidelity matters.

🟡 LOW mergeTraceEnv cross-key invariant only maintained in one direction — src/mcp/trace-propagation.ts

The docstring claims 'the ONE cross-key invariant ... TRACEPARENT and the legacy pair must name the SAME trace', but the implementation only rewrites TRACEPARENT when the override declares the legacy pair without its own TRACEPARENT (overridesIdentity at line 167). The reverse — an override declaring TRACEPARENT without a legacy pair — passes through unchanged via spread: the override's TRACEPARENT wins but the recorder's TRACE_ID/PARENT_SPAN_ID survive. A legacy-only child would join the recorder's trace while a W3C child joins the caller's. This is an unusual override shape (operators typically set TRACE_ID not TRACEPARENT), hence low severity, but the docstr

🟡 LOW parseTraceparent accepts the W3C-forbidden version ff and any non-00 version — src/mcp/trace-propagation.ts

TRACEPARENT_PATTERN accepts any two hex digits as the version ([0-9a-f]{2}). The W3C trace-context spec forbids version ff and says non-00 versions must not be interpreted. The docstring claims the parser 'fails closed: a half-formed value yields no context', but 00-... is the only currently-defined version and a ff-... header would be accepted and its ids honored as if valid. Impact is nil in practice (the extracted ids are still validated by isW3CTraceId/isW3CSpanId), so this is a nit on the strictness claim. Fix: match version 00 only, or reject ff explicitly.

🟡 LOW Doc comment overclaims backward-compat surface for dashed ids — src/otel-export.ts

The JSDoc states 'a DASHED hex id (a UUID-form id) passes through dash-stripped: that is the exact wire id every earlier release exported for it, so cross-version joins survive the strict-W3C upgrade.' Verified by execution: this holds ONLY when the dashless form has the exact W3C length (16 hex span / 32 hex trace). A real 36-char UUID used as a SPAN id (32 hex after dash-strip) does NOT pass through — isW3CSpanId rejects 32-char input, so deriveHexId runs and produces a different wire id (NEW b28357cc062ff81d vs OLD 550e8400e29b41d4). Same for short ids ('abc': NEW d27723013b3a9498 vs OLD abc0000000000000) and uppercase hex (NEW 5bbf8a75c0e1639d vs OLD ABCDEF0123456789 — though uppercase was always invalid W3C, so this is a fix). The existing test at tests/otel-export.test.ts:587 only co

🟡 LOW Non-exact-width hex ids changed from zero-padded/truncated pass-through to derived hash, breaking old exports — src/otel-export.ts

Old slice-and-pad produced VALID hex for wrong-length hex inputs (padEnd/truncate), so those wire ids joined across processes. New code derives a hash instead. Verified: old padSpanId('abcdef12') = 'abcdef1200000000' (valid hex) vs new = 'c37a590a011f2197'; old padTraceId('0123456789abcdef') = '0123456789abcdef0000000000000000' vs new = '158889066d6202d789df6a1f855b92b3'. A caller feeding short hex ids (e.g. an 8-hex-char PARENT_SPAN_ID via the legacy pair) now produces a different wire id than any older release, so mixed-version deployments stop joining for exactly these inputs. Narrow band of real inputs, but the stated 'cross-version joins survive' rationale in the comment is only true for exact-width or dashless-exact-width ids.

🟡 LOW Uppercase hex ids are now derived instead of passed through — silent behavior change vs old exports — src/otel-export.ts

isW3CSpanId/isW3CTraceId match /^[0-9a-f]{16,32}$/ (lowercase only), so an UPPERCASE 32-hex trace id (valid per the W3C grammar, which is case-insensitive) is not recognized and falls to deriveHexId, producing a different id than the old slice-and-pad pass-through (verified: uppercase span 'ABCDEF0123456789ABCDEF0123456789' → old 'ABCDEF0123456789', new 'd5d8ce8a53130238'). All internal minters produce lowercase and parseTraceparent lowercases, so this only affects external callers feeding uppercase hex via the legacy TRACE_ID/PARENT_SPAN_ID pair; those ids stop matching any prior export. Consider lowercasing before the W3C check, or document the uppercase break.

🟡 LOW Uppercase hex legacy env ids derive instead of pass through — src/otel-export.ts

If a legacy TRACE_ID or PARENT_SPAN_ID env var carries uppercase hex (e.g. AB12...CD), isW3CSpanId/isW3CTraceId rejects it (lowercase-only regex ^[0-9a-f]+$), and deriveHexId produces a completely different wire id than the old slice-and-pad which preserved the uppercase value as-is. The standard TRACEPARENT path lowercases via parseTraceparent, so this only affects the dual-write legacy bridge. Low impact: W3C spec mandates lowercase, the package's own generators use lowercase, and the legacy env vars are documented for removal next major. A trivial fix would be to lowercase before the isW3C check: if (isW3CSpanId(id.toLowerCase())) return id.toLowerCase().

🟡 LOW padSpanId/padTraceId exported but lack direct standalone unit tests — src/otel-export.ts

Both functions are now exported (public API surface via src/index.ts) and used externally by src/mcp/trace-propagation.ts:141 (traceContextToEnv), but have no direct unit tests of their own. Their behavior is tested indirectly through loopEventToOtelSpan tests (5 tests, lines 545-638 of tests/otel-export.test.ts) which verify through-pass, dash-strip, and derivation. Adding direct table-driven tests (valid-W3C, dashed-hex, human-id, empty-string, all-zero, uppercase) would improve regression detection.

🟡 LOW padSpanId/padTraceId throw on non-string input despite total contract fallback — src/otel-export.ts

deriveHexId in @tangle-network/agent-trace-contract is explicitly total ('never throws; a non-string is simply not one' for the validators, and deriveHexId coerces non-string input to empty string). padSpanId/padTraceId break that totality: id.replace(/-/g, '') runs BEFORE the deriveHexId fallback and throws TypeError on null/undefined/non-string. The TypeScript signature (id: string) protects TS callers, but the file is published from src/index.ts:191-192 and the contract package itself is written defensively for JS consumers. A null traceId reaching loopEventToOtelSpan would crash the run instead of deriving a stable id. Low likelihood (callers today pass strings), but the comment frames deriveHexId as the safety net when the shim removes it. Fix: coerce once at the top — `const s =

🟡 LOW ledger mutation via indexOf is fragile if the pending reference is absent — src/runtime/supervise/graph.ts

ledger[ledger.indexOf(pending)] = bound uses indexOf which returns -1 if not found, silently setting ledger[-1] (a string property on the array, not an index). In the current flow the reference is always present (record pushes it, hook fires before any rewrite), but a future change to the ledger lifecycle could break this invariant silently. Same pattern at line 786 for the refused-pending rewrite. Fix: guard with const idx = ledger.indexOf(pending); if (idx >= 0) ledger[idx] = bound or use a Map<EdgeTraversal, number> index.

🟡 LOW runGraph does not forward opts.backend to supervise, silently disabling trace-unpropagated journaling — src/runtime/supervise/graph.ts

runGraph always passes makeWorkerAgent: graphWorker to supervise but never passes backend: opts.backend. supervise.ts:1385 computes traceUnpropagated only from options.backend ('A caller-owned makeWorkerAgent is unclassifiable'). So even when a caller provides opts.backend with a non-propagating arm (bridge, cli-worktree, provider), the per-spawn trace-unpropagated events never journal through the graph path — the exact severed-hop observability this feature exists to surface. Fix: forward ...(opts.backend ? { backend: opts.backend } : {}) alongside makeWorkerAgent; supervise already prefers makeWorkerAgent when both are present, and traceUnpropagated is derived independently.

🟡 LOW runId derived from node ids only — graph edits collide on a shared journal — src/runtime/supervise/graph.ts

runId = graph- + 12 hex chars of canonicalCandidateDigest(graph.nodes.map(n => n.id)) — only the node-id set, ignoring edges, directives, versions, profiles. Two distinct graphs sharing a node set (e.g. same workers, different delegates directives or a new analyzes edge) yield the same runId. With the default per-call InMemorySpawnJournal this is invisible, but a caller sharing a durable FileSpawnJournal across runs hits beginTree "already begun" (spawn-journal.ts:254-259) on the second run, or — same-timestamp — silently interleaves both runs' events into one tree. Fix: include edges (directive handles + versions) in the digest.

🟡 LOW stripped ledger mislabels a verbatim re-delivery of a previously-narrowed instruction — src/runtime/supervise/graph.ts

strippedByDigest (593-605) is keyed only by canonicalCandidateDigest of the narrowed instruction, never by worker or authorization call. If the caller's authorizeMessage narrows X→Y once, then the driver later authors a steer whose text is exactly Y (delivered verbatim, not narrowed this call), the steer handler at 701 finds digest(Y) still in the map and ledgers it 'stripped' with reason "authorization narrowed N composed bytes" — a false positive for a legitimately-composed delivery. Observability mislabel only (no data loss), but it undercuts the exact stripped-attribution contract the ledger exists to provide. Fix: store the digest together with the worker id and compare both.

🟡 LOW strippedByDigest.set overwrites composedBytes when two different original instructions yield the same narrowed text — src/runtime/supervise/graph.ts

strippedByDigest.set(canonicalCandidateDigest(decision.instruction), { composedBytes: byteLength(input.instruction) }) keys by digest of the NARROWED instruction. If two different original input.instruction values — with different byte lengths — are both narrowed by authorizeMessage to the same final text (same digest), the second set() overwrites the first. The steer traversal's reason: "authorization narrowed N composed bytes" would then report the second call's composed bytes, not necessarily the ones matching that specific steer. The stripped outcome is still correct; only the reason string detail may be stale. Impact: cosmetic reporting edge case.

🟡 LOW trace-unpropagated declaration never fires for runGraph runs on no-channel backends — src/runtime/supervise/graph.ts

supervise() derives the workerTraceUnpropagated declaration only from options.backend (supervise.ts:1389-1391). runGraph builds its leaf factory via workerFromBackend internally but calls supervise without passing backend (only makeWorkerAgent: graphWorker), so traceUnpropagated is always undefined for graph runs. A runGraph on a bridge/cli-worktree backend with otel enabled therefore never journals the new trace-unpropagated severed-hop event (scope.ts:744-756), silently defeating this PR's own observability feature on the new API surface. Fix: thread the graph's backend (when makeWorkerAgent came from workerFromBackend) into the supervise call so the declaration can be derived.

🟡 LOW promptHandle regex greedily absorbs /vN in surface names, creating ambiguous parse — src/runtime/supervise/prompt-registry.ts

The regex ^(.+)\/v(\d+)$ uses greedy .+. A ref like a/b/v1/v2 parses as surface='a/b/v1', version=2. If a registry contains both { surface:'a/b', version:1 } (key=a/b/v1) and { surface:'a/b/v1', version:2 } (key=a/b/v1/v2), the parse of 'a/b/v1/v2' is ambiguous — it could mean surface a/b/v1 v2 OR surface a/b v1 with extra /v2 in the caller's ref. None of the seeded kernel surfaces trigger this, and keyOf always produces consistent keys within one registry, but a caller could create this ambiguity externally. Consider either documenting the surface-naming convention (no /vN substrings) or switching to a non-greedy (.+?) with end-anchored version extraction.

🟡 LOW trace-unpropagated journal write rejection can cascade into spawn failure — src/runtime/supervise/scope.ts

spawnCommitted = appendEvent('spawned').then(async () => { if (unpropagated && workerTrace) await appendEvent('trace-unpropagated') }). The trace-unpropagated write is chained INTO spawnCommitted, and materializationCommitted = spawnCommitted.then(...), which is passed to runChild. If the observational trace-unpropagated appendEvent rejects (e.g. file-backed journal I/O error), spawnCommitted rejects, cascading to materializationCommitted and potentially marking the child as failed — an observational side-effect coupling to spawn success. Fix: swallow the trace-unpropagated write error (it is explicitly informational) or chain it independently: void appendEvent(...).catch(() => {}) rather than awaiting it inside spawnCommitted's then. Condition is narrow (requires non-propagating backend

🟡 LOW Nested-undefined integration test does not assert canonicalized content — tests/kernel/graph.test.ts

The test asserts outcome === 'delivered' and that the journal twin exists, but never verifies that detail: undefined was actually stripped from the findings payload — the exact canonicalization the test's comment claims to validate. The unit test in coordination.test.ts:canonicalFindingEvent covers the transform directly, so this is a coverage gap in the integration path, not a false positive. Strengthening: assert the finding event's payload in the journal or ledger carries { claim: 'worker looped' } with no detail key.

🟡 LOW Outcome assertion guarded by a redundant kind check — not an independent verification — tests/kernel/graph.test.ts

expect(res.result.kind).toBe('winner') on line 208 already fails the test for any non-winner, so the if (res.result.kind === 'winner') guard on line 209 makes the out === { built: 'worker' } assertion conditional on an already-enforced fact. Harmless but weakens the outcome check's independence. Drop the guard and assert res.result.out directly.

🟡 LOW Spawn-refusal observability asserted only by absence — the test's stated claim is not what it checks — tests/kernel/graph.test.ts

The comment (line 292-293) claims the graphWorker ValidationError is 'folded back to the driver as a tool error; nothing ran, nothing traversed', but the only assertions are res.result.kind not 'winner' and res.ledger length 0. A regression that silently swallowed the spawn error (never surfacing it to the driver) would still pass: the scripted brain's second turn is { content: 'stop' }, so the run ends 'no-winner' with an empty ledger either way. The claim of loud/observable refusal is therefore unverifiable from this test. Fix: pass a seen array to scriptedBrain and assert the driver's next turn contains the refusal error text (e.g. 'not a worker n

🟡 LOW Steer-stripped byte assertion uses JS .length instead of Buffer.byteLength — tests/kernel/graph.test.ts

The test asserts expect(steers[0]!.reason).toContain(${authored.length} composed bytes) using JS string .length, but graph.ts:601 records composedBytes: byteLength(input.instruction) via Buffer.byteLength(text, 'utf8'). These are equal only because the test string ('Focus on the failing integration test; stop re-verifying.') is pure ASCII. A future edit adding non-ASCII test data would fail for the wrong reason. Fix: use Buffer.byteLength(authored, 'utf8') in the assertion template literal.

🟡 LOW leafSeam option-detection heuristic fragile to node name collision — tests/kernel/graph.test.ts

The optionsFor function distinguishes a single LeafOptions from a per-node map by checking in on four known keys ('awaitSteer', 'withTrace', 'fail', 'invalid'). A node named 'fail' or 'invalid' would trigger a false positive. No current test node collides, so this is latent — but an explicit discriminator (e.g. a wrapper { perNode: {...} } or an Array.isArray check) would be more robust.

🟡 LOW Severed-hop journaling is tested only via manual injection — the production auto-derive path is untested — tests/kernel/supervise-worker-trace.test.ts

Both new severed-hop tests inject workerTraceUnpropagated directly into createSupervisor().run() (lines 362, 386), bypassing the derive the PR added in production: supervise.ts:1389-1391 (workerTraceUnpropagatedDeclaration(options.backend.backend)) and the wiring at supervise.ts:1687 (recorder && traceUnpropagated ? { workerTraceUnpropagated }). The front-door tests (supervise({ backend, otel })) all use channel-capable backends (cli at line 507, sandbox at 428/455), so the derived severed-h

🟡 LOW Emitter branch stamping tangle.trace.unpropagated=true is never exercised — tests/mcp/trace-propagation.test.ts

The PR adds ctx.unpropagated handling AND an emitter branch (src/mcp/trace-propagation.ts:108-118) that appends {key:'tangle.trace.unpropagated', value:{boolValue:true}} to every exported span when ctx.unpropagated===true. The test file only asserts ctx.unpropagated===true on the TraceContext object (line 88), never that the emitter actually stamps the attribute. The sole emitter test ('child spans reference parent via parentSpanId', line 100) deletes OTEL_EXPORTER_OTLP_ENDPOINT so createOtelExporter() returns undefined

🟡 LOW cross-version join claim only holds for already-hex ids — tests/mcp/trace-propagation.test.ts

The comment 'Legacy-only reader (a child running the previous release of this package)' scopes the round-trip to hex ids (lines 59-76). For human ids the legacy child's export path in a previous release used slice-and-pad (not deriveHexId), so it would emit a different wire id than this release's TRACEPARENT — the cross-version join breaks exactly when the dual-write matters. The assertion itself is correct; the comment overstates compatibility. Suggest either pinning the previous release's wire id in a test or rewording the comment to 'a legacy-only reader of this release'.

🟡 LOW mergeTraceEnv override declaring only PARENT_SPAN_ID (no TRACE_ID) is untested — tests/mcp/trace-propagation.test.ts

overridesIdentity in src/mcp/trace-propagation.ts:167-170 fires when overrides.TRACE_ID !== undefined OR overrides.PARENT_SPAN_ID !== undefined (with no overrides.TRACEPARENT). The four mergeTraceEnv tests cover: legacy-pair override, no-identity override, own-TRACEPARENT override, and TRACE_ID-only override. The PARENT_SPAN_ID-only branch (caller restamps the parent span while inheriting the recorder's trace id) triggers the rewrite path using merged.TRACE_ID from the recorder plus the override's parentSpanId — a plausible real caller action — but has no test. Low impact since it shares the rewrite code path with the tested TRACE_ID+PARENT_SPAN_ID case.

🟡 LOW mergeTraceEnv override-declares-only-PARENT_SPAN_ID case untested — tests/mcp/trace-propagation.test.ts

mergeTraceEnv's overridesIdentity is true when the override declares PARENT_SPAN_ID but not TRACE_ID, in which case src/mcp/trace-propagation.ts:173-179 rebuilds TRACEPARENT from the RECORDER's TRACE_ID + the override's parent span id. The four merge tests cover caller-pair, no-identity, override-own-TRACEPARENT, and drop-when-no-parent-span, but not this partial-identity edge. It behaves as documented in the source, so this is a coverage nit, not a bug.

🟡 LOW parseTraceparent test misses all-zero span ID rejection case — tests/mcp/trace-propagation.test.ts

The test verifies all-zero trace ID rejection ('00-00000000...00000-0000000000000000-01') but not the case of a valid trace ID with an all-zero span ID (e.g. '00-a3ce929d0e0e4736a3ce929d0e0e4736-0000000000000000-01'). isW3CSpanId rejects all-zero span IDs, so this case correctly returns undefined, but the test doesn't document this expectation. Impact: minor; the behavior is correct and the contract function was independently verified.

🟡 LOW unpropagated span-stamping behavior is never asserted — tests/mcp/trace-propagation.test.ts

The test asserts the read-side flag (ctx.unpropagated === true, line 88) but never drives createPropagatingTraceEmitter to export and check the tangle.trace.unpropagated=true attribute that src/mcp/trace-propagation.ts:108-118 adds on every span. The PR's headline observability claim (a severed hop is a queryable fact) is therefore unpinned at the export boundary. Fix: set OTEL_EXPORTER_OTLP_ENDPOINT to a local listener (or inject a config-aware exporter) and assert the attribute on the emitted OtelSpan, plus the negative case when unpropagated is false.

🟡 LOW unpropagated-undefined not asserted in TRACEPARENT-wins and malformed-fallback tests — tests/mcp/trace-propagation.test.ts

The 'reads traceId and parentSpanId from env' test (line 37) asserts ctx.unpropagated is undefined, but the TRACEPARENT-wins test (line 40) and the malformed-TRACEPARENT-fails-closed test (line 50) assert only traceId/parentSpanId and omit the unpropagated check. Since the severed-hop flag is the load-bearing new field, every read path should prove it stays undefined when an inbound con


tangletools · 2026-08-02T06:18:34Z · trace

tangletools
tangletools previously approved these changes Aug 2, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 51 non-blocking findings — bcb23d74

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

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-02T06:18:34Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 8cf94bc7

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-08-02T06:28:10Z

@drewstone
drewstone merged commit eca9c24 into main Aug 2, 2026
4 checks passed
@drewstone
drewstone deleted the feat/agent-graph-694 branch August 2, 2026 06:30

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

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

💰 Value — sound

Adds a declarative agent-graph API (runGraph) over supervise() with a per-traversal edge ledger and a versioned prompt registry; it composes the existing execution core instead of replacing it and removes a prior duplicated supervisor policy.

  • What it does: Introduces three capabilities. (1) runGraph/AgentGraph (src/runtime/supervise/graph.ts:402): agent topologies as plain data — nodes are canonical AgentProfiles, edges are typed (delegates for work-down, analyzes for findings-to-anywhere), runGraph is an interpretation layer that composes supervise() (graph.ts:747) rather than a second scheduler. (2) An edge ledger: every delegates/
  • Goals it achieves: Read from the change: (a) make agent loops authorable as data so a 2-node driver↔worker topology is ~14 LOC (acceptance test at tests/kernel/graph.test.ts:166); (b) make every edge traversal observable with byte counts — the stated motivating incident was a filter silently replacing 1,700 chars of steering with 241 chars of boilerplate for three rounds with no artifact saying so, which the `stri
  • Assessment: Good change on its merits. It is greenfield in the right grain: git log on graph.ts/prompt-registry.ts before this commit is empty (no prior implementation), yet runGraph deliberately composes supervise() (graph.ts:11-13, 747) instead of forking the execution core. The edge ledger extends the existing SpawnJournal with a new informational event kind rather than a parallel observability
  • Better / existing approach: Searched runGraph|AgentGraph|PromptRegistry|EdgeTraversal (grep across src/ and tests/) and topology|graphNode patterns — all hits are introduced by this commit; nothing pre-exists. Considered whether the edge ledger could be derived from existing SpawnJournal events (spawned/down/settled) instead of a new edge kind: it cannot, because those events do not carry directive byte counts, d
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Agent graphs as typed data over supervise(), plus a versioned prompt registry that already consolidates the existing supervisor front doors and a coordination-layer route/canonicalization generalization wired into today's callers.

  • Integration: Well-wired, much of it lands on surface that runs TODAY. runGraph (src/runtime/supervise/graph.ts:402) composes supervise() (the existing execution core at src/runtime/supervise/supervise.ts:1157) — explicitly NOT a second scheduler; same makeWorkerAgent/Scope/budget/deliverable machinery. The prompt registry feeds the existing supervisor front doors: `defaultSupervisorPrompt = supervisorP
  • Fit with existing patterns: Matches the codebase grain precisely. Uses the canonical AgentProfile from @tangle-network/agent-interface (validated via agentProfileSchema at graph.ts:255), reuses AnalystRegistry/MakeWorkerAgent/CoordinationEvent from the existing coordination layer, and journals through the same SpawnJournal. Data-driven (nodes/edges are plain values), fail-loud validation runs before any compute
  • Real-world viability: Designed against a real failure (the 1,700→241 char silent-substitution incident named in the docstring) and load-bearing edge paths are covered: cyclic-graph backstop via per-edge maxTraversals with GraphEdgeCapError carrying full evidence (graph.ts:141-161, 801-805); delegates caps refuse spawns, analyzes caps stay observability-only (the audit-fixed bug); producer-side `canonicalFindingEven
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: commented out code tests/mcp/trace-propagation.test.ts

  • // export both become the SAME wire id — one trace, either convention.

🎯 Usefulness Audit

🟡 Steering-driver docstring promises registry resolution the API does not yet perform [integration] ``

src/runtime/steering-drivers.ts:62-64 documents that default continuation texts are 'seeded in the kernel prompt registry (delegates/naive-continuation, delegates/dumb-continuation-pass / -fail)'. The registry entries exist (prompt-registry.ts:202-226) and the seeded naiveContinuationPrompt/dumbContinuation*Prompt texts ARE the canonical strings, but steeringDriver still takes the continuation STRING directly (directive.continuation/onPass/onFail at steering-drivers.ts:70/77/78


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 · 20260802T063106Z

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants