Skip to content

[sprites 5-2] Checkpoint before destructive agent operations - #2025

Merged
2witstudios merged 4 commits into
masterfrom
pu/sprites-5-2-checkpoints
Jul 12, 2026
Merged

2witstudios merged 4 commits into
masterfrom
pu/sprites-5-2-checkpoints

Conversation

@2witstudios

@2witstudios 2witstudios commented Jul 12, 2026 •

Copy link
Copy Markdown
Owner

Summary

Checkpoints are ~300ms copy-on-write filesystem snapshots designed exactly for "unattended agent experiments" and destructive changes (docs.sprites.dev/concepts/checkpoints) — and we used them nowhere, while sprites.ts's own spawnWithSelfHealingCwd doc worries an agent that rm -rfs /workspace would otherwise brick the sandbox. This checkpoints before each agent bash batch, fail-open, at most once per agent turn.

Restore is explicitly out of scope (future epic, manual/admin action only) — this leaf only ever creates checkpoints.

Requirements → how satisfied

  • Given an agent bash tool batch about to execute (flag on), should create a checkpoint tagged with a recognizable comment, at most once per agent turn.
    checkpoint-policy.ts's pure shouldCheckpoint({flagEnabled, turnId, lastCheckpointTurnId}) — same turnId as the last checkpoint → skip; a different turnId → always checkpoint. tool-runners.ts's maybeCheckpointBeforeBatch calls it before runCommand in runBashInSandbox, tagging with checkpointComment(turnId) → pagespace-pre-agent-<turnId>. turnId is lazily stamped once per streamText run onto ToolExecutionContext (same mutate-in-place pattern as the existing activeMachine field) and threaded into SandboxActorContext.turnId. Concurrent tool calls in the same turn (the AI SDK can dispatch several via Promise.all) are coalesced onto one checkpoint attempt via coalesceCheckpointAttempt — see "Review fixes" below.

  • Given checkpoint API failure, should proceed with the batch (fail-open) and log — never block agent work on checkpoint availability.
    maybeCheckpointBeforeBatch wraps the whole decision+call in try/catch; any failure (including a bounded timeout — see below) calls safeLogWarn and returns normally. A failed checkpoint is not recorded, so a later batch in the same turn gets another attempt.

  • Given repeated tool batches within one turn, should not create additional checkpoints (pure throttle).
    shouldCheckpoint's turnId === lastCheckpointTurnId comparison — a pure per-turn dedup, made race-safe under concurrent dispatch by coalesceCheckpointAttempt.

  • Given the checkpoint list growing, should rely on platform auto-pruning; do not build a custom reaper.
    No reaper of the platform's checkpoint list is built — see Findings below. (The in-process throttle bookkeeping map does get an opportunistic eviction sweep — a different, purely local concern, see Review fixes.)

Review fixes

Two rounds of review — the automated Codex reviewer plus an internal 8-angle multi-agent code-review pass — surfaced real issues, all fixed:

  1. P2 (chatgpt-codex-connector): cross-turn interval throttle could suppress a new turn's checkpoint. discussion — The original shouldCheckpoint also gated on 30s elapsed since the last checkpoint, regardless of turn, so two legitimate turns close together would leave only the OLDER turn's restore point on record. Fixed (2a1a3eba3): removed the interval gate — a different turnId is by definition never-before-checkpointed, so it always checkpoints now.

  2. P2 (chatgpt-codex-connector): a stalled checkpoint stream could hang the batch forever. discussion — The SDK exposes no timeout for createCheckpoint/its stream, so an await on a hung connection never reached the catch, holding the concurrency/billing slot until the outer request timed out — the opposite of "fail-open." Fixed (e56ab7b95): bounded with CHECKPOINT_TIMEOUT_MS (10s) at both the shell layer (tool-runners.ts, bounds the promise regardless of the injected implementation) and the driver layer (sprites.ts, also best-effort stream.close()s on timeout to release the connection).

  3. (internal review, 2 independent finder agents) Concurrent-turn race in "at most once per turn." The AI SDK can execute multiple tool calls from one agent step concurrently; two bash calls in the same turn could both pass shouldCheckpoint's synchronous check before either recorded, producing two checkpoints for one turn. Fixed (e56ab7b95): coalesceCheckpointAttempt registers an in-flight promise synchronously so concurrent callers for the same sandbox share one attempt regardless of exact timing.

  4. (internal review, 2 independent finder agents) MachineHandle.createCheckpoint was optional purely for a hypothetical future non-Sprite backend that doesn't exist. Premature abstraction per project convention. Fixed (e56ab7b95): made it required (matching ExecutableSandbox.createCheckpoint); removed the runtime Promise.reject fallback in machine-host-adapter.ts.

  5. (internal review) Dead CheckpointState field / duplicated branches. A repo-wide grep confirmed nothing read lastCheckpointAt for the throttle decision anymore after fix Upload files, agents, dm's, more #1 — re-justified by repurposing it for an opportunistic eviction sweep (below) rather than deleting it outright. Also de-duplicated the two near-identical return branches of createResolveSandboxActorContext in sandbox-tools-runtime.ts (9 of 10 fields were byte-identical) into one parallel fetch + shared base object, preserving the original findDrive/findUser/getActorInfo concurrency.

  6. (internal review) In-process checkpoint state map had no bound. stateBySandboxId had no symmetric acquire/release and would grow by one entry per distinct sandbox ever seen for the life of the process. Fixed: added an opportunistic 24h-TTL eviction sweep, mirroring quota.ts's machineActivityByKey pattern.

Findings (named-checkpoint accumulation vs. auto-pruning)

I could not exercise a live Sprite in this environment, so I read the SDK (@fly/sprites sprite.d.ts/checkpoint.d.ts) and the docs instead of hitting the API directly. What I found:

  • The docs describe automatic background checkpoints tagged with auto- ids as "pruned over time" and explicitly call them "a safety net rather than a retention strategy."
  • Our checkpoints are created via the SDK's sprite.createCheckpoint(comment) — the same explicit, user-facing creation path as sprite checkpoint create --comment "..." from the CLI, not the platform's own automatic background mechanism. The docs don't say whether explicitly-created checkpoints are also subject to automatic pruning, or whether they accumulate until a human/admin prunes them.
  • Given that ambiguity, this leaf does not assume auto-pruning applies to our checkpoints — it just doesn't build a reaper of the platform's list (as instructed) and flags this for a reviewer/operator to confirm against the live API (e.g. GET /v1/sprites/{name}/checkpoints after some days of agent activity) before this ships broadly. If explicit checkpoints do NOT get pruned, a lightweight follow-up (e.g. keep only the last N pagespace-pre-agent-* checkpoints) would be worth scoping as a separate leaf.

Other implementation notes

  • Feature flag: isCheckpointBeforeAgentBatchEnabled() — explicit SANDBOX_CHECKPOINT_BEFORE_AGENT_BATCH=true|false always wins; unset defaults ON outside production, OFF in production. The leaf spec explicitly deferred the production default to PR discussion — please weigh in.
  • SDK plumbing: SpriteInstanceLike.createCheckpoint(comment?) mirrors the real SDK's Sprite.createCheckpoint (returns a CheckpointStream); wrap() drains it via processAll, bounded by CHECKPOINT_TIMEOUT_MS, surfacing the first error-type message as a rejection (pure checkpointStreamErrorMessage helper, unit tested).
  • State: in-process Map<sandboxId, {lastCheckpointAt, lastCheckpointTurnId}> + a coalescing Map<sandboxId, Promise<void>> for in-flight attempts (both mirror quota.ts's in-process-Map conventions). A process restart just re-checkpoints on the next batch, which is harmless (COW, ~300ms).
  • package.json: added the ./services/sandbox/checkpoint-policy subpath export (needed it after hitting the "new subpath modules need an exports entry" gotcha from a prior leaf).

Test evidence

packages/lib: 1140/1140 sandbox+machines-dir tests passed
  (bunx vitest run src/services/sandbox src/services/machines)
apps/web:     1077/1078 passed (1 unrelated pre-existing failure —
  activity-tools.test.ts, local DB-role infra issue, predates this branch)
Full monorepo typecheck (lib, db, web, realtime): clean
@pagespace/lib lint: clean (0 warnings/errors)

Checklist for the orchestrator

  • Decide the production default for SANDBOX_CHECKPOINT_BEFORE_AGENT_BATCH (currently OFF-by-default in prod pending this discussion).
  • Verify against the live Sprites API whether explicitly-created (non-auto-) checkpoints are pruned automatically, or need a follow-up reaper leaf.
  • Confirm scoping bash-only (not writeFile/editFile) is correct — I limited the checkpoint hook to runBashInSandbox since that's the explicitly named "batch about to execute" surface and the highest-risk one (arbitrary shell); file-write tools already go through path-escape checks and single-file diffs. (An internal review pass also flagged this exact question independently — noted here for visibility, not acted on without a scope decision.)

🤖 Generated with Claude Code

https://claude.ai/code/session_018LbMMKiXdkHrcxdtLsZH4Y

Summary by CodeRabbit

  • New Features

    • Added automatic filesystem checkpoints before agent bash batches.
    • Checkpoints are limited to once per agent turn and tagged for easier identification.
    • Added checkpoint support across sandbox integrations.
  • Reliability

    • Checkpoint operations now have a time limit and handle failures without blocking command execution.
    • Concurrent checkpoint requests are consolidated to prevent duplicate checkpoints.
  • Configuration

    • Checkpointing can be enabled or disabled through the sandbox configuration.

Checkpoints are ~300ms copy-on-write filesystem snapshots designed
exactly for unattended agent experiments (docs.sprites.dev/concepts/
checkpoints), and we used them nowhere — while sprites.ts's own
spawnWithSelfHealingCwd doc worried an agent that `rm -rf`s /workspace
would otherwise brick the sandbox.

- checkpoint-policy.ts: pure shouldCheckpoint({flagEnabled,
  lastCheckpointAt, turnId, lastCheckpointTurnId, now}) — at most once
  per agent turn, plus a rapid-batch throttle safety net. Also the
  checkpoint-comment formatter, the flag resolver (default ON outside
  production, OFF in prod pending PR discussion), and in-process
  per-sandbox bookkeeping (mirrors quota.ts's machineActivityByKey —
  no persistence, no reaper).
- sprites.ts: SpriteInstanceLike.createCheckpoint (matches the real
  @fly/sprites SDK's createCheckpoint(comment?) -> CheckpointStream),
  drained via wrap()'s ExecutableSandbox.createCheckpoint.
- machine-host.ts / sprite-machine-host.ts / machine-host-adapter.ts:
  plumb createCheckpoint through MachineHandle (optional — a future
  non-Sprite backend need not support it) so the agent-bash path,
  which goes through MachineHost, gets a real ExecutableSandbox.
- tool-runners.ts: SandboxCheckpointDeps (fully optional seam) +
  maybeCheckpointBeforeBatch, called before runCommand in
  runBashInSandbox. Fail-open: any failure is logged and swallowed,
  never blocks the batch; a failed checkpoint is not recorded, so a
  later batch in the same turn gets another attempt.
- apps/web wiring: turnId is lazily stamped once per streamText run
  onto ToolExecutionContext (same mutate-in-place pattern as
  activeMachine) and threaded into SandboxActorContext.turnId;
  buildRealSandboxRunDeps wires the checkpoint dep to the real SDK
  call + in-process state.

Restore stays a manual/admin action (future epic) — this leaf only
ever creates checkpoints, never restores one. Terminal PTY path
checkpointing can follow once this proves out.

Test evidence: 680 sandbox unit tests green in packages/lib, 2284
apps/web unit tests green (one unrelated pre-existing failure:
activity-tools.test.ts's DB role, and one unrelated pre-existing
ClickHouse container OOM in analytics-gdpr.integration.test.ts — both
predate this branch). Full monorepo typecheck clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018LbMMKiXdkHrcxdtLsZH4Y
@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@2witstudios, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 03dc6651-5aab-4c48-a9f5-1d4d11669a0f

📥 Commits

Reviewing files that changed from the base of the PR and between e56ab7b and 3781c17.

📒 Files selected for processing (1)
  • packages/lib/src/services/sandbox/sandbox-client/__tests__/sprites.test.ts
📝 Walkthrough

Walkthrough

Adds a Sprite-backed checkpoint API, turn-based checkpoint policy and coalescing, fail-open pre-batch checkpoints for sandbox bash execution, and lazy propagation of stable agent turn IDs.

Changes

Sandbox checkpointing

Layer / File(s) Summary
Checkpoint API and Sprite implementation
packages/lib/src/services/sandbox/machine-host.ts, packages/lib/src/services/sandbox/sandbox-client/*, packages/lib/src/services/sandbox/__tests__/*
Sandbox handles and adapters expose createCheckpoint; Sprite checkpoint streams are drained, errors surfaced, and operations bounded by a timeout.
Checkpoint policy and state
packages/lib/src/services/sandbox/checkpoint-policy.ts, packages/lib/package.json, packages/lib/src/services/sandbox/__tests__/checkpoint-policy.test.ts
Environment flags, per-turn decisions, per-sandbox state, TTL eviction, and concurrent-attempt coalescing are implemented and tested.
Pre-batch bash checkpoint flow
packages/lib/src/services/sandbox/tool-runners.ts, packages/lib/src/services/sandbox/__tests__/tool-runners.test.ts
Bash execution optionally creates a checkpoint before commands, skips repeated turns, coalesces concurrent calls, and continues when checkpointing fails or times out.
Turn identity and runtime wiring
apps/web/src/lib/ai/core/types.ts, apps/web/src/lib/ai/tools/sandbox-tools-runtime.ts, apps/web/src/lib/ai/tools/__tests__/*
Tool contexts lazily receive stable turn IDs, resolved actor contexts carry them, and production sandbox dependencies provide checkpoint operations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AgentTool
  participant resolveSandboxActorContext
  participant runBashInSandbox
  participant ExecutableSandbox
  participant SpriteCheckpointStream
  AgentTool->>resolveSandboxActorContext: resolve shared tool context
  resolveSandboxActorContext-->>AgentTool: stable turnId
  AgentTool->>runBashInSandbox: run bash batch with turnId
  runBashInSandbox->>ExecutableSandbox: createCheckpoint(comment)
  ExecutableSandbox->>SpriteCheckpointStream: drain checkpoint stream
  SpriteCheckpointStream-->>ExecutableSandbox: completion or error
  ExecutableSandbox-->>runBashInSandbox: checkpoint result
  runBashInSandbox->>runBashInSandbox: execute bash batch
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check ❓ Inconclusive The linked issue text is too vague to verify whether the checkpoint changes satisfy its requirements. Provide the actual acceptance criteria or a clearer linked issue description so compliance can be checked.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is specific and matches the main change: adding checkpoints before destructive agent operations.
Out of Scope Changes check ✅ Passed The diff is tightly focused on sandbox checkpoint plumbing, tests, and type updates with no obvious unrelated changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pu/sprites-5-2-checkpoints

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bdb12b72c7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +105 to +106
if (lastCheckpointAt !== null && now.getTime() - lastCheckpointAt.getTime() < CHECKPOINT_MIN_INTERVAL_MS) {
return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Checkpoint new turns even inside the interval

With the default CHECKPOINT_MIN_INTERVAL_MS of 30s, this skips checkpointing whenever a different turn starts soon after the previous one. Separate chat turns can easily happen within that window, so if the second turn runs destructive bash, the newest restore point is still from before the previous turn and a restore would discard any legitimate work done in between. The interval throttle should not suppress the first checkpoint for a new turn.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed — good catch. Fixed in 2a1a3eb: removed the CHECKPOINT_MIN_INTERVAL_MS cross-turn gate entirely. shouldCheckpoint is now purely flagEnabled && turnId !== lastCheckpointTurnId — a new turnId always checkpoints regardless of how recently a different turn's checkpoint was taken, since the policy only ever remembers the single most-recent turn and a differing id is by definition never-before-checkpointed.

Added a regression test (both at the pure-policy level and through the full runBashInSandbox path) that pins two different turns checkpointing at the identical instant — the most adversarial case for a time-based throttle. Left the doc comment on shouldCheckpoint explaining why a cross-turn interval gate is unsafe here, so it doesn't get reintroduced by a future "let's rate-limit this" change without solving the underlying tension.

Leaving this thread open for you to verify the fix rather than resolving it myself.

… a new turn's checkpoint

Addresses a P2 review finding from chatgpt-codex-connector on PR #2025.

shouldCheckpoint's CHECKPOINT_MIN_INTERVAL_MS gate suppressed a NEW
turn's first checkpoint if it fell within 30s of a prior, DIFFERENT
turn's checkpoint. Since the policy only ever remembers the single
most-recent turnId, a turnId that differs from it is by definition a
turn that has never been checkpointed — so throttling it on elapsed
time silently defeated the safety net: two legitimate turns close
together (an ordinary rapid back-and-forth) would leave only the
OLDER turn's restore point on record, and a restore after the newer
turn's destructive work would discard the newer turn's real work too.

Removed the interval gate entirely (and the now-dead envInt/
CHECKPOINT_MIN_INTERVAL_MS/lastCheckpointAt/now plumbing that only
existed to support it). shouldCheckpoint is now exactly what the
leaf's actual requirement needs: at most once per agent turn via a
pure turnId comparison, with a documented warning against
reintroducing a cross-turn interval gate without solving this.

Added a regression test at both the pure-policy level and the full
runBashInSandbox integration level, asserting a new turn checkpoints
even when `now` is identical to the prior (different) turn's
checkpoint — the most adversarial case for a time-based throttle.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018LbMMKiXdkHrcxdtLsZH4Y
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a1a3eba3a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

) {
return;
}
await checkpoint.createCheckpoint({ sandbox, comment: checkpointComment(turnId) });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bound checkpoint creation so fail-open cannot hang

When checkpointing is enabled and the Sprites checkpoint call returns a stream that stalls instead of rejecting, this await never reaches the catch, so the bash command is never started and the quota/billing slot stays held until the outer request times out. Since the new policy is meant to be fail-open, wrap checkpoint creation/draining in a short timeout (or otherwise abort it) and proceed on timeout just like other checkpoint failures.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed, and good catch — fixed in e56ab7b.

Bounded checkpoint.createCheckpoint(...) with a CHECKPOINT_TIMEOUT_MS (10s) race at the shell layer (maybeCheckpointBeforeBatch, this file) so the promise from the caller's perspective is bounded regardless of what deps.checkpoint happens to be wired to — a timeout is caught by the existing try/catch and treated exactly like any other checkpoint failure (logged, swallowed, batch proceeds). Also bounded it at the driver layer (sprites.ts's createCheckpoint), since the SDK exposes no timeout/abort of its own for either the initial call or the stream it returns; on timeout there it also best-effort calls stream.close() so a stalled read doesn't hold the underlying connection open indefinitely in the background after we've given up waiting on it.

While digging into this I also found (via an internal multi-agent review pass) a related real bug: the check-then-act on the in-process per-turn state isn't atomic, so two bash tool calls dispatched concurrently by the AI SDK in one agent step could both pass the "already checkpointed this turn" check before either recorded — producing two checkpoints for one turn. Fixed that too, in the same commit, with a coalesceCheckpointAttempt helper that registers an in-flight promise synchronously so concurrent callers for the same sandbox share one attempt.

Added regression tests for both (fake-timer timeout tests in sprites.test.ts/tool-runners.test.ts, and a concurrent-dispatch test for the race).

Addresses a P2 finding from chatgpt-codex-connector plus a multi-agent
code-review pass on PR #2025 (8 independent finder angles, several
converging on the same issues from different directions).

Critical fixes:
- Bound the checkpoint SDK call so it can never hang the batch.
  maybeCheckpointBeforeBatch's fail-open contract ("never block agent
  work on checkpoint availability") was violated by an unbounded
  await: a stalled checkpoint stream held the bash command's
  concurrency/billing slot until the outer request timed out. Added
  CHECKPOINT_TIMEOUT_MS (10s) at the shell layer (tool-runners.ts,
  bounds the promise regardless of the injected implementation) AND
  at the driver layer (sprites.ts, also best-effort closes the
  stalled stream to release its underlying connection — SDK exposes
  no timeout/abort of its own).
- Fix a real concurrency race in "at most once per turn". The AI SDK
  can execute multiple tool calls from one agent step concurrently
  (Promise.all); two bash calls in the same turn could both pass
  shouldCheckpoint's synchronous check before either recorded,
  producing two checkpoints for one turn. Added
  coalesceCheckpointAttempt (checkpoint-policy.ts): concurrent
  attempts for the same sandbox share one in-flight promise,
  registered synchronously so the race window closes regardless of
  timing between callers.

Simplifications (2 independent review agents converged on the same
finding for MachineHandle.createCheckpoint):
- MachineHandle.createCheckpoint is now required, not optional — it
  existed as optional only to guard a hypothetical future non-Sprite
  backend that doesn't exist; the only implementer (sprite-machine-host)
  always supplies it. Removed the runtime Promise.reject fallback in
  machine-host-adapter.ts.
- De-duplicated the two return branches of
  createResolveSandboxActorContext (9 of 10 fields were identical)
  into one parallel fetch + shared base object, preserving the
  original findDrive/findUser/getActorInfo concurrency.
- Added an opportunistic eviction sweep to the in-process checkpoint
  state map (mirrors quota.ts's machineActivityByKey pattern) so it
  doesn't grow unbounded for the life of the process.

New/updated tests: checkpoint-policy.test.ts (coalescing, eviction),
tool-runners.test.ts (timeout fail-open, concurrent-turn race),
sprites.test.ts (stream timeout + close() on hang),
machine-host-adapter.test.ts (createCheckpoint delegation), plus
createCheckpoint stubs added to every MachineHandle test fixture now
that the field is required.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018LbMMKiXdkHrcxdtLsZH4Y
@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e56ab7b95d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const drain = async (): Promise<void> => {
stream = await sprite.createCheckpoint(comment);
let streamError: string | undefined;
await stream.processAll((message) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Drain checkpoint streams with the Node SDK API

When this runs against the real @fly/sprites Node SDK, the checkpoint stream returned by sprite.createCheckpoint(...) is an async iterable; the official Node example drains it with for await (const msg of stream) on https://sprites.dev/api/sprites/checkpoints, while processAll is shown for the Go client. In environments where checkpointing is enabled, this call will throw TypeError: stream.processAll is not a function, the fail-open wrapper will swallow it, and no pre-agent checkpoint will ever be created or recorded before destructive bash runs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This one I'm going to push back on — I believe it's factually incorrect for this repo's pinned SDK version, though I appreciate the scrutiny given how consequential it would be if true.

Both apps/web/package.json and apps/realtime/package.json pin @fly/sprites to the EXACT version 0.0.1-rc37 (no range), and that's what's actually installed (confirmed via bun.lock's hash). I read the real, compiled, installed source directly rather than relying on the docs site:

node_modules/@fly/sprites/dist/checkpoint.d.ts:

export declare class CheckpointStream {
    next(): Promise<StreamMessage | null>;
    processAll(handler: (msg: StreamMessage) => void | Promise<void>): Promise<void>;
    close(): void;
    [Symbol.asyncIterator](): AsyncIterableIterator<StreamMessage>;
}

node_modules/@fly/sprites/dist/checkpoint.js (the actual runtime implementation, not just the type declaration):

async processAll(handler) {
    try {
        let msg;
        while ((msg = await this.next()) !== null) {
            await handler(msg);
        }
    }
    finally {
        this.close();
    }
}

So processAll is a real, implemented method on CheckpointStream in the Node SDK we actually ship — it will NOT throw TypeError: stream.processAll is not a function. The class supports BOTH consumption styles: for await (const msg of stream) via [Symbol.asyncIterator], and the processAll(handler) convenience method I'm using — both drive the same internal next() loop. The docs page's Node example apparently just didn't happen to show the processAll variant, but that doesn't mean it's Go-only or absent from the Node client.

One thing this DID surface, though: processAll's own finally block already calls this.close() on completion or error — so my own stream.close() call in sprites.ts's timeout branch is redundant on the happy/error path, but still meaningful for the actual timeout case: if next() is mid-await this.reader.read() when our outer timer fires, calling close() from outside cancels that pending read via reader.cancel(), which is the real unstick mechanism. So that part of the fix stands as designed.

If I'm wrong about the pinned version or missed something, happy to be corrected — but I wanted to show my work with the actual installed source rather than either blindly applying the suggestion or dismissing it without evidence.

@2witstudios

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 12, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/lib/src/services/sandbox/sandbox-client/__tests__/sprites.test.ts`:
- Around line 1128-1140: Rename the test case around createCheckpoint in
sprites.test.ts to state that it propagates the SDK rejection and lets the
caller decide the fail-open policy. Keep the existing rejecting assertion and
test behavior 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

Run ID: 2f69a318-ece0-4d35-9275-59d0ca9d33b7

📥 Commits

Reviewing files that changed from the base of the PR and between bb72a70 and e56ab7b.

📒 Files selected for processing (26)
  • apps/web/src/lib/ai/core/types.ts
  • apps/web/src/lib/ai/tools/__tests__/sandbox-tools-runtime.test.ts
  • apps/web/src/lib/ai/tools/__tests__/sandbox-tools.test.ts
  • apps/web/src/lib/ai/tools/sandbox-tools-runtime.ts
  • packages/lib/package.json
  • packages/lib/src/services/machines/__tests__/agent-terminals.test.ts
  • packages/lib/src/services/machines/__tests__/machine-branches.test.ts
  • packages/lib/src/services/machines/__tests__/machine-projects.test.ts
  • packages/lib/src/services/sandbox/__tests__/checkpoint-policy.test.ts
  • packages/lib/src/services/sandbox/__tests__/git-tool-runners.test.ts
  • packages/lib/src/services/sandbox/__tests__/machine-diff.test.ts
  • packages/lib/src/services/sandbox/__tests__/machine-fs.test.ts
  • packages/lib/src/services/sandbox/__tests__/machine-git-blob.test.ts
  • packages/lib/src/services/sandbox/__tests__/persistent-machine-fs.test.ts
  • packages/lib/src/services/sandbox/__tests__/tool-runners.test.ts
  • packages/lib/src/services/sandbox/checkpoint-policy.ts
  • packages/lib/src/services/sandbox/machine-host.ts
  • packages/lib/src/services/sandbox/sandbox-client/__tests__/machine-host-adapter.test.ts
  • packages/lib/src/services/sandbox/sandbox-client/__tests__/sprite-machine-host.test.ts
  • packages/lib/src/services/sandbox/sandbox-client/__tests__/sprites.test.ts
  • packages/lib/src/services/sandbox/sandbox-client/__tests__/wake-retry.test.ts
  • packages/lib/src/services/sandbox/sandbox-client/machine-host-adapter.ts
  • packages/lib/src/services/sandbox/sandbox-client/sprite-machine-host.ts
  • packages/lib/src/services/sandbox/sandbox-client/sprites.ts
  • packages/lib/src/services/sandbox/sandbox-client/types.ts
  • packages/lib/src/services/sandbox/tool-runners.ts

Comment thread packages/lib/src/services/sandbox/sandbox-client/__tests__/sprites.test.ts Outdated
Addresses a CodeRabbit review nit on PR #2025: "resolves cleanly when
the SDK call itself rejects" asserted .rejects.toThrow(...) — the
promise rejects, it does not resolve. Renamed to "propagates the
SDK's rejection (caller decides fail-open policy)".

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018LbMMKiXdkHrcxdtLsZH4Y
@2witstudios
2witstudios merged commit 7133fb6 into master Jul 12, 2026
10 checks passed
@2witstudios
2witstudios deleted the pu/sprites-5-2-checkpoints branch July 12, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant