Repository navigation
[sprites 1-2] Split checkAuth: pure access decision, lazy sprite resolution - #2000
Conversation
… lazy sprite [sprites 1-2]
Splits the fused makeAgentTerminalCheckAuth into two jobs: a pure access
decision over gathered DB data (decideAgentTerminalAccess) and a lazy sprite
resolution (resolveTerminalSandbox / resolveAgentTerminalSandbox) that runs
only when a fresh PTY must be created. A Sprite is woken automatically by any
exec (docs.sprites.dev/concepts/lifecycle), so authorization never needs to
touch it — the access half now performs zero sprite SDK calls, letting the
re-auth interval (leaf 3-1) re-check access without re-resolving the Sprite.
- New apps/realtime/src/terminal/agent-terminal-access.ts:
- decideAgentTerminalAccess(inputs): pure, gate order preserved byte-for-byte
(no_edit_access -> page_not_found -> canRunCode reason -> drive_not_found ->
concurrency_limit), surfacing driveId/payerId on allow.
- resolveTerminalSandbox(target, deps): single getSprite; provision_failed on
a vanished Sprite (full getSprite collapse is leaf 1-4).
- buildAgentTerminalCheckAuth(deps): DI factory composing both halves.
- index.ts wires the real IO deps; deny reasons, released slots and the socket
error surface are unchanged.
TDD: pure core tested with no mocks; shells tested with injected fakes
(18 new tests). Full realtime suite green (601 tests), typecheck + lint clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XrMtPG59TTgGX4rYzGCr1m
📝 WalkthroughWalkthroughAgent-terminal authorization is split into pure access decisions and lazy Sprite resolution. Realtime wiring now composes these stages through injected callbacks, with expanded tests covering denials, slot handling, sandbox failures, auditing, and successful PTY payload assembly. ChangesAgent terminal authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant makeAgentTerminalCheckAuth
participant buildAgentTerminalCheckAuth
participant resolveAgentTerminalSandbox
participant SpriteSDK
Client->>makeAgentTerminalCheckAuth: request terminal access
makeAgentTerminalCheckAuth->>buildAgentTerminalCheckAuth: evaluate access and reserve slot
buildAgentTerminalCheckAuth->>resolveAgentTerminalSandbox: resolve terminal sandbox
resolveAgentTerminalSandbox->>SpriteSDK: getSprite
SpriteSDK-->>resolveAgentTerminalSandbox: Sprite
resolveAgentTerminalSandbox-->>buildAgentTerminalCheckAuth: launch data and Sprite
buildAgentTerminalCheckAuth-->>Client: PTY session payload
Possibly related PRs
🚥 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 |
…verage gate CI's apps/realtime test:coverage failed at 97.93% branch (threshold 98%): the new agent-terminal-access.ts had uncovered branches — the pageRow-missing path, the non-provision_failed sandbox-failure path, and the tier/email fallbacks. Adds three tests covering them; the module is now 100% stmts/branch/funcs/lines and the global branch coverage clears the 98% gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrMtPG59TTgGX4rYzGCr1m
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@apps/realtime/src/terminal/agent-terminal-access.ts`:
- Around line 218-223: Ensure the terminal setup flow releases the acquired slot
when deps.resolveSandbox rejects: wrap the resolveSandbox call and subsequent
handling in try/catch, call releaseSlot exactly once on rejection, then rethrow
the original error to preserve socket error behavior. Keep the existing
!sandbox.ok cleanup intact without double release, and add a regression test for
the rejecting resolver that verifies releaseSlot is called once.
🪄 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: 6170adc1-659c-4c02-aa0c-f2b1c39f982d
📒 Files selected for processing (3)
apps/realtime/src/index.tsapps/realtime/src/terminal/__tests__/agent-terminal-access.test.tsapps/realtime/src/terminal/agent-terminal-access.ts
…olution rejects CodeRabbit (Major): resolveSandbox can reject during its DB/Sprite work (a failed store/SDK lookup or a DB error inside resolveAgentTerminal), which bypasses the !sandbox.ok deny branch after the slot was already acquired, permanently consuming the user's concurrency capacity. The pre-split fused checkAuth had the same latent leak (it awaited resolveAgentTerminal with no try/catch), so this is not a regression — but the split is the right place to close it. Wrap the resolveSandbox await, release the slot on rejection, and re-throw so the socket surface is unchanged (onConnect's .catch still emits the generic error). Adds a test asserting the slot is released and the original error propagates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrMtPG59TTgGX4rYzGCr1m
…eckAuth + test hardening Addresses a high-effort review pass on the checkAuth split: 1. (correctness) Restore the fused checkAuth's sequential short-circuit input gathering: no page read without edit access, and no drive/user read once code execution is denied. The interim eager/parallel gather issued DB round-trips a denied attach then ignored and — worse — let a transient DB error on drives/users turn a clean, specific denial (e.g. code_execution_disabled) into a thrown generic connection error. Deny paths are byte-for-byte again. 2. (cleanup) Collapse the double decideAgentTerminalAccess invocation to a single read-only decision plus explicit slot acquisition (the slot is a reservation, not a read); removes the redundant re-evaluation. 3. (tests) Assert the provision_failed branch routes to logSandboxLookupFailed with the sandboxId (not logDenied); add short-circuit tests proving a code-exec / no-edit denial never queries the downstream tables. Counters live on the covered default deps so no never-called override arrows drag function coverage. apps/realtime test:coverage green (607 tests; agent-terminal-access.ts 100% stmts/branch/funcs/lines, global funcs 85.47% / branch 98.14%). typecheck + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrMtPG59TTgGX4rYzGCr1m
|
@coderabbitai review |
✅ Action performedReview finished.
|
What & why
makeAgentTerminalCheckAuth(apps/realtime/src/index.ts) fused two jobs: (a) deciding whether the user may attach (DB-only), and (b) resolving/waking the Sprite. Per the platform lifecycle model — a Sprite is woken automatically by any exec (docs.sprites.dev/concepts/lifecycle) — authorization never needs to touch the Sprite. This leaf splits (a) into a pure decision and (b) into a lazy resolver that runs only when a fresh PTY must be created, so leaf 3-1 can re-check access alone.New module:
apps/realtime/src/terminal/agent-terminal-access.tsdecideAgentTerminalAccess(inputs)— pure function over plain data (access level, page/drive rows,canRunCodeverdict, slot availability). SurfacesdriveId/payerIdon allow.resolveTerminalSandbox(target, deps)— resolves the agent-terminal row + reads the Sprite exactly once (full getSprite collapse is leaf 1-4).provision_failedon a vanished Sprite.buildAgentTerminalCheckAuth(deps)— DI factory composing both halves back into theAgentTerminalCheckAuthFnthe PTY bridge consumes. Gathers inputs sequentially with short-circuit (no page read without edit access; no drive/user read once code-exec is denied), decrypts the actor email before reserving the slot, and releases the slot on every failure path — matching the fused implementation byte-for-byte on the socket surface.index.tsnow wires the real IO dependencies into both halves (plus aresolveAgentTerminalSandboxshell binding the stores +buildMachineSandbox).Requirements → how satisfied
decideAgentTerminalAccesswith unit tests: no access level, canRunCode false, billing-slot exhaustion, happy pathbuildAgentTerminalCheckAuthtest injects aresolveSandboxwired through the realresolveTerminalSandboxwith a spygetSprite; onno_edit_access/concurrency_limitthe spy records 0 calls andresolveSandboxis never invokedresolveTerminalSandboxthat resolves the sprite exactly once (single getSprite)getSprite.calls.length === 1; resolve-failure path asserts 0 sprite readsreasonstrings in the same gate order (no_edit_access → page_not_found → canRunCode reason → drive_not_found → concurrency_limit); sequential short-circuit gather; slot released exactly once on every failure incl. aresolveSandboxrejection; actorEmail decrypted before slot reservation; existingagent-terminal-handler.test.ts(58) +index.test.ts(101) unchanged & greenReview-driven refinements (already applied)
resolveSandboxrejections (DB error insideresolveAgentTerminal, failed store/SDK lookup) now release the slot and re-throw, so a transient failure can't permanently consume concurrency capacity. Socket surface unchanged.Out of scope (untouched)
onConnect reordering (1-3), getSprite collapse + wake removal (1-4), re-auth interval change (3-1).
Rename note
Branched from current master (post-#1992): uses
PageType.MACHINE/machineIdthroughout; noterminalId/PageType.TERMINALreintroduced. No DB schema change.Test evidence
CI green (Unit Tests, Lint & TypeScript). Mergeable, no conflicts.
🤖 Generated with Claude Code