fix(runtime): make retained interactive runs durable - #876
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 2266d195
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T05:42:43Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 0c6166e6
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T06:27:27Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Coverage | 1 of 2 lenses (value) |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 707.7s (2 bridge agents) |
| Total | 707.7s |
⚠️ Partial audit — the verdict covers only value. usefulness: agent returned no usable verdict. Treat the missing lens as unexamined, not as clear.
💰 Value — sound
Migrates the turn-input surface to the canonical AgentTurnInput vocabulary and hardens every durable interaction adapter (dispatch identity, scoped cancellation, capability negotiation, replay binding) with validation-first guardrails, all in the grain of the existing codebase.
- What it does: The change replaces the ad-hoc
string | { messages }turn input with the canonicalAgentTurnInputfrom agent-interface 0.54.0 (src/runtime/stream-agent-turn.ts:147, re-exported), migrating every caller (bench, examples, strategy.ts, profile-chat-client.ts, agentic-generator.ts) to{ prompt }or{ providerOptions: { messages } }. It adds a single projection seam in src/runtime/turn-input.ts - Goals it achieves: 1) One canonical input vocabulary across the kernel, provider, and Sandbox surfaces so identity/control coordinates (turnId, sessionId, executionId, controlRef) are preserved end-to-end rather than reconstructed per-adapter. 2) Durable interactions are only dispatched to providers that prove replay + response-idempotency, so a retry cannot silently re-apply a side-effecting response. 3) Scoped can
- Assessment: Good on its merits. It is a coherent, tightly-scoped hardening that reuses existing machinery rather than reinventing it:
parseCanonicalTransportEvent(sandbox-transport-events.ts, pre-existing),sameControlCoordinates/canonicalCandidateDigest,AgentExactRunControlRefSchema,awaitAbortable, and the retained-run receipt/schema discipline are all extended in place. The validation-first, sc - Better / existing approach: No materially better architecture found, and no existing equivalent was being reinvented. I checked: the canonical event decoder already existed in sandbox-transport-events.ts and is correctly reused/wrapped, not duplicated;
interaction-capabilities.tsis genuinely new (no prior pre-dispatch interaction negotiation); the input projection is centralized into turn-input.ts and shared by both the k - Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: opencode: opencode error
🎯 Usefulness — error
usefulness agent produced no parseable value-audit JSON.
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge error: opencode/zai-coding-plan/glm-5.2: opencode: opencode error; opencode/kimi-for-coding/k2p7: opencode: opencode error
💰 Value Audit
🟡 turnIntent does not read providerOptions.messages, so intent metadata is 'structured agent turn' for messages-only turns [maintenance] ``
This PR adds
providerMessageText(turn-input.ts:82) and uses it as the message seed in backends.ts:61 and conversation-backend.ts:52, butstreamAgentTurn'sturnIntent(stream-agent-turn.ts:184) still only readsprompt/partsand returns the 'structured agent turn' sentinel for a turn submitted as{ providerOptions: { messages } }. So a messages-only turn getstask.intent = 'structured agent turn'while the backend correctly extracts the last message. The fix is trivial (fall back to
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.
…on-path-20260815 # Conflicts: # docs/api/primitive-catalog.md
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 04f22889
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T08:47:15Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 04f22889
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T08:47:23Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 3 (3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 342.1s (2 bridge agents) |
| Total | 342.1s |
💰 Value — sound-with-nits
Adds a crash-safe durable path for native interactive agent processes (intent-before-create admissions, exact replay, validated 0.56 controls) and canonicalizes AgentTurnInput from agent-interface; built squarely in the existing retained-run grain with heavy primitive reuse — ship.
- What it does: Three things. (1) New durable interactive-run lifecycle: startRetainedInteractiveRun persists a sanitized intent admission BEFORE provider.create (src/runtime/retained-interactive.ts:67-74), mints deterministic session/execution identity from the two idempotency keys, admits environment and started phases after their exact coordinates exist, and replays the same material/keys after crashes via rec
- Goals it achieves: No duplicate environment or native process after a crash between admission phases; no caller-visible interactive run whose recovery reference could be lost; changed replay material is rejected before any provider side effect. Secondary: one canonical turn-input vocabulary across the ecosystem instead of a divergent runtime-local union, and provider acknowledgements that cannot be mis-bound to anot
- Assessment: Good on its merits and firmly in the codebase's grain. The sibling retained-run architecture (retained-run-start.ts, admitDurably, mintRetainedIdentity, awaitAbortable, detachedSnapshot, exact-binding errors) is reused rather than reinvented — admitDurably was generified to serve both admission unions (src/runtime/retained-run-start.ts:348-370) and errors.ts derives both admission errors from one
- Better / existing approach: none — this is the right approach. I searched for an existing equivalent (git grep -i interactive at merge-base ecda1bd across src/: only permission-interaction code in tool-loop.ts/tui/top-app.ts; no prior startInteractive/AgentInteractiveSession usage) and considered unifying with the retained-run pipeline, but the phase sets and binding semantics differ enough (pre-create intent phase, session
- 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, in-grain durability layer for native interactive agent processes that mirrors the established retained-run family, is exported on the kernel surface, and has a published provider counterparty ready to serve it.
- Integration: Fully wired and reachable. The start/recover/reconnect triple is exported from the kernel entry (src/runtime/index.ts:374-382) via the retained-run facade (src/runtime/retained-run.ts:8-21), the two new error classes from the package root (src/index.ts:105-106), and docs/canonical-api.md:148 names it the canonical way to start a native coding-agent TUI (with direct environment.startInteractive cal
- Fit with existing patterns: Follows the established retained-run pattern precisely rather than inventing a second one: same admission-hook durability contract (admitDurably shared via retained-run-start.ts:348-360), same deterministic coordinate minting (mintRetainedIdentity, retained-run-start.ts:47-58), same binding-error taxonomy with sanitized payloads (src/errors.ts:187-215), and the interactive admission phases extend
- Real-world viability: Built for the crash cases, not the happy path: intent admission precedes provider.create so a pre-create crash replays without duplicate environments (retained-interactive.ts:67-74), changed replay material is rejected by digest before any provider work (assertExactInteractiveIntent, retained-interactive.ts:342-350), response-loss recovery reuses the exact recorded request and idempotency keys (re
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Merging widens the Interface peer floor ahead of the published ecosystem [maintenance] ``
package.json:173 now requires agent-interface >=0.56.0 <0.57.0, but published @tangle-network/agent-knowledge@8.0.2 (peer <0.55.0) and agent-profile-materialize@0.15.2 (peer <0.54.0) cannot co-install with it — the PR body confirms the package verifier is blocked at real npm resolution. This gates the 0.137.0 npm release on upstream peer-range releases (or merging to main leaves the published-package install graph broken until then). Disclosed and not bypassed, so not design-level; just a merge-
🟡 Structural mirroring between retained-interactive.ts and retained-run-start.ts [duplication] ``
reconstructEnvironment + capability assertions + create-metadata stamping (src/runtime/retained-interactive.ts:392-423, 83-102) parallel recoverRetainedRun/reconnectRetainedRun and the metadata block in src/runtime/retained-run-start.ts:399-422, 100-108 with slightly different null/outcome conventions. The shared primitives (admitDurably, mintRetainedIdentity, awaitAbortable) are already extracted, and the remaining differences are semantic, so this is acceptable — but a third lifecycle variant
🎯 Usefulness Audit
🟡 Adoption is gated on two upstream peer-range releases the PR does not control [integration] ``
Verified via npm registry: agent-knowledge@8.0.2 requires interface >=0.54.0 <0.55.0 and agent-profile-materialize@0.15.2 requires >=0.47.0 <0.54.0, while this PR floors interface at >=0.56.0 <0.57.0 (package.json peerDependencies). Until those packages publish 0.56-compatible releases, no consumer can npm-install agent-runtime 0.137.0 alongside them, so the new durable interactive path cannot actually be adopted in a fresh install. The PR handles this correctly (pins the CI/publish cohort, does
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.
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (1 medium-concern, 1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.1s |
| Interrogation | 692.3s (2 bridge agents) |
| Total | 692.4s |
💰 Value — sound-with-nits
Adds a crash-durable retained path for native interactive coding-agent TUIs (start/recover/reconnect with pre-create intent admission and exact binding validation), plus a canonical AgentTurnInput migration onto Interface 0.56 — coherent, well-tested, built on the existing retained-run machinery; sh
- What it does: Three deltas in one dependency-cohort bump. (1) New durable interactive-run lifecycle: startRetainedInteractiveRun persists a sanitized interactive_intent admission BEFORE provider.create (src/runtime/retained-interactive.ts:67-102), admits environment/started phases only after exact provider coordinates exist, validates every provider acknowledgement binds to the exact request (control claim, pro
- Goals it achieves: Give native interactive (TUI) coding-agent processes the same retry-safety the headless retained-run path already has: a crash between intent and create leaves a durable record that replays the exact material with the same idempotency keys instead of leaking or duplicating an environment; provider responses that don't bind to the requested start are rejected without copying untrusted payloads; eve
- Assessment: Good change, in the grain of the codebase. It reuses the retained-run machinery where it is genuinely shared — mintRetainedIdentity, admitDurably (generalized to both admission types in retained-run-start.ts:348-360), assertStableText/awaitAbortable from retained-run-binding, detachedSnapshot — and the error taxonomy is extended, not forked (abstract RetainedAdmissionError keeps RetainedRunAdmissi
- Better / existing approach: none — this is the right approach. Searched for prior art: no startInteractive/AgentInteractiveSession usage in src/ on origin/main; the retained-run path (retained-run-start.ts) covers headless detached turns only and the new module imports its shared helpers rather than re-implementing them. The structural rhyme between startRetainedInteractiveRun and startRetainedRun (capabilities → create-with
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 3
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A coherent, in-grain extension of the retained-run durability family to Interface 0.56 interactive TUI sessions, with one incomplete in-repo migration: seven bench scripts still pass bare strings to the new AgentTurnInput-only streamAgentTurn and CI cannot catch it.
- Integration: Wired correctly and reachable. The three new entrypoints (startRetainedInteractiveRun / recoverRetainedInteractiveRun / reconnectRetainedInteractiveRun, src/runtime/retained-interactive.ts:46,155,230) flow through the established /kernel barrel (src/runtime/retained-run.ts:8-21 -> src/runtime/index.ts:347-384) and are documented as the canonical path in docs/canonical-api.md (direct environment.st
- Fit with existing patterns: Follows the established pattern rather than competing with one: it mirrors the retained-run family exactly (same admission-phase durability protocol via admitDurably/mintRetainedIdentity from retained-run-start.ts, same awaitAbortable/freeze/binding-check helpers from retained-run-binding.ts, same start/recover/reconnect trio), extends the error taxonomy by refactoring RetainedRunAdmissionError in
- Real-world viability: Error and crash paths are the primary design surface and are well covered: intent admission is durable before provider.create (retained-interactive.ts:71), recovery replays exact material and rejects conflicts without duplicate environments (assertExactInteractiveIntent, retained-interactive.ts:342-350), every provider op is abort-wrapped, malformed provider data throws typed errors without copyin
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Durability asymmetry: headless retained runs still lack the pre-create intent admission the interactive path now has [maintenance] ``
startRetainedRun calls provider.create with no admission persisted before it — its first admission is the 'environment' phase after create returns (src/runtime/retained-run-start.ts:100-108, 272-280) — so a crash in that window still leaves an unrecorded environment on the headless path, exactly the hole this PR closes for interactive runs (src/runtime/retained-interactive.ts:67-74 persists intent first). Not a regression and arguably follow-up scope, but porting the intent-admission pattern to
🎯 Usefulness Audit
🟠 ? [integration] ``
The AgentTurnInput migration adapted only appworld.ts and router-turn.ts in bench, but seven more callers still pass bare strings: bench/src/commit0-gate.mts:333, bench/src/humaneval-repair-gate.mts:92, bench/src/quant-arena/quant-loop.mts:252, bench/src/agent-graphs-improve.mts:229, bench/src/mcp-mount-probe.mts:73, bench/src/atom-mcp-e2e.mts:106, bench/src/swe-arena/arms.ts:528. Verified:
npx tsc --noEmit -p bench/tsconfig.jsonreports TS2559 'Type string has no properties in common with Age
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.
|
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 04f22889
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T10:07:18Z
tangletools
left a comment
There was a problem hiding this comment.
⚠️ Value Audit did not run — no verdict
This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.
| Status | audit-incomplete (could not run) |
| Why | value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated) |
| Lenses answered | 0 of 2 |
| What to do | re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force |
💰 Value — error
value agent never ran: the CLI bridge refused admission (no model was started).
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 4
- Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=4/48 — no model was started
🎯 Usefulness — error
usefulness agent never ran: the CLI bridge refused admission (no model was started).
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 4
- Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=5/48 — no model was started
No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Value audit could not run (value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)). This approval was provisional on that audit running, so it is dismissed. Re-run the reviewer once the CLI bridge has capacity.
…60815' into fix/durable-interaction-path-20260815 # Conflicts: # .github/workflows/ci.yml # .github/workflows/publish.yml
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — bfeb0076
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T11:06:30Z
tangletools
left a comment
There was a problem hiding this comment.
⚠️ Value Audit did not run — no verdict
This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.
| Status | audit-incomplete (could not run) |
| Why | value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated) |
| Lenses answered | 0 of 2 |
| What to do | re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force |
💰 Value — error
value agent never ran: the CLI bridge refused admission (no model was started).
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 4
- Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=5/48 — no model was started
🎯 Usefulness — error
usefulness agent never ran: the CLI bridge refused admission (no model was started).
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 4
- Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=6/48 — no model was started
No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Value audit could not run (value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)). This approval was provisional on that audit running, so it is dismissed. Re-run the reviewer once the CLI bridge has capacity.
|
| State | Detail |
|---|---|
| Interrupted | webhook restarted |
No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.
tangletools · #876 · model: kimi-for-coding · updated 2026-08-16T11:24:02Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 5 (5 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.1s |
| Interrogation | 264.3s (2 bridge agents) |
| Total | 264.4s |
💰 Value — sound-with-nits
Adds a crash-durable retained path for native interactive coding-agent TUIs (pre-create intent admission, exact replay/recovery, provider acknowledgement binding) by extending the existing retained-run family rather than reinventing it; minor nits only.
- What it does: Three things. (1) A new retained-interactive lifecycle (src/runtime/retained-interactive.ts, -types, -handle, -lifecycle, -control): startRetainedInteractiveRun persists a sanitized digest-only intent admission BEFORE provider.create (retained-interactive.ts:72-79), then environment and started admissions as exact provider coordinates appear; recover/reconnect replay the same material and idempote
- Goals it achieves: Interactive agent processes become as crash-safe as the existing headless retained runs: a crash between environment create and the first durable record can no longer orphan an environment or duplicate one on retry, because the pre-create interactive_intent record is replayable and rejects changed material without starting a duplicate (retained-interactive.ts:346-353). Secondary goals: exactness a
- Assessment: Good on its merits and in the grain of the codebase. It extends the existing retained-run primitives instead of forking them: mintRetainedIdentity (retained-run-start.ts:47), admitDurably extended to discriminate interactive admissions (retained-run-start.ts:348-370), assertStableText/awaitAbortable (retained-run-binding.ts), detachedSnapshot, and RetainedRunProviderContractError are all reused. T
- Better / existing approach: none — this is the right approach. Searched for prior interactive capability: 'startInteractive' appears only in this branch's new files (rg over src/), and the existing headless path has no attach/terminal/control-claim surface to host a TUI, so nothing existing is reinvented. The closest alternative — folding interactive start into startRetainedRun — would couple two different provider facets an
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
Completes the retained-run durability family with an interactive-TUI sibling that mirrors the established headless admission/binding discipline, plus an Interface-canonical AgentTurnInput migration that all in-repo callers adopt in the same PR; the provider half of the contract already exists in the
- Integration: Reachable and wired. The new API (startRetainedInteractiveRun / recoverRetainedInteractiveRun / reconnectRetainedInteractiveRun / claimRetainedInteractiveControl) is exported from the public kernel surface (src/runtime/index.ts:348-386, re-exported in src/runtime/retained-run.ts:9-15) and documented as the canonical way to start a native TUI (docs/canonical-api.md:148, docs/api/runtime.md:24057+,
- Fit with existing patterns: Extends the established pattern rather than competing. The interactive path reuses the headless retained-run machinery verbatim — admitDurably, mintRetainedIdentity, assertStableText, awaitAbortable, detachedSnapshot digest binding (retained-interactive.ts:37-42) — and its errors share an abstract base with RetainedRunAdmissionError (errors.ts:167-190) so the two paths fail identically. Replacing
- Real-world viability: Built for the crash regime, not the happy path. Each admission window (intent -> environment -> started) has a recovery path and a rejection path for changed material (retained-interactive.ts:159-231, tests at retained-interactive.test.ts:100-251). Cancellation races are owner-tracked so an environment resolving after abort is destroyed, not leaked (retained-interactive-lifecycle.ts:15-42, tested
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Pre-create intent admission not back-ported to the headless retained path [maintenance] ``
The durability hole this PR fixes for interactive runs still exists for headless runs: startRetainedRun calls provider.create at retained-run-start.ts:100-108 before any durable record — its first admission is phase 'environment' inside dispatchRetainedRun (~line 272) — so a crash in that window orphans the environment exactly as the new interactive_intent phase prevents (retained-interactive.ts:73-79 comment: 'the only admission that can be written before provider.create'). The two retained fam
🟡 Abort-race and destroy-cleanup patterns duplicated within the PR [duplication] ``
createInteractiveEnvironment and startInteractiveProcess (retained-interactive-lifecycle.ts:7-84) are the same owner-state/abort-race promise template copy-pasted with different cleanup payloads, and destroyUnusedEnvironment (retained-interactive.ts:506-518) mirrors the inline destroy+AggregateError blocks in retained-run-start.ts:121-144. A shared race/dispose helper would remove ~50 lines. Small; does not gate shipping.
🟡 Breaking AgentTurnInput signature rides a minor bump gated on unpublished upstream peers [maintenance] ``
streamAgentTurn's input changes from string|{messages} to the canonical object (stream-agent-turn.ts diff); callers pinned ^0.136 break on manual upgrade, and the PR's own proof shows the packed-consumer npm install fails until agent-knowledge and agent-profile-materialize publish Interface-0.56-compatible releases (peer floor package.json:174-177). Moving the type to agent-interface is architecturally right; this is release-coordination debt, not a design flaw — the PR correctly refuses to bypa
🎯 Usefulness Audit
🟡 Interactive path has no in-repo production provider; exercised only via test fakes and the sibling SDK provider [integration] ``
sandboxClientAsProvider/sandboxInstanceAsEnvironment (src/runtime/environment-provider.ts) do not expose startInteractive, so nothing in this repo can serve startRetainedInteractiveRun end-to-end; the only implementations are the durable-retained-provider test helper and agent-provider-tangle in the sibling agent-sdk PR (tangle-environment.ts:389). This is the accepted capability-ahead-of-caller case — the caller and provider land in the same Interface 0.56 cohort — but it means the first real e
🟡 providerMessageText fallback drops the old last-user-message preference [ergonomics] ``
src/runtime/turn-input.ts:63-71 reads providerOptions.messages.at(-1).content regardless of role, while the replaced turnIntent logic scanned backwards for the last user message (stream-agent-turn.ts pre-diff). This only affects intent/fallback seeds when message/messages are absent. Note for the reviewer; align it with the old role preference if conversation seeds start drifting.
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.
❌ Needs Work —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 5 | 55 | 4 | 4 |
| Confidence | 95 | 95 | 95 | 95 |
| Correctness | 5 | 55 | 4 | 4 |
| Security | 5 | 55 | 4 | 4 |
| Testing | 5 | 55 | 4 | 4 |
| Architecture | 5 | 55 | 4 | 4 |
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 65 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 65 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 65 changed files. Global verifier still owns final merge decision.
Blocking
🔴 HIGH Committed typedoc pages are stale: claimRetainedInteractiveControl sections missing, CI docs:check will fail — docs/api/runtime.md
Evidence: at head bfeb007, running the repo's own generator (pnpm run build && npx typedoc && node scripts/gen-primitive-catalog.mjs; typedoc 0.28.20 pinned in package.json:161; output byte-identical across two runs) modifies docs/api/index.md, docs/api/runtime.md, docs/api/testing.md. The committed runtime.md is missing the '### ClaimRetainedInteractiveControlOptions' interface section (source src/runtime/retained-interactive-control.ts:12) and the '### claimRetainedInteractiveControl()' function section (source line 29), and still shows the one-arg 'SandboxClient.create(options?)' instead of 'create(options?, requestOptions?)'. Yet docs/api/primitive-catalog.md at head
Other
🟠 MEDIUM Renumbered cross-reference anchors do not resolve to intended headings under GitHub's anchor scheme — docs/api/runtime.md
The 6 anchor references renumbered in this PR still point at the wrong headings. Under GitHub's heading-slug algorithm (github-slugger: first occurrence = bare slug, duplicates get -1, -2...), the actual anchors differ from what the links say. Concrete evidence: (a) runtime.md has exactly ONE '### RootHandle' heading (line 17109) yet the 'Inherited from' link at 17247 references '#roothandle-1', which does not exist; the same link's '#signal-24' resolves to line 9998 (StreamAgentTurnOptions.signal?) while the intended heading RootHandle.signal() a
🟠 MEDIUM 'safe to persist' doc overstates what the error carries — src/errors.ts
The doc (errors.ts:197-199) claims malformed provider data is never copied into the error 'so the error remains safe to persist'. Two gaps: (1)
requestedis the full AgentInteractiveSessionStart, which includesprofilewith systemPrompt/instructions, mcp server configs, and model metadata (agent-interface environment-interactive.d.ts:531-604) — material the durable intent admission deliberately reduces to digests (retained-run-types.ts:117-123 'never carries environment variables, secrets, or provider options'). The error is only as safe to persist as the caller's profile content. (2) The parse-failure branch passes{ cause: error }(retained-interactive.ts:438) where error is a ZodError; zod embeds received primitive/enum values in issue messages, so malformed provider values can re
🟠 MEDIUM Abort of an executionId-scoped provider stream silently drops the session cancel — src/runtime/environment-provider.ts
In the cancel path, assertScopedSessionForInput(session, input) (line 590, defined line 1224) throws when input.executionId is set but the provider session does not expose a synchronous exact controlRef, e.g. the tangle sandbox wrapper created without an expected ref. The throw is caught and sets cancellationFailed=true, so session.cancel() at line 593 is never invoked. Base behavio
🟠 MEDIUM Canonical event precedence changes box-stream vocabulary, drops delta accumulation and tool-part projection — src/runtime/stream-agent-turn.ts
driveBoxTurn now does
const canonical = canonicalStreamEventFromSandboxEvent(event); if (canonical) { yield canonical; continue }before mapSandboxToolEvent/mapSandboxEvent/foldEvent. Real opencode parts carry id/sessionID/messageID, so CanonicalStreamEventSchema parses them and canonical wins. Consequences: (a) consumers of the box path stop receiving text_delta and instead receive message.part.updated — payload-shape-dependent, so the same consumer sees different vocabularies from different providers and must handle both; (b) canonical events skip foldEvent, so acc.deltaText stays empty and final.text falls back solely to the terminal frame's finalText/text/response/content (stream-agent-turn.ts:1200) — a terminal frame without a string text field now yields an empty final where deltas
🟠 MEDIUM turnBackendInput drops AgentBackendInput.messages for 'chat' backends — src/runtime/stream-agent-turn.ts
With AgentTurnInput canonicalized, multi-message conversations now arrive as input.providerOptions.messages. turnBackendInput emits message (from input.prompt), parts, interactions, and providerOptions, but never sets the AgentBackendInput.messages field (still present in types.ts:493 and read by third-party backends). An external kind:'chat' backend reading input.messages directly receives undefined and loses the full history; only task.intent (turnIntent -> providerMessageText, a single trailing-user-message fallback) survives. Built-ins (conversation-backend.ts:49-53, createProfileExecutionBackend) were updated to also read providerOptions, so this is a coordinated migration, but it is a public-surface breaking change for un-migrated chat backends that is easy to miss. Confirm this is c
🟠 MEDIUM Canonical stream-event vocabulary is silently dropped from final text aggregation — src/types.ts
RuntimeStreamEvent(marked@stable) is widened withRuntimeCanonicalStreamEvent, admittingmessage.part.updated(text-bearingPart) alongside the existingtext_deltavariant. Concrete evidence of the drop: src/run.ts:285 accumulatesRunRecord.textonly fromevent.type === 'text_delta', and src/sanitize.ts:573 accumulatesRuntimeStreamEventSummary.finalTextthe same way.normalizeBackendStreamEvent(src/backends.ts:77-92) backfills task/session/timestamp but does NOT mapmessage.part.updated->text_delta; that mapping exists only insidemapCommonBackendEvent(src/backends.ts:106), which the runAgentTaskStream path (src/run.ts:278) never calls. A backend that emits the now-legal canonical text variant produces afinal.textempty of the user's output — silent data
🟠 MEDIUM Dual text channel: canonical message.part.updated carries assistant text that text_delta-only consumers silently drop — src/types.ts
Adding RuntimeCanonicalStreamEvent to the @stable RuntimeStreamEvent union admits text-bearing events alongside the existing text_delta member: I confirmed CanonicalStreamEventSchema (agent-interface 0.56.0) accepts {type:'message.part.updated', part:{id,sessionID,messageID,type:'text',text}, delta}, and driveBoxTurn (src/runtime/stream-agent-turn.ts:888-892) yields such events via the canonical branch, skipping mapSandboxEvent's text_delta projection. Public consumers that fold only text_delta — e.g. pickPublicStreamFields (src/sanitize.ts:488) and RuntimeStreamEventSummary.finalText (src/sanitize.ts:519) — will under-report turn text whenever a provider emits schema-valid canonical parts instead of sandbox-shaped deltas. Impact is bounded (final text prefers terminal payloads per stream-
🟡 LOW Intent label for messages input now uses last message of any role instead of last user message — bench/src/router-turn.ts
Base turnIntent searched messages backward for the last role==='user' entry with string content (old stream-agent-turn.ts:176-181); head providerMessageText (src/runtime/turn-input.ts:82-92) takes messages.at(-1) regardless of role. For a trailing assistant message, task.intent in backend_start metadata now shows assistant content instead of the last user turn. Affects only the display/label field and taskDigest derivation input shape — prompts sent to the provider and usage/cost accounting are unchanged. Informational; no fix required, but task.intent values in bench traces will differ from base runs.
🟡 LOW Object-input variant changes turn intent metadata semantics — bench/src/router-turn.ts
The preserved { messages } variant is now wrapped as { providerOptions: { messages } }. Runtime turnIntent (src/runtime/stream-agent-turn.ts:188) via providerMessageText returns the LAST message's content regardless of role, whereas the old code (turnIntent) returned the last message with role==='user' or the 'structured agent turn' fallback; taskDigest (canonicalCandidateDigest) also differs. Reachable executor path is unaffected (executorTaskValue re-extracts messages → taskToMessages sees identical input), and grep confirms zero callers pass the object form today, so impact is trace metadata only. Fix if the API is kept: derive intent from providerOptions with the old role filter, or drop the variant.
🟡 LOW structuredClone of messages adds a throw path for non-JSON-cloneable message payloads — bench/src/router-turn.ts
Base passed messages by reference into the executor; head's executorTaskValue (src/runtime/stream-agent-turn.ts:1018) structuredClones them, so a message containing a function, symbol, or class instance now throws DataCloneError where base executed. The declared type Readonly<Record<string, unknown>> permits such values, but every bench caller passes JSON-shaped records (router bridge chat payloads), so this is theoretical. Also the shallow copy {...message} at router-turn.ts:127 followed by structuredClone is a redundant double copy — harmless. If hardened, validate or drop the pre-copy; not merge-blocking.
🟡 LOW New public API return types render as empty object in generated pages — docs/api/index.md
RetainedInteractiveBindingError.requested and .returned render as 'object' / '{ }' (and freshly-regenerated claimRetainedInteractiveControl() renders as 'Promise<{ }>') because the concrete types (AgentInteractiveSessionStart, AgentInteractiveSessionRef, AgentInteractiveSessionStatus, AgentInteractiveSessionControlClaim) are type-only imports from @tangle-network/agent-interface that typedoc does not inline into these pages. This matches generator output (not staleness), so it is a toolchain rendering limitation, but it leaves the documented shape of a new @stable public API opaque. Consider declaring or re-exporting these shapes in a documented module so the pages show the actual fields.
🟡 LOW RetainedRunAdmissionError entry drops recovery semantics previously documented — docs/api/index.md
Refactoring the class to extend the abstract (non-exported) RetainedAdmissionError collapses the section to 'The caller could not persist one detached-run recovery record.' The prior text told error-handlers the operationally important facts: on the 'dispatched' phase provider work may already be live, the environment is intentionally kept for recovery, and the error carries capture_integrity (code 'backend'/'capture_integrity' still in src/errors.ts:165-185). Because the abstract base is not rendered as a documented symbol, those semantics vanish from the reference page. Fix: keep the recovery/kept-environment guidance in the concrete class's rendered summary.
🟡 LOW AgentTurnInput definition dropped while still referenced; shape changed but undocumented — docs/api/runtime.md
The
### AgentTurnInputtype-alias section was removed from runtime.md (wasstring | { messages: ... }), and primitive-catalog.md now lists AgentTurnInput under 'Undocumented supporting types' (line 1190) instead of the documented export table. streamAgentTurn'sinputparam now renders as a bare, unlinkedAgentTurnInput(line 25007) with no visible definition anywhere in docs/api. The type's actual shape changed this PR: src/runtime/stream-agent-turn.ts:151 re-exports it from @tangle-network/agent-interface/environment-provider, and consume
🟡 LOW RetainedRunAdmissionHook prose describes a 'pre-create intent' phase that detached runs never produce — docs/api/runtime.md
The rewritten doc says 'The runtime blocks after the pre-create intent, environment creation, and provider work until the hook resolves.' But this hook's admission union RetainedRunAdmission has only 'environment' and 'dispatched' phases (src/runtime/retained-run-types.ts:161), and startRetainedRun persists only those two (src/runtime/retained-run-start.ts:272,327). The 'pre-create intent' statement is lifted from the interactive flow (startRetainedInteractiveRun, retained-interactive.ts:76) and is false for detached runs, which can mislead users building recovery around an intent admission that never fires. Fix: revert to the previous detached-specific wording ('after environment creation and again after dispatch') or split the prose between the two hook types.
🟡 LOW RetainedRunAdmissionHook prose names a 'pre-create intent' phase absent from detached-run union — docs/api/runtime.md
RetainedRunAdmissionHook types
RetainedRunAdmission= RetainedRunEnvironmentAdmission | RetainedRunDispatchedAdmission (line 20460) — only 'environment' and 'dispatched' phases. The updated prose at line 20478 says the runtime 'blocks after the pre-create intent, environment creation, and provider work', but 'pre-create intent' (interactive_intent) belongs to RetainedInteractiveAdmission, whose hook (RetainedInteractiveAdmissionHook, [line 20400](https://github.com/tangle-network/agent-runtime/blob/bfeb007665bbb2555fae2b505edfec83a1bc89e3/docs/
🟡 LOW Abstract RetainedAdmissionError root is not exported though docs show it as the parent type — src/errors.ts
docs/api/index.md:1057,1083 document RetainedRunAdmissionError and RetainedInteractiveAdmissionError as extending RetainedAdmissionError<...>, but that class is not exported from src/errors.ts or src/index.ts. Consumers wanting one catch for both must list both subclasses or fall back to code 'capture_integrity' plus admission-shape checks. Consistent with the file's documented pattern-match strategy, so informational; if cross-package handlers are expected, export the base (or a type guard).
🟡 LOW Interactive admission error message still says 'retained run admission' — src/errors.ts
The message template is shared by both concrete subclasses:
retained run admission (${admission.phase}) was not persisted; ${recovery}. For RetainedInteractiveAdmissionError with phase 'interactive_intent'/'interactive_environment'/'interactive_started', the wording reads 'retained run admission' even though the record is an interactive-process admission. This is cosmetic (no test asserts the string; only .code/.phase are asserted) but mislabels interactive admissions in operator logs. Fix: derive the noun from TAdmission, e.g. use 'retained admission' or branch on isInteractiveAdmission to print 'interactive admission'.
🟡 LOW Interactive errors reuse 'retained run admission' message prefix — src/errors.ts
The base message template says 'retained run admission (...) was not persisted' (errors.ts:179), which now also formats RetainedInteractiveAdmissionError instances, e.g. 'retained run admission (interactive_started) was not persisted'. The runtime introduced a dedicated interactive error class precisely to distinguish the two, but the message still says 'run'. Consumers parsing the message (or logs) cannot tell an interactive admission failure from a detached-run one. Cosmetic today since the class carries
phase, but the string contradicts the class's stated purpose. Fix: derive the noun from the admission kind ('retained run admission' vs 'retained interactive admission').
🟡 LOW No test covers admission-hook rejection at the interactive_intent phase, and error message text is unasserted — src/errors.ts
The new branch
admission.phase === 'interactive_intent' ? 'no provider work has started' : 'the environment is kept for recovery'is only reachable when the intent admission at retained-interactive.ts:76 rejects. retained-interactive.test.ts covers rejections at 'interactive_environment' (line 299) and 'interactive_started' (line 364) but never at 'interactive_intent', and no test asserts either message string. A future edit to the phase-conditional could silently regress the recovery guidance operators rely on. Add a test that rejects the hook at the intent p
🟡 LOW Shallow Object.freeze in RetainedInteractiveBindingError depends on caller-side deep freeze — src/errors.ts
Object.freeze(requested)andObject.freeze(returned)are shallow; nested profile/hook/status objects stay mutable. All current call sites pass detachedSnapshot(...) results, which the test at retained-interactive.test.ts:327-330 proves are deep-frozen, so today this is redundant defense. But direct construction (the class is exported and @stable) yields a half-frozen error, and sibling RetainedRunDispatchBindingError (errors.ts:263-264) freezes nothing, so the taxonomy is inconsistent. Either deep-freeze in the constructor or drop the freeze and document the caller-side snapshot contract.
🟡 LOW Shallow Object.freeze; safety depends on caller pre-detaching — src/errors.ts
The constructor freezes
requested/returnedshallowly (errors.ts:222-223). All current call sites passdetachedSnapshotoutput (deep clone + deep freeze, snapshot.ts:5-11), and tests assert deep-frozen (retained-interactive.test.ts:480-483), so behavior today is correct. But the error class itself does not guarantee the nestedprofile/ref/statusgraphs are immutable — a future caller passing a live object gets only a shallow freeze while the doc says 'detached snapshots'. Also inconsistent with the sibling RetainedRunDispatchBindingError (errors.ts:263-264), which does not freeze at all. Fix: deep-freeze inside the constructor (reuse the snapshot helper) so the guarantee lives at the error boundary, or drop the freeze and document the caller contract.
🟡 LOW Environment-level respondToInteraction assumes sandbox session support and throws TypeError when absent — src/runtime/environment-provider.ts
The environment adapter exposes respondToInteraction whenever hasSession(box) is true, then calls
box.session(command.binding.sessionId).respondToInteraction(command, options)and readsresponse.acknowledgement. SandboxSessionLike.respondToInteraction is optional; a sandbox whose sessions lack it producesTypeError: ...respondToInteraction is not a functioninstead of a capability error. Fix: gate the environment method on session support (or probe once) and throw a ValidationError naming the missing capability, mirroring how claimControl/sendPrompt are gated in retained-interactive-handle.ts:50.
🟡 LOW Neutral-to-sandbox result() validates executionId but cannot scope the fetch — src/runtime/environment-provider.ts
In sandboxSessionFromAgentSession.result,
scopedExecutionId(controlRef, options?.executionId)discards its return value and callssession.result()unscoped because AgentSession.result accepts only {signal}. For a provider session serving multiple executions, the returned PromptResult is whatever the provider picks — no execution binding is checked on this bridging path (the durable retained path does check via assertResultBinding). Low because the mismatch case still throws when the caller supplies a divergent executionId and the retained path is the authoritative one. Fix: document the trust boundary or validate the result's execution identity when the provider echoes one.
🟡 LOW Strict latestExecutionId/activeExecutionId status binding can false-positive on reused sandbox sessions — src/runtime/environment-provider.ts
sandbox SessionInfo defines latestExecutionId as 'most recent execution id, retained after completion'. If a sandbox session is ever reused by a later execution, every wrapper status() call for the older retained run throws ValidationError('sandbox status returned a different execution'), making the retained handle unusable. Fail-closed by design and correct while sessions are dedicated per retained run (mintRetainedIdentity guarantees that), but the check is stricter than the sandbox contract requires. Suggest validating runControlRef only, or treating latestExecutionId as advisory, to avoid a reconnect that is actually healthy being refused.
🟡 LOW readBoxMetadata is invoked twice when building environment metadata — src/runtime/environment-provider.ts
...(readBoxMetadata(box) ? { metadata: readBoxMetadata(box) } : {})calls readBoxMetadata twice; if the underlying accessor is not idempotent this could produce a different object than the guard tested. Minor: assign to a const once.
🟡 LOW Accepted control claim with already-past expiresAt is fatal instead of retriable — src/runtime/retained-interactive-control.ts
After 'accepted'/'replayed',
Date.parse(control.expiresAt) <= Date.now()throws 'provider returned an expired interactive control claim'. A provider with modest clock skew (or a lease minted with a short TTL that expires in flight) turns a successful acquisition into a hard failure even though the retry loop already has the machinery to re-CAS at the returned generation. Fix: on expiry-within-the-loop, advance to the claim's generation and retry the CAS rather than throwing, or at minimum distinguish skew from a genuinely stale lease.
🟡 LOW Post-start handle construction can leak a started process — src/runtime/retained-interactive.ts
The interactive_started admission is persisted (line 150-154), then createRetainedInteractiveRunHandle is called (line 155). assertInteractiveMethods (line 500) only checks environment.startInteractive and environment.interactive exist; it does not verify the concrete session returned by environment.interactive(ref) exposes sendPrompt. If the provider declares interactive.sendPr
🟡 LOW abortError now discards error name for Error reasons — src/runtime/retained-run-binding.ts
abortError now emits reason.message when reason is an Error (previously String(reason)), then unconditionally sets error.name = 'AbortError'. A caller-supplied AbortController.abort(new TimeoutError(...)) loses the TimeoutError name/type in favor of a generic AbortError carrying only the message. Consumers keying on error.name (e.g., to distinguish timeout from user cancel) that previously received the original name now always see 'AbortError'. Low impact given the name was already overwritten to 'AbortError' in both versions, but the message-only extraction drops any structured reason (cause/stack/extra fields) that String(reason) also did not carry — verify no caller relied on reason identity.
🟡 LOW RetainedInteractiveEnvironmentAdmission persists raw profile and initialPrompt, unlike the sanitized intent admission — src/runtime/retained-run-types.ts
The interactive_intent admission deliberately stores only digests (doc: 'never carries environment variables, secrets, or provider options'), but the interactive_environment admission stores the exact AgentInteractiveSessionStart including profile and initialPrompt. Recovery needs the exact request, so the tradeoff is necessary, but initialPrompt may carry sensitive text and the two admission records have inconsistent secret hygiene. The interactive_environment admission should be flagged in the recovery store as containing caller prompt text, or the start should be re-derived from digest-verified replay material instead of stored verbatim.
🟡 LOW A single malformed canonical sandbox frame throws and fails the entire box turn — src/runtime/sandbox-events.ts
parseCanonicalTransportEvent throws (not returns undefined) when data is not a record, when embeddedType/normalized.type conflicts with the transport type, or when a present data.normalized fails CanonicalStreamEventSchema. driveBoxTurn and environmentEventFromSandboxEvent call this on every canonical-typed frame, so one misbehaving frame aborts the whole turn/stream with backend_error instead of dropping the frame. Fail-closed is defensible for the normalized contract, but the runtime previously tolerated arbitrary backend payloads; consider bounding the failure to the frame (skip) rather than the turn for non-interaction canonical types.
🟡 LOW turnIntent label for carried conversations degrades to generic fallback — src/runtime/strategy.ts
With messages now routed through providerOptions, turnIntent (stream-agent-turn.ts:188-192) falls back to providerMessageText, which reads only the LAST message's string content. On a depth-carried conversation the last message is often an assistant message with
content: nullplus tool_calls, so task.intent becomes 'structured agent turn' rather than a content-derived label. Intent is a display/provenance label only — no behavioral or billing impact — so informational; if intent matters for trace search, derive from the last message with string content instead.
🟡 LOW Box-turn canonical precedence suppresses mapped incremental/tool events for strictly-canonical frames — src/runtime/stream-agent-turn.ts
canonicalStreamEventFromSandboxEvent(...) with
continueyields message.part.updated/status/raw/etc. instead of text_delta/reasoning_delta/tool_call/tool_result for frames that parse under the strict CanonicalStreamEventSchema (verified experimentally: real opencode parts carry extra fields and do NOT parse, so the impact is limited to exact-shape frames and normalized frames). Consumers rendering incremental text or tool activity from box turns lose those events for canonical-shaped frames. Tests assert this precedence ('emits one canonical semantic event for one %s source frame'), so it is deliberate, but it is a silent contract shift for downstream box-stream consumers and is worth a changelog note plus a follow-up check on consumers that filter text_delta.
🟡 LOW Executor brutalKill teardown is fire-and-forget once the deadline has already fired — src/runtime/stream-agent-turn.ts
The finally block wraps executor.teardown('brutalKill') in awaitAbortable with the turn's deadline signal. awaitAbortable rejects immediately when signal.aborted (deadline already fired), so on timeout/abort the teardown promise is started but never awaited. A hung subprocess may outlive the turn. This is a reasonable bound (the prior un-awaited variant could hang forever), but 'brutalKill' semantics imply the process should be reaped before the turn is reported done; consider awaiting teardown with a separate bounded wait not gated on the aborted deadline.
🟡 LOW taskToPrompt parts branch silently skips image parts carrying inline content — src/runtime/supervise/runtime.ts
InputPartSchema (portable-context-shared.ts:34-51) models the image variant as a strictObject WITHOUT a
contentfield (file parts allow content, image parts do not). An image part passed with inline base64 content therefore fails safeParse, so the entire parts array falls back to JSON.stringify(task) rather than rendering text plus attachment refs. This is a safe fallback (no data loss, no exception) but means inline-image tasks won't benefit from the new rendering path. Not blocking; document or extend if inline images are expected.
🟡 LOW taskToPrompt renders empty parts array as an empty prompt — src/runtime/supervise/runtime.ts
When a task object carries
parts: [](or parts whose text fields are all empty),AgentTurnInputSchema.safeParsesucceeds,parsed.data.partsis truthy (arrays are always truthy), andrenderInputPartsAsText([])returns ''. taskToPrompt then returns an empty string, and callers build{ role: 'user', content: '' }(runtime.ts:4972, 952) — an empty user message sent to the router instead of failing loud. Impact is limited to degenerate authored input (previously it produced the JSON blob '{"parts":[]}'); suggest returning JSON.stringify fallback or throwing when the rendered text is empty. Also note the new parts branch has no direct unit test (only indirect coverage via strategy/stream suites).
🟡 LOW taskToPrompt returns an empty prompt for a valid but empty parts array — src/runtime/supervise/runtime.ts
AgentTurnInputSchema.safeParse({ parts: obj.parts })succeeds forparts: [](and for all-blank text parts), andrenderInputPartsAsText([])returns'', sotaskToPrompt({ parts: [] })now returns an empty string where it previously returned the non-empty'{"parts":[]}'serialization. Downstream,taskToMessages(runtime.ts:4970-4973) builds{ role: 'user', content: '' }, and an empty user message sent to the Router can be rejected or produce a degenerate turn.turnIntent(stream-agent-turn.ts:188-192) has the same shape of exposure but already treats the empty render as its fallback string. Fix: guard the branch, e.g. render first and only return when the rendered text is non-empty, otherwise fall through toJSON.stringify(task). Reachability is edge-case (a caller passing
🟡 LOW taskToPrompt: empty parts array renders to empty string instead of falling through — src/runtime/supervise/runtime.ts
When obj.parts === [] (a valid but empty array), AgentTurnInputSchema.safeParse({parts: []}) succeeds and parsed.data.parts is a truthy [] (empty arrays are truthy), so renderInputPartsAsText([]) returns '' (parts.ts:169-171 trims an empty join). The task then yields an empty prompt where the prior path would have produced JSON.stringify(task). Degenerate case with no data loss, but an empty-parts task now sends an empty prompt silently. Consider guarding
parsed.data.parts.length > 0or falling through to JSON.stringify on empty.
🟡 LOW Prompt-part projection regressed error class from ValidationError to Error — src/runtime/turn-input.ts
The moved promptPartFromInputPart throws plain Error for file parts carrying inline content/path (line 97) and for missing filename/url (line 102). The pre-existing implementation in environment-provider.ts threw ValidationError for these same conditions. Any caller that specifically catches ValidationError to distinguish malformed user input from transport/provider faults will now miss these. Minor, but a silent error-typing regression on a moved function; restore ValidationError to preserve contract.
🟡 LOW Turn intent label now uses last message regardless of role — src/runtime/turn-input.ts
providerMessageText reads messages.at(-1) and returns its string content regardless of role. The replaced code (stream-agent-turn.ts old turnIntent) scanned backwards for the last message with role === 'user' and string content. For conversations ending in an assistant or tool message, task.intent now labels turns with the assistant's own text or degrades to 'structured agent turn' when content is an array. Affects only the intent label/task digest metadata, not execution. Fix: filter for the last user message before falling back.
🟡 LOW @stable union now silently grows with an externally-versioned package — src/types.ts
RuntimeStreamEvent is marked @stable and this PR embeds agent-interface's open-ended StreamEvent union in it. The dependency is pinned '>=0.56.0 <0.57.0' (package.json), so any 0.56.x release that adds a StreamEvent variant expands RuntimeStreamEvent without a runtime release — downstream exhaustive switches with a never-default break compile on a dependency bump. I grepped the repo: no in-repo exhaustive/assertNever consumers of RuntimeStreamEvent exist today, so nothing breaks now, but the stability contract should note the union is open over agent-interface's vocabulary (docs/api/index.md:12295 already reflects the openness). Fix: one doc line on RuntimeStreamEvent stating canonical members track @tangle-network/agent-interface and consumers must tolerate unknown types.
🟡 LOW AgentBackendInput.parts/interactions are ignored by createSandboxPromptBackend — src/types.ts
The new
@stablefieldsparts?: InputPart[]andinteractions?: RequestedInteractionsadvertise channels thatcreateSandboxPromptBackend.streamdoes not honor: its message chain isinput.message ?? input.messages?.at(-1)?.content ?? providerMessageText(input.providerOptions) ?? context.task.intent(src/backends.ts:58-62) with nopartsterm. A caller passingparts(the type now permits it;streamAgentTurn'sturnBackendInputproduces exactly this) silently has the user's input replaced bycontext.task.intent. Fix: addparts !== undefined ? renderInputPartsAsText(parts) : ...to the fallback chain, or document that this backend rejectsparts.
🟡 LOW Optional task/session/timestamp on canonical events makes attribution caller-dependent — src/types.ts
task?,session?,timestamp?are all optional onRuntimeCanonicalStreamEvent, unlike the concrete Runtime variants where they are required. Attribution then depends entirely on the caller remembering to route events throughnormalizeBackendStreamEvent(src/backends.ts:77-92); a backend emitting canonical events directly intoRunRecord/telemetry loses task/session correlation silently. Additionally,AgentBackendInputcan now carrymessageandpartssimultaneously (both are produced byturnBackendInput, src/runtime/stream-agent-turn.ts:194-204, sinceAgentTurnInputallowsprompt+partstogether), and which channel wins is backend-dependent (sandbox prompt backend picksmessage, executor path picksparts). Recommend: make canonical eventtask/timestamprequired
🟡 LOW RuntimeCanonicalStreamEvent missing @stable marker — src/types.ts
Every other public export in this file carries a /** @stable / JSDoc marker (AgentTaskSpec, RuntimeStreamEvent, AgentBackendInput, etc.), but the newly exported RuntimeCanonicalStreamEvent (re-exported at src/index.ts:303) only has a plain comment. This breaks the file's stated convention that the module 'owns the public shape of every cross-cutting record' and marks public types stable. Impact: documentation/API-contract consistency only, no runtime effect. Fix: add /* @stable */ above the type, or if the widening is provisional, mark it unstable explicitly so downstream callers don't assume a frozen contract.
🟡 LOW interactions forwarded ungated on the stream path — src/types.ts
interactions?: RequestedInteractionsis forwarded verbatim into provider backends on the runAgentTaskStream/streamAgentTurn path (src/runtime/turn-input.ts:60, src/runtime/stream-agent-turn.ts:199, src/runtime/environment-provider.ts:1129) withoutassertRequestedInteractionCapabilities(src/runtime/interaction-capabilities.ts:11), which is only applied on the new durable retained-run-start path (src/runtime/retained-run-start.ts:88,116,202,266). A caller on the stream path can therefore request interaction kinds the provider cannot replay or acknowledge idempotently, defeating the durability guarantee the rest of this PR hardens. Fix: gate the stream path with the same assertion.
🟡 LOW parts/interactions added to @stable AgentBackendInput with no precedence rule; createSandboxPromptBackend ignores them — src/types.ts
The stable input contract now has four text/content channels — message, messages, parts, providerOptions.messages (read via providerMessageText at src/backends.ts:61) — with no documented precedence. Concrete gap: createSandboxPromptBackend.stream (src/backends.ts:56-63) resolves input.message ?? input.messages?.at(-1)?.content ?? providerMessageText(input.providerOptions) ?? context.task.intent and its streamPrompt signature only accepts a string, so a caller that follows the new contract and supplies only parts (e.g. image/file parts, which turn-input.ts:46-47 shows are the canonical multimodal path) gets silent degradation to task.intent on this backend family — silent content loss. The sandbox path honors parts (src/runtime/turn-input.ts:45-48, promptFromAgentTurnInput). Fix: document
🟡 LOW providerOptions is an untyped bag with three ad-hoc shapes — src/types.ts
providerOptions?: Record<string, unknown>is now part of the@stablepublic contract, but consumers read three different shapes from it with no shared schema:messages(src/runtime/turn-input.ts:85 providerMessageText),backend(src/runtime/turn-input.ts:53), andsandboxCreateOptions(src/runtime/environment-provider.ts:494). A caller supplying the wrong shape is not rejected at the type or runtime boundary; it silently degrades to thetask.intentfallback (backends.ts:62) or an empty turn message. Weak contract for a@stablefield; consider a discriminated/schema-validated shape or per-backend validation.
🟡 LOW Interaction event identity is read-time-dependent across state mutations — tests/helpers/durable-retained-provider.ts
events() rebinds the stored interaction event to Object.values(nativeOperations).at(-1)?.result.controlRef at every call. The same event id therefore carries a different runId/executionId/requestDigest before vs after a native continuation commits. Within all current flows this is consistent (replay happens after continueNative advances the runtime's activeControlRef, verified by the reconnect test asserting runId === 'native-run-restart-native-operation'), but a consumer holding the original controlRef that replays the same session after a continuation has committed would hit assertCanonicalEventBinding ('provider returned an interaction for another retained execution'). Consider documenting the contract that retained interaction events are bound to the latest committed controlRef, or fil
🟡 LOW Latest native controlRef relies on object insertion order — tests/helpers/durable-retained-provider.ts
Object.values(current.nativeOperations).at(-1)?.result.controlReftreats the last-inserted key as the most recent continuation. This holds for string operationIds (insertion order is preserved) and for the current single-continuation test flow, but it is an implicit invariant: if a session ever recorded two continuations with non-monotonic operationIds (or an integer-like key, which JS sorts numerically first), the 'latest' pick would be wrong and events would rebind to a stale runId. Pre-existing pattern at lines 176 and 365 shares this. Suggest recording an explicit latestControlRef on the session, or documenting the single-continuation assump
🟡 LOW Third replication of insertion-order-dependent '.at(-1)' for latest native operation — tests/helpers/durable-retained-provider.ts
Object.values(current.nativeOperations).at(-1) assumes object key insertion order equals chronological order. This already existed in result() (line 176) and cancelRun() (line 365); the diff adds a third usage. For non-integer operation ids (all current tests use 'restart-native-operation') insertion order holds, but integer-like operation ids ('1','2','10') would enumerate in numeric order and make .at(-1) return an older operation's controlRef, silently rebinding interaction events to a stale execution.
tangletools · 2026-08-16T12:56:34Z · trace
tangletools
left a comment
There was a problem hiding this comment.
❌ 1 Blocking Finding — bfeb0076
Full multi-shot audit completed 8/8 planned shots over 65 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 65 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 65 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-16T12:56:34Z · immutable trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — f5855e8f
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T14:16:45Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 4 (4 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.1s |
| Interrogation | 459.2s (2 bridge agents) |
| Total | 459.3s |
💰 Value — sound-with-nits
Extends the codebase's proven retained-run durability model (pre-create intent admissions, idempotent replay, exact provider binding validation) to crash-safe native interactive TUI sessions on the new Interface surface, and migrates all call sites to the canonical AgentTurnInput shape — coherent, i
- What it does: Adds a durable interactive-session API: startRetainedInteractiveRun persists a sanitized 'interactive_intent' admission (digest-only, no secrets) before provider.create, then 'interactive_environment' and 'interactive_started' admissions around the process start (src/runtime/retained-interactive.ts:52-157); recoverRetainedInteractiveRun replays the exact intent/start material after a crash and rej
- Goals it achieves: A coordinator crash between 'about to create' and 'process started' can no longer lose or duplicate a provider environment or interactive process — the exact property the headless retained-run path already provides (src/runtime/retained-run-start.ts:66-75), now extended to interactive TUI agents where the start may be long-running and the response can be lost. Secondarily: one canonical turn-input
- Assessment: Good on its merits. The new ~1,100 source lines deliberately mirror the existing retained-run phase structure and reuse its machinery rather than inventing a second durability model: mintRetainedIdentity, admitDurably (extended generic over both admission unions, src/runtime/retained-run-start.ts:359-371), retainedCreateMaterial, awaitAbortable/assertStableText, detachedSnapshot, and a shared abst
- Better / existing approach: None found — this is the right approach. Searched for existing interactive/durable session capability at the merge base (git grep startInteractive/AgentInteractiveSession/reattach on ecda1bd): nothing equivalent exists; the closest prior art is the retained headless path this PR correctly extends. Unifying startRetainedRun and startRetainedInteractiveRun into one function would be forced — their p
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A durable interactive-run control layer that correctly mirrors the established retained-run pattern against the real published Interface 1.0.0 contract, with only the last-mile sandbox-provider adapter still missing.
- Integration: Fully reachable as library surface. The interactive API (startRetainedInteractiveRun / recoverRetainedInteractiveRun / reconnectRetainedInteractiveRun / claimRetainedInteractiveControl) is exported from the kernel entry (src/runtime/index.ts:347-389 via ./retained-run:8-32) and the new errors from the root (src/index.ts:105-106), with 828 lines of API docs and a CHANGELOG entry. The dependency con
- Fit with existing patterns: Excellent fit — this extends, not competes. The interactive path deliberately reuses the headless retained-run machinery: shared admitDurably parameterized over both admission types (src/runtime/retained-run-start.ts:359-381), shared mintRetainedIdentity for deterministic crash-replay coordinates, mirrored three-phase admissions (interactive_intent → interactive_environment → interactive_started)
- Real-world viability: Built for the hard paths, and the tests prove it. src/runtime/retained-interactive.test.ts (1084 lines) covers: crash-before-create intent replay, crash-after-create environment reuse, changed-replay-material rejection before any provider call, lost-start-response recovery, admission persistence failure with environment kept, deep-freeze/detach of records before the caller's hook, corrupted recove
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Intent parser and assertDigest duplicated verbatim between interactive and headless paths [duplication] ``
parseInteractiveIntent (src/runtime/retained-interactive.ts:331-363) is a near-verbatim copy of parseRetainedRunIntent (src/runtime/retained-run-start.ts:512-544) — same detachedSnapshot, same allowed-keys allowlist pattern, same field loop — and assertDigest is byte-identical in both files (retained-interactive.ts:365-369, retained-run-start.ts:546-550; confirmed by grep: exactly two copies in src/). The per-type field sets differ ('turnId' vs 'interactiveIdempotencyKey') so separate allowlists
🟡 Two near-identical abort-race scaffolds in the new lifecycle file [duplication] ``
createInteractiveEnvironment (src/runtime/retained-interactive-lifecycle.ts:7-42) and startInteractiveProcess (retained-interactive-lifecycle.ts:45-84) are ~35-line copies of the same owner-tracking abort race, differing only in the promise disposed on abort (the creation promise vs a resolved environment). One helper parameterized by an onAbort disposal callback would halve the file and make the race semantics single-sourced — important because this race is exactly the code path where a subtle
🟡 agent-interface peer range >=1.0.0 <2.0.0 is far wider than repo convention and the PR body's stated floor [maintenance] ``
package.json:176 pins '@tangle-network/agent-interface': '>=1.0.0 <2.0.0' while the PR body states '>=0.56.0 <0.57.0' and sibling peers use pinned-minor windows (agent-eval '>=0.145.21 <0.146.0'). The runtime's correctness here depends on exact interface schema shapes (agentInteractiveSessionRefMatchesStart, acknowledgement matchers in retained-interactive-handle.ts:119-189), so a <2.0.0 window trusts every future 1.x minor to preserve those shapes sight-unseen. If 1.0.0 is the republished 0.56
🎯 Usefulness Audit
🟡 No concrete provider implements the interactive AgentEnvironment surface yet — the repo's own sandbox bridge omits it [integration] ``
startRetainedInteractiveRun requires environment.startInteractive + environment.interactive plus interactiveAgent.* capability flags (src/runtime/retained-interactive.ts:455-478), but the repo's own adapters do not expose them: rg 'startInteractive' across src/ matches only the retained-interactive modules and their tests; sandboxInstanceAsEnvironment (src/runtime/environment-provider.ts) maps dispatch/session/stream but not the published SandboxInstance.interactive() handle that sandbox@0.27.1
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.
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — f5855e8f
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.
tangletools · auto-approval · reason: drewstone_author · 2026-08-16T15:27:20Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 3 (3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.1s |
| Interrogation | 287.8s (2 bridge agents) |
| Total | 287.9s |
💰 Value — sound-with-nits
Adds a crash-retry-safe durable control path for provider-retained interactive coding-agent sessions, mirroring the existing headless retained-run ledger and reusing its primitives — coherent, in-grain, and no existing equivalent to duplicate.
- What it does: Introduces startRetainedInteractiveRun / recoverRetainedInteractiveRun / reconnectRetainedInteractiveRun / claimRetainedInteractiveControl (src/runtime/retained-interactive*.ts, new in this PR). A three-phase admission ledger (interactive_intent -> interactive_environment -> interactive_started) is persisted through a required awaited onAdmission hook before/after each provider side effect, so a c
- Goals it achieves: Interactive native coding-agent runs previously had no retry-safe start: a crash between provider.create and process start could orphan an environment or duplicate a process, and nothing verified provider answers bound to the request. After merge, the exact sanitized intent is durable before any provider work, replays with changed material are rejected by digest, control-claim acquisition is an id
- Assessment: Good on its merits. It extends the proven headless retained-run pattern (same admission-ledger shape, same facade at retained-run.ts, same error taxonomy via a shared RetainedAdmissionError base in errors.ts) and reuses shared primitives (admitDurably, mintRetainedIdentity, awaitAbortable, retainedCreateMaterial, detachedSnapshot) rather than forking them. Coverage is proportionate to the headless
- Better / existing approach: none — this is the right approach. Searched origin/main for InteractiveSession/startInteractive (zero hits in src/), so no in-repo interactive capability existed to reuse; the session API is new in agent-interface and this PR is its durability layer, built on the existing retained-run primitives rather than beside them.
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A coherent, in-grain durability layer for retained interactive sessions that extends the established retained-run pattern, plus a canonical AgentTurnInput migration; only minor concerns (silent string-input degradation, widened interface peer range diverging from the repo's narrow-minor peer policy)
- Integration: Fully wired and reachable. All four new functions (startRetainedInteractiveRun, recoverRetainedInteractiveRun, reconnectRetainedInteractiveRun, claimRetainedInteractiveControl) and their types export from the public /kernel facade (src/runtime/index.ts:346-386, via the retained-run.ts:8-25 compatibility facade), and docs/canonical-api.md:148 names them THE canonical way to run a native coding-agen
- Fit with existing patterns: Excellent fit — it extends rather than competes. The interactive path reuses the retained-run machinery (admitDurably, mintRetainedIdentity, awaitAbortable, assertStableText, detachedSnapshot from retained-run-start.ts / retained-run-binding.ts), mirrors the same three-phase admission lifecycle (intent -> environment -> started), and this PR retrofits the same intent-before-create improvement onto
- Real-world viability: Built for realistic failure, not the happy path. retained-interactive-lifecycle.ts tracks explicit ownership (pending/caller/runtime) through abort races so a cancelled create destroys the environment instead of leaking it, with a bounded 30s cleanup timeout and process.emitWarning on cleanup failure. claimRetainedInteractiveControl uses compare-and-swap with deterministic operation ids (retained-
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Interactive intent validation near-duplicates the headless validator [duplication] ``
parseInteractiveIntent + assertDigest (src/runtime/retained-interactive.ts:331-369) is a field-renamed copy of parseRetainedRunIntent + assertDigest (src/runtime/retained-run-start.ts:512-550): same allowed-keys check, same per-field string validation, same digest regex. A single field-spec-driven validator shared by both admission shapes would prevent the two ledgers' validation rules from drifting. Minor: the shapes differ by one field (turnId vs interactiveIdempotencyKey) and the parallelism
🎯 Usefulness Audit
🟡 String turn inputs silently degrade instead of failing fast after the AgentTurnInput migration [ergonomics] ``
streamAgentTurn previously accepted
string; now only the object form is valid, but there is no runtime shape guard at the boundary (stream-agent-turn.ts:445-456 asserts only timeout and identity). A JS caller passing 'do the task' gets turnIntent -> 'structured agent turn' and promptFromAgentTurnInput -> '' (turn-input.ts:45-48), i.e. an empty prompt silently dispatched. TS callers are protected by types and the repo is TS-first, but a one-line ValidationError for non-object input would make t
🟡 Interface peer range widened to the whole 1.x major, diverging from the repo's narrow-minor peer policy and from the PR body's stated floor [problem-fit] ``
package.json:175 sets
@tangle-network/agent-interface: >=1.0.0 <2.0.0while every other peer stays narrow (agent-eval>=0.145.21 <0.146.0) and the PR body itself claims '>=0.56.0 <0.57.0'. The runtime deep-depends on exact acknowledgement schemas and digest functions (agentInteractiveSessionRefMatchesStart, exactAgentInteractiveSessionStart), so a future 1.x with changed acknowledgement semantics would resolve cleanly and break at runtime. This PR's own verifier blockage shows how tightly th
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.
✅ No Blockers —
|
Superseded by re-review — no blocking findings on latest commit.
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 18 non-blocking findings — f5855e8f
Full multi-shot audit completed 8/8 planned shots over 70 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-16T16:42:33Z · immutable trace
# Conflicts: # .github/workflows/ci.yml # .github/workflows/publish.yml # CHANGELOG.md # bench/CHANGELOG.md # bench/package.json # docs/api/primitive-catalog.md # docs/canonical-api.md # package.json # pnpm-lock.yaml # pnpm-workspace.yaml # src/testing/fixtures/agent-improvement-proposal.json # src/testing/fixtures/agent-profile-improvement-proposal.json
|
Rebased onto main (0.138.1 released). All twelve conflicts were release/cohort metadata that main now owns — cohort refs, catalog, lockfile, changelogs, generated docs and fixtures all taken from main; the durable-retained-run change in Local proof on this head: @tangletools review now |
A wall-clock expiry turns the suite red once that instant passes. The claim must be in the future for the acquisition to succeed, so derive it.
|
Rebased onto main and fixed a real defect the rebase surfaced.
Local proof on this head: @tangletools review now |
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 5 (1 low, 4 weak-concern) |
| Heuristic | 0.4s |
| Duplication | 0.1s |
| Interrogation | 458.0s (2 bridge agents) |
| Total | 458.5s |
💰 Value — sound-with-nits
Adds a crash-safe durable ladder (intent→environment→started admissions with exact provider-response binding) for interactive native coding-agent TUIs on Interface 0.56, extending — not duplicating — the existing retained-run machinery; coherent, in-grain, heavily tested, with only minor internal du
- What it does: Introduces startRetainedInteractiveRun/recoverRetainedInteractiveRun/reconnectRetainedInteractiveRun (src/runtime/retained-interactive.ts:52,160,235) which persist a sanitized digest-only intent before provider.create, then an exact-start-request admission after environment create, then the provider-issued session ref — so a coordinator crash before/during any provider call can be replayed idempot
- Goals it achieves: A UI-driven live agent session (tests name 'braid-ui' as the holder) survives process crashes with at-most-once provider effects and exactly-once recovery; provider responses that don't bind to the exact request fail loudly with sanitized, persistable errors instead of silently controlling the wrong process (RetainedInteractiveBindingError carries no untrusted payload, errors.ts); aborted/deadline
- Assessment: Good on its merits. It extends the established retained-run admission pattern rather than forking it — admitDurably was made generic over both admission families (retained-run-start.ts:359-381), identities/material/digests come from shared modules, and the file layout follows the repo's documented facade-plus-implementation-modules grain (retained-run.ts:1-6). Every provider operation is abort-wra
- Better / existing approach: none — this is the right approach. I searched for an existing interactive-durability implementation (git grep 'startInteractive' on main: zero hits; no interactive session control existed anywhere in src/ on main) and for alternative homes for this logic (retained-run-start.ts handles the headless dispatch ladder with genuinely different record shapes — turnId vs interactiveIdempotencyKey, embedde
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A coherent, well-tested durability state machine for retained interactive runs that extends the existing retained-run pattern, plus an in-repo-complete migration to the canonical Interface AgentTurnInput; the interactive primitives are the runtime half of a two-sided provider contract whose provider
- Integration: Fully wired at the package boundary: startRetainedInteractiveRun/recoverRetainedInteractiveRun/reconnectRetainedInteractiveRun/claimRetainedInteractiveControl plus all admission types and two new error classes are exported from src/runtime/index.ts:345-390 and re-exported at the package root (src/index.ts:105-106, src/errors.ts). The parts of the diff with immediate callers are real and complete:
- Fit with existing patterns: Strong fit — it extends an established pattern rather than competing with one. The headless sibling startRetainedRun (shipped on main in #827, src/runtime/retained-run-start.ts:74-141) already uses the same onAdmission-hook + idempotency-key + recover/reconnect shape, and this PR mirrors it for interactive processes (retained-interactive.ts:52-157) while extending the headless path with the same p
- Real-world viability: Built for the non-happy path, not just the happy one. Crash-recovery tests exist at every admission phase (retained-interactive.test.ts:100, 169, 253: crash before create, crash after create, lost start response), replay-conflict material is rejected (retained-interactive.test.ts:218-247), malformed provider data is schema-rejected without being copied into errors and without destroying the enviro
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/runtime/retained-interactive.test.ts
expiresAt: new Date(Date.now() + 60 * 60 * 1000).toISOString(),
💰 Value Audit
🟡 Three pairs of near-identical twin functions across the headless and interactive ladders [duplication] ``
assertDigest is byte-identical in retained-interactive.ts:365-369 and retained-run-start.ts:546-550; parseInteractiveIntent (retained-interactive.ts:331-363) and parseRetainedRunIntent (retained-run-start.ts:512-544) are structurally parallel allow-list validators differing only in field names; interactiveIntent (retained-interactive.ts:280-308) mirrors retainedRunIntent (retained-run-start.ts:473-500). Additionally retained-interactive-lifecycle.ts contains two copies of the same promise-race-w
🟡 Interactive API ships with no in-repo production consumer [maintenance] ``
startRetainedInteractiveRun/claimRetainedInteractiveControl are exported from src/runtime/index.ts:349-390 and documented (docs/api/runtime.md:24248+, primitive-catalog.md:568,738) but referenced only by tests (holderId 'braid-ui' in retained-interactive.test.ts:84 indicates an external UI consumer). For a published contract library this is normal — the export is the product — but it means the first real exercise of the sandbox adapter's interactive surface will happen downstream (the sandboxCli
🎯 Usefulness Audit
🟡 Interactive primitives have no provider that can serve them in this repo yet [integration] ``
startRetainedInteractiveRun requires provider.get plus interactive capabilities (retained-interactive.ts:61-63, 455-473), but the Sandbox adapters expose none of it: grep for startInteractive/interactive/interactiveAgent in src/runtime/environment-provider.ts returns zero matches, so the only implementing provider is the test fixture (retained-interactive.test.ts) and tests/helpers/durable-retained-provider.ts. This is ahead-of-caller surface for an external/native Interface provider and matches
🟡 Release bookkeeping in the PR body is stale relative to the merged branch [robustness] ``
The PR body describes Runtime 0.137.0 with an Interface peer floor '>=0.56.0 <0.57.0' and an npm-install blocker on upstream peer ranges, but HEAD after the merge with main is 0.138.1 with the catalog resolved to agent-interface 1.0.0 and agent-knowledge 8.0.6 (package.json:172-176, pnpm-workspace.yaml). The code itself is unaffected; re-verify the publish cohort and the blocker claim against the merged state before release so the npm-publish step is not skipped or mis-pinned based on stale numb
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.
|
Merging with The genuine multi-shot Approved — 18 non-blocking covers I reviewed the delta Proof on the merged head: all four CI checks green; |
What changed
This PR finishes the durable retained interactive path on Interface 0.56.
Proof
@tangle-network/agent-knowledge@8.0.2 requires Interface >=0.54.0 <0.55.0.
@tangle-network/agent-profile-materialize@0.15.2 requires Interface >=0.47.0 <0.54.0.
No compatible published releases exist yet. This PR does not bypass npm peer resolution.
The Runtime code and local proof are ready. Merge and npm release require the two upstream peer-range releases, or their source fixes to be published first.