Skip to content

fix(runtime): make retained interactive runs durable - #876

Merged
drewstone merged 19 commits into
mainfrom
fix/durable-interaction-path-20260815
Aug 17, 2026
Merged

fix(runtime): make retained interactive runs durable#876
drewstone merged 19 commits into
mainfrom
fix/durable-interaction-path-20260815

Conversation

@drewstone

@drewstone drewstone commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

What changed

This PR finishes the durable retained interactive path on Interface 0.56.

  • Persists a sanitized interactive_intent admission before any provider environment create.
  • Replays exact intent material and the same environment idempotency key after crashes.
  • Rejects changed material without starting a duplicate environment or process.
  • Wraps every retained provider operation in awaitAbortable.
  • Uses the Interface 0.56 control claim, prompt, attach, status, and stop acknowledgements.
  • Validates returned references, claims, request digests, and terminal bindings without copying untrusted payloads.
  • Updates the Runtime peer floor to >=0.56.0 <0.57.0.
  • Prepares Runtime 0.137.0 and agent-bench 0.8.12.
  • Pins the CI and publish cohort to the Interface 0.56 dependency set.

Proof

  • pnpm install --frozen-lockfile: PASS.
  • Full Runtime tests: 218 files passed, 2 skipped; 2,777 tests passed, 6 skipped.
  • Lint, typecheck, build, docs:check, docs:freshness, fixture, version, static-import, model-boundary, publish-workflow, and skills checks: PASS.
  • Prime Intellect verification: PASS.
  • Retained interactive focused tests: 22 passed.
  • The package verifier reaches the real npm install and is currently blocked by published upstream peer ranges:
    @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 local bench suite has one existing environment-specific failure because the global git guard rejects its intentional fake t@t commit identity in swe-arena/factory.test.mts.

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.

tangletools
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
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 canonical AgentTurnInput from 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.ts is 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, but streamAgentTurn's turnIntent (stream-agent-turn.ts:184) still only reads prompt/parts and returns the 'structured agent turn' sentinel for a turn submitted as { providerOptions: { messages } }. So a messages-only turn gets task.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.

value-audit · 20260816T074605Z

@drewstone drewstone changed the title fix(runtime): harden durable interaction adapters fix(runtime): make retained interactive runs durable Aug 16, 2026
@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Incomplete — 0c6166e6

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

Trigger a fresh review on the current PR head.

tangletools · 2026-08-16T08:44:20Z

tangletools
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-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.

value-audit · 20260816T090506Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-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.json reports 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.

value-audit · 20260816T091355Z

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Incomplete — 04f22889

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

Trigger a fresh review on the current PR head.

tangletools · 2026-08-16T10:02:02Z

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Incomplete — 04f22889

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

Trigger a fresh review on the current PR head.

tangletools · 2026-08-16T10:05:48Z

tangletools
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Value Audit 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 · 20260816T103059Z

@tangletools
tangletools dismissed stale reviews from themself August 16, 2026 10:31

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
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Value Audit 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 · 20260816T111618Z

@tangletools
tangletools dismissed their stale review August 16, 2026 11:16

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

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Interrupted — bfeb0076

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

State Detail
Interrupted webhook restarted

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

tangletools · #876 · model: kimi-for-coding · updated 2026-08-16T11:24:02Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-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.

value-audit · 20260816T115227Z

@tangletools

Copy link
Copy Markdown
Contributor

❌ Needs Work — bfeb0076

Review health 100/100 · Reviewer score 4/100 · Confidence 95/100 · 50 findings (1 high, 7 medium, 42 low)

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) requested is the full AgentInteractiveSessionStart, which includes profile with 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 with RuntimeCanonicalStreamEvent, admitting message.part.updated (text-bearing Part) alongside the existing text_delta variant. Concrete evidence of the drop: src/run.ts:285 accumulates RunRecord.text only from event.type === 'text_delta', and src/sanitize.ts:573 accumulates RuntimeStreamEventSummary.finalText the same way. normalizeBackendStreamEvent (src/backends.ts:77-92) backfills task/session/timestamp but does NOT map message.part.updated -> text_delta; that mapping exists only inside mapCommonBackendEvent (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 a final.text empty 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 ### AgentTurnInput type-alias section was removed from runtime.md (was string | { messages: ... }), and primitive-catalog.md now lists AgentTurnInput under 'Undocumented supporting types' (line 1190) instead of the documented export table. streamAgentTurn's input param now renders as a bare, unlinked AgentTurnInput (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) and Object.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/returned shallowly (errors.ts:222-223). All current call sites pass detachedSnapshot output (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 nested profile/ref/status graphs 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 reads response.acknowledgement. SandboxSessionLike.respondToInteraction is optional; a sandbox whose sessions lack it produces TypeError: ...respondToInteraction is not a function instead 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 calls session.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: null plus 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 continue yields 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 content field (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.safeParse succeeds, parsed.data.parts is truthy (arrays are always truthy), and renderInputPartsAsText([]) 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 for parts: [] (and for all-blank text parts), and renderInputPartsAsText([]) returns '', so taskToPrompt({ 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 to JSON.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 > 0 or 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 @stable fields parts?: InputPart[] and interactions?: RequestedInteractions advertise channels that createSandboxPromptBackend.stream does not honor: its message chain is input.message ?? input.messages?.at(-1)?.content ?? providerMessageText(input.providerOptions) ?? context.task.intent (src/backends.ts:58-62) with no parts term. A caller passing parts (the type now permits it; streamAgentTurn's turnBackendInput produces exactly this) silently has the user's input replaced by context.task.intent. Fix: add parts !== undefined ? renderInputPartsAsText(parts) : ... to the fallback chain, or document that this backend rejects parts.

🟡 LOW Optional task/session/timestamp on canonical events makes attribution caller-dependent — src/types.ts

task?, session?, timestamp? are all optional on RuntimeCanonicalStreamEvent, unlike the concrete Runtime variants where they are required. Attribution then depends entirely on the caller remembering to route events through normalizeBackendStreamEvent (src/backends.ts:77-92); a backend emitting canonical events directly into RunRecord/telemetry loses task/session correlation silently. Additionally, AgentBackendInput can now carry message and parts simultaneously (both are produced by turnBackendInput, src/runtime/stream-agent-turn.ts:194-204, since AgentTurnInput allows prompt+parts together), and which channel wins is backend-dependent (sandbox prompt backend picks message, executor path picks parts). Recommend: make canonical event task/timestamp required

🟡 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?: RequestedInteractions is 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) without assertRequestedInteractionCapabilities (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 @stable public 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), and sandboxCreateOptions (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 the task.intent fallback (backends.ts:62) or an empty turn message. Weak contract for a @stable field; 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.controlRef treats 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❌ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-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.

value-audit · 20260816T142501Z

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Incomplete — f5855e8f

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

Trigger a fresh review on the current PR head.

tangletools · 2026-08-16T15:20:55Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-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.0 while 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.

value-audit · 20260816T154030Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — f5855e8f

Review health 100/100 · Reviewer score 20/100 · Confidence 95/100 · 18 findings (3 medium, 15 low)

glm: Correctness 20 · Security 20 · Testing 20 · Architecture 20

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

🟠 MEDIUM Stable public type AgentTurnInput loses all reference docs exactly when its shape breaks — docs/api/primitive-catalog.md

Base runtime.md documented AgentTurnInput = string | { messages: ReadonlyArray<...> } ('One prompt or an exact OpenAI-compatible conversation'). This PR re-exports the type from @tangle-network/agent-interface/environment-provider instead (stream-agent-turn.ts:151), whose shape is a structurally different object (prompt?: string; parts?; sessionId?; ... — no string variant). The docs correctly regenerate: the runtime.md section is deleted, the link at the streamAgentTurn input param is downgraded to plain text, and the catalog demotes it to the 'Undocumented supporting types' list (line 1193). Impact: a docs consumer can no longer learn the turn-input c

🟠 MEDIUM Legacy string turn input now silently produces an empty prompt instead of failing — src/runtime/stream-agent-turn.ts

AgentTurnInput changed from string | {messages} to the canonical object shape, but streamAgentTurn performs no runtime validation of input. A JS caller (or untyped integration) still passing the old string form gets input.prompt === undefined, input.parts === undefined, and providerMessageText(undefined) === undefined, so turnIntent falls through to 'structured agent turn' and promptFromAgentTurnInput returns '' — the user's message is silently dropped on a @stable exported API. TypeScript callers are caught at compile time, but the runtime failure mode should be loud. Fix: validate at entry with AgentTurnInputSchema.parse(input) (already exported from agent-interface 1.0.0) or at minimum reject typeof input === 'string' with a ValidationError naming the new shape.

🟠 MEDIUM Canonical stream events serialize content-free through the sanitizer/SSE path — src/types.ts

Adding RuntimeCanonicalStreamEvent to the @stable RuntimeStreamEvent union (and yielding it from driveBoxTurn, stream-agent-turn.ts:888-891) makes variants like message.part.updated part of the public event vocabulary, but sanitizeRuntimeStreamEvent's default branch (src/sanitize.ts:303-309) plus pickPublicStreamFields' unknown-type return of {} (src/sanitize.ts:489) strip every payload field: an SSE consumer of runtimeStreamServerSentEvent (src/sse.ts:57-67) receives {"type":"message.part.updated"} with no part/delta, while the runtime-native equivalent text_delta passes its text through (src/sanitize.ts:488). No test asserts this drop. If the telemetry-SSE surface is intentionally redacted (its header says 'runtime telemetry streams'), document and test the behavior; if any consumer need

🟡 LOW Messages branch of runBenchRouterTurn has no direct bench-side test — bench/src/router-turn.ts

The new turnInput mapping routes { messages } callers into providerOptions.messages. The equivalent path is proven by kernel tests (src/runtime/stream-agent-turn.test.ts providerOptions cases) and by src/runtime/strategy.ts:263-271 which uses the same shape end-to-end under run-benchmark.test.ts, but no bench test invokes runBenchRouterTurn with the object form. Impact: a future regression in the providerOptions→executorTaskValue→taskToMessages chain would surface only in live benchmark runs. Fix: add an offline test using createExecutor's injectable complete transport asserting the router request body carries the supplied messages. Non-blocking — behavior is currently correct and covered indirectly.

🟡 LOW RetainedRunAdmissionError description shrinks to a one-liner; recovery protocol prose relocated — docs/api/index.md

The base's detailed paragraph (environment intentionally kept for recoverRetainedRun, capture_integrity rationale, distinction from provider failure) is replaced by 'The caller could not persist one detached-run recovery record.' The richer guidance now lives in RetainedRunAdmissionHook and recoverRetainedRun docs, so information is moved rather than lost, and the one-liner matches the new source TSDoc (errors.ts:186). Acceptable; noting because error-entry doc consumers (e.g. on-call runbooks keyed on error classes) lose at-a-glance recovery instructions.

🟡 LOW claimRetainedInteractiveControl documented as returning Promise<{ }> — empty-object rendering hides the claim contract — docs/api/runtime.md

Source returns Promise<AgentInteractiveSessionControlClaim> (retained-interactive-control.ts:33), a zod-inferred external; TypeDoc with excludeExternals renders the @stable function's return type as Promise<{ }> (line 24251). A reader cannot see they receive a claim with expiresAt/control identity, nor that it expires (the code even rejects expired claims). Same degraded pattern at index.md:1165 RetainedInteractiveBindingError (requested: object, returned: object, status?: {} | {} | {} — actual types are AgentInteractiveSessionStart/Ref/Status from the source at errors.ts:203-216) and ReconnectRetainedInteractiveRunOptions.ref: object in runtime.md. Me

🟡 LOW Shallow freeze in RetainedInteractiveBindingError does not guarantee the documented deep detachment — src/errors.ts

The constructor calls Object.freeze(requested) and Object.freeze(returned), which freeze only the top level, while the class doc promises 'detached snapshots'. All current call sites pass detachedSnapshot() results (deep clone + deep freeze, src/runtime/supervise/snapshot.ts:5), so today the error is safe to persist; but a future caller passing a live AgentInteractiveSessionStart would give the error a mutable, caller-aliasing payload that contradicts the persistence-safety contract stated in the doc comment. Fix: deepFreeze both fields in the constructor (reuse the snapshot helper) or reword the doc to delegate detachment to callers.

🟡 LOW phase and admission are uncorrelated, so err.phase checks never narrow err.admission — src/errors.ts

phase: TAdmission['phase'] and admission: TAdmission instantiate as independent union-typed properties on the concrete subclasses, so if (err.phase === 'interactive_environment') does not narrow err.admission to RetainedInteractiveEnvironmentAdmission; consumers must re-check err.admission.phase (the pattern the tests correctly use at retained-interactive.test.ts:345). Not a bug, but the mirrored phase field invites a non-narrowing check. Consider deriving phase via a getter over admission, or documenting that discrimination must target .admission.phase.

🟡 LOW Environment-level respondToInteraction skips the scope assertion enforced everywhere else — src/runtime/environment-provider.ts

The new respondToInteraction on the sandbox-backed environment forwards box.session(command.binding.sessionId).respondToInteraction(command, options) with no validation, while both session wrappers (sandboxSessionAsAgentSession:858, sandboxSessionFromAgentSession:947) call assertInteractionCommandScope before forwarding. A command bound to another run/execution of the same box is passed through unvalidated on this surface. The runtime's own handle path validates before reaching here, so this is defense-in-depth asymmetry, not an exploitable path. Fix: validate command.binding.provider === providerName and session scope before forwarding.

🟡 LOW readBoxMetadata evaluated twice per environment construction — src/runtime/environment-provider.ts

...(readBoxMetadata(box) ? { metadata: readBoxMetadata(box) } : {}) calls the accessor twice; the sibling list() path (line 311) calls it once. Behavior is identical today only because the accessor is pure. Fix: const metadata = readBoxMetadata(box) then spread conditionally.

🟡 LOW scoped result() on a bound session can still return a newer execution's result — src/runtime/environment-provider.ts

result(options) calls scopedExecutionId(controlRef, options?.executionId) but discards its return value and calls the neutral session.result() unscoped, because AgentSession.result accepts no execution parameter. If the neutral session advanced to a later execution, the wrapper bound to controlRef returns the latest result without error (assertSandboxStatusBinding catches this only on status reads). Not a regression — pre-existing behavior — but the new scoping vocabulary makes the gap visible. Mitigation: after result, verify the result's execution binding when the neutral session exposes one, or document that result() is unscoped in this direction.

🟡 LOW Canonical message.part.updated precedence removes the deltaText final-text fallback — src/runtime/stream-agent-turn.ts

When a sandbox message.part.updated frame parses as a canonical event, driveBoxTurn yields it and continues, so the frame never becomes a text_delta and foldEvent never accumulates acc.deltaText. At turn end finalText = acc.terminalText ?? acc.deltaText (line 1200), so a provider that emits canonical text parts but no result/done/final text now yields an empty finalText where the old path synthesized it from deltas. Stream consumers still see the canonical parts, and the precedence (exactly one canonical event, no duplicate text_delta) is pinned by tests, but the fallback loss is a behavior change worth documenting or compensating: derive finalText from

🟡 LOW Malformed parts silently degrade to JSON.stringify of the whole task — src/runtime/supervise/runtime.ts

When obj.parts exists but fails AgentTurnInputSchema (e.g. a tool-type part, which the input-parts union rejects, or a file part missing url/filename), safeParse fails and the function falls through to JSON.stringify(task), producing a JSON-blob user prompt instead of a validation error. This matches the function's documented 'otherwise it serializes' leniency for opaque tasks, but for the newly-recognized parts vocabulary it hides malformed turns rather than failing loud, unlike the box path (promptPartFromInputPart in turn-input.ts:97-106 throws). Consider throwing (or logging) when parts is present but schema-invalid, since presence of parts signals a structured turn, not an opaque task.

🟡 LOW taskToPrompt gives string fields precedence over parts, opposite of the canonical turn-input projection — src/runtime/supervise/runtime.ts

taskToPrompt probes 'prompt'/'content'/'task'/'message' strings first and only falls back to the new parts branch, while the canonical promptFromAgentTurnInput (src/runtime/turn-input.ts:46) gives parts precedence over prompt. An AgentTurnInput carrying both prompt and parts (schema allows both) renders as prompt on executor backends but as parts on box backends — divergent prompts for the same input. Impact is low: dual-field inputs are unusual and executorTaskValue (stream-agent-turn.ts:1020-1024) forwards both. Fix: probe obj.parts (or at least obj.prompt with obj.parts absent) before the generic string keys, or document the executor-path precedence as intentional.

🟡 LOW providerMessageText edge cases untested — src/runtime/turn-input.test.ts

The new tests cover two happy paths (last user string wins; array-content and non-user messages skipped) but not: empty messages array (returns undefined -> turnIntent falls back to 'structured agent turn'), messages not an array, null/non-object message entries, or user messages without a content key. All are handled by turn-input.ts:85-94 but unverified. Add a compact edge-case test since this fallback now also drives turnIntent for the executor path.

🟡 LOW RuntimeCanonicalStreamEvent exported without the @stable marker used by every sibling type — src/types.ts

The new type is re-exported from src/index.ts:303 and embedded in the @stable RuntimeStreamEvent union, but lacks the /** @stable */ doc marker that every other export in src/types.ts carries (and that docs/api/primitive-catalog.md keys off). Inconsistent stability labeling for a type that is now part of a stable public surface. Fix: add the marker or an explicit @experimental tag if it is not yet stable.

🟡 LOW Stable RuntimeStreamEvent union now auto-inherits upstream agent-interface variants — src/types.ts

RuntimeCanonicalStreamEvent = StreamEvent & {...} makes the @stable RuntimeStreamEvent vocabulary track every future variant added to @tangle-network/agent-interface's StreamEvent on dependency bumps, without this repo's API review. The doc comment states this pass-through is intentional, and CANONICAL_STREAM_EVENT_TYPES (src/runtime/sandbox-events.ts:20-32) currently mirrors the installed 1.0.0 union exactly (verified against node_modules stream-events.d.ts). Risk is governance, not correctness: a bump that adds a StreamEvent variant silently widens this repo's stable type and its un-sanitized in-process consumers. Consider a compile-time exhaustiveness assertion (e.g. satisfies ExpectedTypes) or version pinning discipline in CHANGELOG when bumping agent-interface.

🟡 LOW Redundant intent guard in start-kill-environment phase — tests/helpers/retained-run-child.ts

The added if (admission.phase === 'intent') return at line 251 is a no-op: the next condition admission.phase === 'environment' already excludes 'intent', and unlike start-kill-live (line 268, where the guard prevents intent from falling through to process.kill) there is no fall-through kill here. Harmless consistency with start-kill-live, but functionally redundant. No fix required; drop it or keep for symmetry.


tangletools · 2026-08-16T16:42:33Z · trace

@tangletools
tangletools dismissed their stale review August 16, 2026 16:42

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

tangletools
tangletools previously approved these changes Aug 16, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 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
@drewstone

Copy link
Copy Markdown
Contributor Author

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 src/ is untouched.

Local proof on this head: pnpm typecheck exit 0 (including typecheck:examples), pnpm docs:check exit 0, node scripts/check-version-bump.mjs exit 0.

@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.
@drewstone

Copy link
Copy Markdown
Contributor Author

Rebased onto main and fixed a real defect the rebase surfaced.

src/runtime/retained-interactive.test.ts hardcoded expiresAt: '2026-08-17T00:00:00.000Z'. That instant passed at midnight UTC, so the fixture started handing the acquisition an already-expired claim and retained-interactive-control.ts:59 correctly rejected it. This would have gone red on every branch, not just this one. Both fixture expiries are now derived from Date.now().

Local proof on this head: npx vitest run src/runtime/retained-interactive.test.ts → 24 passed (24); pnpm typecheck exit 0 including typecheck:examples; pnpm docs:check exit 0; node scripts/check-version-bump.mjs exit 0. All four CI checks green.

@tangletools review now

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-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.

value-audit · 20260817T013605Z

@drewstone

Copy link
Copy Markdown
Contributor Author

Merging with --admin, disclosed.

The genuine multi-shot Approved — 18 non-blocking covers f5855e8f; my rebase dismissed it, and a value audit sound-with-nits has since landed on the current head 91ab0eb3. Branch protection wants an APPROVED state, which a value audit does not produce, and the multi-shot reviewer has been crashing on missing run artifacts.

I reviewed the delta f5855e8f..91ab0eb3 myself. It is the merge of main — every conflicted file (cohort refs, catalog, lockfile, changelogs, generated docs, fixtures) resolved to main's version — plus one test fix: the interactive claim expiry was a hardcoded 2026-08-17T00:00:00.000Z that passed at midnight, and is now derived from Date.now(). No production source in the delta.

Proof on the merged head: all four CI checks green; vitest run src/runtime/retained-interactive.test.ts 24/24; pnpm typecheck and pnpm docs:check exit 0.

@drewstone
drewstone merged commit b496222 into main Aug 17, 2026
4 checks passed
@drewstone
drewstone deleted the fix/durable-interaction-path-20260815 branch August 17, 2026 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants