feat(web-ui): AgentTerminal — XTerm.js interactive terminal for session workspace (#506)#516
Conversation
… workspace (#506) - Backend: add `/ws/sessions/{id}/terminal` WebSocket endpoint in `terminal_ws.py` that spawns bash in the session's workspace_path and relays stdio - Register terminal_ws router in server.py - Frontend: `useTerminalSocket` hook — WebSocket lifecycle, binary/text relay, exponential-backoff reconnect, sendInput/sendResize helpers - Frontend: `AgentTerminal` component — XTerm.js with FitAddon, dark theme (#0a0a0c bg, #a855f7 cursor), ResizeObserver, ReconnectingOverlay - Tests: 7 pytest tests (auth, relay, cleanup) + 9 Jest tests for hook
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new backend WebSocket terminal endpoint that authenticates via JWT, validates session and user, spawns a shell subprocess in the session workspace and relays stdin/stdout; adds a client XTerm-based AgentTerminal component with a reconnecting useTerminalSocket hook, tests, and package dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Web Client (React)
participant WS as FastAPI WS Endpoint
participant DB as Database
participant Shell as Bash Subprocess
User->>Client: Open AgentTerminal
Client->>WS: WS connect /ws/sessions/{id}/terminal?token=JWT
WS->>DB: Validate JWT & load user + session
DB-->>WS: user & session record
alt auth & ownership valid
WS->>Shell: spawn bash in workspace_path (cwd, env)
WS-->>Client: websocket.accept()
loop I/O relay
User->>Client: keystroke
Client->>WS: frame (text/binary)
WS->>Shell: write stdin
Shell-->>WS: stdout bytes
WS-->>Client: relay bytes
Client->>User: terminal.write()
end
Client->>WS: {"type":"resize",cols,rows}
WS-->>WS: resize ignored (no PTY)
else auth fails
WS-->>Client: websocket.close(code, reason)
end
User->>Client: close/unmount
Client->>WS: close()
WS->>Shell: terminate/kill subprocess
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Comment |
…ble in useTerminalSocket.test.ts
|
Review comment for AgentTerminal + terminal_ws.py - test post |
|
Review: AgentTerminal + terminal_ws.py - Please see the full review below. (This is a placeholder — the full formatted review will follow as a separate comment due to sandbox formatting constraints.) |
|
CRITICAL - Server environment leaked to subprocess: In terminal_ws.py, os.environ.copy() passes the full server environment including ANTHROPIC_API_KEY, DATABASE_PATH, CODEFRAME_API_KEY_SECRET into the user-controlled bash session. An authenticated user can run env or printenv to exfiltrate all credentials. Fix: allowlist only TERM, HOME, PATH, LANG, USER. |
|
BUG - _text_stdin_relay is dead code: _text_stdin_relay is defined inside the handler but never scheduled as a task. Only _stdin_relay (binary) is launched via asyncio.create_task. Meanwhile, _stdin_relay catches all non-WebSocketDisconnect exceptions from receive_bytes() and silently breaks — which is exactly what Starlette does when a text frame arrives on a binary receive call. Text frames from clients are silently dropped. Since the frontend hook sends resize as JSON text, resize messages never reach the server. Fix: launch both relay coroutines as tasks and await all three, or unify into a single loop using websocket.receive() dispatching on event type. |
|
SECURITY - JWT in URL query parameter: The token in the WS URL query param ends up in server logs, browser history, and reverse-proxy logs. This matches the existing session_chat_ws.py pattern, so worth tracking as a follow-up. Standard mitigations: short-lived one-time token via REST before connecting, or auth as the first WS message after handshake. REACT - mutation during render: In AgentTerminal.tsx, wsUrlRef.current = buildWsUrl(sessionId) runs during render. This is fragile under React 18 strict mode (double-invokes renders). Move it into a useMemo or useEffect/useLayoutEffect. What is working well:
Minor: workspace_path falls back to the FastAPI server CWD when unset — consider rejecting sessions missing it. The close useCallback in useTerminalSocket omits maxRetries/retryDelay from deps (intentional, but a brief comment would help). Summary: Architecture and test coverage are in good shape. The two blockers are the env-var leak (trivial fix, high severity) and the dead text-relay task (explains why resize likely does not work end-to-end). Both are straightforward fixes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
codeframe/ui/routers/terminal_ws.py (1)
100-103: Consider handlingpausedsession state explicitly.Per the schema in
codeframe/persistence/schema_manager.py, valid states areactive,paused, andended. Currently, onlyendedis blocked. Consider whether terminal access should also be blocked forpausedsessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/terminal_ws.py` around lines 100 - 103, The session check currently only blocks "ended" sessions; update the conditional in the terminal WebSocket connection handling (where session is fetched via db.interactive_sessions.get using session_id) to also treat "paused" as disallowed—i.e., change the if to verify session is not None and that session.get("state") is not in ("ended", "paused") or equivalently check if session.get("state") in ("ended", "paused") then await websocket.close(... ) and return; ensure you reference the same websocket.close call and session/state keys so paused sessions are blocked like ended ones.web-ui/src/components/sessions/AgentTerminal.tsx (2)
38-51: Consider using Hugeicons instead of inline SVG.Per coding guidelines, the web UI should use
@hugeicons/reactrather than custom SVG icons. Check if Hugeicons provides a suitable loading spinner icon.As per coding guidelines: "Web UI must use Shadcn/UI (Nova preset) with gray color scheme and Hugeicons (
@hugeicons/react); never use lucide-react"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 38 - 51, Replace the inline SVG spinner in AgentTerminal (the <svg ...> block rendering the loading spinner) with the appropriate Hugeicons component from `@hugeicons/react` (e.g., the library's spinner/loading icon), import that icon at the top of the file and use it where the current svg is rendered, apply the component the same sizing/animation classes but switch to the gray color scheme (e.g., "animate-spin h-5 w-5 text-gray-400") to follow the Shadcn/UI Nova preset, and remove the inline SVG so no custom SVG or lucide-react icon remains.
77-80: Setting ref during render is unconventional.While this works for the initial mount, it's a side effect during render. Consider moving this initialization to a
useMemoor early in theuseEffectfor clarity.♻️ Alternative using useMemo
+import { useEffect, useRef, useMemo } from 'react'; - const wsUrlRef = useRef<string | null>(null); - - // Build the WS URL once per sessionId (requires client side) - if (typeof window !== 'undefined' && !wsUrlRef.current) { - wsUrlRef.current = buildWsUrl(sessionId); - } + const wsUrl = useMemo(() => { + if (typeof window === 'undefined') return null; + return buildWsUrl(sessionId); + }, [sessionId]); const { status, sendInput, sendResize } = useTerminalSocket({ - url: wsUrlRef.current, + url: wsUrl,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 77 - 80, In AgentTerminal, setting wsUrlRef.current during render is a side effect; move the initialization out of render by computing the value with useMemo (e.g., const wsUrl = useMemo(() => buildWsUrl(sessionId), [sessionId]) and assign to wsUrlRef.current inside a useEffect) or by assigning wsUrlRef.current at the start of a useEffect that depends on sessionId; update references that currently read wsUrlRef.current to use the memoized wsUrl or ensure the ref is set in the effect to avoid performing side effects during render.web-ui/src/hooks/useTerminalSocket.ts (2)
83-89: Consider caching the TextEncoder instance.A new
TextEncoderis instantiated on every string message. SinceTextEncoderis stateless, it can be created once at module scope or as a constant outside the hook.♻️ Proposed optimization
import { useEffect, useRef, useCallback, useState } from 'react'; +const textEncoder = new TextEncoder(); + export type TerminalSocketStatus = 'idle' | 'connecting' | 'open' | 'closed' | 'error';Then update the usage:
} else if (typeof event.data === 'string') { - onDataRef.current(new TextEncoder().encode(event.data)); + onDataRef.current(textEncoder.encode(event.data)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/hooks/useTerminalSocket.ts` around lines 83 - 89, Cache a single TextEncoder instance instead of instantiating a new one for every string message: create one TextEncoder at module scope or as a const outside the hook and use it inside the ws.onmessage handler (the current block referencing ws.onmessage, event.data, and onDataRef.current). Replace new TextEncoder().encode(event.data) with the cached encoder.encode(event.data) so string messages reuse the same encoder.
5-24: Consider moving exported types to the central types file.The exported types
TerminalSocketStatus,UseTerminalSocketOptions, andUseTerminalSocketReturnare defined inline. Per project guidelines, TypeScript types should be defined inweb-ui/src/types/index.tsand re-imported here.As per coding guidelines: "TypeScript types must be defined in
web-ui/src/types/index.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/hooks/useTerminalSocket.ts` around lines 5 - 24, Move the exported type declarations TerminalSocketStatus, UseTerminalSocketOptions, and UseTerminalSocketReturn out of this file and into the central types barrel (web-ui/src/types/index.ts), then import and re-export or use them here; specifically, create/type-add these three symbols in the central types file with the same definitions, update this module to remove the inline definitions and import { TerminalSocketStatus, UseTerminalSocketOptions, UseTerminalSocketReturn } from the central types entry, and ensure any existing exports or usages (sendInput, sendResize, status) still reference the imported types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 186-213: The _text_stdin_relay coroutine is never started, so text
frames sent by the frontend (via websocket.receive_text()) are ignored and cause
_stdin_relay (which uses websocket.receive_bytes()) to fail; create and start an
asyncio.Task named text_stdin_task for _text_stdin_relay (similar to how
_stdin_relay is started), ensure it runs alongside the existing stdin_relay
task, and update the cleanup/shutdown logic to cancel and await text_stdin_task
(handle asyncio.CancelledError) just like the other relay tasks to avoid
dangling tasks or exceptions.
In `@tests/ui/test_terminal_ws.py`:
- Line 7: Remove the unused import statement "import asyncio" from the test
module (the top-level import in the test file) so the ruff lint check no longer
fails; locate the line with the literal token import asyncio, delete it, and run
the test/lint to confirm the pipeline passes.
In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts`:
- Around line 98-109: The test fails because it compares two different
Uint8Array instances by reference; update the assertion in the useTerminalSocket
test so it checks values not identity: when simulating binary frames with
MockWebSocket.simulateBinaryMessage(bytes) expect the onData callback (passed to
useTerminalSocket) to receive a Uint8Array with matching byte contents rather
than the same object reference — e.g., use a matcher like expect.any(Uint8Array)
for the call and add a separate assertion that compares contents (e.g., convert
to Array or compare with a new Uint8Array of the expected bytes) or assert
calledWith a new Uint8Array containing the same bytes; reference
functions/objects: useTerminalSocket, onData,
MockWebSocket.simulateBinaryMessage, and the test variable bytes.
In `@web-ui/src/components/sessions/AgentTerminal.tsx`:
- Around line 10-24: The buildWsUrl function currently inserts the raw token
into the query string which can break the URL if the JWT contains characters
like + or /; update buildWsUrl to URL-encode the token (use encodeURIComponent
on the value returned by getToken()) before interpolating it into the returned
string, preserving the existing null handling (i.e., still return null when
getToken() is falsy) and keeping the same base URL construction logic.
---
Nitpick comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 100-103: The session check currently only blocks "ended" sessions;
update the conditional in the terminal WebSocket connection handling (where
session is fetched via db.interactive_sessions.get using session_id) to also
treat "paused" as disallowed—i.e., change the if to verify session is not None
and that session.get("state") is not in ("ended", "paused") or equivalently
check if session.get("state") in ("ended", "paused") then await
websocket.close(... ) and return; ensure you reference the same websocket.close
call and session/state keys so paused sessions are blocked like ended ones.
In `@web-ui/src/components/sessions/AgentTerminal.tsx`:
- Around line 38-51: Replace the inline SVG spinner in AgentTerminal (the <svg
...> block rendering the loading spinner) with the appropriate Hugeicons
component from `@hugeicons/react` (e.g., the library's spinner/loading icon),
import that icon at the top of the file and use it where the current svg is
rendered, apply the component the same sizing/animation classes but switch to
the gray color scheme (e.g., "animate-spin h-5 w-5 text-gray-400") to follow the
Shadcn/UI Nova preset, and remove the inline SVG so no custom SVG or
lucide-react icon remains.
- Around line 77-80: In AgentTerminal, setting wsUrlRef.current during render is
a side effect; move the initialization out of render by computing the value with
useMemo (e.g., const wsUrl = useMemo(() => buildWsUrl(sessionId), [sessionId])
and assign to wsUrlRef.current inside a useEffect) or by assigning
wsUrlRef.current at the start of a useEffect that depends on sessionId; update
references that currently read wsUrlRef.current to use the memoized wsUrl or
ensure the ref is set in the effect to avoid performing side effects during
render.
In `@web-ui/src/hooks/useTerminalSocket.ts`:
- Around line 83-89: Cache a single TextEncoder instance instead of
instantiating a new one for every string message: create one TextEncoder at
module scope or as a const outside the hook and use it inside the ws.onmessage
handler (the current block referencing ws.onmessage, event.data, and
onDataRef.current). Replace new TextEncoder().encode(event.data) with the cached
encoder.encode(event.data) so string messages reuse the same encoder.
- Around line 5-24: Move the exported type declarations TerminalSocketStatus,
UseTerminalSocketOptions, and UseTerminalSocketReturn out of this file and into
the central types barrel (web-ui/src/types/index.ts), then import and re-export
or use them here; specifically, create/type-add these three symbols in the
central types file with the same definitions, update this module to remove the
inline definitions and import { TerminalSocketStatus, UseTerminalSocketOptions,
UseTerminalSocketReturn } from the central types entry, and ensure any existing
exports or usages (sendInput, sendResize, status) still reference the imported
types.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 299959bf-3a54-4310-aaa4-2320cf927fa1
⛔ Files ignored due to path filters (1)
web-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
codeframe/ui/routers/terminal_ws.pycodeframe/ui/server.pytests/ui/test_terminal_ws.pyweb-ui/package.jsonweb-ui/src/__tests__/hooks/useTerminalSocket.test.tsweb-ui/src/components/sessions/AgentTerminal.tsxweb-ui/src/components/sessions/index.tsweb-ui/src/hooks/index.tsweb-ui/src/hooks/useTerminalSocket.ts
| async def _text_stdin_relay() -> None: | ||
| assert process is not None | ||
| assert process.stdin is not None | ||
| try: | ||
| while True: | ||
| try: | ||
| raw = await websocket.receive_text() | ||
| except WebSocketDisconnect: | ||
| raise | ||
| except Exception: | ||
| break | ||
|
|
||
| try: | ||
| msg = json.loads(raw) | ||
| if isinstance(msg, dict) and msg.get("type") == "resize": | ||
| continue | ||
| process.stdin.write(raw.encode()) | ||
| await process.stdin.drain() | ||
| except (json.JSONDecodeError,): | ||
| process.stdin.write(raw.encode()) | ||
| await process.stdin.drain() | ||
|
|
||
| except WebSocketDisconnect: | ||
| raise | ||
| except asyncio.CancelledError: | ||
| pass | ||
| except Exception as exc: | ||
| logger.debug("Terminal text relay error: %s", exc) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the frontend sends terminal input
rg -n 'sendInput|send\(' web-ui/src/hooks/useTerminalSocket.tsRepository: frankbria/codeframe
Length of output: 382
🏁 Script executed:
cat -n codeframe/ui/routers/terminal_ws.py | sed -n '180,225p'Repository: frankbria/codeframe
Length of output: 2169
🏁 Script executed:
cat -n codeframe/ui/routers/terminal_ws.py | sed -n '150,185p'Repository: frankbria/codeframe
Length of output: 1764
Add _text_stdin_relay task to handle text frames from the frontend.
The function _text_stdin_relay() is defined but never invoked. Meanwhile, _stdin_relay() only calls websocket.receive_bytes() and breaks on any exception (line 163), which occurs when the client sends text frames. Since the frontend sends text frames via wsRef.current.send(data), this function must be added as a task for terminal input to work.
Fix: Add task for text relay
stdout_to_ws_task = asyncio.create_task(_stdout_relay())
ws_to_stdin_task = asyncio.create_task(_stdin_relay())
+ text_stdin_task = asyncio.create_task(_text_stdin_relay())
# Wait for either task to finish (disconnect or process exit)
await asyncio.wait(
- [stdout_to_ws_task, ws_to_stdin_task],
+ [stdout_to_ws_task, ws_to_stdin_task, text_stdin_task],
return_when=asyncio.FIRST_COMPLETED,
)Also update cleanup logic to handle text_stdin_task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/ui/routers/terminal_ws.py` around lines 186 - 213, The
_text_stdin_relay coroutine is never started, so text frames sent by the
frontend (via websocket.receive_text()) are ignored and cause _stdin_relay
(which uses websocket.receive_bytes()) to fail; create and start an asyncio.Task
named text_stdin_task for _text_stdin_relay (similar to how _stdin_relay is
started), ensure it runs alongside the existing stdin_relay task, and update the
cleanup/shutdown logic to cancel and await text_stdin_task (handle
asyncio.CancelledError) just like the other relay tasks to avoid dangling tasks
or exceptions.
|
placeholder |
Follow-up ReviewTwo commits since opening — the test fix (static imports) is good. The three issues from prior reviews are still unaddressed in the current code; flagging status and adding one new finding. Still Open from Prior Reviews[CRITICAL] os.environ.copy() leaks server credentials (terminal_ws.py line 112) [BUG] _text_stdin_relay is dead code (terminal_ws.py line 152) [SECURITY] JWT in URL query param (AgentTerminal.tsx line 22, terminal_ws.py line 39) New Finding[BUG] wsUrlRef never resets on sessionId change (AgentTerminal.tsx lines 88-90) The guard on !wsUrlRef.current means the URL is built once and frozen for the component lifetime. If the component renders with a different sessionId (navigating between sessions), it continues connecting to the original session terminal. Replace the render-body assignment with useMemo keyed on sessionId so the URL and socket refresh when the session changes. What is Good
Summary: The credential leak is a blocker. The _text_stdin_relay dead-code bug and wsUrlRef stale-URL bug are both functional issues that should be fixed in this PR. JWT-in-URL can go to a follow-up issue. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/src/__tests__/hooks/useTerminalSocket.test.ts (1)
105-107:⚠️ Potential issue | 🟡 MinorThe test will fail due to reference inequality in the assertion.
The
simulateBinaryMessage()passesbytes.bufferto theonmessagehandler. The hook then creates a newUint8Arrayfrom this buffer internally. Since Jest'stoHaveBeenCalledWith()uses reference equality, the assertion will fail.🔧 Proposed fix to compare array contents
const bytes = new Uint8Array([104, 101, 108, 108, 111]); // "hello" act(() => ws.simulateBinaryMessage(bytes)); - expect(onData).toHaveBeenCalledWith(bytes); + expect(onData).toHaveBeenCalledTimes(1); + expect(Array.from(onData.mock.calls[0][0])).toEqual([104, 101, 108, 108, 111]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 105 - 107, The test fails because simulateBinaryMessage passes bytes.buffer and the hook creates a new Uint8Array, so reference equality fails; update the assertion for the onData mock used by useTerminalSocket to compare contents instead of references (e.g., assert the received value is a Uint8Array and use toEqual against the original bytes or compare onData.mock.calls[0][0] to bytes) — locate the simulateBinaryMessage call and the onData mock in the test and replace expect(onData).toHaveBeenCalledWith(bytes) with a content-based equality check for the Uint8Array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts`:
- Around line 118-120: The test fails due to reference equality — both the hook
and the test create distinct Uint8Array instances with identical bytes. Update
the assertion after act(() => ws.simulateTextMessage('hi')) to fetch the actual
argument from the onData mock (onData.mock.calls[0][0]) and compare byte
contents rather than object identity (e.g., compare Array.from(actual) to
Array.from(expected) or use Buffer/TypedArray element-wise comparison) so the
test verifies equal bytes for the callback argument instead of reference
equality.
---
Duplicate comments:
In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts`:
- Around line 105-107: The test fails because simulateBinaryMessage passes
bytes.buffer and the hook creates a new Uint8Array, so reference equality fails;
update the assertion for the onData mock used by useTerminalSocket to compare
contents instead of references (e.g., assert the received value is a Uint8Array
and use toEqual against the original bytes or compare onData.mock.calls[0][0] to
bytes) — locate the simulateBinaryMessage call and the onData mock in the test
and replace expect(onData).toHaveBeenCalledWith(bytes) with a content-based
equality check for the Uint8Array.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 121cad8b-b653-4183-b0a1-0f1c623634aa
📒 Files selected for processing (1)
web-ui/src/__tests__/hooks/useTerminalSocket.test.ts
…nings)
CodeRabbit critical fixes:
- terminal_ws.py: replace two competing receive_bytes/receive_text tasks with
single unified _stdin_relay using websocket.receive() — fixes broken terminal input
- test_terminal_ws.py: remove unused asyncio import (ruff F401)
- AgentTerminal.tsx: encodeURIComponent(token) in WS URL query string
- useTerminalSocket.test.ts: value-based Uint8Array assertion
Pre-existing lint fixes (0 → 0 errors):
- tailwind.config.ts: require() → import typography from '@tailwindcss/typography'
- useAgentChat.ts: remove unused AgentChatStatus import
- useTaskStream.ts: remove unused SSEStatus import
- ShellCommandEvent.tsx: remove unused CheckmarkCircle01Icon, Cancel01Icon
- MarkdownEditor.tsx: remove unused TabsContent import
- prd/page.tsx: <a href> → <Link> (next/link)
- useAgentChat.test.ts: remove unused WsEventMap type and originalLocalStorage
- TaskBoardView.test.tsx: replace require('@/lib/api') with top-level import
- e2e/execution.spec.ts: rename unused route → _route
- e2e/fixtures/test-setup.ts: remove unused Page import; eslint-disable for
Playwright fixture use() calls flagged as React hooks
- e2e/{prd,tasks,workspace}.spec.ts: remove unused mock-data imports
PR Review: AgentTerminal — XTerm.js interactive terminal (#516)Good implementation overall — clean separation of concerns, solid test coverage, and the reconnect logic in Security1. JWT token in query parameter ( 2. No rate limiting on the WebSocket endpoint ( 3. Reconnect ignores close codes ( ws.onclose = (event) => {
wsRef.current = null;
const isAuthFailure = event.code === 4001 || event.code === 4003;
if (!isAuthFailure && retriesRef.current < maxRetries) {
// retry...
} else {
setStatus('error');
}
};Correctness4. Side effect during render ( if (typeof window !== 'undefined' && !wsUrlRef.current) {
wsUrlRef.current = buildWsUrl(sessionId);
}Mutating a ref during render is a React anti-pattern — it runs on every render pass including in StrictMode's double-invoke. Move this into a 5. No input size cap on stdin relay ( Maintenance6. Deprecated packages ( 7. Minor / Nits
What's well done
Items 3, 4, and 6 are the most important to address. 1 and 2 can be tracked as follow-up issues if the session_chat_ws pattern is intentionally preserved for consistency. |
|
Status check on prior reviews. Fixed: the dead stdin relay split is gone, current _stdin_relay handles both text and binary via websocket.receive() correctly. Test import cleanup is clean. Still blocking: [CRITICAL] os.environ.copy() credential leak (terminal_ws.py line 122) — users can run env and see ANTHROPIC_API_KEY, CODEFRAME_API_KEY_SECRET etc; needs an allowlist (PATH, HOME, USER, SHELL, TERM, LANG). [BUG] wsUrlRef never resets on sessionId change (AgentTerminal.tsx lines 88-90) — frozen URL can wire terminal to wrong session on navigation; use useMemo keyed on sessionId. Lower priority follow-ups: JWT in URL (matches session_chat_ws pattern so ok to track separately), reconnect ignores close codes (4001/4003 should not retry), deprecated xterm packages, no rate limit on WS endpoint, bash hardcoded. Credential leak is a hard blocker. wsUrlRef bug should also be fixed in this PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
web-ui/__tests__/components/tasks/TaskBoardView.test.tsx (1)
245-245: Indentation inconsistency on mock setup lines.These 6 lines are inside
it()callbacks but have no leading indentation, while the rest of the test body is properly indented with 4 spaces. The code works correctly, but inconsistent formatting affects readability.🔧 Example fix (apply same pattern to all 6 lines)
it('calls stopExecution and mutates when Stop is clicked', async () => { -tasksApi.stopExecution.mockResolvedValue(undefined); + tasksApi.stopExecution.mockResolvedValue(undefined); mockMutate.mockResolvedValue(undefined);Also applies to: 259-259, 273-273, 323-323, 364-364, 389-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/tasks/TaskBoardView.test.tsx` at line 245, Several mock setup lines (e.g., tasksApi.stopExecution.mockResolvedValue(...)) inside it() callbacks are not indented to match the rest of the test bodies; fix by adding the same 4-space indentation as the surrounding test code so those lines align with the rest of the it() block. Update each occurrence (including the instances at the positions referenced: the tasksApi.stopExecution.mockResolvedValue line and the other similar mockResolvedValue lines noted) to use the consistent 4-space indentation used throughout TaskBoardView.test.tsx.web-ui/src/__tests__/hooks/useTerminalSocket.test.ts (3)
111-122: Assertion style inconsistent with binary frame test.The binary frame test (lines 107-108) uses
Array.from()comparison to avoid reference equality issues, but this test still relies ontoHaveBeenCalledWith(expected). While modern Jest performs deep equality on typed arrays, applying the same pattern as the binary test would ensure consistency and avoid potential fragility.♻️ Suggested fix for consistency
act(() => ws.simulateTextMessage('hi')); - const expected = new TextEncoder().encode('hi'); - expect(onData).toHaveBeenCalledWith(expected); + const received = onData.mock.calls[0][0] as Uint8Array; + // 'hi' encodes to [104, 105] + expect(Array.from(received)).toEqual([104, 105]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 111 - 122, The test in useTerminalSocket.test.ts should avoid direct typed-array reference equality by mirroring the binary-frame test: when asserting the onData call after MockWebSocket.simulateTextMessage('hi'), convert the received arg and the expected TextEncoder().encode('hi') to plain arrays (e.g., via Array.from) and assert equality (use the same Array.from pattern as used in the binary-frame test) so you compare values rather than references for the onData mock call.
74-217: Consider adding test coverage for WebSocket error events.The test suite covers the core functionality well, but there's no test for the
onerrorhandler behavior. If the hook handles errors differently from connection closes (e.g., immediate error state without retry), that behavior should be validated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 74 - 217, Add a test that covers WebSocket error events for useTerminalSocket: identify the MockWebSocket instance (MockWebSocket.instances[0]), simulate an error (implement or call a simulateError method on MockWebSocket), and assert the hook's behavior (e.g., result.current.status becomes 'error' immediately or follows retry logic) and that reconnects/cleanup behave as expected (respecting maxRetries/retryDelay or no retries). Ensure the test calls act(() => ws.simulateError()) and then advances timers if needed (jest.advanceTimersByTime) to validate retry behavior or final state; reference the hook functions sendInput/sendResize only if needed to confirm socket closed/cleaned up.
58-70: Consider restoring originalWebSocketinafterEach.The mock replaces
global.WebSocketbut never restores it. While Jest typically isolates test files, storing and restoring the original helps prevent interference if test isolation is incomplete.♻️ Suggested improvement
+let originalWebSocket: typeof WebSocket | undefined; + beforeEach(() => { MockWebSocket.instances = []; + originalWebSocket = (global as any).WebSocket; (global as any).WebSocket = MockWebSocket; // jsdom doesn't ship TextEncoder; polyfill from Node if (typeof (global as any).TextEncoder === 'undefined') { (global as any).TextEncoder = NodeTextEncoder; } jest.useFakeTimers(); }); afterEach(() => { jest.useRealTimers(); + (global as any).WebSocket = originalWebSocket; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts` around lines 58 - 70, The test replaces global.WebSocket with MockWebSocket but never restores the original, so modify the setup/teardown around beforeEach/afterEach: in the beforeEach capture the existing global.WebSocket into a local variable (e.g., origWebSocket) before assigning MockWebSocket, and in afterEach restore global.WebSocket from that saved origWebSocket (and clear MockWebSocket.instances as needed); reference the existing beforeEach, afterEach, MockWebSocket and uses of (global as any).WebSocket to locate where to add the save/restore.tests/ui/test_terminal_ws.py (2)
41-98: Auth tests cover key rejection scenarios but use overly broad exception catching.The tests validate missing token, invalid token, session not found, ended session, and ownership mismatch. However,
pytest.raises(Exception)is too broad and could mask unrelated failures.♻️ Consider catching specific WebSocket exceptions
FastAPI's TestClient raises
WebSocketDisconnector similar when the connection is rejected. Using a more specific exception type would make test failures clearer:+from starlette.websockets import WebSocketDisconnect def test_missing_token_closes_4001(self): app = _make_app() client = TestClient(app) - with pytest.raises(Exception): + with pytest.raises(Exception): # WebSocket rejection raises generic Exception in TestClient # No token → closed before accepting; TestClient raises on non-101 with client.websocket_connect("/ws/sessions/s1/terminal"): passIf
WebSocketDisconnectis not directly catchable, adding a comment explaining whyExceptionis used would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ui/test_terminal_ws.py` around lines 41 - 98, The tests in class TestTerminalWsAuth use pytest.raises(Exception) which is too broad; update each test (e.g., test_missing_token_closes_4001, test_invalid_token_closes_4001, test_valid_token_session_not_found, test_valid_token_ended_session, test_ownership_mismatch_closes) to assert the specific WebSocket disconnect exception instead of Exception — import WebSocketDisconnect from starlette.websockets (or the appropriate WebSocketDisconnect used by TestClient) and replace pytest.raises(Exception) with pytest.raises(WebSocketDisconnect); if that specific exception cannot be caught by TestClient in your test environment, leave the Exception but add a brief comment above the with pytest.raises(...) explaining why the broad Exception is required.
143-170: Test verifies no crash but lacks assertion on behavior.The test sends a resize message and implicitly passes if no exception is raised. Adding an assertion (e.g., that stdin.write was not called with the resize JSON) would strengthen the test.
💡 Proposed enhancement: assert resize message is not forwarded
with client.websocket_connect("/ws/sessions/s1/terminal?token=x") as ws: # Sending a resize message should not raise ws.send_text(json.dumps({"type": "resize", "cols": 80, "rows": 24})) + # Verify resize message was NOT written to subprocess stdin + # (stdin.write should not have been called with the resize JSON) + for call in mock_proc.stdin.write.call_args_list: + if call.args: + written = call.args[0] + assert b'"type": "resize"' not in written, "resize should not be forwarded to stdin"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ui/test_terminal_ws.py` around lines 143 - 170, The test test_resize_message_does_not_crash should assert that the resize JSON is not forwarded to the process: after calling ws.send_text(json.dumps({"type":"resize","cols":80,"rows":24})), add an assertion that mock_proc.stdin.write (or mock_proc.stdin.write.assert_not_called()/assert_not_awaited) was not invoked with the resize payload (or not invoked at all), referencing mock_proc.stdin and ws.send_text to locate the relevant mocks and call; ensure to use the appropriate AsyncMock assertion (e.g., assert_not_awaited or assert_not_called) for the async stdin write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 116-117: The current code copies the full server environment into
env using os.environ.copy() which may leak secrets; instead construct a minimal,
explicit environment for the subprocess (keep TERM="xterm-256color" and any safe
variables like PATH if required) or build a whitelist of allowed keys and
populate env only from that list; locate the env assignment and the
subprocess-launching call in terminal_ws.py (the env variable set by env["TERM"]
= "xterm-256color") and replace the os.environ.copy() usage with an explicit
dict or a filtered os.environ that only includes approved keys.
- Line 111: Do not silently default workspace_path to ".", instead assert/read
it explicitly from the session and fail fast if missing: replace the fallback
usage of session.get("workspace_path") that assigns to workspace_path with a
guard that checks session["workspace_path"] (or raises/returns an error) and
log/raise a descriptive error (including the session id or user context) so the
websocket/handler in terminal_ws.py does not spawn a shell in the server CWD;
refer to the workspace_path variable and the session.get call to locate the
change and ensure any caller (e.g., the terminal connection initializer) handles
the raised error cleanly.
- Around line 100-103: The current check in the terminal websocket handler that
only forbids "ended" sessions is too permissive — change the condition in the
block that retrieves the session (session = await
asyncio.to_thread(db.interactive_sessions.get, session_id)) from checking
session.get("state") == "ended" to session.get("state") != "active" so only
"active" sessions are allowed (and likewise update the identical check in
session_chat_ws at the referenced location); keep the existing
websocket.close(...) call but ensure it triggers when state is not "active".
---
Nitpick comments:
In `@tests/ui/test_terminal_ws.py`:
- Around line 41-98: The tests in class TestTerminalWsAuth use
pytest.raises(Exception) which is too broad; update each test (e.g.,
test_missing_token_closes_4001, test_invalid_token_closes_4001,
test_valid_token_session_not_found, test_valid_token_ended_session,
test_ownership_mismatch_closes) to assert the specific WebSocket disconnect
exception instead of Exception — import WebSocketDisconnect from
starlette.websockets (or the appropriate WebSocketDisconnect used by TestClient)
and replace pytest.raises(Exception) with pytest.raises(WebSocketDisconnect); if
that specific exception cannot be caught by TestClient in your test environment,
leave the Exception but add a brief comment above the with pytest.raises(...)
explaining why the broad Exception is required.
- Around line 143-170: The test test_resize_message_does_not_crash should assert
that the resize JSON is not forwarded to the process: after calling
ws.send_text(json.dumps({"type":"resize","cols":80,"rows":24})), add an
assertion that mock_proc.stdin.write (or
mock_proc.stdin.write.assert_not_called()/assert_not_awaited) was not invoked
with the resize payload (or not invoked at all), referencing mock_proc.stdin and
ws.send_text to locate the relevant mocks and call; ensure to use the
appropriate AsyncMock assertion (e.g., assert_not_awaited or assert_not_called)
for the async stdin write.
In `@web-ui/__tests__/components/tasks/TaskBoardView.test.tsx`:
- Line 245: Several mock setup lines (e.g.,
tasksApi.stopExecution.mockResolvedValue(...)) inside it() callbacks are not
indented to match the rest of the test bodies; fix by adding the same 4-space
indentation as the surrounding test code so those lines align with the rest of
the it() block. Update each occurrence (including the instances at the positions
referenced: the tasksApi.stopExecution.mockResolvedValue line and the other
similar mockResolvedValue lines noted) to use the consistent 4-space indentation
used throughout TaskBoardView.test.tsx.
In `@web-ui/src/__tests__/hooks/useTerminalSocket.test.ts`:
- Around line 111-122: The test in useTerminalSocket.test.ts should avoid direct
typed-array reference equality by mirroring the binary-frame test: when
asserting the onData call after MockWebSocket.simulateTextMessage('hi'), convert
the received arg and the expected TextEncoder().encode('hi') to plain arrays
(e.g., via Array.from) and assert equality (use the same Array.from pattern as
used in the binary-frame test) so you compare values rather than references for
the onData mock call.
- Around line 74-217: Add a test that covers WebSocket error events for
useTerminalSocket: identify the MockWebSocket instance
(MockWebSocket.instances[0]), simulate an error (implement or call a
simulateError method on MockWebSocket), and assert the hook's behavior (e.g.,
result.current.status becomes 'error' immediately or follows retry logic) and
that reconnects/cleanup behave as expected (respecting maxRetries/retryDelay or
no retries). Ensure the test calls act(() => ws.simulateError()) and then
advances timers if needed (jest.advanceTimersByTime) to validate retry behavior
or final state; reference the hook functions sendInput/sendResize only if needed
to confirm socket closed/cleaned up.
- Around line 58-70: The test replaces global.WebSocket with MockWebSocket but
never restores the original, so modify the setup/teardown around
beforeEach/afterEach: in the beforeEach capture the existing global.WebSocket
into a local variable (e.g., origWebSocket) before assigning MockWebSocket, and
in afterEach restore global.WebSocket from that saved origWebSocket (and clear
MockWebSocket.instances as needed); reference the existing beforeEach,
afterEach, MockWebSocket and uses of (global as any).WebSocket to locate where
to add the save/restore.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41882019-8a8a-4280-9832-c12d0dff9296
📒 Files selected for processing (17)
codeframe/ui/routers/terminal_ws.pytests/ui/test_terminal_ws.pyweb-ui/__tests__/components/tasks/TaskBoardView.test.tsxweb-ui/e2e/execution.spec.tsweb-ui/e2e/fixtures/test-setup.tsweb-ui/e2e/prd.spec.tsweb-ui/e2e/tasks.spec.tsweb-ui/e2e/workspace.spec.tsweb-ui/src/__tests__/hooks/useAgentChat.test.tsweb-ui/src/__tests__/hooks/useTerminalSocket.test.tsweb-ui/src/app/prd/page.tsxweb-ui/src/components/execution/ShellCommandEvent.tsxweb-ui/src/components/prd/MarkdownEditor.tsxweb-ui/src/components/sessions/AgentTerminal.tsxweb-ui/src/hooks/useAgentChat.tsweb-ui/src/hooks/useTaskStream.tsweb-ui/tailwind.config.ts
💤 Files with no reviewable changes (1)
- web-ui/src/tests/hooks/useAgentChat.test.ts
✅ Files skipped from review due to trivial changes (9)
- web-ui/src/components/execution/ShellCommandEvent.tsx
- web-ui/e2e/prd.spec.ts
- web-ui/src/components/prd/MarkdownEditor.tsx
- web-ui/e2e/workspace.spec.ts
- web-ui/src/hooks/useTaskStream.ts
- web-ui/e2e/tasks.spec.ts
- web-ui/e2e/execution.spec.ts
- web-ui/e2e/fixtures/test-setup.ts
- web-ui/src/hooks/useAgentChat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/components/sessions/AgentTerminal.tsx
| session = await asyncio.to_thread(db.interactive_sessions.get, session_id) | ||
| if session is None or session.get("state") == "ended": | ||
| await websocket.close(code=4004, reason="Session not found or ended") | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what session states exist in the codebase
rg -n "state.*=.*['\"]" --type py -g '*session*' | head -30
# Also check the session model/schema for state enum
ast-grep --pattern 'state = $_'Repository: frankbria/codeframe
Length of output: 12943
🏁 Script executed:
#!/bin/bash
# Find session state enum or constants
rg -n "state.*=.*\[" --type py | grep -i session | head -20
# Look for the interactive_sessions schema/model
fd -e py -path '*interactive*session*' | head -10
# Check for state validation or enum
rg -n "class.*State|State.*=|VALID.*STATE|ACTIVE.*STATE" --type py | head -20Repository: frankbria/codeframe
Length of output: 2270
🏁 Script executed:
#!/bin/bash
# Look at the test that shows paused transition to understand intent
sed -n '240,260p' tests/unit/test_interactive_sessions_api.py
# Check what happens in the terminal websocket - get more context
cat -n codeframe/ui/routers/terminal_ws.py | head -120Repository: frankbria/codeframe
Length of output: 6079
🏁 Script executed:
#!/bin/bash
# Find all places that set session state to understand the state machine
rg -B2 -A2 "state.*=.*['\"]" --type py codeframe/persistence/repositories/interactive_sessions.py
# Check the interactive sessions service/repository for state handling
cat -n codeframe/persistence/repositories/interactive_sessions.py | head -150Repository: frankbria/codeframe
Length of output: 6872
🏁 Script executed:
#!/bin/bash
# Search for VALID_STATES definition
rg -n "VALID_STATES|SESSION.*STATE" --type py
# Search for where "paused" state is used
rg -B3 -A3 '"paused"' --type py
# Check if there's a session model/enum
fd -e py | xargs grep -l "class.*Session" | grep -E "model|schema|type" | head -5Repository: frankbria/codeframe
Length of output: 5476
🏁 Script executed:
#!/bin/bash
# Check session_chat_ws.py for the same pattern
cat -n codeframe/ui/routers/session_chat_ws.py | sed -n '145,165p'
# Look for any session state machine or state transitions documentation
fd -e md -o -e txt | xargs grep -l "session.*state" 2>/dev/null | head -5Repository: frankbria/codeframe
Length of output: 1124
Only "active" sessions should have terminal access; currently "paused" sessions are allowed.
The state check rejects only "ended" sessions, but "paused" is also an invalid state per VALID_STATES = frozenset({"active", "paused", "ended"}) in interactive_sessions_v2.py. Change the check from session.get("state") == "ended" to session.get("state") != "active" to ensure only active sessions can access the terminal. This same issue exists in session_chat_ws.py:154.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/ui/routers/terminal_ws.py` around lines 100 - 103, The current
check in the terminal websocket handler that only forbids "ended" sessions is
too permissive — change the condition in the block that retrieves the session
(session = await asyncio.to_thread(db.interactive_sessions.get, session_id))
from checking session.get("state") == "ended" to session.get("state") !=
"active" so only "active" sessions are allowed (and likewise update the
identical check in session_chat_ws at the referenced location); keep the
existing websocket.close(...) call but ensure it triggers when state is not
"active".
…or subprocess - Replace silent workspace_path fallback to '.' with explicit error: closes the WebSocket with 4008 if session has no workspace configured, preventing bash from spawning in the server CWD - Replace os.environ.copy() with a minimal explicit env dict: only TERM, HOME, PATH, SHELL, LANG, USER are passed to the subprocess, preventing server secrets (API keys, DB credentials) from leaking to the child process
Follow-up ReviewFixes confirmedBoth blockers from prior reviews are resolved in the current diff:
Still open: wsUrlRef stale URL bug
if (typeof window !== 'undefined' && !wsUrlRef.current) {
wsUrlRef.current = buildWsUrl(sessionId);
}The const wsUrl = useMemo(
() => (typeof window !== 'undefined' ? buildWsUrl(sessionId) : null),
[sessionId]
);
// pass wsUrl to useTerminalSocket instead of wsUrlRef.currentLower-priority items (can be follow-up issues)These were flagged in prior reviews and remain unchanged — acceptable to track separately since they match the existing
The wsUrlRef stale URL bug is the one remaining issue that should be fixed in this PR. Everything else can be tracked as follow-ups. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
codeframe/ui/routers/terminal_ws.py (1)
100-103:⚠️ Potential issue | 🟠 MajorSession state check should only allow
"active"sessions.The current check rejects only
"ended"sessions, but"paused"sessions should also be denied terminal access. Change the condition to explicitly requirestate == "active".session = await asyncio.to_thread(db.interactive_sessions.get, session_id) - if session is None or session.get("state") == "ended": - await websocket.close(code=4004, reason="Session not found or ended") + if session is None: + await websocket.close(code=4004, reason="Session not found") + return + if session.get("state") != "active": + await websocket.close(code=4004, reason="Session is not active") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/terminal_ws.py` around lines 100 - 103, The session validation currently only rejects sessions with state "ended"; update the check after session = await asyncio.to_thread(db.interactive_sessions.get, session_id) to require session.get("state") == "active" (i.e., treat any non-"active" state, including "paused" or missing state, as invalid). Replace the existing condition (session is None or session.get("state") == "ended") with a negative check (session is None or session.get("state") != "active") so that websocket.close(code=4004, reason=...) runs for all non-active sessions.
🧹 Nitpick comments (1)
codeframe/ui/routers/terminal_ws.py (1)
146-148: Consider explicit guards instead ofassertfor production resilience.The
assertstatements serve as type narrowing, but they can be disabled withpython -O. Since these nested functions are created only after successful subprocess spawn, this is unlikely to cause issues in practice, but explicit guards would be more defensive:♻️ Optional: Replace asserts with explicit guards
async def _stdout_relay() -> None: - assert process is not None - assert process.stdout is not None + if process is None or process.stdout is None: + return try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/terminal_ws.py` around lines 146 - 148, The nested function _stdout_relay currently uses assert process is not None and assert process.stdout is not None; replace those asserts with explicit runtime guards: check if process is None or process.stdout is None at the start of _stdout_relay, log or raise a clear RuntimeError (or return early) with contextual message including the function name, and ensure any callers handle that error/path; reference the symbols _stdout_relay, process, and process.stdout when making the change so the guard replaces the asserts and provides deterministic behavior in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 105-109: The ownership check currently assumes
session.get("user_id") exists but the interactive_sessions schema lacks that
column, so update the check to fail-closed: if session.get("user_id") is None or
int(session.get("user_id")) != user_id then call websocket.close(code=4003,
reason="Forbidden: session belongs to another user") and return; additionally,
if you can, replace reliance on session data by validating against the
persistent record (e.g., query the interactive_sessions record for the session
id and compare its owner column to user_id) so the check in terminal_ws.py (the
session_user_id / session.get("user_id") branch around websocket.close) always
enforces ownership even when schema/session data is missing.
---
Duplicate comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 100-103: The session validation currently only rejects sessions
with state "ended"; update the check after session = await
asyncio.to_thread(db.interactive_sessions.get, session_id) to require
session.get("state") == "active" (i.e., treat any non-"active" state, including
"paused" or missing state, as invalid). Replace the existing condition (session
is None or session.get("state") == "ended") with a negative check (session is
None or session.get("state") != "active") so that websocket.close(code=4004,
reason=...) runs for all non-active sessions.
---
Nitpick comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 146-148: The nested function _stdout_relay currently uses assert
process is not None and assert process.stdout is not None; replace those asserts
with explicit runtime guards: check if process is None or process.stdout is None
at the start of _stdout_relay, log or raise a clear RuntimeError (or return
early) with contextual message including the function name, and ensure any
callers handle that error/path; reference the symbols _stdout_relay, process,
and process.stdout when making the change so the guard replaces the asserts and
provides deterministic behavior in production.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05185d6e-0d6b-485a-80ce-52b711a2ca69
📒 Files selected for processing (1)
codeframe/ui/routers/terminal_ws.py
| # --- Ownership check --- | ||
| session_user_id = session.get("user_id") | ||
| if session_user_id is not None and int(session_user_id) != user_id: | ||
| await websocket.close(code=4003, reason="Forbidden: session belongs to another user") | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if interactive_sessions table schema includes user_id column
rg -n "user_id" codeframe/persistence/repositories/interactive_sessions.py
# Check the schema definition
rg -n "CREATE TABLE.*interactive_sessions" -A 20 codeframe/persistence/
# Check if session_chat_ws has the same pattern
rg -n "session.get.*user_id" codeframe/ui/routers/Repository: frankbria/codeframe
Length of output: 1882
Critical: Ownership check is silently bypassed due to missing user_id column in schema.
The interactive_sessions table schema (defined in codeframe/persistence/schema_manager.py:842-856) does not include a user_id column. When session.get("user_id") is called at line 106, it will always return None, causing the condition at line 107 to be False and the entire ownership validation to be unreachable. This allows any authenticated user to access any session's terminal.
Implement fail-closed behavior:
Suggested fix
# --- Ownership check ---
session_user_id = session.get("user_id")
- if session_user_id is not None and int(session_user_id) != user_id:
+ if session_user_id is None:
+ logger.warning("Session %s missing user_id; denying terminal access", session_id)
+ await websocket.close(code=4003, reason="Session ownership cannot be verified")
+ return
+ if int(session_user_id) != user_id:
await websocket.close(code=4003, reason="Forbidden: session belongs to another user")
return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/ui/routers/terminal_ws.py` around lines 105 - 109, The ownership
check currently assumes session.get("user_id") exists but the
interactive_sessions schema lacks that column, so update the check to
fail-closed: if session.get("user_id") is None or int(session.get("user_id")) !=
user_id then call websocket.close(code=4003, reason="Forbidden: session belongs
to another user") and return; additionally, if you can, replace reliance on
session data by validating against the persistent record (e.g., query the
interactive_sessions record for the session id and compare its owner column to
user_id) so the check in terminal_ws.py (the session_user_id /
session.get("user_id") branch around websocket.close) always enforces ownership
even when schema/session data is missing.
Correctness:
- useTerminalSocket: skip retry on auth/authz close codes (4001/4003/4004/4008)
to avoid pointless retry loops when credentials are permanently rejected
- AgentTerminal: move wsUrl build from render-time side effect into useMemo
(safe under React StrictMode double-invoke)
- terminal_ws.py: cap stdin frame size at 64KB to prevent unbounded writes
Security:
- terminal_ws.py: enforce per-user concurrent terminal cap (max 3) with
in-process counter; decrement in finally block on disconnect
Maintenance:
- Migrate xterm/xterm-addon-fit → @xterm/xterm/@xterm/addon-fit (scoped
packages; old packages deprecated on npm registry)
- terminal_ws.py: use shutil.which("bash") or "sh" fallback for Alpine/minimal
container compatibility
Tests:
- Update MockWebSocket.close/simulateClose to pass CloseEvent with code
- Add test: auth failure close code skips retry and goes straight to error
Final Review PassAll three issues from prior reviews are now resolved in the current diff:
Two other items from earlier reviews are also resolved as a side effect:
Remaining (acceptable follow-ups)These can be tracked as separate issues rather than blocking this PR:
This PR is ready to merge. All blockers are resolved and the implementation is solid. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
codeframe/ui/routers/terminal_ws.py (2)
105-108:⚠️ Potential issue | 🟠 MajorState check allows "paused" sessions to spawn terminals.
The condition only rejects
"ended"sessions, but"paused"sessions should also be denied terminal access. Only"active"sessions should have an interactive terminal.Proposed fix
session = await asyncio.to_thread(db.interactive_sessions.get, session_id) - if session is None or session.get("state") == "ended": + if session is None or session.get("state") != "active": await websocket.close(code=4004, reason="Session not found or ended") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/terminal_ws.py` around lines 105 - 108, The session state check allows "paused" sessions to open terminals; update the condition around the session fetched via db.interactive_sessions.get (variable session) to only permit sessions with state == "active" (or explicitly deny states "paused" and "ended") and call websocket.close(code=4004, reason="Session not found, paused, or ended") when denied; adjust the check near the websocket handling logic so any session.get("state") != "active" triggers the close.
110-114:⚠️ Potential issue | 🔴 CriticalCritical: Ownership check is ineffective—any authenticated user can access any session's terminal.
The
interactive_sessionstable has nouser_idcolumn. When the ownership check attemptssession.get("user_id")at line 111, it returnsNone, causing the conditional at line 112 to short-circuit and skip access denial. Combined with the fact that sessions are created without user association (as noted ininteractive_sessions_v2.py), any authenticated user can connect to any session's terminal.Use a fail-closed approach: deny access if ownership cannot be verified.
Proposed fix
# --- Ownership check --- session_user_id = session.get("user_id") - if session_user_id is not None and int(session_user_id) != user_id: + if session_user_id is None: + logger.warning("Session %s has no user_id; denying terminal access", session_id) + await websocket.close(code=4003, reason="Session ownership cannot be verified") + return + if int(session_user_id) != user_id: await websocket.close(code=4003, reason="Forbidden: session belongs to another user") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/terminal_ws.py` around lines 110 - 114, The ownership check is currently ineffective because session.get("user_id") can be None; update the logic to fail-closed: if session_user_id is None OR int(session_user_id) != user_id then immediately call websocket.close(code=4003, reason="Forbidden: session belongs to another user") and return. Additionally, because the interactive_sessions table has no user_id, add a verification step that queries the session owner from the authoritative source in interactive_sessions_v2 (e.g., call the existing lookup/query function in interactive_sessions_v2.py to retrieve the session owner by session_id) and compare that owner to user_id; if that lookup yields no owner or a mismatch, close the websocket. Ensure you reference the variables session_user_id, session.get("user_id"), and the websocket.close(...) call when making the change.
🧹 Nitpick comments (1)
web-ui/src/components/sessions/AgentTerminal.tsx (1)
37-51: Consider using Hugeicons for consistency.The coding guidelines specify using
@hugeicons/reactfor icons. If Hugeicons has a suitable loading/spinner icon, consider using it for consistency with the rest of the UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 37 - 51, Replace the inline SVG spinner in AgentTerminal.tsx with the Hugeicons spinner component to match project conventions: import the appropriate spinner icon from `@hugeicons/react` (e.g., Spinner or SpinnerSolid) and use that component in place of the SVG block shown in the diff (the <svg ...> / </svg> spinner). Ensure you pass the same sizing and color classes (e.g., animate-spin h-5 w-5 text-purple-400) to the Hugeicons component so appearance and animation remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@codeframe/ui/routers/terminal_ws.py`:
- Around line 105-108: The session state check allows "paused" sessions to open
terminals; update the condition around the session fetched via
db.interactive_sessions.get (variable session) to only permit sessions with
state == "active" (or explicitly deny states "paused" and "ended") and call
websocket.close(code=4004, reason="Session not found, paused, or ended") when
denied; adjust the check near the websocket handling logic so any
session.get("state") != "active" triggers the close.
- Around line 110-114: The ownership check is currently ineffective because
session.get("user_id") can be None; update the logic to fail-closed: if
session_user_id is None OR int(session_user_id) != user_id then immediately call
websocket.close(code=4003, reason="Forbidden: session belongs to another user")
and return. Additionally, because the interactive_sessions table has no user_id,
add a verification step that queries the session owner from the authoritative
source in interactive_sessions_v2 (e.g., call the existing lookup/query function
in interactive_sessions_v2.py to retrieve the session owner by session_id) and
compare that owner to user_id; if that lookup yields no owner or a mismatch,
close the websocket. Ensure you reference the variables session_user_id,
session.get("user_id"), and the websocket.close(...) call when making the
change.
---
Nitpick comments:
In `@web-ui/src/components/sessions/AgentTerminal.tsx`:
- Around line 37-51: Replace the inline SVG spinner in AgentTerminal.tsx with
the Hugeicons spinner component to match project conventions: import the
appropriate spinner icon from `@hugeicons/react` (e.g., Spinner or SpinnerSolid)
and use that component in place of the SVG block shown in the diff (the <svg
...> / </svg> spinner). Ensure you pass the same sizing and color classes (e.g.,
animate-spin h-5 w-5 text-purple-400) to the Hugeicons component so appearance
and animation remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecb94ae9-e646-47da-87b6-e0d44cf604ce
⛔ Files ignored due to path filters (1)
web-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
codeframe/ui/routers/terminal_ws.pyweb-ui/package.jsonweb-ui/src/__tests__/hooks/useTerminalSocket.test.tsweb-ui/src/components/sessions/AgentTerminal.tsxweb-ui/src/hooks/useTerminalSocket.ts
✅ Files skipped from review due to trivial changes (2)
- web-ui/package.json
- web-ui/src/tests/hooks/useTerminalSocket.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/hooks/useTerminalSocket.ts
Review: AgentTerminal WebSocket Terminal (#506)Good implementation overall. The architecture is clean, the security-conscious subprocess env (not copying Bug: Close code 4029 not in the no-retry list
Fix in const isPermanentFailure =
event.code === 4001 || event.code === 4003 ||
event.code === 4004 || event.code === 4008 || event.code === 4029;Session state check (echoing CodeRabbit) CodeRabbit has flagged this four times and it remains unresolved. The check if session is None or session.get("state") != "active":
await websocket.close(code=4004, reason="Session not found or not active")
returnworkspace_path: existence check before subprocess spawn
import pathlib
if not pathlib.Path(workspace_path).is_dir():
await websocket.close(code=4008, reason="Session workspace not found on disk")
returnMinor: empty USER env var
Things that look good
The 4029 close code omission is the only functional bug. The session state check is worth fixing before staging given how many times CodeRabbit has flagged it. |
1 participant
Summary
terminal_ws.pyrouter —WS /ws/sessions/{id}/terminal?token=<JWT>spawnsbashin the session'sworkspace_pathand relays stdio over WebSocket; auth pattern mirrorssession_chat_ws.pyuseTerminalSocket— WebSocket lifecycle management with binary/text relay, exponential-backoff reconnect (max 3),sendInput/sendResizehelpers; modeled afteruseEventSourceAgentTerminal— XTerm.js with FitAddon, dark theme (#0a0a0cbg,#a855f7cursor),ResizeObserverfor auto-fit,ReconnectingOverlayon disconnectNotes
asyncio.create_subprocess_execwith pipes (no PTY). Arrow keys and colour in interactive TUI apps (vim, htop) require a PTY — noted as known limitation.localStorage['auth_token'](same asuseAgentChat)NEXT_PUBLIC_WS_URLenv var (same asuseAgentChat)Test plan
useTerminalSocket: idle/connecting/open states, binary frames, text frames, sendInput, sendResize, reconnect backoff, error state after maxRetries, cleanup on unmountuv run ruff check— no new lint errorsCloses #506
Summary by CodeRabbit
New Features
Tests
Chores