Repository navigation
feat(terminal): real PTY terminal via xterm.js + Sprites Socket.IO relay - #1672
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughReplaces the Gridland-based terminal with a socket-driven PTY terminal. The realtime server acquires Fly Sprites sandboxes, spawns a bash PTY, and routes I/O over Socket.IO events. The web client introduces an ChangesReal-time PTY Terminal Feature
Sequence DiagramsequenceDiagram
participant Browser as XtermTerminal
participant SocketIO as Socket.IO
participant RealtimeIndex as index.ts (realtime)
participant makeTerminalCheckAuth
participant SpritesSandbox as Fly Sprites Sandbox
participant openPtyShell
Browser->>SocketIO: terminal:connect {pageId, cols, rows}
SocketIO->>RealtimeIndex: onConnect handler
RealtimeIndex->>makeTerminalCheckAuth: checkAuth(userId, pageId)
makeTerminalCheckAuth->>RealtimeIndex: page/permission/drive checks
makeTerminalCheckAuth->>SpritesSandbox: acquireTerminalSandbox
SpritesSandbox-->>makeTerminalCheckAuth: {sandboxId, sprite}
makeTerminalCheckAuth-->>RealtimeIndex: {ok: true, sandboxId, sprite}
RealtimeIndex->>openPtyShell: spawn bash (cols, rows) on sprite
openPtyShell-->>RealtimeIndex: PtyShell
RealtimeIndex->>SocketIO: emit terminal:ready
SocketIO->>Browser: terminal:ready → onReady()
openPtyShell->>SocketIO: emit terminal:output (stdout/stderr)
SocketIO->>Browser: terminal:output → terminal.write(data)
Browser->>SocketIO: terminal:input {data}
SocketIO->>openPtyShell: shell.write(data)
Browser->>SocketIO: terminal:resize {cols, rows}
SocketIO->>openPtyShell: shell.resize(cols, rows)
Browser->>SocketIO: terminal:disconnect (unmount)
SocketIO->>openPtyShell: shell.kill(SIGKILL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: ae01a7b06a
ℹ️ 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".
|
|
||
| export async function getRealtimeSpritesSdk(): Promise<SpritesSdk> { | ||
| if (cachedSdk) return cachedSdk; | ||
| const { SpritesClient } = await import('@fly/sprites'); |
There was a problem hiding this comment.
In the production image the realtime service is compiled with tsc and run from dist by Node, while apps/realtime/tsconfig.json sets module: commonjs; TypeScript lowers this dynamic import to a CommonJS require('@fly/sprites'). The repo already treats @fly/sprites as ESM-only, so the first terminal:connect in the built realtime container will fail with ERR_REQUIRE_ESM before a shell can open. Keep this boundary as native ESM/dynamic import or move the SDK construction into an ESM-built module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The production runtime is Node 24 (which has native require(esm) support, unflagged since Node 22.12). TypeScript compiling import() to require() via the commonjs module target works correctly on this runtime — there is no ERR_REQUIRE_ESM. Reverted the new Function() workaround; the simpler import("@fly/sprites") is sufficient with a one-line comment explaining why.
There was a problem hiding this comment.
Not a bug with the current setup. TypeScript 5.8 with module: commonjs preserves dynamic import() expressions as-is in the emitted JavaScript — it only lowers static import X from 'Y' to require(). The dynamic await import('@fly/sprites') in the compiled output calls Node's native ESM loader, which works correctly from a CJS context.
Production runs Node 22.17.0 (see apps/realtime/Dockerfile: FROM node:22.17.0-alpine). Node 22.12+ enabled require(esm) by default, so even if TypeScript did emit a require() call here, it would work. The PR commit message said "Node 24" — that was imprecise; 22.17.0 is what we actually run, and it's sufficient.
No code change needed. The pattern is consistent with how apps/web handles the same ESM-only package via apps/web/src/lib/sandbox/sprites-client.ts.
| const codeAuth = await canRunCode({ userId, driveId, requestOrigin: 'user' }); | ||
| if (!codeAuth.ok) return { ok: false, reason: codeAuth.reason }; |
There was a problem hiding this comment.
This new realtime auth path only checks canRunCode before handing out a long-lived shell, but the deleted HTTP terminal route also enforced checkCodeExecutionQuota, per-user concurrency slots, budget charging, and code-execution audit records for executed commands. With the PTY path, an authorized user can run unlimited commands through terminal:input without any quota/billing/audit coverage, bypassing the production controls that still protect the other sandbox execution paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged as a genuine gap. The PTY terminal is currently admin-only at the canRunCode gate (role check via drive permissions) and each shell is tied to a sandbox acquired through acquireTerminalSandbox, which enforces session uniqueness. The quota/billing/audit controls from the deleted batch route (per-command charging and audit records) are not directly applicable to a long-lived PTY session — you cannot measure "how much code ran" per keystroke. Tracking this as a follow-up: add a session-open audit event and periodic idle timeout. The admin gate significantly limits exposure in the interim.
There was a problem hiding this comment.
Fixed in commit c4769a8f0. The PTY connect path now has full quota and audit coverage:
- Concurrency slot:
acquireCodeExecutionSlot({ userId, tier })is called before the sandbox is acquired. If at capacity, the connect is denied withconcurrency_limit. The slot is threaded throughCheckAuthResult.releaseSlotand stored onTerminalSession, then released on shell exit, reconnect cleanup, or socket disconnect — including the error path whereopenShellthrows. - Budget charge:
chargeCodeExecutionBudget({ userId, driveId, tenantId })is called fire-and-forget after successful sandbox acquire. - Audit record:
writeCodeExecutionAudit()is called on session open (PTY session, originuser).
The user's subscriptionTier is fetched alongside the drive row to determine the tier for quota calculations.
| onInput(payload: unknown) { | ||
| const session = sessionMap.get(socket.id); | ||
| if (!session) return; | ||
| const p = payload as { data?: string }; | ||
| if (typeof p?.data === 'string') session.command.write(p.data); |
There was a problem hiding this comment.
After terminal:connect succeeds, every later terminal:input writes directly to the PTY without re-checking page access or the code-execution role. If a user's page/drive permissions are revoked while the Socket.IO connection remains open, the already-open shell can keep receiving keystrokes and executing code; the existing document_update path is wrapped with per-event auth specifically to close this revocation window. Reauthorize input or close the terminal when access is revoked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern. PTY sessions (like SSH) traditionally do not re-auth on every keystroke since the entire stream is authenticated at connection time and the session lifetime is tied to the transport. The practical mitigation: when the WebSocket disconnects (including on token expiry or forced kick), the PTY is killed immediately. Re-checking permissions on every terminal:input would add significant DB load and is not how any interactive shell protocol works. Tracking as a follow-up: add server-side eviction when a user's drive role is changed (subscribe to a role-revoked event and kill the terminal session for affected socket IDs).
There was a problem hiding this comment.
Fixed in commit 5bc2a42c7. A 60-second re-authorization interval is started inside onConnect after the shell opens. On every tick, checkAuth is re-invoked. If it returns ok: false, the interval clears itself, the slot is released, the shell receives SIGKILL, the session is removed from the map, and the client receives terminal:closed with exitCode: -2.
The interval is also cleared on natural shell exit (onExit) and on explicit disconnect (onDisconnect), so no dangling timers.
Tests added for: interval is registered on the session, re-auth success keeps session alive, re-auth failure kills the session and emits the correct event.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/layout/middle-content/page-views/terminal/TerminalView.tsx (1)
35-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t mount the terminal when the user is not admin.
The warning banner is shown, but
XtermTerminalstill mounts and attemptsterminal:connect, causing avoidable denied requests and a perpetual connecting overlay.Suggested fix
- {socket && ( + {isAdmin && socket && ( <XtermTerminal socket={socket} pageId={pageId} onReady={handleReady} onError={handleError} /> )} - {!connected && ( + {isAdmin && !connected && ( <div className="absolute inset-0 bg-black flex items-center justify-center">Also applies to: 43-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/layout/middle-content/page-views/terminal/TerminalView.tsx` around lines 35 - 41, The XtermTerminal component is being mounted unconditionally even when the user is not an admin, which causes unnecessary connection attempts and a perpetual connecting overlay. Wrap the XtermTerminal component rendering (referenced in the code section around lines 43-60) with a conditional check so that it only mounts when isAdmin is true, similar to how the warning banner is conditionally shown for non-admin users. This prevents the terminal from attempting to connect when the user lacks administrator privileges.
🧹 Nitpick comments (3)
apps/realtime/src/terminal/__tests__/validation.test.ts (1)
4-80: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd regression tests for
NaN/Infinitydimension inputs.The current suite misses finite-number edge cases, so the validation gap can regress unnoticed. Add explicit cases for
cols: NaN,rows: NaN,cols: Infinity, androws: Infinity.Suggested test additions
describe('validateTerminalConnectPayload', () => { @@ + it('given cols is NaN, should return ok:false', () => { + const result = validateTerminalConnectPayload({ pageId: 'abc', cols: Number.NaN, rows: 24 }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toBe('invalid cols'); + }); + + it('given rows is Infinity, should return ok:false', () => { + const result = validateTerminalConnectPayload({ pageId: 'abc', cols: 80, rows: Number.POSITIVE_INFINITY }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toBe('invalid rows'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/realtime/src/terminal/__tests__/validation.test.ts` around lines 4 - 80, The test suite for validateTerminalConnectPayload is missing regression tests for edge cases involving NaN and Infinity values. Add four new test cases to the describe block for validateTerminalConnectPayload that verify the validation function correctly rejects cols and rows parameters when they are NaN or Infinity, each expecting ok:false with the appropriate error message (invalid cols or invalid rows). This ensures that the validator properly handles these finite-number edge cases to prevent future regressions.apps/realtime/src/terminal/__tests__/sprites-shell.test.ts (1)
43-120: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd tests for command-error and missing-stdin paths.
The suite currently covers only happy-path PTY wiring. Please add cases for command
errorpropagation and foropenPtyShellthrowing whenstdinis missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/realtime/src/terminal/__tests__/sprites-shell.test.ts` around lines 43 - 120, Add two new test cases to the openPtyShell describe block: first, add a test that verifies when the command emits an error event (similar to how cmd._emitter.emit('exit', 0) is used), the error is properly propagated or handled by onExit or through an error callback; second, add a test that verifies openPtyShell throws an error when the command's stdin property is undefined or missing, ensuring the function validates that stdin exists before attempting to use it.apps/realtime/src/terminal/__tests__/terminal-handler.test.ts (1)
36-127: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd regression tests for reconnect replacement and resize sanitization.
The suite does not currently assert behavior for (1) double
onConnecton the same socket and (2) invalid resize payloads (NaN/Infinity/negative/out-of-range). Adding these would lock in the intended safety behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/realtime/src/terminal/__tests__/terminal-handler.test.ts` around lines 36 - 127, Add two new regression test cases to lock in safety behavior: first, in the onConnect describe block, add a test that verifies calling onConnect twice on the same socket replaces the previous session (closing the old shell and storing only the new one), and second, in the onResize describe block, add test cases that verify invalid resize payloads such as NaN, Infinity, negative values, and out-of-range dimensions are handled safely without throwing or causing state corruption, either by sanitizing the values or emitting an error appropriately.
🤖 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/index.ts`:
- Around line 954-955: In the socket listener for 'terminal:connect' event where
terminalHandlers.onConnect is called, remove the void operator that is currently
suppressing the promise, and add error handling using .catch() to properly
handle any rejections from the onConnect method. This ensures that if
authentication, database, or sprite initialization fails, the rejection is
caught rather than becoming an unhandled promise rejection, allowing you to send
a deterministic error event to the client.
In `@apps/realtime/src/terminal/sprites-shell.ts`:
- Around line 20-23: The code in the sprites-shell.ts file currently only
listens to the 'exit' event on the cmd object but does not handle the 'error'
event. Add an error event listener to the cmd object that routes command errors
through the same onExit callback used for the exit event to ensure consistent
session cleanup and teardown when command failures occur. This will guarantee
deterministic error handling and prevent the session from being left in an
undefined state.
- Line 25: The write property in the openPtyShell function uses a non-null
assertion on cmd.stdin which defers validation until the first write call.
Instead, validate that cmd.stdin exists immediately after the spawn call and
throw an error early if it is undefined or missing. This allows the caller to
handle initialization failures cleanly rather than having the crash occur later
during the first write operation. Remove the non-null assertion and add an early
validation check that throws a descriptive error if stdin is not available as
expected.
In `@apps/realtime/src/terminal/terminal-handler.ts`:
- Around line 52-70: When a socket reconnects and the terminal session is
created, check if an existing session already exists in sessionMap for that
socket.id before setting the new one. If an old session exists, properly
terminate the old shell by calling its cleanup or close method to prevent orphan
PTY processes. Only after cleaning up the old session should you create and
store the new session in sessionMap using sessionMap.set(socket.id, ...).
- Around line 80-86: The onResize method currently passes raw numeric values
(p.cols and p.rows) to session.command.resize without validating they are valid
dimension values. While the typeof checks ensure they are numbers, they do not
prevent invalid values like NaN, Infinity, negative numbers, or excessively
large values. Find the validation logic used in the connect path that already
clamps dimensions to valid bounds, and apply the same clamping validation to
p.cols and p.rows in the onResize method before passing them to
session.command.resize to ensure consistency and prevent invalid resize
operations.
In `@apps/realtime/src/terminal/validation.ts`:
- Around line 15-21: The validation logic for terminal dimensions (cols and
rows) currently allows NaN and Infinity to pass because they are technically of
type number, but these are invalid for TTY dimensions. Add Number.isFinite()
checks alongside the existing typeof and <= 0 validations for both p.cols and
p.rows to ensure they are finite numbers. Update the validation conditions in
the blocks checking cols (around line 15) and rows (around line 18) to reject
non-finite values before they propagate to clampTerminalDimensions() and
sprite.spawn().
In
`@apps/web/src/components/layout/middle-content/page-views/terminal/XtermTerminal.tsx`:
- Around line 36-52: The socket event listeners for terminal:output,
terminal:ready, terminal:closed, and terminal:error are being registered after
the terminal:connect event is emitted, which can cause early events from the
server to be missed. Move all the socket.on() calls for handleOutput,
handleReady, handleClosed, and handleError to execute before the socket.emit()
call for terminal:connect so that the handlers are attached and ready to receive
events immediately.
- Around line 22-73: The async IIFE starting at line 22 lacks error handling for
dynamic imports and terminal initialization. Wrap the entire async function body
in a try/catch block and in the catch handler, call the onError callback
(similar to how handleError is implemented) with the error message. This ensures
that if imports fail or terminal setup fails, the parent component receives an
error notification instead of remaining stuck in a loading state.
---
Outside diff comments:
In
`@apps/web/src/components/layout/middle-content/page-views/terminal/TerminalView.tsx`:
- Around line 35-41: The XtermTerminal component is being mounted
unconditionally even when the user is not an admin, which causes unnecessary
connection attempts and a perpetual connecting overlay. Wrap the XtermTerminal
component rendering (referenced in the code section around lines 43-60) with a
conditional check so that it only mounts when isAdmin is true, similar to how
the warning banner is conditionally shown for non-admin users. This prevents the
terminal from attempting to connect when the user lacks administrator
privileges.
---
Nitpick comments:
In `@apps/realtime/src/terminal/__tests__/sprites-shell.test.ts`:
- Around line 43-120: Add two new test cases to the openPtyShell describe block:
first, add a test that verifies when the command emits an error event (similar
to how cmd._emitter.emit('exit', 0) is used), the error is properly propagated
or handled by onExit or through an error callback; second, add a test that
verifies openPtyShell throws an error when the command's stdin property is
undefined or missing, ensuring the function validates that stdin exists before
attempting to use it.
In `@apps/realtime/src/terminal/__tests__/terminal-handler.test.ts`:
- Around line 36-127: Add two new regression test cases to lock in safety
behavior: first, in the onConnect describe block, add a test that verifies
calling onConnect twice on the same socket replaces the previous session
(closing the old shell and storing only the new one), and second, in the
onResize describe block, add test cases that verify invalid resize payloads such
as NaN, Infinity, negative values, and out-of-range dimensions are handled
safely without throwing or causing state corruption, either by sanitizing the
values or emitting an error appropriately.
In `@apps/realtime/src/terminal/__tests__/validation.test.ts`:
- Around line 4-80: The test suite for validateTerminalConnectPayload is missing
regression tests for edge cases involving NaN and Infinity values. Add four new
test cases to the describe block for validateTerminalConnectPayload that verify
the validation function correctly rejects cols and rows parameters when they are
NaN or Infinity, each expecting ok:false with the appropriate error message
(invalid cols or invalid rows). This ensures that the validator properly handles
these finite-number edge cases to prevent future regressions.
🪄 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: 1269448e-0321-4f58-a901-2140e0520a2b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
apps/realtime/package.jsonapps/realtime/src/index.tsapps/realtime/src/terminal/__tests__/sprites-shell.test.tsapps/realtime/src/terminal/__tests__/terminal-handler.test.tsapps/realtime/src/terminal/__tests__/terminal-session-map.test.tsapps/realtime/src/terminal/__tests__/validation.test.tsapps/realtime/src/terminal/realtime-sprites-client.tsapps/realtime/src/terminal/sprites-shell.tsapps/realtime/src/terminal/terminal-handler.tsapps/realtime/src/terminal/terminal-session-map.tsapps/realtime/src/terminal/validation.tsapps/web/next.config.tsapps/web/package.jsonapps/web/src/app/api/pages/[pageId]/terminal/execute/__tests__/route.test.tsapps/web/src/app/api/pages/[pageId]/terminal/execute/route.tsapps/web/src/app/globals.cssapps/web/src/components/layout/middle-content/page-views/terminal/GridlandTerminal.tsxapps/web/src/components/layout/middle-content/page-views/terminal/TerminalView.tsxapps/web/src/components/layout/middle-content/page-views/terminal/XtermTerminal.tsxapps/web/src/webpack-loaders/__tests__/gridland-process-nexttick.test.tspackages/lib/src/services/sandbox/sandbox-client/sprites.ts
💤 Files with no reviewable changes (5)
- apps/web/src/app/api/pages/[pageId]/terminal/execute/tests/route.test.ts
- apps/web/src/app/api/pages/[pageId]/terminal/execute/route.ts
- apps/web/src/webpack-loaders/tests/gridland-process-nexttick.test.ts
- apps/web/next.config.ts
- apps/web/src/components/layout/middle-content/page-views/terminal/GridlandTerminal.tsx
…rm.js + Sprites - Remove batch HTTP execute/route.ts and GridlandTerminal — no persistent shell state - Add PTY relay in apps/realtime: validation, session map, sprites-shell, terminal-handler - Wire terminal:connect/input/resize/disconnect Socket.IO events into realtime/src/index.ts - Add XtermTerminal.tsx: dynamic xterm.js + FitAddon, ResizeObserver auto-reflow - Rewrite TerminalView.tsx: drop document-save logic, use Socket.IO + XtermTerminal - Extend SpriteCommandLike/SpriteInstanceLike interfaces for PTY (stdin, resize, tty options) - Add @xterm/xterm + @xterm/addon-fit to apps/web, @fly/sprites to apps/realtime - Remove @gridland/web + @gridland/utils + their webpack shims Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7kNHMm64nVgU5RySdim3K
- validation: reject NaN/Infinity in cols/rows (Number.isFinite guard) - sprites-shell: add error event handler, null-safe stdin.write, null-coalesce exit code - terminal-handler: kill existing PTY session before opening a new one on reconnect - terminal-handler: clamp + validate resize payloads via clampTerminalDimensions - index.ts: catch rejected onConnect promises and emit terminal:error instead of unhandled rejection - XtermTerminal: register socket listeners before emitting terminal:connect (avoid missed events) - XtermTerminal: wrap async setup in try/catch, forward errors to onError callback - realtime-sprites-client: revert to simple import() — production runs Node 24 which supports require(esm) natively - tests: add coverage for NaN/Infinity, error event, null stdin, reconnect kill, resize clamp Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7kNHMm64nVgU5RySdim3K
ae01a7b to
6ab9749
Compare
Lock file was generated with bun 1.2.15; regenerating to match CI's bun 1.3.x. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7kNHMm64nVgU5RySdim3K
2witstudios
left a comment
2witstudios
left a comment
There was a problem hiding this comment.
Code review for — real PTY terminal via xterm.js + Sprites Socket.IO relay.
Architecture and test coverage are solid. Two must-fix items in TerminalView.tsx, a few should/nice-to-have items inline. See individual comments.
| /> | ||
| </div> | ||
| <div className="flex-1 min-h-0 relative"> | ||
| {socket && ( |
There was a problem hiding this comment.
🔴 Must — Client-side admin gate doesn't block XtermTerminal from mounting
XtermTerminal mounts and fires socket.emit('terminal:connect', ...) regardless of isAdmin. A non-admin user sees the warning banner and the "Connecting to shell…" spinner simultaneously, then waits indefinitely because the server correctly denies them.
The server-side canRunCode gate is the real enforcement; this is a UX fix, not a security one. Change the condition:
// Before
{socket && (
<XtermTerminal ... />
)}
// After — only mount when admin
{socket && isAdmin && (
<XtermTerminal ... />
)}There was a problem hiding this comment.
Fixed in commit b881d63db. Changed {socket && ( to {socket && isAdmin && ( on line 47 — XtermTerminal no longer mounts for non-admin users. Non-admins see only the warning banner; no socket event is emitted.
| exit={{ opacity: 0 }} | ||
| className="absolute inset-0 bg-background/80 backdrop-blur-sm flex items-center justify-center" | ||
| > | ||
| {!connected && ( |
There was a problem hiding this comment.
🔴 Must — Spinner persists indefinitely on server error
setConnected(true) only fires on terminal:ready. If the server sends terminal:error instead (auth denied, sandbox unavailable), the overlay stays spinning — the toast fires but the loading state never clears.
Need an error state here. Simplest approach: add a second state const [error, setError] = useState<string | null>(null) and update handleError to set it, then branch the overlay:
{!connected && (
<div className="absolute inset-0 bg-black flex items-center justify-center">
{error ? (
<span className="text-sm text-red-400">{error}</span>
) : (
<div className="flex items-center gap-2">
<div className="w-4 h-4 border-2 border-green-400 border-t-transparent rounded-full animate-spin" />
<span className="text-sm text-green-400">Connecting to shell...</span>
</div>
)}
</div>
)}There was a problem hiding this comment.
Fixed in commit b881d63db. Added const [error, setError] = useState<string | null>(null). handleError now calls setError(message) before toasting. The connecting overlay renders <span className="text-sm text-red-400">{error}</span> on error state instead of the spinner, so a terminal:error response clears the loader and shows the message.
| .from(drives) | ||
| .where(eq(drives.id, driveId)) | ||
| .limit(1); | ||
| const tenantId = driveRow?.ownerId ?? ''; |
There was a problem hiding this comment.
🟡 Should — Bail early if drive row not found
If a drive has been deleted but the page row still references its ID, driveRow is undefined and tenantId falls through as ''. The session key still derives uniquely from pageId + driveId + secret, so this isn't exploitable, but it's a silent degradation that's better to surface explicitly:
if (!driveRow) return { ok: false, reason: 'drive_not_found' };
const tenantId = driveRow.ownerId;There was a problem hiding this comment.
Fixed in commit c4769a8f0. Replaced const tenantId = driveRow?.ownerId ?? '' with an explicit early return: if (!driveRow) return { ok: false, reason: 'drive_not_found' }, then const tenantId = driveRow.ownerId with no nullable fallback. Auth denial is also logged via loggers.realtime.warn.
| .limit(1); | ||
| const tenantId = driveRow?.ownerId ?? ''; | ||
|
|
||
| const [store, sdk] = await Promise.all([createDbTerminalSessionStore(), getRealtimeSpritesSdk()]); |
There was a problem hiding this comment.
🟡 Should — Cache the session store factory
createDbTerminalSessionStore() creates a new wrapper object on every terminal:connect. The factory is cheap (lazy-import is a no-op after the first call), but it's inconsistent with how the SDK is handled. getRealtimeSpritesSdk() caches — this should too.
Move construction to module level alongside terminalSessionMap:
// top of file, near terminalSessionMap
const dbTerminalSessionStorePromise = createDbTerminalSessionStore();
// inside makeTerminalCheckAuth
const [store, sdk] = await Promise.all([dbTerminalSessionStorePromise, getRealtimeSpritesSdk()]);There was a problem hiding this comment.
Fixed in commit c4769a8f0. createDbTerminalSessionStore() is now called once at module level: const dbTerminalSessionStorePromise = createDbTerminalSessionStore(), consistent with getRealtimeSpritesSdk(). The Promise.all inside makeTerminalCheckAuth awaits the cached promise.
| const session = sessionMap.get(socket.id); | ||
| if (!session) return; | ||
| const p = payload as { data?: string }; | ||
| if (typeof p?.data === 'string') session.command.write(p.data); |
There was a problem hiding this comment.
🟡 Should — No size limit on terminal:input data
p.data is written directly to stdin with no length check. A malicious (but authenticated, admin) user could send multi-MB strings per frame. Low severity given the admin-only gate, but worth a guard at the socket layer:
const MAX_INPUT_BYTES = 4096;
if (typeof p?.data === 'string' && p.data.length <= MAX_INPUT_BYTES) {
session.command.write(p.data);
}There was a problem hiding this comment.
Fixed in commit 5bc2a42c7. Added export const MAX_INPUT_BYTES = 4096 and guarded onInput with p.data.length <= MAX_INPUT_BYTES. Frames exceeding 4 KB are silently dropped. The constant is exported so tests can reference it directly.
|
|
||
| const terminalSessionMap = createTerminalSessionMap(); | ||
|
|
||
| async function makeTerminalCheckAuth({ userId, pageId }: { userId: string; pageId: string }): Promise<CheckAuthResult> { |
There was a problem hiding this comment.
🟡 Should — Add logging for terminal auth events
Auth denials and successful connections emit no log entries. Every other socket event in this file uses loggers.realtime. A silent denial makes incident response harder.
// on denial
loggers.realtime.warn('Terminal auth denied', { reason: authResult.reason, userId, pageId });
// on successful connect
loggers.realtime.info('Terminal session opened', { sandboxId: authResult.sandboxId, userId, pageId });
// on exit
loggers.realtime.info('Terminal session closed', { exitCode, sandboxId, socketId: socket.id });There was a problem hiding this comment.
Fixed in commit c4769a8f0. loggers.realtime.warn is now called on every auth denial path in makeTerminalCheckAuth (page_not_found, no_edit_access, canRunCode denial, drive_not_found, sandboxResult failure, concurrency_limit). loggers.realtime.info is called on successful session open and on shell exit.
| @@ -1,4 +1,5 @@ | |||
| @import "../styles/tiptap.css"; | |||
| @import "@xterm/xterm/css/xterm.css"; | |||
There was a problem hiding this comment.
🟢 Nice — xterm CSS loaded on every page
This stylesheet is now bundled into the global CSS for all routes, not just terminal pages. Since XtermTerminal is already dynamic() (no SSR), the CSS import could live in XtermTerminal.tsx or TerminalView.tsx to co-locate it with the component that needs it, though note xterm requires the stylesheet to be present before terminal.open() — so a co-located import must resolve before that call. If Next.js handles that ordering correctly with a co-located import, moving it is a clean win.
There was a problem hiding this comment.
Fixed in commit ba72ef4b6. Removed @import "@xterm/xterm/css/xterm.css" from globals.css and added import '@xterm/xterm/css/xterm.css' at the top of TerminalView.tsx (after "use client"). The stylesheet is now bundled only for routes that render the terminal.
| cancelled = true; | ||
| teardown?.(); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
🟢 Nice — Document why onReady/onError are excluded from deps
The suppress is correct — onReady and onError are stable via useCallback in TerminalView, so excluding them is safe. But a future caller who forgets useCallback would introduce a re-init loop with no obvious cause.
A short comment explaining the invariant costs nothing:
// onReady/onError omitted — callers must stabilise them with useCallback.
// Including them would cause the terminal to remount on every parent render.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [socket, pageId]);There was a problem hiding this comment.
Fixed in commit ba72ef4b6. Added two comment lines above the eslint-disable-next-line in XtermTerminal.tsx explaining that onReady/onError are intentionally excluded because callers must stabilise them with useCallback, and including them would cause the terminal to remount on every parent render.
…d of infinite spinner - Only mount XtermTerminal when user is admin (non-admins see warning only) - Track error state so terminal:error replaces the spinner with an error message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
…ect dep omissions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
… sessions - Reject terminal:input frames exceeding 4096 bytes (MAX_INPUT_BYTES) - Start 60s re-auth interval on session open; kill shell if auth is revoked - Update TerminalSession type to carry the interval for proper cleanup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
…caching, auth logging - Bail early with drive_not_found when drive row is missing (instead of empty tenantId) - Cache createDbTerminalSessionStore promise at module level (consistent with SDK caching) - Add loggers.realtime.warn/info for auth denials, session open, and session close - Acquire concurrency slot on connect; release it on exit or disconnect - Charge sliding-window budget and write code-execution audit record on session open - Thread releaseSlot through CheckAuthResult -> TerminalSession for proper cleanup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
…ease assertions - Add makeAuthSuccess() helper that includes releaseSlot: vi.fn() - Update beforeEach mock to use makeAuthSuccess() so re-auth interval works - Add afterEach vi.useRealTimers() cleanup to prevent timer leakage between tests - Assert releaseSlot is called on disconnect, reconnect, and openShell-throws cases - Add test: openShell throws → slot is released before returning Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
Non-admin users would see both the 'requires administrator privileges' banner and the 'Connecting to shell...' spinner (which never resolved, since XtermTerminal was never mounted). Gate the overlay on isAdmin so non-admins only see the banner. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
…ode version comment The function was exported but never imported — dead code. Also corrected the 'Node 24' comment: production runs Node 22.17.0, and the reason dynamic import() works from CJS is that TS 5.8 preserves import() as-is (doesn't lower to require()). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx
1 participant
Summary
Architecture
```
Browser (xterm.js)
↕ Socket.IO events (terminal:connect/input/resize/output/ready/closed/error)
apps/realtime (new terminal handler)
↕ @fly/sprites WebSocket
Sprites VM (bash PTY)
```
Changes
`apps/realtime/src/terminal/` (new, TDD with 100+ tests):
`apps/realtime/src/index.ts`: wires `terminal:connect/input/resize/disconnect` events; catches rejected `onConnect` promises and emits `terminal:error` to the client; `makeTerminalCheckAuth` handles `canRunCode` + `acquireTerminalSandbox`; integrates `acquireCodeExecutionSlot` (concurrency cap), `chargeCodeExecutionBudget`, and `writeCodeExecutionAudit`; warns on all auth denial paths; caches session store factory at module level
`apps/web`:
`packages/lib/src/services/sandbox/sandbox-client/sprites.ts`: extended interfaces for PTY (`SpriteSpawnOptions` with `tty/rows/cols`, optional `stdin` + `resize` on `SpriteCommandLike`)
Security / Access
Terminal sessions require `canRunCode` (drive permission check) + page edit access via `acquireTerminalSandbox`. Concurrency slot acquired at session open, released on disconnect/shell exit/re-auth revocation. Session lifetime is tied to the WebSocket connection — PTY is killed on disconnect. Re-auth fires every 60s and terminates the session if permissions have been revoked.
Test plan
🤖 Generated with Claude Code
https://claude.ai/code/session_017tpbkTmvziD9iDk7U4nuJx