[sprites 1-5] Apply egress lockdown on fresh create only - #2014
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a60a4a54eb
ℹ️ 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".
The Sprite network policy lives at /.sprite/policy/network.json — a persistent
file that survives pause/hibernation (docs.sprites.dev/concepts/networking).
getOrCreate nonetheless re-pushed it, plus a SANDBOX_ROOT mkdir exec, on EVERY
hand-back: every terminal connect, every tab-back, and every 60s re-auth tick
paid a control-plane round-trip plus an exec for a policy that was already in
place.
The lockdown is now applied exactly when it is not already known-good: a fresh
create (a new Sprite starts on the platform's open outbound), a hash mismatch
(the desired policy changed), or an unknown recorded state (fail closed). The
decision is a pure function — shouldApplyPolicy({fresh, appliedPolicyHash,
desiredPolicyHash}) over a canonical hashPolicy — and the shell records the
confirmed hash on the terminal_sessions row.
The crash window the old unconditional re-apply defended (a crash between
createSprite and its lockdown leaving an open-egress Sprite reachable) is closed
by ORDERING instead: the session row is written only after getOrCreate resolves,
so an unlocked Sprite is never linked to a session. A lockdown failure on fresh
create still destroys the Sprite and rejects the hand-back.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
Addresses the Codex P2 on #2014: `applyEgressLockdown` was also the only thing that recreated SANDBOX_ROOT, so applying it on fresh create only left a warm reconnect exposed if a sandbox command had deleted /workspace — the PTY passes cwd to createSession, the server chdirs into it before spawning, and the session open fails outright. Fixed where the directory is actually consumed rather than by buying the mkdir back on every hand-back (the exec this PR exists to remove): `spawnWithSelfHealingCwd` (pure, in the driver) wraps command+args in an `sh` that recreates and enters the cwd, then execs the real command — preserving the PTY, signals and exit code, with cwd/command/args as positional data args so the no-injection invariant of the arg-array form holds. This is the shape `runCommand` already used; it is now the single definition, shared by the batch path, MachineHost.stream, and the realtime PTY. Also rebased onto master's Terminal→Machine rename (#2012): machine-session-manager, MachineSessionStore/Record, surface 'machine', and the migration renumbered to 0202 on machine_sessions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
a60a4a5 to
2e708d5
Compare
Second half of the same gap the PTY fix closed. The Sprite fs API does not create parent directories, so `writeFiles` also depended on the egress lockdown's per-hand-back `mkdir` to guarantee SANDBOX_ROOT existed. With the lockdown now fresh-create-only, an agent that had `rm -rf`'d /workspace would fail every subsequent file write. Folded into the exec the fs path was already paying for: `fsWithWakeRetry`'s recovery exec now takes the directories the op needs, so the single exec between a failed op and its retry BOTH wakes the VM (the reason it exists — the fs API is a bare fetch that cannot wake a hibernated Sprite) and `mkdir -p`s the parents. Zero extra round-trips on the happy path; the read path is unchanged (`sh -c :`). New pure functions + tests: `parentDir` (POSIX by construction — these are paths inside the Linux VM, so node:path would apply Win32 semantics on a Windows host) and `fsRecoveryExec`. Directories are positional data args, never interpolated into the script. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54f149239a
ℹ️ 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".
…the policy Addresses the Codex P1 on #2014: a policy-hash-only record could not tell one VM from its replacement, which opened a containment hole on the vanish-and-recreate path. Given a session row whose hash matches the desired policy but whose Sprite has vanished, two concurrent getOrCreate calls race: A creates the replacement (which starts on the platform's default OPEN egress) and has not yet reached its lockdown; B then finds that new Sprite by name, sees fresh === false, and — since the recorded hash still describes the DESTROYED Sprite's policy — skipped the push and handed back an unlocked VM. The record is now a LOCKDOWN TOKEN over (Sprite instance id, policy hash): proof that a specific policy was applied to a specific VM. The SDK hydrates `id` from the API response on both getSprite and createSprite, so reading it costs nothing. A replacement VM has a different id, so B's token does not match and B locks it down itself (both callers pushing the same policy is harmless; one skipping it is not). An SDK that reports no id yields NO token — unprovable, so never recorded as proof, and the next hand-back re-applies. Fail closed throughout. The driver now returns the token it confirmed and the shell persists that, rather than the shell computing what it hoped for: machine_sessions.egressPolicyToken (migration 0202), threaded through SandboxHandle/MachineHandle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Resolves the conflict with [sprites 1-5] (#2014), the sibling leaf that also touches sprites-shell.ts. The only textual conflict was the test file's import block — both imports are needed, so both are kept. The source merge is semantically clean but worth stating, since both PRs edit `launchFreshSession`: #2014 wraps the fresh create in `spawnWithSelfHealingCwd` (recreating a deleted /workspace, so the egress lockdown's per-hand-back mkdir can go), while this PR resets `seenTail` there (a brand-new shell shares no history with the one the client was watching, so its banner must not be mistaken for a replay and suppressed). The two are orthogonal and both survive. Verified on the merged tree: apps/realtime 699/699 tests, typecheck 16/16. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CTUvWzi9AkWgDQcKA8DwRH
1 participant
Why
The Sprite egress policy lives at
/.sprite/policy/network.json— a persistent file whose "changes reload live" (docs.sprites.dev/concepts/networking). It survives pause/hibernation.getOrCreatenonetheless re-pushed it (updateNetworkPolicy) plus amkdir -p /workspaceexec on every hand-back — every machine connect, every tab-back, and every 60s re-auth tick — paying a control-plane round-trip plus an exec, on the connect critical path, for a policy that was already in place.get()already skipped this ("the policy persists across hibernation");getOrCreateis now consistent with it.What is recorded: a lockdown token, not a policy hash
The skip is only safe if we can prove this VM is running this policy. A policy hash alone answers only the second half, and the two come apart on the vanish-and-recreate path (see the P1): given a session row whose hash matches but whose Sprite has vanished, two concurrent
getOrCreatecalls race — A creates the replacement (which starts on the platform's default open egress) and has not yet reached its lockdown; B then finds that new Sprite by name, seesfresh === false, matches the hash of the destroyed Sprite, and skips the push.So the record is a lockdown token over
(sprite instance id, policy hash). The SDK hydratesidfrom the API response on bothgetSpriteandcreateSprite, so reading it costs no extra round-trip. A replacement VM has a different id → the token does not match → it is locked down. Two callers pushing the same policy is harmless; one of them skipping it is not.Fail-closed throughout: no id → no token. An unprovable lockdown is never recorded as proof, so a future SDK that stops reporting the id degrades to "re-applies every time", never to "skips it".
Requirements
updateNetworkPolicynor themkdirexecsprites.tsgatesapplyEgressLockdownbehind the pureshouldApplyPolicy. Spy assertions insprites.test.ts(calls.policies === 0,calls.spawned === []) and end-to-end through the real realtime connect wiring incold-connect-sprite-reads.test.ts.freshshort-circuitsshouldApplyPolicytotrue.acquireMachineSessioncallsstore.saveaftergetOrCreateresolves — the ordering that closes the crash window the old file header defended, since an unlocked Sprite is never reachable from a session row. Tests assert one policy push + onemkdir, and that a failing lockdown writes zero session rows.applyEgressLockdowndestroys the just-created Sprite (destroyOnFailure: fresh) and rethrows; normalized toSandboxProvisionError. Destroy-vs-retain is leaf 6-2, untouched.Design
Pure core (
services/sandbox/egress-lockdown.ts, no mocks in its tests):hashPolicy(policy)— canonical, stable sha3-256 fingerprint. Rule key order and rule array order do not change it (the platform resolves precedence by specificity, not position, so a reordered-but-identical rule set is the same policy and must not thrash).hashSandboxEgressPolicy(options)— hashes the built policy, so inputs that sanitize to the same host ('GitHub.com'vs' github.com ') don't trigger a needless re-apply.egressLockdownToken({ spriteId, policyHash })— the proof;undefinedwhen the platform reports no identity.shouldApplyPolicy({ fresh, appliedToken, desiredToken })— apply on fresh, on unknown (either side), on mismatch; skip only on a proven match.Imperative shell:
machine_sessions.egressPolicyToken(nullable, migration0202). The driver returns the token it confirmed and the manager persists that — the record can never claim more than the driver proved. NULL (legacy rows, lost writes, no id) → the next hand-back re-applies and records.Self-healing workspace root (review follow-up)
applyEgressLockdownwas also the only thing that recreatedSANDBOX_ROOT, so making it fresh-create-only left/workspaceunrepaired after a sandbox command deletes it (P2). Fixed where the directory is consumed, rather than by buying the per-connect exec back:spawnWithSelfHealingCwd(pure) wraps command+args in anshthat recreates + enters the cwd and thenexecs the real command (preserving the PTY, signals, exit code). AlreadyrunCommand's shape; now the single definition, shared by the batch path,MachineHost.stream(), and the realtimeopenPtyShell— which passescwdtocreateSession, where the serverchdirs before spawning and would otherwise fail the open outright.fsRecoveryExecfolds the same self-heal into the exec the fs path already pays between a failed op and its retry (the fs API is a barefetchthat cannot wake a hibernated Sprite): that one exec now both wakes the VM andmkdir -ps the write's parent. Zero extra round-trips on the happy path.Both keep paths as positional data args, never interpolated into the script, so the arg-array no-injection invariant holds (tested with metacharacter-laden inputs).
Test evidence
Notable new tests: the concurrent-recreate race (recorded token names
sprite-1, control plane returnssprite-2under the same name and unchanged policy → asserts the policy IS pushed), the no-id fail-closed case, the deleted-/workspacewrite recovery, and pure suites for every new function.Notes for reviewers
machine-session-manager,MachineSessionStore/Record,surface: 'machine'.0202_aspiring_warpath.sqlis a single nullable column add (ALTER TABLE "machine_sessions" ADD COLUMN "egressPolicyToken" text;) — no backfill; NULL means "unproven → re-apply once, then record".machine-branches.tsprovisions without a session row, so it passes no token and always applies — and it only provisions on first create or a vanished VM (warm reuse goes throughattach), so this costs nothing on a hot path.🤖 Generated with Claude Code
https://claude.ai/code/session_01PNvzrqmNYJbT3B6XKmr9dm