feat(machines): propagate Claude Code credential into branch-terminal Sprites#2054
Conversation
… Sprites Each branch-terminal is a physically separate Sprite from its owning Machine, with its own persistent /home/sprite — so Claude Code's OAuth credential (written to /home/sprite/.claude/.credentials.json on whichever Sprite ran `claude`) never reaches a freshly provisioned or reattached branch Sprite. Add resolveRootMachineHandle to MachineBranchesDeps and copy the root Sprite's .credentials.json (and .claude.json config, if present) into the branch Sprite at spawnBranch (both first-provision and warm reattach) and at attachBranch — refreshed on every reattach, not just once, since Claude Code credentials rotate. Gracefully skips when the root Machine has no live session or credential yet; any copy failure is swallowed as best-effort so it never fails the branch spawn/attach itself. Wired in machine-branches-runtime.ts via a direct machine_sessions lookup by pageId (same precedented pattern as machine-storage-billing.ts), avoiding acquireMachineSession's full re-authz + provision-fresh flow since this only ever needs read access to an existing session.
📝 WalkthroughWalkthroughThe change adds live root-machine lookup and Claude credential propagation to branch Sprite lifecycle operations and agent-terminal sandbox resolution. Refreshes are optional, bounded, best-effort, and covered across branch, machine, project, and malformed target scopes. ChangesClaude credential refresh
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant AgentTerminal
participant resolveMachineSandbox
participant propagateClaudeCredential
participant findLiveMachineSandboxId
participant MachineHost
AgentTerminal->>resolveMachineSandbox: resolve branch sandbox
resolveMachineSandbox->>propagateClaudeCredential: refresh credentials
propagateClaudeCredential->>findLiveMachineSandboxId: find live root sandbox
findLiveMachineSandboxId-->>propagateClaudeCredential: return sandbox ID
propagateClaudeCredential->>MachineHost: attach root and copy Claude files
MachineHost-->>resolveMachineSandbox: complete bounded refresh
resolveMachineSandbox-->>AgentTerminal: return resolved sandbox
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: d9a6a069a0
ℹ️ 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".
|
|
||
| // Refresh on every reattach — see `propagateClaudeCredential`'s doc comment | ||
| // on why this isn't a one-time, spawn-only copy. | ||
| await propagateClaudeCredential({ machineId, branchHandle: handle, resolveRootMachineHandle }); |
There was a problem hiding this comment.
When a user opens an existing branch workspace and starts or reconnects a Claude agent terminal, the production path is useAgentTerminals → /api/machines/agent-terminals → realtime resolveAgentTerminalSandbox, which resolves the branch sandboxId directly in apps/realtime/src/index.ts:245-270 and never calls this attachBranch helper or POST /api/machines/branches. That means credentials are copied only during branch creation or an explicit branch POST, so branches created before the root Claude login or before credential rotation still launch with missing/stale credentials. Move this refresh into the branch-scope realtime sandbox resolution, or wire this helper into the actual attach path.
Useful? React with 👍 / 👎.
…h path Codex review on PR #2054 correctly flagged that spawnBranch/attachBranch (machine-branches.ts) are NOT the path a user's branch agent terminal actually opens or reattaches through — that's realtime's resolveAgentTerminalSandbox -> resolveMachineSandbox (agent-terminal-access.ts), which resolves the branch Sprite directly and never calls machine-branches.ts at all. So the credential-copy added in the prior commit only fired on branch creation or an explicit POST /api/machines/branches, leaving a branch opened via its agent terminal (the common case) with a missing or stale credential. - Export propagateClaudeCredential from machine-branches.ts for reuse. - Add findLiveMachineSandboxId to machine-session-manager.ts: a read-only pageId -> sandboxId lookup (same precedented direct query machine-storage-billing.ts already uses), shared by both machine-branches-runtime.ts's resolveRootMachineHandle (deduping its prior inline query) and the new realtime wiring. - Add an optional refreshBranchCredential dep to resolveMachineSandbox (agent-terminal-access.ts), invoked only for branch-scope targets (machine/project scope already run ON the root Sprite). Wrapped in try/catch at both the call site and the dep's own implementation (index.ts) — a credential refresh must never block or fail opening the PTY itself. - Wire it in apps/realtime/src/index.ts's resolveAgentTerminalSandbox, reusing the connect's existing Sprite handle cache so only the root Sprite's read is a genuinely new network call. Tests: 8 new cases in agent-terminal-access.test.ts covering branch vs machine vs project scope, the dep being optional, and defense-in-depth against a misbehaving implementation that throws.
|
|
||
| // Refresh on every reattach — see `propagateClaudeCredential`'s doc comment | ||
| // on why this isn't a one-time, spawn-only copy. | ||
| await propagateClaudeCredential({ machineId, branchHandle: handle, resolveRootMachineHandle }); |
There was a problem hiding this comment.
Good catch — confirmed and fixed in f1936b9.
You're right that spawnBranch/attachBranch are not the path a branch's agent terminal actually opens through. The real path is realtime's resolveAgentTerminalSandbox → resolveMachineSandbox (agent-terminal-access.ts), which resolves the branch Sprite directly via resolveAgentTerminal/branchStore and never touches machine-branches.ts.
Fix:
- Exported
propagateClaudeCredentialfrommachine-branches.tsfor reuse. - Added
findLiveMachineSandboxIdtomachine-session-manager.ts(a read-only pageId→sandboxId lookup), used both to dedupemachine-branches-runtime.ts's existing inline query and for the new realtime wiring. - Added an optional
refreshBranchCredentialdep toresolveMachineSandbox, invoked only for branch-scope targets (machine/project scope already run on the root Sprite, which already has the credential). Wired inapps/realtime/src/index.ts'sresolveAgentTerminalSandbox, reusing the connect's existing Sprite handle cache so only the root Sprite's read is a new network call. - Best-effort at both the call site and the implementation (try/catch in both places) — a credential refresh must never block or fail opening the PTY.
Added 8 new tests in agent-terminal-access.test.ts covering branch vs. machine vs. project scope, the dep being optional, and defense-in-depth against a misbehaving implementation that throws. Full monorepo typecheck + targeted test suites are green.
Leaving this thread open for your verification rather than resolving it myself.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/lib/src/services/machines/machine-branches.ts`:
- Around line 244-246: Update the credential-writing flow around
branchHandle.writeFiles to explicitly apply 0o600 permissions to the Claude
credentials file after writing it, including when the file already exists;
preserve the existing conditional mode handling for other paths.
🪄 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: 1f7ca45b-1f49-4b04-869e-35135cbb3db0
📒 Files selected for processing (7)
apps/realtime/src/index.tsapps/realtime/src/terminal/__tests__/agent-terminal-access.test.tsapps/realtime/src/terminal/agent-terminal-access.tsapps/web/src/lib/machines/machine-branches-runtime.tspackages/lib/src/services/machines/__tests__/machine-branches.test.tspackages/lib/src/services/machines/machine-branches.tspackages/lib/src/services/sandbox/machine-session-manager.ts
…overwrite Independent second-pass review + CodeRabbit both surfaced real issues in the realtime credential-refresh wiring from the prior commit: - CodeRabbit (Major): writeFiles' `mode` only takes effect at file CREATION — POSIX open()'s mode argument is ignored once a file already exists. A refresh on reattach overwrites an ALREADY-EXISTING credential file, so `mode: 0o600` alone silently leaves whatever permissions that file already had. Added an explicit `chmod 600` exec after each write (via the branch handle's own `exec`, argv-array form — never a shell string) so both the credentials file and the config file are re-secured on every copy, not just the first. Also made both files consistently 0o600 (the config file previously had no explicit mode). - Independent review (worth-fixing): the branch-scope gate in resolveMachineSandbox checked `branchName !== undefined` alone. A real resolveAgentTerminal already rejects branchName-without- projectName as invalid_target before Sprite resolution, so this was unreachable in production — but the gate now explicitly checks BOTH `projectName` and `branchName`, so it stays correct even if a caller wires a resolver that doesn't enforce that invariant (as proven by a new test using a permissive fake resolver). - Independent review (nitpick): hoisted the realtime MachineHost construction to build once per connect instead of once inside the refreshBranchCredential closure — construction-only, no behavior change. New tests: chmod-after-write assertion, and the branchName-without- projectName edge case against a permissive fake resolver. All existing + new tests green (40 in machine-branches.test.ts, 38 in agent-terminal-access.test.ts); packages/lib and apps/realtime typecheck clean.
| if (!content) continue; | ||
| // Both restricted 0o600 — the credentials file is the OAuth secret | ||
| // itself, and the config file can carry account/org metadata, so | ||
| // neither belongs world-readable in the branch Sprite's home dir. |
There was a problem hiding this comment.
Confirmed valid — fixed in 3d38b15.
You're right: writeFiles' mode is a creation-time argument (POSIX open() semantics), so it's silently ignored on a write that overwrites an already-existing file — which is exactly what happens on every refresh (reattach, credential rotation). Added an explicit chmod 600 exec (argv-array form via the branch handle's own exec, never a shell string) after each write, so both the credentials file and the config file get re-secured on every copy, not just the first. Also made both files consistently 0o600 (the config file previously had no explicit mode at all).
New test asserts the exact chmod exec calls land in the branch Sprite's exec log for both files. Thanks for catching this.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d38b15c08
ℹ️ 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 { ok: false, reason: cloned.reason, detail: cloned.detail }; | ||
| } | ||
|
|
||
| await propagateClaudeCredential({ machineId, branchHandle: handle, resolveRootMachineHandle: deps.resolveRootMachineHandle }); |
There was a problem hiding this comment.
When two first-time or re-provisioning spawnBranch calls race for the same branch, this new await runs before the branch row is created/updated. If the second caller's clone fails because the shared name-keyed Sprite already has the repo, reconcileProvisionCollision can run while the first caller is still doing root-Sprite credential I/O, see no matching row (or only the old vanished row), and kill the shared Sprite; the first caller can then persist a sandboxId for a destroyed branch. Move this best-effort copy until after the row is recorded, or otherwise avoid adding long I/O inside the rowless collision window.
Useful? React with 👍 / 👎.
… after Codex review caught a real concurrency bug introduced by the credential copy: it ran between a successful clone and the row being persisted, widening that window. A concurrent racer whose OWN clone fails against the same name-keyed shared Sprite calls reconcileProvisionCollision, which looks up the row to decide whether the Sprite it's about to kill is the winner's. With the credential copy's extra I/O (root-Sprite read + branch-Sprite writes/execs) sitting before persistence, the racer could find no matching row yet, conclude the shared Sprite was its own redundant one, and kill it out from under the winner — which was still mid-copy and about to record that exact sandboxId. Fix: move propagateClaudeCredential to run AFTER store.updateSandboxId / store.create succeeds, at each success return, instead of once before both branches. The realtime refreshBranchCredential path has no analogous issue — it only ever reads an already-persisted branch row, never creates/updates one. Added two direct regression tests asserting the row is already visible via store.findByName at the exact moment the credential copy starts, for both the fresh-create and the re-provision-after-vanish paths. 42/42 tests pass in machine-branches.test.ts; packages/lib typecheck clean.
| return { ok: false, reason: cloned.reason, detail: cloned.detail }; | ||
| } | ||
|
|
||
| // Persist the row FIRST, before the credential copy — not after. A |
There was a problem hiding this comment.
Confirmed valid, real race — fixed in 5b2d755.
You're right: the credential copy's extra I/O (root-Sprite read + branch-Sprite writes/execs) was sitting between the successful clone and the row being persisted, widening that window. A concurrent racer whose own clone fails against the same name-keyed shared Sprite calls reconcileProvisionCollision, which looks up the row to decide whether the Sprite it's about to kill is the winner's — with no row yet, it would conclude the shared Sprite was its own redundant one and kill it out from under the still-mid-copy winner.
Moved propagateClaudeCredential to run AFTER store.updateSandboxId/store.create succeeds (at each success return) instead of before both branches. Confirmed the realtime refreshBranchCredential path has no analogous issue — it only ever reads an already-persisted branch row, never creates/updates one.
Added two direct regression tests asserting the row is already visible via store.findByName at the exact moment the credential copy starts, for both the fresh-create and re-provision-after-vanish paths.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b2d7558d1
ℹ️ 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 [row] = await db | ||
| .select({ sandboxId: machineSessions.sandboxId }) | ||
| .from(machineSessions) | ||
| .where(eq(machineSessions.pageId, pageId)) |
There was a problem hiding this comment.
When a Machine page has been moved between drives, machine_sessions can contain more than one row for the same pageId because pageId is only indexed while the unique address is the session key derived from tenant/drive/page, and bulk-move updates pages.driveId without deleting the old session row. In that case this pageId-only LIMIT 1 can return the previous drive's root Sprite, so branch credential propagation may copy Claude credentials from the old owner/session into a branch under the new drive. Resolve via the current drive/owner-derived session key or otherwise enforce/select the current row instead of a bare pageId.
Useful? React with 👍 / 👎.
| // enforce it (e.g. a test double). | ||
| if (target.projectName !== undefined && target.branchName !== undefined && deps.refreshBranchCredential) { | ||
| try { | ||
| await deps.refreshBranchCredential({ machineId: target.machineId, sandboxId: resolved.sandboxId }); |
There was a problem hiding this comment.
For branch terminals this awaits the best-effort credential copy before returning the Sprite to the PTY layer. That copy uses MachineHandle.readFile/writeFiles, and the Sprites filesystem wrapper waits up to the 30s fs timeout before exec-waking and retrying on a hibernated Sprite, so a cold root or branch can delay terminal creation by tens of seconds before openPtyShell even starts. If credential propagation must not block opening the PTY, bound this await tightly or decouple the refresh from the attach path.
Useful? React with 👍 / 👎.
…ow; never block PTY open
Codex review caught two more real issues on the second follow-up pass:
1. (P1) findLiveMachineSandboxId's bare-`pageId` lookup could return a
STALE session row. `machine_sessions.sessionKey` namespaces by
tenant + drive + page — a page moved between drives leaves its OLD
drive's row behind (a drive move has no reason to touch
machine_sessions), so a plain `WHERE pageId = X LIMIT 1` with no
ordering could non-deterministically resolve that old row instead
of the current one, copying a Claude credential from a Sprite that
may belong to a different owner/tenant context entirely.
Fixed by having findLiveMachineSandboxId take the same
{tenantId, driveId, pageId, secret} the store's OWN session-key
derivation uses, deriving the CURRENT sessionKey via
deriveMachineSessionKey, and querying by that (the table's actual
unique constraint) instead of the bare pageId. Both callers now
resolve the page's current driveId + drive owner first:
- machine-branches-runtime.ts's resolveRootMachineHandle reuses the
existing findMachinePage() plus a new drives.ownerId lookup.
- apps/realtime/src/index.ts factors a resolveDriveOwnerContext()
helper — the exact two-query lookup buildMachineSandbox.acquire
already did inline — shared by both acquire (deduped, no behavior
change) and the new refreshBranchCredential wiring.
2. (P2) resolveMachineSandbox awaited refreshBranchCredential before
returning. The Sprite fs API's read/write timeout is 30s, with one
retry (~60s worst case), so blocking every branch-scope PTY
resolution on that would regress this codebase's core invariant
that opening a PTY is itself the Sprite's wake and nothing upstream
waits on it. Changed to fire-and-forget (void ...catch()) — the
refresh still normally lands well before a user manually runs
`claude`; the narrower race (typing faster than an in-flight
refresh) self-heals once the copy completes.
79 tests pass (42 machine-branches.test.ts + 37
agent-terminal-access.test.ts) under vitest; 101 tests pass in
apps/realtime's index.test.ts (unaffected by the acquire dedup);
packages/lib, apps/web, and apps/realtime all typecheck clean.
| * nothing, never a stale one from a prior drive. | ||
| * | ||
| * Lazily resolves the db client, schema table, and `eq` operator so callers | ||
| * that don't exercise this path (most tests) never load the DB module graph. |
There was a problem hiding this comment.
Confirmed valid — this is a real cross-drive credential-leak risk. Fixed in b31b232.
You're right: machine_sessions.sessionKey namespaces by tenant + drive + page, and a page moved between drives leaves its OLD drive's row behind (a drive move has no reason to touch machine_sessions). A bare-pageId lookup with no ordering could then resolve that stale row instead of the current one.
Fixed by having findLiveMachineSandboxId take the same {tenantId, driveId, pageId, secret} the session store's own key derivation uses, deriving the CURRENT sessionKey via deriveMachineSessionKey, and querying by that (the table's actual unique constraint) instead of the bare pageId. Both callers now resolve the page's current driveId + drive owner first — machine-branches-runtime.ts's resolveRootMachineHandle reuses the existing findMachinePage() plus a new drives.ownerId lookup, and apps/realtime/src/index.ts factors a resolveDriveOwnerContext() helper (the same two-query lookup buildMachineSandbox.acquire already did inline) shared by both acquire and the new refreshBranchCredential wiring.
| // matching `resolveScopeKey`/`resolveScopeLocation`'s own discriminator | ||
| // (`agent-terminals.ts`), rather than relying on the implicit invariant | ||
| // that a real `resolveAgentTerminal` already rejects `branchName` without | ||
| // `projectName` as `invalid_target` before ever reaching here. Checking |
There was a problem hiding this comment.
Confirmed valid — fixed in b31b232.
Verified the concrete number: the Sprite fs API's op timeout is 30s (FS_OP_TIMEOUT_MS in sprites.ts), and fsWithWakeRetry retries once on failure, so a cold/hibernating Sprite could stall a single read or write for up to ~60s. Blocking every branch-scope PTY resolution on that would regress this codebase's own stated invariant that opening a PTY is itself the Sprite's wake and nothing upstream should wait on it.
Changed refreshBranchCredential's invocation in resolveMachineSandbox to fire-and-forget (void ...().catch(() => {})) instead of awaited — the refresh still normally completes well before a user manually runs claude; the narrower race (typing claude faster than an in-flight refresh) self-heals moments later once the copy finishes, which is a far smaller cost than a multi-second-to-a-minute stall on every cold terminal open.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b31b232a7a
ℹ️ 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".
|
|
||
| for (const path of [CLAUDE_CREDENTIALS_PATH, CLAUDE_CONFIG_PATH]) { | ||
| const content = await rootHandle.readFile({ path }); | ||
| if (!content) continue; |
There was a problem hiding this comment.
When an existing branch Sprite already has a copied Claude credential and the root machine later no longer has that file (for example after a logout or manual removal), this refresh path just skips the absent root file and leaves the old secret in the branch. Since the refresh now runs on every reattach specifically to keep rotated credentials in sync, this can leave branch terminals authenticated with stale root credentials after the source has been cleared; for a live root handle, treat a missing root credential/config as a branch-side removal instead of a no-op.
Useful? React with 👍 / 👎.
…leared Codex review: since propagateClaudeCredential now refreshes on every reattach specifically to keep rotated credentials in sync, treating a now-missing root file as a pure no-op left a branch Sprite silently authenticated with a stale copy after an explicit `claude logout` (or manual removal) on the root Machine. A revocation on the root is exactly the kind of change this sync must also propagate. Changed: when the root's read for a given path comes back empty, exec `rm -f <path>` on the branch handle to remove any stale copy, instead of skipping to the next file. `rm -f` is a harmless no-op when nothing was ever copied (root never logged in). Two new tests: removal on the next reattach after the root's credential disappears, and that removing a never-copied file is a harmless no-op. The fake test host's `exec` now interprets `rm -f <path>` by default (real removal from its in-memory files map) so the first test can assert actual absence, not just the exec call shape. 44/44 tests pass (bun test and vitest); packages/lib typecheck clean.
| branchHandle: MachineHandle; | ||
| resolveRootMachineHandle: (machineId: string) => Promise<MachineHandle | null>; | ||
| }): Promise<void> { | ||
| try { |
There was a problem hiding this comment.
Good design catch, agreed — fixed in 72e5a4c.
You're right: since this refresh runs on every reattach specifically to keep rotated credentials in sync, a revocation (explicit claude logout or manual removal) on the root is exactly the kind of change that sync should also propagate, not silently skip.
Changed: when the root's read for a given path comes back empty, exec rm -f <path> on the branch handle instead of skipping to the next file — removing any stale copy. rm -f is a harmless no-op when nothing was ever copied (root never logged in).
Added two tests: removal on the next reattach after the root's credential disappears, and that removing a never-copied file is a harmless no-op.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72e5a4c7b0
ℹ️ 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".
| // logout` (or manual removal) — which must propagate as a | ||
| // revocation to any branch Sprite that received a copy, not leave a | ||
| // stale one behind. `rm -f` is a no-op when nothing was ever copied. | ||
| await branchHandle.exec({ cmd: 'rm', args: ['-f', path] }); |
There was a problem hiding this comment.
When a branch already has a Claude credential (copied earlier or created by logging in directly on the branch) and the root read returns null, this rm -f deletes the branch's valid login on every spawn/attach/realtime refresh. The Sprite driver maps not only missing files but also any read failure to null (packages/lib/src/services/sandbox/sandbox-client/sprites.ts:767-772), so this condition is not reliable evidence of a real logout; skip deletion unless revocation can be distinguished from unavailable/never-written credentials.
Useful? React with 👍 / 👎.
| // `claude` faster than an in-flight refresh completes — self-heals | ||
| // moments later once the copy finishes, and is a FAR smaller cost than a | ||
| // multi-second stall on every cold terminal open. | ||
| void deps.refreshBranchCredential({ machineId: target.machineId, sandboxId: resolved.sandboxId }).catch(() => { |
There was a problem hiding this comment.
For a fresh branch-scoped Claude terminal, this queues the refresh and returns before the credential is copied, while the cold path immediately calls openShell with the resolved command (apps/realtime/src/terminal/agent-terminal-handler.ts:922-929). If claude starts before the async copy finishes—most likely on cold/hibernated Sprites—it reads no credential and prompts for login, and copying the file seconds later does not re-authenticate that already-running process, making propagation unreliable on the main PTY attach path.
Useful? React with 👍 / 👎.
…-forget) the PTY-path refresh Codex review caught two more real issues — one in my own immediately preceding commit, one revisiting an earlier tradeoff: 1. Reverted "delete branch credential when root read is empty" (previous commit). The Sprite driver's readFile maps EVERY read failure to the same null a missing file produces (sprites.ts: "a missing file (or any read failure after a wake retry) resolves to null") — there is no way to distinguish a real `claude logout` from a transient root-Sprite read hiccup. Deleting on that ambiguous signal risked destroying a branch's perfectly valid, working credential on a momentary blip — strictly worse than the staleness gap it was meant to close. Back to a pure skip-on-empty, with a regression test guarding against reintroducing the deletion, and the fake test host's now-unused `rm -f` simulation removed. 2. Changed the realtime refreshBranchCredential call from fire-and-forget back to awaited, but now bounded by a tight (5s) timeout instead of either extreme. Fire-and-forget (the prior commit) made propagation unreliable on the primary use case: a fresh branch-scoped Claude terminal calls openShell with `claude` immediately after resolveSandbox returns, so a copy finishing moments later does nothing for a process that already started without the credential. Fully blocking (two commits ago) risked up to ~60s stalls on a cold/hibernating Sprite (30s fs-op timeout, one retry). The bound covers the common warm-Sprite case reliably (normally sub-second) while capping the cold-Sprite worst case to a few seconds instead of a minute. Added a fake-timers test proving the bound actually resolves a never-settling refresh rather than hanging forever. 44 tests pass in machine-branches.test.ts (bun test), 38 in agent-terminal-access.test.ts (vitest — needed for the fake-timers test), packages/lib and apps/realtime typecheck clean.
| const rootHandle = await resolveRootMachineHandle(machineId); | ||
| if (!rootHandle) return; | ||
|
|
||
| for (const path of [CLAUDE_CREDENTIALS_PATH, CLAUDE_CONFIG_PATH]) { |
There was a problem hiding this comment.
Confirmed valid and reverted — this was a real regression I introduced in the prior commit. Fixed in 23832db.
You're right: readFileToBuffer maps EVERY read failure to the same null a missing file produces (sprites.ts: "a missing file (or any read failure after a wake retry) resolves to null"), so there's no reliable way to distinguish a real claude logout from a transient root-Sprite read hiccup. Deleting on that ambiguous signal risked destroying a branch's perfectly valid, working credential on a momentary blip — strictly worse than the staleness gap the deletion was meant to close.
Reverted to a pure skip-on-empty. Added a regression test guarding against reintroducing the deletion (proves an empty root read leaves an existing branch credential untouched), and removed the now-unused rm -f simulation from the fake test host.
|
Re: "Await credential copy before launching branch PTYs" (agent-terminal-access.ts) — confirmed and fixed in 23832db, changed from fire-and-forget to awaited-with-a-tight-bound. You're right that fire-and-forget (my prior commit, in response to your earlier P2 about blocking) made propagation unreliable on exactly the path this feature targets: Rather than going back to fully blocking (risking up to ~60s on a cold/hibernating Sprite — 30s fs-op timeout with one retry), changed the call to Added a fake-timers test ( |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23832dba8e
ℹ️ 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".
| if (handle) { | ||
| // Refresh on every reattach, not just first spawn — Claude Code OAuth | ||
| // credentials rotate, so a one-time copy would drift stale over time. | ||
| await propagateClaudeCredential({ machineId, branchHandle: handle, resolveRootMachineHandle: deps.resolveRootMachineHandle }); |
There was a problem hiding this comment.
When POST /api/machines/branches hits this existing-branch path, the branch is already live but the response now waits for propagateClaudeCredential with no timeout. If the root Sprite is hibernated or its filesystem calls are slow/unreachable, this helper can spend tens of seconds on Sprite reads/writes before returning, whereas the realtime attach path added a 5s cap for the same refresh. Keep this best-effort copy bounded or off the response path so resuming an existing branch does not hang on credential I/O.
Useful? React with 👍 / 👎.
| // already had. An explicit chmod after the write re-secures it either | ||
| // way, regardless of whether this write created or overwrote it. | ||
| await branchHandle.writeFiles([{ path, content, mode: 0o600 }]); | ||
| await branchHandle.exec({ cmd: 'chmod', args: ['600', path] }); |
There was a problem hiding this comment.
MachineHandle.exec resolves to a result with an exitCode rather than throwing for non-zero command exits, so this ignores a failed chmod 600. For an overwrite of an existing credential/config file that already had permissive mode bits, a chmod failure leaves the copied OAuth material at the old mode even though this step is what is supposed to re-secure it; chmod --help confirms it is the command that changes the file mode. Check the exit code and handle failure before treating the copy as successful.
Useful? React with 👍 / 👎.
…chmod exit code Codex review caught two more real issues: 1. (P2) spawnBranch's and attachBranch's direct calls to propagateClaudeCredential were unbounded — only the realtime PTY path had a timeout (its own withTimeout wrapper around the injected refreshBranchCredential dep, which doesn't help THIS file's own direct callers). A hibernating root Sprite's fs read/write can take up to ~60s (30s timeout, one retry), which would hang a branch-attach HTTP response on credential I/O. Fixed by moving the timeout bound INSIDE propagateClaudeCredential itself (a new CREDENTIAL_COPY_TIMEOUT_MS + withTimeout helper in machine-branches.ts, same never-reject/clear-the-timer shape as agent-terminal-access.ts's), so every caller is protected structurally rather than by each call site remembering to wrap it. 2. (P2) The chmod exec's result was ignored. MachineHandle.exec resolves with an exitCode rather than throwing on a nonzero exit, so a failed chmod (e.g. permission issue) would silently leave an OVERWRITE of an already-existing credential/config file at whatever permissive mode it already had, while this function reported success. Now checks chmod's exitCode and throws (caught by the existing best-effort try/catch, so it still never fails the caller) — a failed chmod aborts the rest of the copy for that call rather than being silently trusted. Two new tests: a failing chmod stops the copy without failing the spawn, and a never-settling resolveRootMachineHandle still returns once the bound elapses (fake timers). 46/46 tests pass via the package's actual `vitest run` test script (this file's new fake-timers test needs it — bun test's vi shim doesn't implement advanceTimersByTimeAsync, same known gap as apps/realtime's index.test.ts). Full packages/lib suite: 349 files / 8026 tests pass. packages/lib and apps/realtime typecheck clean.
| * still works, it just needs its own `claude` login. | ||
| * | ||
| * Exported for reuse by the realtime agent-terminal PTY bridge | ||
| * (`apps/realtime/src/index.ts`'s `resolveAgentTerminalSandbox`), which is |
There was a problem hiding this comment.
Confirmed and fixed in a109d0d.
You're right: exec resolves with an exitCode rather than throwing on a nonzero exit, so a failed chmod was silently ignored — meaning an OVERWRITE of an already-existing credential/config file (the exact scenario the chmod step exists for) could stay at whatever permissive mode it already had while this function reported success.
Now checks chmod's exit code and throws on nonzero (caught by the existing best-effort try/catch, so it still never fails the caller) — a failed chmod aborts the rest of the copy for that call rather than being silently trusted. Added a test proving a failing chmod stops the copy (the second file is never reached) without failing the spawn itself.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4784412a0c
ℹ️ 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".
| await branchHandle.exec({ cmd: 'rm', args: ['-f', tempPath] }); | ||
| await branchHandle.writeFiles([{ path: tempPath, content, mode: 0o600 }]); |
There was a problem hiding this comment.
When rm -f returns a nonzero exit, this still proceeds to writeFiles; if a prior failed copy left the fixed .tmp file behind with a permissive mode (or cleanup otherwise fails), the write becomes an overwrite rather than the fresh creation that applies 0o600, and the subsequent mv can promote a readable Claude credential. Please check the cleanup exitCode and skip/throw before writing to the temp path.
Useful? React with 👍 / 👎.
Codex review: the temp-path-clearing `rm -f` result was ignored — if it returned nonzero (unusual for -f, but possible: permission issue, disk error, etc.), the code proceeded to writeFiles anyway, assuming the temp path was now clear. If it wasn't, the write becomes an overwrite rather than a creation, silently keeping the stale file's permissions — reintroducing the exact problem the temp-file flow (and this line's own earlier fix) exists to avoid. Now checks the clear's exitCode and throws (caught by the existing best-effort try/catch) before ever writing to the temp path if it failed. Still safe either way: nothing has touched the live file yet at that point. Added a test proving a failing temp-clear aborts before writeFiles/mv ever run, leaving an existing valid credential at the real path untouched. 48/48 tests pass; full packages/lib suite (357 files / 8191 tests) and typecheck both clean.
| // in review). Checked, not fire-and-forget: if the clear itself | ||
| // fails, abort BEFORE writing rather than assume the temp path is | ||
| // now clear (caught in review, again) — still safe either way, | ||
| // since nothing has touched the live file yet. |
There was a problem hiding this comment.
Confirmed and fixed in 46819ff.
You're right — ignoring the clear's exit code meant a failed rm -f (unusual, but possible) would let the write proceed against a potentially-still-there stale temp file, reintroducing the overwrite-keeps-old-permissions problem this whole flow exists to avoid.
Now checks the clear's exitCode and throws before ever writing to the temp path if it failed (caught by the existing best-effort try/catch) — still safe either way, since nothing has touched the live file yet at that point. Added a test proving a failing temp-clear aborts before writeFiles/mv ever run.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46819ff26c
ℹ️ 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".
| await branchHandle.writeFiles([{ path: tempPath, content, mode: 0o600 }]); | ||
| const move = await branchHandle.exec({ cmd: 'mv', args: [tempPath, path] }); |
There was a problem hiding this comment.
When a credential copy times out, the async body continues running; if it already read an older Claude credential and then stalls before these writes, a later attach can copy a rotated token successfully, only for the older continuation to resume and mv its stale temp file over the newer destination. This defeats the refresh-on-reattach behavior specifically when Sprite FS calls are slow enough to hit the 5s bound, so the live write/rename should be guarded by a still-current deadline/generation or otherwise serialized.
Useful? React with 👍 / 👎.
…ewer one Codex review: withTimeout deliberately keeps a slow call's underlying work running in the background past its own bound (so a slow copy still lands rather than being aborted). That means an overlapping call that read an OLDER root credential and then stalled could finish LATER and mv its stale temp file over a destination a faster, more recent call already updated with a NEWER (rotated) credential — clobbering it. This defeats refresh-on-reattach specifically when Sprite FS calls are slow enough to hit the 5s bound. Initial attempt (in-flight Set + skip-if-already-running) had its own correctness gap: if a call's underlying work never settles at all (e.g. a resolveRootMachineHandle that hangs forever), the in-flight marker would never clear, permanently locking that branch out of all FUTURE refreshes too — a worse regression than the one being fixed. Replaced with a generation counter per branch Sprite machineId: each propagateClaudeCredential call takes a ticket number, and checks (after writing its temp file, right before the risky mv) whether a NEWER call has since started for the same branch — if so, it abandons its own stale attempt instead of committing it. No lock to release, no backstop timer, no permanent-lockout risk: the check is self-contained and only matters at the one moment it's unsafe to proceed. Added a test that directly reproduces the race: an overlapping call with a manually-held-open root resolution (simulating a stall), racing against a normal call with a rotated token — proves the stalled call detects the newer generation and skips its mv rather than clobbering it. 49/49 tests pass; full packages/lib suite (357 files / 8192 tests) and typecheck both clean.
| // | ||
| // `writeFiles`' `mode` reliably applies to a genuine CREATION — | ||
| // and `mv` on the same filesystem is atomic: the destination is | ||
| // either the OLD valid credential or the NEW one, NEVER |
There was a problem hiding this comment.
Confirmed and fixed in ae5f9a0 — this is real, and my first attempt at fixing it (an in-flight Set with skip-if-already-running) had its own correctness gap: if a call's underlying work never settles at all, the marker would never clear, permanently locking that branch out of ALL future refreshes.
Replaced with a generation counter per branch Sprite: each call takes a ticket number and checks — after writing its temp file, right before the risky mv — whether a NEWER call has since started for the same branch. If so, it abandons its own stale attempt instead of committing it. No lock to release, no backstop timer, no permanent-lockout risk.
Added a test that directly reproduces the race: an overlapping call with a manually-held-open root resolution (simulating a stall) racing against a normal call with a rotated token — proves the stalled call detects the newer generation and skips its mv rather than clobbering it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae5f9a0781
ℹ️ 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".
| // wrong-permission or briefly absent in between. If anything | ||
| // fails before the rename, the live file at `path` is completely | ||
| // untouched. | ||
| const tempPath = `${path}.tmp`; |
There was a problem hiding this comment.
When two credential refreshes for the same branch Sprite overlap (for example, two tabs opening the same branch terminal or a spawn followed by the realtime PTY refresh), both calls use the same ${path}.tmp. If the older call reaches the generation mismatch after the newer call has written its temp file but before it runs mv, the older cleanup deletes the newer temp file, so the newer mv fails and the fresh credential/config is not propagated. Use a per-call/per-generation temp path so stale cleanup cannot remove another in-flight refresh's temp file.
Useful? React with 👍 / 👎.
Codex review: my generation-counter fix (previous commit) still had both overlapping calls writing to the SAME fixed temp path (`<path>.tmp`). If an older call's stale-cleanup ran AFTER a newer, overlapping call had already written its OWN content to that shared path (but before the newer call's own mv), the older call's cleanup deleted the newer call's temp file out from under it — making the newer (correct) refresh fail its own mv and never propagate at all. That's worse than the clobber being fixed: the correct refresh now silently fails instead of the stale one being merely skipped. Fixed by suffixing the temp path with the call's own generation number (`<path>.tmp.<generation>`) — no two overlapping calls for the same branch Sprite ever share a temp filename, so one's cleanup can never disturb another's in-progress write. Still clears the temp path first regardless: `generation` resets to 1 on every process restart (in-memory, not persisted), so a stale leftover from a pre-restart crash could coincidentally reuse the same generation number. Updated the temp-path assertions to pattern-match `.tmp.\d+` (the generation number isn't predictable — it's a module-level counter shared across the whole test file, not reset per test) instead of the old fixed-suffix literal. 49/49 tests pass; full packages/lib suite (357 files / 8192 tests) and typecheck both clean.
| // wrong-permission or briefly absent in between. If anything | ||
| // fails before the rename, the live file at `path` is completely | ||
| // untouched. | ||
| // Suffixed with THIS call's own generation number — never shared |
There was a problem hiding this comment.
Confirmed and fixed in a2e417a — this was a real gap in the generation-counter fix.
You're right: both overlapping calls were writing to the same fixed <path>.tmp. If an older call's stale-cleanup ran after a newer call had already written its own content there (but before its own mv), the older cleanup would delete the newer call's temp file out from under it — making the CORRECT, newer refresh fail its own mv entirely, which is worse than the clobber being fixed.
Fixed by suffixing the temp path with the call's own generation number (<path>.tmp.<generation>) — no two overlapping calls ever share a temp filename, so one's cleanup can never disturb another's in-progress write. Still clears the temp path first regardless, since generation resets to 1 on every process restart and could coincidentally collide with a stale leftover from a pre-restart crash.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2e417aaf3
ℹ️ 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 generation = (latestCredentialCopyGeneration.get(branchHandle.machineId) ?? 0) + 1; | ||
| latestCredentialCopyGeneration.set(branchHandle.machineId, generation); |
There was a problem hiding this comment.
Both the web branch API and the realtime PTY path call propagateClaudeCredential, but those run in separate services/processes. Because this generation comes from a module-local Map, concurrent copies in web and realtime both start at 1, so they use the same *.tmp.1 path and the staleness check cannot see the other process. In a spawn-then-open or credential-rotation flow, the slower copy can delete/move the other service's temp file or overwrite a newer credential with an older one, defeating the race protection. Use a filesystem-unique temp name and a cross-process freshness/locking mechanism if stale clobbering must be prevented.
Useful? React with 👍 / 👎.
…nter is process-local Codex review: apps/web (spawnBranch/attachBranch) and apps/realtime (refreshBranchCredential) are TWO SEPARATE PROCESSES, each with its own in-memory latestCredentialCopyGeneration Map. A call in one process cannot see an overlapping call in the other — both would independently compute generation=1 and collide on the same `<path>.tmp.1`, reintroducing the exact "one call's cleanup deletes another's in-progress temp file" bug the per-generation temp path was supposed to close, just across the process boundary instead of within one process. Fixed by keying the temp path on a globally-unique cuid2 id (`createId()`, matching this codebase's established convention for non-DB-primary-key identifiers) instead of the process-local generation number. No two overlapping calls, from either process, can ever share a temp filename now. The generation counter itself remains process-local by nature — fully solving cross-process staleness detection (which credential's `mv` should "win") would need a database-backed lock or a version marker persisted on the Sprite's own filesystem, which is disproportionate for a background sync where every single refresh is already best-effort and self-heals on the next reattach in either process. Documented this explicitly as an accepted, understood tradeoff rather than leaving it implicit. The within-process race (e.g. two nearly simultaneous requests on the same server) is still fully closed. Updated test regexes from `\d+` to `[a-z0-9]+` to match cuid2's alphanumeric ids instead of a decimal counter. 49/49 tests pass; full packages/lib suite (357 files / 8192 tests) and typecheck both clean.
| resolveRootMachineHandle, | ||
| }: { | ||
| machineId: string; | ||
| branchHandle: MachineHandle; |
There was a problem hiding this comment.
Confirmed and fixed in ee3c2aa — you're right that this is a real gap, and I want to be precise about what's fixed vs. accepted.
Fixed: the temp path now uses a globally-unique cuid2 id (createId()) instead of the process-local generation number — no two overlapping calls, from EITHER process, can ever share a temp filename, so one's cleanup can never delete another's in-progress write regardless of which process it's running in.
Accepted, documented explicitly: the staleness CHECK itself (latestCredentialCopyGeneration) is necessarily process-local — a call in apps/web genuinely cannot see an overlapping call in apps/realtime, so which one's mv lands last in a genuine cross-process race isn't something this in-memory counter can arbitrate. Fully solving that would need a database-backed lock or a version marker persisted on the Sprite's own filesystem — I judged that disproportionate for a background sync where every single refresh is already best-effort and self-heals on the next reattach in either process, rather than something that must be prevented outright. Happy to revisit if you think the cross-process ordering guarantee is worth the added complexity.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee3c2aa605
ℹ️ 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 clear itself fails, abort BEFORE writing rather than assume | ||
| // the temp path is now clear — still safe either way, since | ||
| // nothing has touched the live file yet. | ||
| const clearTemp = await branchHandle.exec({ cmd: 'rm', args: ['-f', tempPath] }); |
There was a problem hiding this comment.
When the branch Sprite is cold or the exec transport wedges, this rm command is not actually bounded: the 5s withTimeout only stops awaiting propagateClaudeCredential, while the Sprite runner only installs its SIGKILL timer when timeoutMs is supplied. Because this and the later mv/cleanup execs omit timeoutMs, repeated branch attaches can leave unbounded background execs/sockets after the caller has already returned; pass a short timeoutMs/maxBytes on these housekeeping commands so the refresh really terminates on stalls.
Useful? React with 👍 / 👎.
…imeoutMs Codex review: the Sprite runner only installs its SIGKILL wall-clock timer when timeoutMs is explicitly supplied (sprites.ts's spawn path gates the kill timer on `timeoutMs && timeoutMs > 0`). None of this function's rm/mv exec calls passed one, so on a cold or wedged Sprite they were genuinely unbounded at the transport level — the 5s CREDENTIAL_COPY_TIMEOUT_MS only stops propagateClaudeCredential's CALLER from waiting, it does not touch the exec itself. A wedged rm or mv would never be killed, leaking its process/socket on every such attach attempt. Added a housekeepingExecArgs helper (timeoutMs: the same 5s bound, maxBytes: the existing SANDBOX_MAX_OUTPUT_BYTES constant already used elsewhere for sandbox commands) applied to all four rm/mv exec calls in propagateClaudeCredential. Added a test asserting every rm/mv exec carries a positive timeoutMs and maxBytes. 50/50 tests pass; full packages/lib suite (357 files / 8193 tests) and typecheck both clean.
| // deleted the newer call's temp file out from under it, making | ||
| // the newer (correct) refresh fail its own `mv` (caught in | ||
| // review, twice — once for the intra-process case, once for the | ||
| // inter-process one this id fixes). A globally unique path means |
There was a problem hiding this comment.
Confirmed and fixed in f14f11f.
Verified the claim: the Sprite runner's spawn path only installs its SIGKILL wall-clock timer when timeoutMs is explicitly supplied and positive — none of my rm/mv exec calls passed one, so on a cold or wedged Sprite they were genuinely unbounded at the transport level (the 5s CREDENTIAL_COPY_TIMEOUT_MS only stops propagateClaudeCredential's caller from waiting, it doesn't touch the exec itself).
Added a small housekeepingExecArgs helper (timeoutMs: the same 5s bound; maxBytes: the existing SANDBOX_MAX_OUTPUT_BYTES constant already used elsewhere for sandbox commands) and applied it to all four rm/mv exec calls. Added a test asserting every housekeeping exec carries a positive timeoutMs/maxBytes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f14f11fa60
ℹ️ 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; | ||
| } | ||
|
|
||
| const move = await branchHandle.exec(housekeepingExecArgs('mv', [tempPath, path])); |
There was a problem hiding this comment.
When two credential refreshes for the same branch overlap, the older call can pass the generation check and then stall inside this async mv; a newer refresh can start, copy the rotated credential, and complete before the older mv finally lands. In that case the stale temp file overwrites the fresh credential despite the guard, so branch Claude sessions can keep using an expired token until a later refresh happens.
Useful? React with 👍 / 👎.
…, accepted tradeoff Codex review (raised twice now): the generation check happens before the mv, not atomically with it, so a newer overlapping call could still start, finish its own mv, and then have this call's own (checked-as-current-a-moment-ago) mv land after it, clobbering the fresher credential. This is real and not being fixed further. Closing it fully would need either an atomic compare-and-swap this filesystem abstraction doesn't expose, or a synchronous per-branch mutex serializing every mv — which would queue an unrelated caller behind however long a DIFFERENT in-flight refresh's Sprite I/O takes, reintroducing the unbounded-latency problem this whole design exists to avoid. The impact is staleness (the branch keeps using an old-but-still-valid token a little longer), not exposure, and self-heals on the next reattach in either process — consistent with every other best-effort guarantee already documented in this function. Added an explicit code comment at the check site recording this as a deliberate, reasoned tradeoff rather than an oversight, so it reads as settled if raised again. No behavior change; comment-only.
| // check happens BEFORE the `mv` below, not atomically with it. A | ||
| // newer call could still start, read a rotated credential, and | ||
| // complete its OWN `mv` entirely within the time this call's own | ||
| // `mv` (checked as still-current a moment ago) takes to actually |
There was a problem hiding this comment.
Acknowledged — this is real, and I'm making a deliberate call not to chase it further. Documented in c748537.
The check happens before the mv, not atomically with it, so a newer overlapping call could in principle still land its mv after a check that was current a moment earlier. Fully closing this would need either an atomic compare-and-swap primitive this filesystem abstraction doesn't expose, or a synchronous per-branch mutex serializing every mv — which would then queue an unrelated caller's response behind however long a DIFFERENT in-flight refresh's Sprite I/O takes, reintroducing the unbounded-latency problem this whole multi-round design exists to avoid.
The impact here is staleness, not exposure: worst case the branch keeps using an old-but-still-valid token a little longer, and self-heals on the next reattach in either process — the same best-effort guarantee already documented everywhere else in this function. Given the residual window is now bounded to roughly one exec round-trip (down from the original tens-of-seconds unbounded-stall case), I judge this a reasonable place to stop rather than add a mutex that reintroduces latency risk to close an availability (not security) edge case. Added an explicit comment recording this as a deliberate tradeoff rather than an oversight. Happy to revisit if you disagree with that judgment call.
|
@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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
1 participant
Summary
/home/sprite/.claude/.credentials.jsonon whichever Sprite ranclaude, so a branch Sprite never has it.propagateClaudeCredential(machine-branches.ts) copies the root Sprite's.credentials.json(and.claude.jsonconfig, if present) into a branch Sprite by writing to a globally-unique (cuid2) temp path and atomicallymv-ing it onto the real destination — never writing directly to the live path.writeFiles'modeonly reliably applies at file creation (POSIXopen()semantics), so a temp-then-rename is what makes0o600land correctly even on a refresh, without ever leaving the live file briefly absent or at the wrong permissions. Every housekeepingrm/mvexec carries an explicittimeoutMs/maxBytesso a wedged Sprite can't leak an unbounded background process. A generation counter guards against a stalled, overlapping refresh clobbering a newer one that already landed (process-local — see in-code doc comment on the accepted cross-process residual, which affects staleness only, never exposure).nulla missing file produces, so that signal can't reliably distinguish a realclaude logoutfrom a transient root-Sprite hiccup.spawnBranch(fresh-provision and warm-reattach) andattachBranch— branch creation / the navigator's explicit attach API. The copy runs after the branch row is persisted, not before — doing it earlier would widen the window in which a concurrent racer's collision-reconciliation could kill the shared, name-keyed Sprite out from under the still-in-flight winner. Bounded by a 5s timeout insidepropagateClaudeCredentialitself, so a hibernating root Sprite can't hang a branch-attach HTTP response.resolveAgentTerminalSandbox→resolveMachineSandbox(agent-terminal-access.ts), via an optionalrefreshBranchCredentialdep fired only for true branch-scope targets (projectNameandbranchNameboth set). This is the path a user's branch agent terminal actually opens/reattaches through on every cold PTY open —spawnBranch/attachBranchalone don't cover it. Awaited but bounded by its own 5s timeout rather than fully blocking or fire-and-forget (both extremes were tried and reverted during review).findLiveMachineSandboxId(machine-session-manager.ts) resolves the page's current session key (from its current driveId + drive owner) rather than a bare-pageIdlookup — a page moved between drives can leave its OLD drive's session row behind under a different key, and a bare-pageIdread could non-deterministically resolve that stale row.apps/web/machine-branches-runtime.tsandapps/realtime/index.ts(via a sharedresolveDriveOwnerContexthelper) both resolve the current driveId + owner first.Test plan
bun run typecheck— all 16 packages passbun run build(web) — passespackages/libfull suite (vitest run) — 357 files / 8193 tests passapps/realtimefull suite (vitest run) — 23 files / 832 tests passCoverage includes: credential + config copy on spawn and on attach; no-op when no root session, no credential yet, or an empty-but-ambiguous root read; resolver-throws resilience; refreshed-credential-on-reattach; 0o600 mode via temp-then-atomic-rename; concurrency-safety regression tests for row-persisted-before-copy, a stalled overlapping copy skipping its own stale rename, and a failed rename leaving an existing valid credential untouched; branch-vs-machine-vs-project scope gating in the realtime path; the
refreshBranchCredentialdep being optional and defense-in-depth against a throwing/never-settling implementation (fake-timers tests); every housekeeping exec carrying a boundedtimeoutMs/maxBytes; and current-session-key resolution across a simulated drive move.Review history
This PR went through an extensive multi-round automated review cycle (CodeRabbit + Codex) that surfaced and fixed a series of real concurrency/security issues in the credential-copy path — each caught a genuine gap in the previous fix, converging on the write-to-temp-then-atomic-rename design described above. Final Codex pass: "Didn't find any major issues." All CI green, no unresolved merge conflicts.
Summary by CodeRabbit
New Features
Bug Fixes