feat: full repl mode - native CLI integration for chat tab with session sync#546
feat: full repl mode - native CLI integration for chat tab with session sync#546ivnnv wants to merge 15 commits intositeboon:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds Full REPL Mode: Chat can spawn the native Claude CLI via a PTY, stream/parse JSONL events, synchronize sessions between Chat and Shell via a project registry, read/persist permissions in ~/.claude/settings.json, and expose CLI session lifecycle, query, abort, and status APIs on the server. Changes
Sequence DiagramsequenceDiagram
participant UI as Chat UI
participant Browser as Browser\r\n(localStorage)
participant Server as Server\r\n(index.js + claude-cli-query)
participant CLI as Claude CLI\r\n(node-pty)
participant FS as Filesystem\r\n(~/.claude/)
participant WS as WebSocket
UI->>Browser: User sends message (fullReplMode=true)
Browser->>Server: claude-command (WebSocket)
Server->>Server: Check fullReplMode, registry, sessionId
Server->>CLI: Spawn PTY: bash -c 'claude --output-format stream-json --print ...'
CLI->>FS: Read ~/.claude/settings.json and project session files
CLI->>Server: Stream JSONL events (system/init, assistant, result...)
Server->>Server: Parse JSONL → map to WS events (session-created, claude-response, token-budget, claude-complete)
Server->>WS: Send session-created, flush buffered messages, stream responses
WS->>UI: Render response
alt User approves a tool
UI->>Server: approval request
Server->>FS: persistAllowedTool() → update ~/.claude/settings.json
end
alt User switches to Shell (resume)
Server->>Server: findLatestSessionForProject / registry lookup
Server->>CLI: Spawn PTY with --resume sessionId
CLI->>FS: Load session history and resume
end
Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/chat/hooks/useChatComposerState.ts (1)
674-688:⚠️ Potential issue | 🟠 MajorDon't turn an unset REPL preference into an explicit opt-out.
isFullReplModeActive()returnsfalseboth when the key is absent and when the user explicitly disabled the mode. Becauseserver/utils/settings-reader.js:isFullReplMode()treats any boolean as authoritative, Line 687 prevents the env-backed default from ever applying on first load. Use a tri-state preference here and omitfullReplModewhen the client has not stored a choice yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 674 - 688, The code currently always sends fullReplMode: isFullReplModeActive(), which turns an unset client preference into an explicit false; change this to use a tri-state preference (true | false | undefined) — e.g., call a getter that returns undefined when the user has not chosen (replace or augment isFullReplModeActive with a tri-state getter like getFullReplPreference or have isFullReplModeActive return undefined for unset) and only include the fullReplMode field in the options object when that getter returns a defined boolean; update the sendMessage call site (the options passed to sendMessage in useChatComposerState) to omit fullReplMode when the preference is undefined so the server-side env default can apply.server/index.js (1)
1803-1828:⚠️ Potential issue | 🟠 MajorValidate
registrySessionIdbefore interpolating into shell command.The explicit
sessionIdfrom client input is validated at line 1746 withsafeSessionIdPattern, butregistrySessionIdobtained fromgetProjectSessionId()bypasses this check before being interpolated into the shell command at lines 1822-1824.While session IDs from the CLI are typically safe, defense-in-depth suggests validating all session IDs before shell interpolation to prevent command injection if the registry is ever corrupted or poisoned.
🔒 Proposed fix to validate registry session ID
if (!resumeSessionId) { // Full REPL Mode: check if Chat created a session for this project const registrySessionId = getProjectSessionId(resolvedProjectPath); - if (registrySessionId) { + if (registrySessionId && safeSessionIdPattern.test(registrySessionId)) { resumeSessionId = registrySessionId; console.log(`[Full REPL v2] Shell resuming Chat session from registry: ${registrySessionId}`); // Kill any active Chat CLI process for this project if (abortClaudeCLISession(registrySessionId)) { console.log(`[Full REPL v2] Killed Chat CLI process for session handoff`); } + } else if (registrySessionId) { + console.warn(`[Full REPL v2] Invalid registry session ID, ignoring: ${registrySessionId}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1803 - 1828, The registrySessionId returned by getProjectSessionId(resolvedProjectPath) must be validated against the same safeSessionIdPattern used for client sessionId before using it in resumeSessionId or interpolating into shellCommand; change the block that assigns registrySessionId so you only set resumeSessionId = registrySessionId if safeSessionIdPattern.test(registrySessionId) (otherwise leave resumeSessionId null and log a warning about an invalid/poisoned registry session id), and ensure any use of resumeSessionId in the shellCommand construction happens only after this validation (affecting getProjectSessionId, resumeSessionId, and the shellCommand construction that follows).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/claude-cli-query.js`:
- Around line 223-266: The parsing loop currently drops any non-JSON CLI output;
change logic in the JSON-parsing block around mapCliEventToWsMessages and the
catch clause so you retain the last plaintext line (store it in a variable like
lastPlaintextLine whenever cleaned doesn't parse as JSON) and, on process exit
with a non-zero code when no structured error event was received, emit a ws
message of type "claude-error" containing that lastPlaintextLine and the exit
code (use the same ws.send pathway and respect buffering/sessionCreatedSent
behavior); apply the same change to the second parsing section (the one
referenced by the reviewer) so both places capture plaintext failures, set
lastPlaintextLine when non-JSON lines occur, and send a "claude-error" if no
structured error was parsed by the time the client exits.
- Around line 114-130: The code determines isValidUUID from sessionId but sets
session-new metadata later incorrectly from sessionId; compute and use a derived
isResumed based on the final resolved resumeId (after the
getProjectSessionId(resolvedCwd) lookup) instead of sessionId so resumed
sessions from the registry are not marked new. Concretely: after the registry
lookup that sets resumeId, set const isResumed = Boolean(resumeId) and use
isResumed (or its inverse) when building the completion payload (the
isNewSession flag) and any related metadata; ensure args still receives --resume
when resumeId exists and any checks that previously used sessionId now use
isResumed/resumeId.
In `@server/claude-sdk.js`:
- Around line 166-189: When applying
diskSettings.skipDangerousModePermissionPrompt in the fullRepl branch, avoid
downgrading an existing plan mode: check permissionMode !== 'plan' before
setting sdkOptions.permissionMode to 'bypassPermissions' (i.e., only set
bypassPermissions when not in 'plan' mode); locate the logic around fullRepl,
diskSettings, permissionMode and sdkOptions in the full REPL block and wrap the
assignment of sdkOptions.permissionMode = 'bypassPermissions' in that
conditional.
In `@server/index.js`:
- Around line 1866-1884: Replace the fragile 5s setTimeout approach in the
provider === 'claude' && !isPlainShell branch: stop relying on the hardcoded
delay and instead detect the session ID from the Shell process output or
file-system events and then call setProjectSessionId and update ptySessionsMap.
Specifically, remove the setTimeout block and add a listener on the PTY output
stream for the session-created JSONL event (the same parsing used in
claude-cli-query.js) or attach a file watcher (e.g., chokidar) to the project
sessions directory to wait for the new session file; once you parse the session
ID, call setProjectSessionId(resolvedProjectPath, sessionId) and update
ptySessionsMap.get(ptySessionKey).sessionId, and ensure the listener/watcher is
removed on PTY onExit to avoid race conditions or cross-project picks.
In `@server/routes/settings.js`:
- Around line 283-310: The endpoint handler for router.get('/repl-mode') only
calls getSettings() when process.env.CLAUDE_FULL_REPL_MODE is true, so
settingsExists, allowedTools, deniedTools, permissionCount and mcpServerCount
are empty when the toggle is off; always call and await getSettings() (use the
existing getSettings() call and settings variable) outside/above the envMode
check so those fields are populated regardless of envMode, then compute
settingsExists = !!settings, allowedTools = settings?.permissions?.allow || [],
deniedTools = settings?.permissions?.deny || [], permissionCount =
allowedTools.length and mcpServerCount = Object.keys(settings?.mcpServers ||
{}).length before sending the JSON response (keep SETTINGS_PATH in the
response).
In `@server/utils/settings-reader.js`:
- Around line 69-103: persistAllowedTool currently does read/modify/write
without mutual exclusion so concurrent calls can overwrite each other's changes;
wrap the entire read/parse/modify/write/rename sequence in a process-wide lock
to serialize mutations (e.g., use a simple in-memory mutex/queue or a lock
helper) so only one caller at a time reads SETTINGS_PATH, updates
settings.permissions.allow, writes the temp file and renames it, then clears
cachedMtime/cachedSettings; ensure the lock is acquired at the top of
persistAllowedTool and always released in a finally block even on error to avoid
deadlocks.
---
Outside diff comments:
In `@server/index.js`:
- Around line 1803-1828: The registrySessionId returned by
getProjectSessionId(resolvedProjectPath) must be validated against the same
safeSessionIdPattern used for client sessionId before using it in
resumeSessionId or interpolating into shellCommand; change the block that
assigns registrySessionId so you only set resumeSessionId = registrySessionId if
safeSessionIdPattern.test(registrySessionId) (otherwise leave resumeSessionId
null and log a warning about an invalid/poisoned registry session id), and
ensure any use of resumeSessionId in the shellCommand construction happens only
after this validation (affecting getProjectSessionId, resumeSessionId, and the
shellCommand construction that follows).
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 674-688: The code currently always sends fullReplMode:
isFullReplModeActive(), which turns an unset client preference into an explicit
false; change this to use a tri-state preference (true | false | undefined) —
e.g., call a getter that returns undefined when the user has not chosen (replace
or augment isFullReplModeActive with a tri-state getter like
getFullReplPreference or have isFullReplModeActive return undefined for unset)
and only include the fullReplMode field in the options object when that getter
returns a defined boolean; update the sendMessage call site (the options passed
to sendMessage in useChatComposerState) to omit fullReplMode when the preference
is undefined so the server-side env default can apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c6a4502-825f-48d0-8f55-226dbfe2d828
📒 Files selected for processing (9)
server/claude-cli-query.jsserver/claude-sdk.jsserver/index.jsserver/routes/settings.jsserver/utils/settings-reader.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/utils/chatStorage.tssrc/components/chat/view/ChatInterface.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/PermissionsContent.tsx
|
All CodeRabbit review findings addressed in d3e877b: Outside-diff findings:
Inline findings:
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
server/claude-cli-query.js (1)
280-294:⚠️ Potential issue | 🟠 MajorKeep the last plaintext exit chunk for failed CLI startups.
The main parsing loop now remembers non-JSON lines, but this exit-path still only tries to parse JSON. If the CLI writes its final error line without a trailing newline,
lastPlaintextLinenever gets updated and the client still only seesclaude-complete.🛠️ Proposed fix
if (partialLine.trim()) { const cleaned = partialLine.trim().replace(/\x1b\[[0-9;]*[a-zA-Z]/g, '').trim(); - if (cleaned && cleaned[0] === '{') { + if (cleaned && cleaned[0] !== '{') { + lastPlaintextLine = cleaned; + } else if (cleaned) { try { const event = JSON.parse(cleaned); const wsMessages = mapCliEventToWsMessages(event, session); for (const msg of wsMessages) { ws.send(msg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/claude-cli-query.js` around lines 280 - 294, The exit-path currently only tries to parse JSON from the buffered partialLine and ignores non-JSON final plaintext; update this block so that when cleaned is present but not valid JSON you preserve and emit the final plaintext (the same data tracked by lastPlaintextLine in the main loop) instead of swallowing it—i.e., after trimming and stripping ANSI sequences in the partialLine handling, attempt JSON.parse as now, but on parse failure fall back to treating cleaned as the final plaintext event: create or reuse the same message path as mapCliEventToWsMessages/ ws.send (or emit a plaintext ws message consistent with how lastPlaintextLine is used elsewhere) so the client receives the last CLI error chunk when the CLI exits without a trailing newline.server/index.js (1)
1868-1913:⚠️ Potential issue | 🟠 MajorTear down the Shell session detector when the PTY exits.
The new
onDatalistener and 15-second fallback timer survive process exit. If Claude exits before a session is detected, the delayed disk scan can still overwrite the registry with a stale session ID after the PTY has already been removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1868 - 1913, The session detector leaves an onData listener and a 15s fallback timer running after the PTY exits; add a teardown tied to the PTY lifecycle so the detector cannot overwrite state after the PTY is removed. Specifically, when you create sessionDetectionDisposable and sessionDetectionTimeout, also register a shellProcess exit/close handler (e.g., shellProcess.onExit / onClose) that: disposes sessionDetectionDisposable, clears and nulls sessionDetectionTimeout, and prevents running the fallback (check sessionDetected) — also remove or update any ptySessionsMap entries for ptySessionKey so stale session IDs cannot be written by the findLatestSessionForProject callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/claude-cli-query.js`:
- Around line 42-44: The code computing 'encoded' currently only replaces '/' in
projectPath, causing mismatches for paths with spaces or special characters;
update the encoding used for 'encoded' (and thus 'projectDir') to match the rest
of the server by replacing every non-alphanumeric character except '-' with '-',
e.g. use projectPath.replace(/[^A-Za-z0-9-]/g, '-') (or call the shared encoding
helper if one exists) so Claude project-path encoding is consistent everywhere.
- Around line 149-163: The image handling assumes img.mediaType and raw base64
but the server sends images as { data: "data:<mime>;base64,...", mimeType };
update the loop in server/claude-cli-query.js (the block using tempDir,
tempImagePaths, img.mediaType, img.data, finalCommand) to accept both payload
shapes: prefer img.mimeType if present, otherwise parse the MIME from a data URL
in img.data (if img.data starts with "data:") and split out the base64 payload;
fall back to treating img.data as raw base64 only if it does not match the data
URL pattern; derive the file extension from the resolved MIME (e.g., split on
'/' or default to png), decode the correct base64 substring to a Buffer and
write it to disk, and then continue building finalCommand from tempImagePaths as
before.
In `@server/index.js`:
- Around line 1804-1817: When resuming a Chat session the code only calls
abortClaudeCLISession for registry-derived sessions, leaving an explicit resume
(hasSession && sessionId) with the old Chat CLI still running; move the "kill
any active Chat CLI process" logic so it runs for any valid resumeSessionId
after it is determined (whether from sessionId or registrySessionId), i.e. once
resumeSessionId is set and passes safeSessionIdPattern, call
abortClaudeCLISession(resumeSessionId) and log the handoff result; reference
resumeSessionId, hasSession, sessionId, getProjectSessionId,
safeSessionIdPattern, and abortClaudeCLISession to locate and update the flow.
- Around line 1529-1533: Session-scoped WebSocket actions currently trust
caller-provided sessionId; before calling abortClaudeCLISession,
abortClaudeSDKSession, reconnectSessionWriter, or similar functions in
server/index.js and server/polling.js, validate that the requesting user/socket
owns the sessionId: lookup the session owner from the authoritative session
registry (e.g., activeSessions/sessionRegistry or whatever maps sessionId ->
ownerId) and compare against the authenticated requester (socket.userId /
req.user.id); if the owner doesn't match, reject the request and do not call
abortClaudeCLISession, abortClaudeSDKSession, reconnectSessionWriter or any
status/query methods—apply this ownership check to the other session-scoped
branches you flagged (the abort/status/reconnect branches).
- Around line 1473-1483: Loop is currently killing any PTY matching projectPath;
restrict the condition to only target Claude Full REPL PTYs by adding a check on
the session's REPL/provider/type field before killing. Update the for-loop that
iterates ptySessionsMap (referencing ptySessionsMap, session.projectPath,
session.pty.kill()) to also require the session identifies as the Claude
full-REPL (e.g., session.replProvider === 'claude' or session.replType ===
'full-repl' or the actual property your sessions use) so plain-shell, Cursor,
Codex, Gemini PTYs are not affected.
---
Duplicate comments:
In `@server/claude-cli-query.js`:
- Around line 280-294: The exit-path currently only tries to parse JSON from the
buffered partialLine and ignores non-JSON final plaintext; update this block so
that when cleaned is present but not valid JSON you preserve and emit the final
plaintext (the same data tracked by lastPlaintextLine in the main loop) instead
of swallowing it—i.e., after trimming and stripping ANSI sequences in the
partialLine handling, attempt JSON.parse as now, but on parse failure fall back
to treating cleaned as the final plaintext event: create or reuse the same
message path as mapCliEventToWsMessages/ ws.send (or emit a plaintext ws message
consistent with how lastPlaintextLine is used elsewhere) so the client receives
the last CLI error chunk when the CLI exits without a trailing newline.
In `@server/index.js`:
- Around line 1868-1913: The session detector leaves an onData listener and a
15s fallback timer running after the PTY exits; add a teardown tied to the PTY
lifecycle so the detector cannot overwrite state after the PTY is removed.
Specifically, when you create sessionDetectionDisposable and
sessionDetectionTimeout, also register a shellProcess exit/close handler (e.g.,
shellProcess.onExit / onClose) that: disposes sessionDetectionDisposable, clears
and nulls sessionDetectionTimeout, and prevents running the fallback (check
sessionDetected) — also remove or update any ptySessionsMap entries for
ptySessionKey so stale session IDs cannot be written by the
findLatestSessionForProject callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 25d619ba-7fed-4e72-a3ef-984ed91c9281
📒 Files selected for processing (7)
server/claude-cli-query.jsserver/claude-sdk.jsserver/index.jsserver/routes/settings.jsserver/utils/settings-reader.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/utils/chatStorage.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/chat/utils/chatStorage.ts
- server/utils/settings-reader.js
- src/components/chat/hooks/useChatComposerState.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/index.js (1)
1805-1865:⚠️ Potential issue | 🟠 MajorRecord the resolved Claude session ID in
ptySessionsMap.When
resumeSessionIdcomes from the registry, the PTY record is still inserted withsessionId: null. The Chat-side handoff loop only kills Shell PTYs whosesession.sessionIdis already populated, so a quick tab switch can leave both tabs attached to the same Claude session until the detector catches up.🛠️ Suggested fix
- let shellCommand; + let shellCommand; + let resolvedPtySessionId = sessionId || null; @@ let resumeSessionId = (hasSession && sessionId) ? sessionId : null; @@ if (!resumeSessionId) { @@ } + resolvedPtySessionId = resumeSessionId; @@ ptySessionsMap.set(ptySessionKey, { pty: shellProcess, ws: ws, buffer: [], timeoutId: null, projectPath, - sessionId + sessionId: resolvedPtySessionId });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1805 - 1865, The PTY entry is being saved with sessionId:null when a registry-provided resumeSessionId was used, allowing duplicate attachments; update the object inserted into ptySessionsMap (the ptySessionsMap.set call) to store the resolved session id (use resumeSessionId if present, otherwise the original sessionId) so the stored record reflects the actual Claude session being used and the handoff/kill logic will work correctly.
♻️ Duplicate comments (1)
server/index.js (1)
1867-1914:⚠️ Potential issue | 🟠 MajorCancel the session-detection fallback when the PTY dies.
sessionDetectionTimeoutsurvives process exit here, so a short-lived Shell can still runfindLatestSessionForProject()15 seconds later and rewrite the registry with a stale or newer session. That reopens the same stale-session race the earlier timeout fix was trying to remove.🛠️ Suggested fix
+ let sessionDetectionDisposable = null; + let sessionDetectionTimeout = null; + // Shell → Chat sync: detect session ID from PTY output // by watching for UUID patterns in early output (e.g. "Resuming Claude session <uuid>") if (provider === 'claude' && !isPlainShell) { const uuidPattern = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i; let sessionDetected = false; - let sessionDetectionTimeout = null; @@ - const sessionDetectionDisposable = shellProcess.onData(sessionDetectionHandler); + sessionDetectionDisposable = shellProcess.onData(sessionDetectionHandler); @@ shellProcess.onExit((exitCode) => { + sessionDetectionDisposable?.dispose(); + if (sessionDetectionTimeout) { + clearTimeout(sessionDetectionTimeout); + sessionDetectionTimeout = null; + } console.log('🔚 Shell process exited with code:', exitCode.exitCode, 'signal:', exitCode.signal);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1867 - 1914, The session-detection timeout and listener must be cancelled when the PTY dies to avoid running the fallback after process exit: add a PTY-exit handler (e.g. shellProcess.onExit or equivalent) that clears sessionDetectionTimeout, disposes sessionDetectionDisposable, and sets sessionDetected = true (or otherwise prevents findLatestSessionForProject from running); reference the existing symbols sessionDetectionTimeout, sessionDetectionDisposable, sessionDetectionHandler, shellProcess, ptySessionsMap, ptySessionKey, findLatestSessionForProject, and setProjectSessionId so the exit handler can safely clean up and avoid rewriting the registry after the PTY has terminated.
🧹 Nitpick comments (1)
server/claude-cli-query.js (1)
236-236: Extract the duplicated ANSI escape regex into a constant to avoid code duplication and control-character issues.The regex pattern
/\x1b\[[0-9;]*[a-zA-Z]/gis duplicated at lines 236 and 290. Defining it as a constant using the escaped form\u001bremoves the duplication and improves maintainability. While not currently enforced by the project's linter (which only checkssrc/), this change follows best practices for string escaping and reduces code redundancy.Suggested fix
+const ANSI_ESCAPE_REGEX = /\u001b\[[0-9;]*[A-Za-z]/g; + // Parse PTY output as JSONL cliProcess.onData((rawData) => { @@ - const cleaned = trimmed.replace(/\x1b\[[0-9;]*[a-zA-Z]/g, '').trim(); + const cleaned = trimmed.replace(ANSI_ESCAPE_REGEX, '').trim(); @@ - const cleaned = partialLine.trim().replace(/\x1b\[[0-9;]*[a-zA-Z]/g, '').trim(); + const cleaned = partialLine.trim().replace(ANSI_ESCAPE_REGEX, '').trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/claude-cli-query.js` at line 236, Extract the duplicated ANSI escape regex into a shared constant (e.g., ANSI_ESCAPE_REGEX or ANSI_ESCAPE_RE) and replace both inline uses (the occurrences in the cleaned = trimmed.replace(...) statement and the other replace at the later occurrence) with that constant; define the constant using the escaped form '\u001b' (e.g., /\u001b\[[0-9;]*[a-zA-Z]/g) near the top of the module so both functions reference it, removing duplication and preventing control-character escaping issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/claude-cli-query.js`:
- Around line 149-150: tempDir created by fs.mkdtemp when images exist can leak
because cleanup only runs when tempImagePaths.length > 0 and rmdir races
fire-and-forget unlink calls; modify the upload flow (references: tempDir,
tempImagePaths, fs.mkdtemp, unlink, rmdir) to always perform cleanup in a
finally block: collect unlink promises for every created temp image (even those
skipped), await Promise.all to finish removals, then remove the directory (use
fs.rm or fs.rmdir awaited) so directory removal does not race file deletions;
apply the same change pattern to the other occurrence around the code handling
lines 323-329.
In `@server/index.js`:
- Around line 1473-1488: The branch derives projectPath from data.options?.cwd
|| data.options?.projectPath and uses it to clean up ptySessionsMap, but then
calls queryClaudeCLI(data.command, data.options) without ensuring
data.options.cwd is set to that normalized projectPath, causing queryClaudeCLI
to see $HOME when only projectPath was provided; before calling queryClaudeCLI,
normalize and assign the resolved absolute path to data.options.cwd (e.g.,
resolve projectPath via path.resolve or similar) when projectPath exists so the
CLI runner and session registry use the same canonical cwd; update references to
data.options, projectPath, ptySessionsMap and queryClaudeCLI accordingly.
---
Outside diff comments:
In `@server/index.js`:
- Around line 1805-1865: The PTY entry is being saved with sessionId:null when a
registry-provided resumeSessionId was used, allowing duplicate attachments;
update the object inserted into ptySessionsMap (the ptySessionsMap.set call) to
store the resolved session id (use resumeSessionId if present, otherwise the
original sessionId) so the stored record reflects the actual Claude session
being used and the handoff/kill logic will work correctly.
---
Duplicate comments:
In `@server/index.js`:
- Around line 1867-1914: The session-detection timeout and listener must be
cancelled when the PTY dies to avoid running the fallback after process exit:
add a PTY-exit handler (e.g. shellProcess.onExit or equivalent) that clears
sessionDetectionTimeout, disposes sessionDetectionDisposable, and sets
sessionDetected = true (or otherwise prevents findLatestSessionForProject from
running); reference the existing symbols sessionDetectionTimeout,
sessionDetectionDisposable, sessionDetectionHandler, shellProcess,
ptySessionsMap, ptySessionKey, findLatestSessionForProject, and
setProjectSessionId so the exit handler can safely clean up and avoid rewriting
the registry after the PTY has terminated.
---
Nitpick comments:
In `@server/claude-cli-query.js`:
- Line 236: Extract the duplicated ANSI escape regex into a shared constant
(e.g., ANSI_ESCAPE_REGEX or ANSI_ESCAPE_RE) and replace both inline uses (the
occurrences in the cleaned = trimmed.replace(...) statement and the other
replace at the later occurrence) with that constant; define the constant using
the escaped form '\u001b' (e.g., /\u001b\[[0-9;]*[a-zA-Z]/g) near the top of the
module so both functions reference it, removing duplication and preventing
control-character escaping issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7bcd783f-98b2-491e-8399-be066d034c5a
📒 Files selected for processing (2)
server/claude-cli-query.jsserver/index.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/claude-cli-query.js (1)
77-100: Consider checking executability, not just existence.
fs.access(candidate)defaults to checking existence only. For a binary, considerfs.access(candidate, fs.constants.X_OK)to verify the file is actually executable. This would prevent selecting a non-executable file that happens to exist at one of these paths.💡 Suggested improvement
+import { constants as fsConstants } from 'fs'; async function resolveClaudeBinary() { if (cachedClaudeBin) return cachedClaudeBin; const candidates = [ path.join(os.homedir(), '.local', 'bin', 'claude'), '/usr/local/bin/claude', '/opt/homebrew/bin/claude', ]; for (const candidate of candidates) { try { - await fs.access(candidate); + await fs.access(candidate, fsConstants.X_OK); cachedClaudeBin = candidate;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/claude-cli-query.js` around lines 77 - 100, The resolveClaudeBinary function currently checks only for existence; change the access check to verify executability by calling fs.access(candidate, fs.constants.X_OK) (using the existing fs module's constants) when iterating the candidates array, so cachedClaudeBin is set only when the candidate path is executable; keep the same fallback to 'claude' and the same logging behavior if none are executable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/claude-cli-query.js`:
- Around line 77-100: The resolveClaudeBinary function currently checks only for
existence; change the access check to verify executability by calling
fs.access(candidate, fs.constants.X_OK) (using the existing fs module's
constants) when iterating the candidates array, so cachedClaudeBin is set only
when the candidate path is executable; keep the same fallback to 'claude' and
the same logging behavior if none are executable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c31f8fd-b700-4826-84c7-d832bdd81fed
📒 Files selected for processing (2)
server/claude-cli-query.jsserver/index.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/claude-cli-query.js (1)
121-128: Validate registry session ID for consistency with explicit session ID check.Line 117 validates explicit
sessionIdagainst a UUID regex, butregistryIdfrom the project registry (line 123) is used without the same validation. While shell escaping provides protection, applying the same UUID regex check toregistryIdwould ensure defense in depth and match the validation mentioned in the PR summary.🛡️ Suggested fix
if (!resumeId) { // Check registry for a session created by Shell or a previous Chat query const registryId = getProjectSessionId(resolvedCwd); - if (registryId) { + const isValidRegistryId = registryId && /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(registryId); + if (isValidRegistryId) { resumeId = registryId; console.log(`[Full REPL v2] Chat resuming session from registry: ${resumeId}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/claude-cli-query.js` around lines 121 - 128, The registry-provided session ID (registryId from getProjectSessionId(resolvedCwd)) is used without the same UUID validation applied to explicit sessionId; update the logic around getProjectSessionId/resumeId to validate registryId against the same UUID regex/pattern used earlier for sessionId and only assign resumeId = registryId when it matches, otherwise ignore it (and optionally log a warning); reference the registryId variable, the getProjectSessionId function, and the existing sessionId UUID check to implement the identical validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/claude-cli-query.js`:
- Line 214: The guard around structuredErrorSent is ineffective because
structuredErrorSent is never set; update mapCliEventToWsMessages (and the
result-event handling) to detect Claude CLI structured errors by checking result
events for is_error === true (e.g., in the result case inside
mapCliEventToWsMessages) and call the existing structured-error sending path,
then set structuredErrorSent = true after sending (so the check at the later
guard works), or if you prefer to ignore structured CLI errors remove the
structuredErrorSent variable and its guard; reference structuredErrorSent,
mapCliEventToWsMessages, and the result event handler when making the change.
---
Nitpick comments:
In `@server/claude-cli-query.js`:
- Around line 121-128: The registry-provided session ID (registryId from
getProjectSessionId(resolvedCwd)) is used without the same UUID validation
applied to explicit sessionId; update the logic around
getProjectSessionId/resumeId to validate registryId against the same UUID
regex/pattern used earlier for sessionId and only assign resumeId = registryId
when it matches, otherwise ignore it (and optionally log a warning); reference
the registryId variable, the getProjectSessionId function, and the existing
sessionId UUID check to implement the identical validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db11215c-e5aa-4a3f-b2af-24c355250c16
📒 Files selected for processing (1)
server/claude-cli-query.js
|
@ivnnv Thank you for your contribution. The claude --print option is something we used in the beginning and you can still find the implementation of it in server/cli.js but it was deprecated when Claude released the claude agent sdk. The shell and chat tabs should be aligned and if they are not that can be considered as a bug. |
|
Hey @viper151, thanks for looking at this. The core problem: a user on their phone or tablet using the web UI via a tunnel exposing the host ClaudeCode to the internet, the Chat tab runs through the Agent SDK on the "server". But the server IS their workstation, the same machine where they run
The This is opt-in (toggle in Settings), backwards compatible (SDK mode is still the default), and scoped to Claude only. It doesn't touch the existing SDK path. The Shell tab already spawns the CLI via PTY. This PR makes the Chat tab do the same thing but with structured JSON output, so both tabs share the same conversation seamlessly. Hope this helps getting the motivation of the PR. I'm already running this as my daily driver. |
|
@ivnnv That's a very thorough reply, thank you! One important thing to clarify first: the Agent SDK is not a separate reimplementation of Claude Code. It's a wrapper around the same claude binary you have installed on your machine. When the Chat tab uses the SDK, it's still running your local Claude Code installation underneath with the same engine, the same config loading, the same session persistence. The difference is in how we pass options to it, not in what it runs. So the framing of "SDK vs native CLI" isn't quite right because the SDK IS the native CLI, just invoked programmatically. With that context, let me go through your points: On sessions (point 2): Sessions created through the SDK are persisted to ~/.claude/projects/ on disk, not in the browser. If you start a conversation in the Chat tab and then run claude -r in your terminal, you can continue it. That's the whole point of this project. If the handoff between Chat and Shell tabs doesn't feel seamless, that's something we should look into as a bug. On CLAUDE.md, MCP servers, hooks, plugins (point 3): Since the SDK wraps the same binary, these are loaded the same way when settingSources is configured, which we do. If you're finding that something specific isn't working (like plugins), I'd like to understand the exact scenario so we can fix it in how we configure the SDK rather than bypassing it. The core concern with this PR is that maintaining two parallel execution paths adds significant complexity. Every feature, every bug fix, every future integration would need to work in both modes. Since the SDK already runs the same claude binary, the right fix for any gap is to make sure we're passing the right options to it. That's a configuration issue, not an architectural one. If there's a concrete case where the SDK genuinely can't do what the CLI does even with proper configuration, I'd want to know about it. Could you share a specific example of something that works in the Shell tab but breaks in Chat? That would help us fix the actual issue. |
2 participants
Closes #545
What changed
When Full REPL Mode is enabled (toggle in Settings > Claude > Permissions), the Chat tab spawns the native
claudeCLI instead of using the Agent SDK. This gives Chat the same behavior as the native CLI: same permissions, same session storage, same CLAUDE.md loading, and bidirectional session sync with the Shell tab.New files
server/utils/settings-reader.js- reads/caches~/.claude/settings.jsonfor permission parityserver/claude-cli-query.js- spawnsclaude --output-format stream-json --print, parses JSONL events, maps to existing Chat UI WebSocket messagesModified files
server/claude-sdk.js- permission overrides from settings.json when in REPL modeserver/routes/settings.js-/api/settings/repl-modestatus endpointserver/index.js- routes Chat queries to CLI or SDK based on mode, session handoff between tabssrc/components/chat/utils/chatStorage.ts- localStorage helpers for mode togglesrc/components/chat/hooks/useChatComposerState.ts- passesfullReplModeflagsrc/components/chat/view/ChatInterface.tsx- info banner about MCP tools in Shell tabsrc/components/settings/.../PermissionsContent.tsx- Full REPL Mode toggle with settings.json statusSession sync
projectPath -> sessionId--resumeScope
--printmode to avoid 30s+ startup overhead from server initializationCode review fixes (d3e877b)
Changes made after automated code review:
isFullReplModeActive()returnsundefinedwhen unset so server env var default appliessafeSessionIdPatternbefore shell interpolationisNewSessionfrom wrong sourceisResumedfrom finalresumeId, not rawsessionIdclaude-erroron non-zero exitskipDangerousModePermissionPromptwithpermissionMode !== 'plan'getSettings()regardless ofCLAUDE_FULL_REPL_MODEpersistAllowedToolCode review fixes - round 2 (d432125)
[^a-zA-Z0-9-]regex to match Claude's actual encodingmimeTypeandmediaTypeabortClaudeCLISessionoutside registry branch so it runs for all resume pathsCode review fixes - round 3 (33b7e42)
resumeSessionId || sessionIdin ptySessionsMapANSI_ESCAPE_REconstantPromise.allfor unlinks before rmdirdata.options.cwd = projectPathwhen cwd is missingTest plan
Summary by CodeRabbit
New Features
Enhancements