auto-claude: 229-implement-account-aware-terminal-session-persisten#1819
auto-claude: 229-implement-account-aware-terminal-session-persisten#1819
Conversation
… machine Add XState v5 state machine for terminal lifecycle with states: idle, shell_ready, claude_starting, claude_active, swapping, pending_resume, exited. Context tracks claudeSessionId, profileId, swap state, isBusy, and error. Includes guards (hasActiveSession, isSwapping) and assign actions for all context updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ate to TerminalProcess Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…el export Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Async and preserve YOLO mode - Add optional `options` parameter with `migratedSession` flag to resumeClaudeAsync() - When migratedSession is true, log post-swap resume context and skip sessionId deprecation warning - Preserve dangerouslySkipPermissions flag from terminal's stored state during resume Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation and options passthrough Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ED event and migratedSession to TERMINAL_RESUME_CLAUDE Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…profile change Replace manual "Run: claude --resume" message with automatic resume call via resumeClaudeInTerminal IPC. Updated preload API and IPC types to pass migratedSession option. YOLO mode is preserved automatically via the main process terminal object. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…minal-store Add module-level terminalActors Map and helper functions (getOrCreateTerminalActor, sendTerminalMachineEvent, deriveTerminalStateFromMachine) for XState actor management. Store actions (setClaudeMode, setClaudeSessionId, setClaudeBusy, setPendingClaudeResume) now forward corresponding events to the XState machine. Actors are cleaned up on terminal removal and clearAllTerminals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…UI messages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…l-machine Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughPropagates Claude-mode and YOLO permission flags through profile-swap flows, makes session migration async and persisted, extends resume APIs to accept migratedSession, adds server-side migrated-session flag storage, introduces an XState terminal machine with per-terminal actors, wires automatic renderer resume, i18n keys, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant TerminalManager
participant Migrator
participant ClaudeIntegration
participant Claude
Renderer->>TerminalManager: notify profile switch / request swap
TerminalManager->>ClaudeIntegration: capture sessionId
ClaudeIntegration-->>TerminalManager: sessionId
TerminalManager->>Migrator: migrateSession(source, target, sessionId)
Migrator-->>TerminalManager: { sessionMigrated, newSessionId? }
TerminalManager->>TerminalManager: storeMigratedSessionFlag(sessionId, dangerouslySkipPermissions)
TerminalManager->>ClaudeIntegration: resumeClaudeAsync(terminal, sessionId, { migratedSession: true })
ClaudeIntegration->>Claude: resume (sessionId + --dangerously-skip-permissions?)
Claude-->>ClaudeIntegration: resumed
TerminalManager->>Renderer: push terminalsNeedingRefresh { isClaudeMode, dangerouslySkipPermissions, sessionMigrated }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the terminal experience by introducing account-aware session persistence, particularly for Claude Code sessions. It formalizes terminal lifecycle management using an XState state machine, which enables automatic and seamless resumption of Claude sessions when users swap authentication profiles. This change eliminates manual intervention during profile changes, providing a more fluid and integrated user experience while improving the robustness and predictability of terminal state management. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
area/frontend
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to terminal session management, particularly around account profile swapping. The introduction of an XState machine to formalize the terminal lifecycle is a great step towards more robust state management. The automatic resumption of Claude sessions after a profile swap is also a major user experience enhancement. I've identified a critical issue where the new profile swap orchestration logic isn't being utilized by its intended caller, and a high-severity issue in the new state machine where a state is unreachable. Addressing these will ensure the new features work as expected.
| async swapProfileAndResume( | ||
| terminalId: string, | ||
| sourceConfigDir: string, | ||
| targetConfigDir: string, | ||
| targetProfileId: string | ||
| ): Promise<{ sessionMigrated: boolean; sessionId?: string; isClaudeMode: boolean }> { | ||
| const terminal = this.terminals.get(terminalId); | ||
| if (!terminal) { | ||
| return { sessionMigrated: false, isClaudeMode: false }; | ||
| } | ||
|
|
||
| const isClaudeMode = terminal.isClaudeMode; | ||
| const sessionId = terminal.claudeSessionId; | ||
| const sourceProfileId = terminal.claudeProfileId ?? 'unknown'; | ||
|
|
||
| // If no active Claude session or no session ID, skip migration | ||
| if (!isClaudeMode || !sessionId) { | ||
| debugLog('[terminal-manager] No active Claude session for terminal:', terminalId, '- skipping migration'); | ||
| return { sessionMigrated: false, isClaudeMode }; | ||
| } | ||
|
|
||
| // Mark swap state: capturing session | ||
| terminal.swapState = { | ||
| isSwapping: true, | ||
| phase: 'capturing_session', | ||
| targetProfileId, | ||
| sourceProfileId, | ||
| sessionMigrated: false | ||
| }; | ||
|
|
||
| // Migrate the session from source to target profile | ||
| terminal.swapState.phase = 'migrating'; | ||
| debugLog('[terminal-manager] Migrating session for terminal:', terminalId, 'session:', sessionId); | ||
|
|
||
| const migrationResult = migrateSession( | ||
| sourceConfigDir, | ||
| targetConfigDir, | ||
| terminal.cwd, | ||
| sessionId | ||
| ); | ||
|
|
||
| if (!migrationResult.success) { | ||
| debugError('[terminal-manager] Session migration failed for terminal:', terminalId, 'error:', migrationResult.error); | ||
| terminal.swapState.error = migrationResult.error; | ||
| terminal.swapState.sessionMigrated = false; | ||
| return { sessionMigrated: false, sessionId, isClaudeMode }; | ||
| } | ||
|
|
||
| // Migration succeeded | ||
| terminal.swapState.sessionMigrated = true; | ||
| terminal.swapState.phase = 'recreating'; | ||
| debugLog('[terminal-manager] Session migrated successfully for terminal:', terminalId); | ||
|
|
||
| // Notify renderer about swap progress | ||
| const win = this.getWindow(); | ||
| if (win && !win.isDestroyed()) { | ||
| win.webContents.send('TERMINAL_SWAP_PROGRESS', { | ||
| terminalId, | ||
| phase: 'migrating', | ||
| sessionMigrated: true | ||
| }); | ||
| } | ||
|
|
||
| return { sessionMigrated: true, sessionId, isClaudeMode }; | ||
| } |
There was a problem hiding this comment.
This new method swapProfileAndResume provides essential orchestration for profile swaps, including session migration and state management via swapState. However, it appears its intended caller, the CLAUDE_PROFILE_SET_ACTIVE IPC handler in terminal-handlers.ts, has not been updated to use this function. The old migration logic remains in the IPC handler, which means this new, more robust implementation is currently dead code and the new swapState logic for tracking swap progress will not be utilized. This will lead to inconsistent behavior and defeats the purpose of this refactoring. The IPC handler should be updated to call this method to ensure the new profile swap flow works as designed.
| CLAUDE_START: { target: 'claude_starting', actions: 'setProfileId' }, | ||
| SHELL_EXITED: { target: 'exited', actions: 'clearSession' }, | ||
| RESET: { target: 'idle', actions: 'resetContext' }, |
There was a problem hiding this comment.
The pending_resume state is currently unreachable because the RESUME_REQUESTED event, dispatched from terminal-store.ts, is not handled by the state machine. This breaks the state machine's ability to track deferred resumes.
To fix this, you should first add a transition in the shell_ready state to handle the RESUME_REQUESTED event. Then, update the setClaudeSessionId action (lines 140-148) to handle the RESUME_REQUESTED event:
// ...
setClaudeSessionId: assign({
claudeSessionId: ({ event }) => {
if (event.type === 'CLAUDE_ACTIVE') return event.claudeSessionId;
if (event.type === 'RESUME_COMPLETE') return event.claudeSessionId;
if (event.type === 'RESUME_REQUESTED') return event.claudeSessionId; // Add this line
return undefined;
},
// ... CLAUDE_START: { target: 'claude_starting', actions: 'setProfileId' },
RESUME_REQUESTED: { target: 'pending_resume', actions: 'setClaudeSessionId' },
SHELL_EXITED: { target: 'exited', actions: 'clearSession' },
RESET: { target: 'idle', actions: 'resetContext' },![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/frontend/src/main/terminal/claude-integration-handler.ts (1)
1200-1217:⚠️ Potential issue | 🟡 MinorSync
resumeClaudedoes not preserve YOLO mode, unlikeresumeClaudeAsync.The deprecated sync
resumeClaude(Line 1214) constructscommandwithout checkingterminal.dangerouslySkipPermissions, so--dangerously-skip-permissionsis lost on sync resume. Since the method is deprecated and callers are being migrated to async, this may be acceptable, but it's worth noting.apps/frontend/src/renderer/stores/terminal-store.ts (2)
403-425:⚠️ Potential issue | 🟠 MajorMachine event dispatch may silently fail depending on current machine state.
setClaudeMode(id, true)always sendsCLAUDE_ACTIVE, but the machine only handlesCLAUDE_ACTIVEin theclaude_startingstate. If the machine is inidleorshell_ready, the event is silently dropped, causing the Zustand boolean and the machine state to diverge.Similarly,
setClaudeMode(id, false)sendsCLAUDE_EXITED, which is only handled inclaude_starting,claude_active, andswapping— not inshell_readyoridle.Since the store continues to set the boolean regardless, the XState machine becomes an unreliable secondary source of truth. Consider either:
- Making the machine the single source of truth and deriving booleans from it, or
- Adding a guard/check before dispatching to avoid silent mismatches, or
- Ensuring callers always follow the machine's expected event sequence (SHELL_READY → CLAUDE_START → CLAUDE_ACTIVE).
465-485:⚠️ Potential issue | 🟠 Major
RESUME_REQUESTEDevent is dispatched but not handled by the machine.As noted in the
terminal-machine.tsreview, no state handlesRESUME_REQUESTED, sosendTerminalMachineEvent(id, { type: 'RESUME_REQUESTED', ... })at Line 470 will be silently dropped. Thepending_resumestate is unreachable from within the machine.This means the fallback check at Line 475 (
String(actor.getSnapshot().value) === 'pending_resume') will always be false through normal operation, making theRESUME_COMPLETEdispatch at Line 476 dead code.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts`:
- Around line 327-335: The IPC send is using a hardcoded channel and a stale
literal phase; replace the string 'TERMINAL_SWAP_PROGRESS' with the shared
constant (IPC_CHANNELS.TERMINAL_SWAP_PROGRESS) and send the actual current phase
value (swapState.phase) instead of the literal 'migrating' when calling
win.webContents.send in the getWindow() block so the renderer receives the
up-to-date phase.
- Around line 304-314: The call to migrateSession uses synchronous fs ops
(cpSync/copyFileSync/mkdirSync) which will block the Electron main event loop;
replace it with a non-blocking implementation by creating an async
migrateSessionAsync that uses fs.promises equivalents (or node:fs
cp/copyFile/mkdir with their Promise variants) or offload the migration to a
worker thread, then await its Promise from this function (update the caller to
be async) or schedule it via setImmediate/queueMicrotask so the main loop
yields; update references to migrateSession (and any calls around
terminal.swapState.phase, terminalId, sessionId) to use the new async function
and handle errors via try/catch and proper logging.
In `@apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts`:
- Around line 110-124: The try/catch in useTerminalProfileChange currently calls
window.electronAPI.resumeClaudeInTerminal without awaiting it, so rejections
bypass the catch and setPendingClaudeResume is never executed; update the call
to await window.electronAPI.resumeClaudeInTerminal(...) inside the try block (or
explicitly handle the returned Promise with .catch that calls
store.setPendingClaudeResume) so that errors are caught and debugError and
store.setPendingClaudeResume(newTerminal.id, true) run as intended; ensure the
surrounding function is async if needed.
In `@apps/frontend/src/shared/i18n/locales/fr/terminal.json`:
- Around line 15-21: Update the French strings under the "swap" object to
include proper accents: change "demarrer" to "démarrer" in swap.resumeFailed,
and change "change" → "changé" and "a reprendre" → "à reprendre" in
swap.noSession so the value reads "Profil changé. Aucune session active à
reprendre."; verify the other keys swap.inProgress, swap.resumingSession, and
swap.sessionResumed remain unchanged except for accent fixes.
In `@apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts`:
- Around line 226-266: Add a test that reaches pending_resume via the actual
transition instead of seeding the initial state: use runEvents starting from
'shell_ready' and dispatch { type: 'RESUME_REQUESTED', ... } (matching the event
shape used by the machine) and assert the snapshot transitions into
'pending_resume' and preserves/sets expected context (e.g.,
profileId/claudeSessionId), so you exercise the transition that should be
handled by the machine (symbols: pending_resume, shell_ready, RESUME_REQUESTED,
runEvents).
In `@apps/frontend/src/shared/state-machines/terminal-machine.ts`:
- Around line 62-68: The machine's 'pending_resume' state is never targeted
because no transitions handle the RESUME_REQUESTED event; add a RESUME_REQUESTED
transition in the appropriate states (at least in shell_ready and likely
claude_active) that targets 'pending_resume' and performs any needed actions
(e.g., preserve context or set a flag). Locate the state definitions for
shell_ready and claude_active in terminal-machine.ts and add an on: {
RESUME_REQUESTED: { target: 'pending_resume', actions: /* any existing action
like setProfileId or prepareResume */ } } entry so sendTerminalMachineEvent(...,
{ type: 'RESUME_REQUESTED', ... }) triggers the expected transition.
- Around line 130-133: The guard isSwapping in the guards object (checking
context.swapTargetProfileId) is dead code—either remove the isSwapping entry or
wire it into the state transitions that should gate swapping logic; if you
choose wiring, replace inline checks of context.swapTargetProfileId in relevant
transitions with the isSwapping guard name so the machine uses guards
consistently, or simply delete the isSwapping function from the guards object to
remove the unused code.
| // Notify renderer about swap progress | ||
| const win = this.getWindow(); | ||
| if (win && !win.isDestroyed()) { | ||
| win.webContents.send('TERMINAL_SWAP_PROGRESS', { | ||
| terminalId, | ||
| phase: 'migrating', | ||
| sessionMigrated: true | ||
| }); | ||
| } |
There was a problem hiding this comment.
Hardcoded IPC channel string and stale phase in the event payload.
Two issues:
-
Line 330 uses the string literal
'TERMINAL_SWAP_PROGRESS'instead of anIPC_CHANNELSconstant. All other IPC sends in the codebase use the constant (e.g.,IPC_CHANNELS.TERMINAL_TITLE_CHANGE). -
Line 332 reports
phase: 'migrating', butswapState.phasewas already updated to'recreating'at Line 324. The renderer receives a stale phase value.
🐛 Proposed fix
// Notify renderer about swap progress
const win = this.getWindow();
if (win && !win.isDestroyed()) {
- win.webContents.send('TERMINAL_SWAP_PROGRESS', {
+ win.webContents.send(IPC_CHANNELS.TERMINAL_SWAP_PROGRESS, {
terminalId,
- phase: 'migrating',
+ phase: 'recreating',
sessionMigrated: true
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Notify renderer about swap progress | |
| const win = this.getWindow(); | |
| if (win && !win.isDestroyed()) { | |
| win.webContents.send('TERMINAL_SWAP_PROGRESS', { | |
| terminalId, | |
| phase: 'migrating', | |
| sessionMigrated: true | |
| }); | |
| } | |
| // Notify renderer about swap progress | |
| const win = this.getWindow(); | |
| if (win && !win.isDestroyed()) { | |
| win.webContents.send(IPC_CHANNELS.TERMINAL_SWAP_PROGRESS, { | |
| terminalId, | |
| phase: 'recreating', | |
| sessionMigrated: true | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts` around lines 327 - 335,
The IPC send is using a hardcoded channel and a stale literal phase; replace the
string 'TERMINAL_SWAP_PROGRESS' with the shared constant
(IPC_CHANNELS.TERMINAL_SWAP_PROGRESS) and send the actual current phase value
(swapState.phase) instead of the literal 'migrating' when calling
win.webContents.send in the getWindow() block so the renderer receives the
up-to-date phase.
apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts
Show resolved
Hide resolved
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 7 issue(s) require attention.
7 issue(s) must be addressed (2 required, 5 recommended), 2 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 2 issue(s)
- Medium: 5 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (9 selected of 9 total)
🟠 [b46b072e42ee] [HIGH] Unreachable pending_resume state — no transition leads into it
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:113
The pending_resume state (lines 113-119) is defined and tested, but no state in the machine has an outgoing transition that targets it. The RESUME_REQUESTED event (line 33) is defined in the TerminalEvent union and is sent by setPendingClaudeResume() in terminal-store.ts (line 470), but no state's on: handlers include RESUME_REQUESTED. This means the event is silently dropped by XState and the machine can never reach pending_resume.
Verified by examining every state's on: handlers in the machine definition (lines 56-127). None reference RESUME_REQUESTED or target pending_resume. The test file works around this by using resolveState() to inject pending_resume as an initial state (line 227), masking the issue.
The machine needs a transition like claude_active → pending_resume on RESUME_REQUESTED (or from shell_ready for restored terminals). Without it, the XState machine and Zustand store will drift out of sync: Zustand sets pendingClaudeResume: true but the machine stays in its current state. | Three related dead-code issues in the terminal machine: (1) The pending_resume state (line 113) has no inbound transitions — no state ever targets it, making it unreachable during normal operation (only reachable via snapshot restoration in tests). (2) The RESUME_REQUESTED event (line 33) is defined in the TerminalEvent union but never handled by any state transition. (3) The isSwapping guard (line 132) is defined but never referenced in any transition. By contrast, the existing task-machine has all states reachable and all guards used. The deriveTerminalStateFromMachine function (terminal-store.ts:52) is also exported but never called.
Suggested fix:
Add a `RESUME_REQUESTED` transition from `shell_ready` (and/or `claude_active`) to `pending_resume`. For example, in the `shell_ready` state:
RESUME_REQUESTED: { target: 'pending_resume', actions: 'setClaudeSessionId' },
This will make the state reachable and keep the XState machine in sync with the Zustand boolean flags.
🟡 [bcb22400f4eb] [MEDIUM] Dead IPC channel: TERMINAL_SWAP_PROGRESS sent but never received
📁 apps/frontend/src/main/terminal/terminal-manager.ts:328
The swapProfileAndResume() method sends a TERMINAL_SWAP_PROGRESS event via win.webContents.send() (line 330), but:
- No renderer-side code listens for this event (searched all renderer, preload, and hook files)
- The channel name is not defined in the
IPC_CHANNELSconstant insrc/shared/constants/ipc.ts - No preload API method exposes a listener for it
- A hardcoded string
'TERMINAL_SWAP_PROGRESS'is used instead of the project's standardIPC_CHANNELS.*constant pattern
This is dead code that sends IPC messages into the void, and breaks the project's convention of defining all channel names in the IPC_CHANNELS constant.
Suggested fix:
Either: (a) Remove this dead IPC send if swap progress is tracked via the existing `onTerminalProfileChanged` event flow, or (b) complete the implementation by adding `TERMINAL_SWAP_PROGRESS` to `IPC_CHANNELS`, adding a preload API listener method, and consuming it in the renderer (e.g., to drive the XState machine's swap phase transitions).
🟡 [1e5525560365] [MEDIUM] Ineffective try/catch around fire-and-forget IPC send
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:110
The try/catch block wrapping window.electronAPI.resumeClaudeInTerminal() (lines 110-124) will never catch errors from the actual resume operation. The underlying implementation uses ipcRenderer.send() (fire-and-forget, line 170 of terminal-api.ts), which is synchronous and non-throwing for the send itself. Any errors during the actual resume happen asynchronously in the main process and are caught by the .catch() in terminal-handlers.ts (line 619).
The catch block sets pendingClaudeResume as a fallback (line 123), but this fallback will never execute since ipcRenderer.send() doesn't throw. If the main process resume fails, the terminal will be left in a state where Claude appears to be starting but nothing happens — no fallback is triggered. | The try/catch block wrapping window.electronAPI.resumeClaudeInTerminal() (line 114) will never catch errors from the main process. The underlying implementation (terminal-api.ts:169-170) uses ipcRenderer.send() which is a one-way fire-and-forget call that returns void synchronously. Any errors in terminalManager.resumeClaudeAsync() on the main process are caught and logged via .catch() in terminal-handlers.ts:620-622, but never propagated back to the renderer. This means the fallback at line 123 (store.setPendingClaudeResume(newTerminal.id, true)) is dead code — if resume fails on the main process, the user gets a silent failure (empty shell prompt) with no pending-resume indicator.
Suggested fix:
Remove the try/catch since `ipcRenderer.send()` won't throw. Instead, consider either: (a) switching to `ipcRenderer.invoke()` for the resume call so errors propagate back, or (b) implementing a callback/event-based error notification from the main process (e.g., via the `TERMINAL_SWAP_PROGRESS` channel with a failure phase) so the renderer can react to failed resumes.
🔵 [dba64be435d3] [LOW] Unused guard isSwapping defined but never referenced
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:132
The isSwapping guard is defined at line 132 but is never used in any state transition within the machine. Only the hasActiveSession guard is referenced (line 83 in the claude_active → swapping transition). This is dead code that adds to the maintenance surface without providing value.
Suggested fix:
Remove the `isSwapping` guard definition, or if it's intended for future use (e.g., guarding swap sub-phase transitions), add a TODO comment explaining the planned usage.
🔵 [74cfb611c8c4] [LOW] Inconsistent swap phase naming between main-process and XState types
📁 apps/frontend/src/main/terminal/types.ts:38
The swap phase names differ between the main-process TerminalSwapPhase type and the XState machine's swapPhase context field:
- Main process (
types.tsline 38-42):'capturing_session' | 'migrating' | 'recreating' | 'resuming' - XState machine (
terminal-machine.tsline 13):'capturing' | 'migrating' | 'recreating' | 'resuming'
The first phase uses capturing_session in the main process but capturing in the XState machine. While these are separate type systems (main vs renderer), they represent the same conceptual phases. This inconsistency could cause confusion during debugging and makes it harder to correlate swap progress between main and renderer processes. | The main-process TerminalSwapPhase type uses 'capturing_session' (snake_case with suffix) while the XState machine's swapPhase context uses 'capturing' (no suffix). Both represent the same conceptual phase. This creates confusion: the main process sets terminal.swapState.phase = 'capturing_session' (terminal-manager.ts:298) while the XState machine uses swapPhase: 'capturing' (terminal-machine.ts:169). The other three phases (migrating, recreating, resuming) are consistent between both. Searched Grep('capturing_session|capturing', 'src/') to confirm the mismatch across files.
Suggested fix:
Align the phase names. Either rename the main-process phase to `'capturing'` to match XState, or create a shared enum/const that both can reference. Consider exporting a single `SwapPhase` type from the shared types directory.
🟠 [3935fb45acd8] [HIGH] [NEEDS REVIEW] YOLO mode flag lost during profile swap terminal recreation
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:112
The comment on lines 112-113 claims 'YOLO mode (dangerouslySkipPermissions) is preserved on the main process terminal object', but this is incorrect. The profile swap flow destroys the old terminal (line 60: destroyTerminal(terminalId)) and creates a brand new one (line 82: createTerminal(...)). In terminal-lifecycle.ts:100-108, a new TerminalProcess is initialized with only {id, pty, isClaudeMode: false, projectPath, cwd, outputBuffer, title, shellType} — dangerouslySkipPermissions defaults to undefined. When resumeClaudeAsync runs on the new terminal, terminal.dangerouslySkipPermissions is falsy, so extraFlags becomes empty string and --dangerously-skip-permissions is omitted from the resume command. Users who had YOLO mode enabled will have their session resumed with permission prompts they previously bypassed.
Suggested fix:
Pass `dangerouslySkipPermissions` from the old terminal state through the profile change event (add it to `TerminalProfileChangedEvent.terminals[]`), and either: (a) set it on the new main-process terminal after creation, or (b) pass it as part of the resume options so `resumeClaudeAsync` can use it directly instead of reading from the terminal object.
🟡 [5658e8cee559] [MEDIUM] XState machine permanently stuck in idle — all dispatched events silently dropped
📁 apps/frontend/src/renderer/stores/terminal-store.ts:403
The XState machine actors are lazily created starting in idle state. The idle state only accepts SHELL_READY and RESET events (terminal-machine.ts:56-60). However, no code anywhere in the renderer dispatches SHELL_READY or CLAUDE_START events. The Zustand store methods directly send CLAUDE_ACTIVE (setClaudeMode, setClaudeSessionId), CLAUDE_EXITED (setClaudeMode), CLAUDE_BUSY (setClaudeBusy), and RESUME_REQUESTED (setPendingClaudeResume) — none of which are handled in idle. In XState, unhandled events are silently dropped. This means the machine never transitions from idle, and all events sent to it have no effect. The deriveTerminalStateFromMachine function (defined at line 52) is also never called, confirming the machine is fully disconnected from the actual state.
Suggested fix:
Add `SHELL_READY` event dispatch when a terminal is first created/restored (e.g., in `addTerminal` or when the terminal status changes to 'running'), and add `CLAUDE_START` dispatch in `setClaudeMode(id, true)` before `CLAUDE_ACTIVE`. Alternatively, restructure the machine to accept `CLAUDE_ACTIVE` directly from `idle` if the prerequisites aren't meaningful for the renderer's perspective.
🟡 [ab2e8bacea47] [MEDIUM] Hardcoded IPC channel string instead of IPC_CHANNELS constant
📁 apps/frontend/src/main/terminal/terminal-manager.ts:330
The swapProfileAndResume method uses a hardcoded string 'TERMINAL_SWAP_PROGRESS' for webContents.send(). Every other IPC send in the terminal system uses IPC_CHANNELS.* constants from src/shared/constants/ipc.ts (verified: TERMINAL_TITLE_CHANGE, TERMINAL_RATE_LIMIT, TERMINAL_OAUTH_TOKEN, TERMINAL_CLAUDE_SESSION, TERMINAL_CLAUDE_EXIT, TERMINAL_CLAUDE_BUSY, TERMINAL_ONBOARDING_COMPLETE all use the constant). Additionally, the TERMINAL_SWAP_PROGRESS channel is never registered in IPC_CHANNELS and has no listener in the renderer — it's sent but never received, making it dead code.
Suggested fix:
Either register `TERMINAL_SWAP_PROGRESS` in `IPC_CHANNELS` in `src/shared/constants/ipc.ts` and add a renderer listener, or remove this dead send entirely if the renderer doesn't need it (the XState machine tracks swap state independently).
🟡 [a9c07fea8239] [MEDIUM] French translations missing required accent characters
📁 apps/frontend/src/shared/i18n/locales/fr/terminal.json:19
The new swap section in French translations is missing accent characters that are standard in French. The rest of the file correctly uses accents (e.g., 'précédente', 'trouvé', 'détaché'). Line 19 has 'demarrer' (should be 'démarrer') and line 20 has 'change' (should be 'changé') and 'a reprendre' (should be 'à reprendre'). This is inconsistent with the existing French translations in the same file.
Suggested fix:
Fix the accents:
- Line 19: "Impossible de reprendre la session. Vous pouvez démarrer une nouvelle session."
- Line 20: "Profil changé. Aucune session active à reprendre."
This review was generated by Auto Claude.
…reservation - Make pending_resume state reachable by adding RESUME_REQUESTED transitions from shell_ready and claude_active states - Add CLAUDE_ACTIVE transition from shell_ready for direct activation path - Wire SHELL_READY dispatch in setTerminalStatus and setClaudeMode to prevent XState machine from being stuck in idle - Remove dead TERMINAL_SWAP_PROGRESS IPC send (no listener existed) - Remove unused isSwapping guard from terminal machine - Remove ineffective try/catch around fire-and-forget ipcRenderer.send() - Preserve dangerouslySkipPermissions (YOLO mode) through profile swap terminal recreation flow via resume options - Align swap phase naming: capturing_session → capturing (matches XState) - Fix French translation accents: démarrer, changé, à Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| debugLog('[useTerminalProfileChange] Session migrated, auto-resuming:', sessionId); | ||
| // Store the session ID for tracking | ||
| store.setClaudeSessionId(newTerminal.id, sessionId); | ||
| // Set pending resume flag - user can trigger resume from terminal tab | ||
| store.setPendingClaudeResume(newTerminal.id, true); | ||
| // Send a message to the terminal about the session | ||
| window.electronAPI.sendTerminalInput( | ||
|
|
||
| // Auto-resume the Claude session with --continue | ||
| // Pass dangerouslySkipPermissions so the new terminal preserves YOLO mode | ||
| window.electronAPI.resumeClaudeInTerminal( | ||
| newTerminal.id, | ||
| `# Profile switched. Previous Claude session available.\n# Run: claude --resume ${sessionId}\n` | ||
| sessionId, | ||
| { migratedSession: true, dangerouslySkipPermissions } | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/stores/terminal-store.ts (1)
395-406:⚠️ Potential issue | 🟠 MajorSend
SHELL_EXITEDevent when terminal status transitions to'exited'.The
setTerminalStatusmethod sendsSHELL_READYwhen status becomes'running'but neglects to send a correspondingSHELL_EXITEDevent when status becomes'exited'. This asymmetry causes the XState machine to diverge from the Zustand store: the store will track the terminal as exited, but the machine remains in its previous state, unable to transition to theexitedstate.The
exitCodeis available at the call site inuseTerminalEvents.ts(line ~43, passed fromonTerminalExit), so it should be passed tosetTerminalStatusand forwarded in theSHELL_EXITEDevent dispatch.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts`:
- Around line 279-333: The method swapProfileAndResume in TerminalManager is
dead code and duplicates logic already implemented inline in the
CLAUDE_PROFILE_SET_ACTIVE IPC handler (which directly calls migrateSession);
remove swapProfileAndResume and its associated dead swapState handling on the
Terminal object (fields and any initialization/reads/writes to
terminal.swapState) to avoid duplication, and ensure no other callers reference
swapProfileAndResume (search for swapProfileAndResume and terminal.swapState),
leaving the CLAUDE_PROFILE_SET_ACTIVE handler and its migrateSession call as the
single source of truth for session migration.
In `@apps/frontend/src/preload/api/terminal-api.ts`:
- Line 50: The TerminalAPI interface's resumeClaudeInTerminal signature omits
the dangerouslySkipPermissions flag in its options type; update the options type
for resumeClaudeInTerminal in TerminalAPI to include
dangerouslySkipPermissions?: boolean (matching the implementation used by
resumeClaudeInTerminal and the ElectronAPI definition) so callers get correct
autocomplete and type-checking for that flag.
In `@apps/frontend/src/renderer/stores/terminal-store.ts`:
- Around line 475-488: The setPendingClaudeResume handler can set Zustand's
pendingClaudeResume without ever sending RESUME_REQUESTED when a terminal lacks
claudeSessionId, causing state divergence; update setPendingClaudeResume to
handle the missing claudeSessionId path by either clearing the pending flag
and/or sending a warning: when pending is true and get().terminals.find(...)
returns a terminal without terminal.claudeSessionId, call the same setter to
reset pendingClaudeResume for that id (or set it false) and emit a
processLogger.warn or console.warn that RESUME_REQUESTED was skipped for that
terminal; keep the existing branch that sends RESUME_REQUESTED via
sendTerminalMachineEvent when claudeSessionId is present and keep the
pending-clear logic for the false branch that inspects terminalActors.get(id).
- Line 7: The import uses a relative path; change it to the project path-alias
form so it follows tsconfig aliases—replace the relative import of
terminalMachine and TerminalEvent with the `@shared` alias (importing
terminalMachine and type TerminalEvent from '@shared/state-machines') so the
file consistently uses the project's path-alias convention.
- Around line 505-512: clearAllTerminals currently stops XState actors and
clears terminalActors but leaves stale xtermCallbacks and terminalBufferManager
entries; update clearAllTerminals to mirror removeTerminal by iterating all
terminal IDs (or the keys from terminalActors before clearing), for each id call
any cleanup used in removeTerminal (remove xtermCallbacks[id] / unregister any
xterm event handlers and delete the xtermCallbacks entry) and call
terminalBufferManager.remove(id) (or the equivalent removal method), then stop
actors, clear terminalActors and finally set({ terminals: [], activeTerminalId:
null, hasRestoredSessions: false }).
- Around line 52-62: Replace direct string comparisons against
String(snapshot.value) with the XState v5 idiomatic snapshot.matches(...) to
make state checks resilient to future nesting: in deriveTerminalStateFromMachine
replace the three checks (currently computing isClaudeMode, isClaudeBusy uses
context) to use snapshot.matches('claude_starting'),
snapshot.matches('claude_active'), snapshot.matches('swapping') (or combine
matches with logical OR), and replace other occurrences where
String(snapshot.value) === '...' (notably the other checks referenced near the
uses around the current file's earlier/later usages) with
snapshot.matches('pending_resume') or the appropriate state name; update all
checks at the referenced call sites (including the checks noted near lines 413
and 485) to use snapshot.matches instead of String(snapshot.value) comparisons
to follow XState v5 patterns.
In `@apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts`:
- Around line 9-12: The test helper runEvents currently types initialContext as
Record<string, unknown>, which permits arbitrary keys; change its signature to
accept initialContext?: Partial<TerminalContext> so callers get compile-time
checking for valid TerminalContext fields. Update any call sites that pass
context to match the Partial<TerminalContext> type and import or reference the
TerminalContext type where runEvents is declared to ensure proper typing.
In `@apps/frontend/src/shared/state-machines/terminal-machine.ts`:
- Around line 79-91: The CLAUDE_EXITED transition in the claude_active state
currently only runs the clearSession action and drops event.error; change that
transition to run both setError and clearSession (same as claude_starting's
CLAUDE_EXITED which uses ['setError','clearSession']) so that event.error is
preserved before clearing the session; update the claude_active state's
CLAUDE_EXITED transition to include the setError action alongside clearSession.
| ) => Promise<IPCResult<import('../../shared/types').TerminalRestoreResult>>; | ||
| clearTerminalSessions: (projectPath: string) => Promise<IPCResult>; | ||
| resumeClaudeInTerminal: (id: string, sessionId?: string) => void; | ||
| resumeClaudeInTerminal: (id: string, sessionId?: string, options?: { migratedSession?: boolean }) => void; |
There was a problem hiding this comment.
TerminalAPI interface is missing dangerouslySkipPermissions in the options type.
The interface declares options?: { migratedSession?: boolean }, but the implementation at line 169 and the ElectronAPI in ipc.ts (line 255) both include dangerouslySkipPermissions?: boolean. Callers using the TerminalAPI type won't see the flag in autocomplete/type hints.
🐛 Proposed fix
- resumeClaudeInTerminal: (id: string, sessionId?: string, options?: { migratedSession?: boolean }) => void;
+ resumeClaudeInTerminal: (id: string, sessionId?: string, options?: { migratedSession?: boolean; dangerouslySkipPermissions?: boolean }) => void;🤖 Prompt for AI Agents
In `@apps/frontend/src/preload/api/terminal-api.ts` at line 50, The TerminalAPI
interface's resumeClaudeInTerminal signature omits the
dangerouslySkipPermissions flag in its options type; update the options type for
resumeClaudeInTerminal in TerminalAPI to include dangerouslySkipPermissions?:
boolean (matching the implementation used by resumeClaudeInTerminal and the
ElectronAPI definition) so callers get correct autocomplete and type-checking
for that flag.
| import { v4 as uuid } from 'uuid'; | ||
| import { arrayMove } from '@dnd-kit/sortable'; | ||
| import type { TerminalSession, TerminalWorktreeConfig } from '../../shared/types'; | ||
| import { terminalMachine, type TerminalEvent } from '../../shared/state-machines'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use path alias instead of relative import.
Line 7 is a new import and should use the @shared/* alias per project conventions.
Suggested fix
-import { terminalMachine, type TerminalEvent } from '../../shared/state-machines';
+import { terminalMachine, type TerminalEvent } from '@shared/state-machines';As per coding guidelines: "Frontend code must use path aliases defined in tsconfig.json (@/*, @shared/*, @preload/*, @features/*, @components/*, @hooks/*, @lib/*)"
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts` at line 7, The import
uses a relative path; change it to the project path-alias form so it follows
tsconfig aliases—replace the relative import of terminalMachine and
TerminalEvent with the `@shared` alias (importing terminalMachine and type
TerminalEvent from '@shared/state-machines') so the file consistently uses the
project's path-alias convention.
| export function deriveTerminalStateFromMachine(snapshot: SnapshotFrom<typeof terminalMachine>): { | ||
| isClaudeMode: boolean; | ||
| isClaudeBusy: boolean; | ||
| pendingClaudeResume: boolean; | ||
| } { | ||
| const state = String(snapshot.value); | ||
| const isClaudeMode = state === 'claude_starting' || state === 'claude_active' || state === 'swapping'; | ||
| const isClaudeBusy = snapshot.context.isBusy; | ||
| const pendingClaudeResume = state === 'pending_resume'; | ||
| return { isClaudeMode, isClaudeBusy, pendingClaudeResume }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
fd 'terminal-machine.ts' --type fRepository: AndyMik90/Auto-Claude
Length of output: 124
🏁 Script executed:
cat -n apps/frontend/src/shared/state-machines/terminal-machine.tsRepository: AndyMik90/Auto-Claude
Length of output: 8859
🏁 Script executed:
wc -l apps/frontend/src/renderer/stores/terminal-store.tsRepository: AndyMik90/Auto-Claude
Length of output: 120
🏁 Script executed:
sed -n '52,62p' apps/frontend/src/renderer/stores/terminal-store.ts && echo "---" && sed -n '410,420p' apps/frontend/src/renderer/stores/terminal-store.ts && echo "---" && sed -n '480,490p' apps/frontend/src/renderer/stores/terminal-store.tsRepository: AndyMik90/Auto-Claude
Length of output: 1413
Consider using snapshot.matches() for more robust XState v5 patterns.
The terminal machine uses only flat top-level states (idle, shell_ready, claude_starting, claude_active, swapping, pending_resume, exited), so String(snapshot.value) works correctly here and does not produce unexpected behavior.
However, snapshot.matches() is the more idiomatic XState v5 API and is more resilient if nested states are ever introduced. Refactoring String(snapshot.value) === '...' to snapshot.matches('...') at lines 57–59, 413, and 485 would align with XState v5 best practices without requiring any functional changes.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts` around lines 52 - 62,
Replace direct string comparisons against String(snapshot.value) with the XState
v5 idiomatic snapshot.matches(...) to make state checks resilient to future
nesting: in deriveTerminalStateFromMachine replace the three checks (currently
computing isClaudeMode, isClaudeBusy uses context) to use
snapshot.matches('claude_starting'), snapshot.matches('claude_active'),
snapshot.matches('swapping') (or combine matches with logical OR), and replace
other occurrences where String(snapshot.value) === '...' (notably the other
checks referenced near the uses around the current file's earlier/later usages)
with snapshot.matches('pending_resume') or the appropriate state name; update
all checks at the referenced call sites (including the checks noted near lines
413 and 485) to use snapshot.matches instead of String(snapshot.value)
comparisons to follow XState v5 patterns.
| setPendingClaudeResume: (id: string, pending: boolean) => { | ||
| // Send RESUME_REQUESTED or RESUME_COMPLETE to XState machine | ||
| if (pending) { | ||
| const terminal = get().terminals.find(t => t.id === id); | ||
| if (terminal?.claudeSessionId) { | ||
| sendTerminalMachineEvent(id, { type: 'RESUME_REQUESTED', claudeSessionId: terminal.claudeSessionId }); | ||
| } | ||
| } else { | ||
| // Resume cleared - either completed or cancelled | ||
| const actor = terminalActors.get(id); | ||
| if (actor && String(actor.getSnapshot().value) === 'pending_resume') { | ||
| sendTerminalMachineEvent(id, { type: 'RESUME_COMPLETE' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
XState/Zustand state divergence when claudeSessionId is absent.
When pending is true but the terminal has no claudeSessionId, RESUME_REQUESTED is silently skipped (line 479 guard) while the Zustand pendingClaudeResume flag is still set to true (line 492). This creates a divergence — the UI-facing state says resume is pending, but the machine never entered pending_resume. Consider logging a warning or aligning the two paths.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts` around lines 475 - 488,
The setPendingClaudeResume handler can set Zustand's pendingClaudeResume without
ever sending RESUME_REQUESTED when a terminal lacks claudeSessionId, causing
state divergence; update setPendingClaudeResume to handle the missing
claudeSessionId path by either clearing the pending flag and/or sending a
warning: when pending is true and get().terminals.find(...) returns a terminal
without terminal.claudeSessionId, call the same setter to reset
pendingClaudeResume for that id (or set it false) and emit a processLogger.warn
or console.warn that RESUME_REQUESTED was skipped for that terminal; keep the
existing branch that sends RESUME_REQUESTED via sendTerminalMachineEvent when
claudeSessionId is present and keep the pending-clear logic for the false branch
that inspects terminalActors.get(id).
apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts
Outdated
Show resolved
Hide resolved
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 8 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 9 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 10 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 9 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- security: YOLO mode flag accepted from renderer via IPC without server-side validation
- quality: CLAUDE_ACTIVE event dropped in claude_active state — claudeSessionId never set in XState context
- quality: swapProfileAndResume method defined but never called — dead code
- quality: deriveTerminalStateFromMachine exported but never imported or used
- quality: clearAllTerminals does not clean up xtermCallbacks or terminalBufferManager
- quality: Missing SHELL_EXITED event dispatch when terminal status transitions to 'exited'
- quality: [FROM COMMENTS] clearAllTerminals does not clean up xtermCallbacks or terminalBufferManager
- quality: [FROM COMMENTS] Missing SHELL_EXITED event when terminal exits
Verdict
CI Status: ✅ All 20 CI checks passing — no CI blockers.
Previous findings: Excellent progress — 8 of 9 previous findings are fully resolved, 1 (French accents) is partially resolved (the new swap section is correct; remaining accent issues are pre-existing out-of-scope code). The commit title accurately describes the work done.
However, the fix commit introduces new issues that need attention:
-
Security [HIGH]: The
TERMINAL_RESUME_CLAUDEIPC handler acceptsdangerouslySkipPermissionsdirectly from the renderer without server-side validation (NEW-001/NEW-007). UnlikeTERMINAL_INVOKE_CLAUDEwhich reads from the settings file, this handler trusts the renderer's input, creating an escalation path where a compromised renderer could enable YOLO mode on any terminal. Fix: read the flag from the terminal's already-stored value or settings file on the main process side. -
XState Logic [HIGH]:
setClaudeSessionIdsendsCLAUDE_ACTIVEto the XState machine, but this event is silently dropped when the machine is already inclaude_activestate (NEW-005). This meanscontext.claudeSessionIdis never set, causing thehasActiveSessionguard to always blockSWAP_INITIATEDtransitions (NEW-008). Note: this doesn't break current user-facing functionality since the XState machine state is not yet consumed (deriveTerminalStateFromMachine is unused), but it must be fixed before the machine becomes the source of truth. -
Dead code [MEDIUM]:
swapProfileAndResumemethod (NEW-002) andderiveTerminalStateFromMachinefunction (NEW-004) are defined but never called anywhere. -
Incomplete XState wiring [MEDIUM]:
setTerminalStatussendsSHELL_READYfor 'running' but never sendsSHELL_EXITEDfor 'exited' (CMT-003).clearAllTerminalscleans up XState actors but leavesxtermCallbacksandterminalBufferManagerstale (CMT-002). -
Type mismatch [LOW]:
TerminalAPIinterface missingdangerouslySkipPermissionsin options type (CMT-001).
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator, finding-validator, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (10 selected of 10 total)
🟠 [NEW-001] [HIGH] YOLO mode flag accepted from renderer via IPC without server-side validation
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:620
The TERMINAL_RESUME_CLAUDE IPC handler accepts dangerouslySkipPermissions from the renderer process and passes it through to terminal-manager without validation. Unlike TERMINAL_INVOKE_CLAUDE (line 72) which reads the flag from the settings file server-side, this handler trusts renderer input. A compromised renderer could call resumeClaudeInTerminal with dangerouslySkipPermissions:true on any terminal to escalate to YOLO mode. Fix: ignore the renderer's value and read from the terminal's already-stored dangerouslySkipPermissions or from the settings file.
Suggested fix:
In the TERMINAL_RESUME_CLAUDE handler, ignore options.dangerouslySkipPermissions from the renderer. Instead read the terminal's stored dangerouslySkipPermissions value (already set by the main process during invokeClaudeAsync) or read from the settings file.
🟠 [NEW-005] [HIGH] CLAUDE_ACTIVE event dropped in claude_active state — claudeSessionId never set in XState context
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:79
setClaudeMode(true) sends CLAUDE_ACTIVE without a sessionId, transitioning the machine to claude_active. Then setClaudeSessionId sends CLAUDE_ACTIVE with the sessionId, but the claude_active state doesn't handle CLAUDE_ACTIVE (only handles CLAUDE_BUSY, CLAUDE_EXITED, SWAP_INITIATED, RESUME_REQUESTED, SHELL_EXITED, RESET). XState silently drops the event, so context.claudeSessionId remains undefined. This blocks the hasActiveSession guard from ever returning true, preventing SWAP_INITIATED transitions. Fix: add a self-transition for CLAUDE_ACTIVE in claude_active state, or use a separate SET_SESSION_ID event type.
Suggested fix:
Add a self-transition in the claude_active state:
claude_active: {
on: {
CLAUDE_ACTIVE: { actions: 'setClaudeSessionId' },
// ... existing handlers
}
}
🟡 [NEW-002] [MEDIUM] swapProfileAndResume method defined but never called — dead code
📁 apps/frontend/src/main/terminal/terminal-manager.ts:279
The swapProfileAndResume method (lines 279-333) was added as part of the fix commit but is never called anywhere. The CLAUDE_PROFILE_SET_ACTIVE IPC handler performs session migration directly using migrateSession() instead. The method's JSDoc incorrectly claims it is called from the IPC handler. Either wire it in or remove it.
Suggested fix:
Remove the dead code method, or refactor the CLAUDE_PROFILE_SET_ACTIVE handler to use it.
🟡 [NEW-004] [MEDIUM] deriveTerminalStateFromMachine exported but never imported or used
📁 apps/frontend/src/renderer/stores/terminal-store.ts:52
The function deriveTerminalStateFromMachine (lines 52-62) is defined and exported but never imported or called from any file. It was intended to bridge XState machine state back to Zustand boolean fields, but the actual wiring code updates booleans directly. This makes the XState machine a write-only side channel. Either wire this function in to make XState the source of truth, or remove it.
Suggested fix:
Either remove the unused function or integrate it into the store actions so Zustand booleans are derived from the XState machine.
🟡 [CMT-002] [MEDIUM] clearAllTerminals does not clean up xtermCallbacks or terminalBufferManager
📁 apps/frontend/src/renderer/stores/terminal-store.ts:505
clearAllTerminals (lines 505-512) stops and clears XState actors but does not call xtermCallbacks.clear() or clean up terminalBufferManager entries. Compare with removeTerminal (lines 359-368) which properly cleans up all three resources. This leaves stale callback references and buffer data in memory when clearAllTerminals is called.
Suggested fix:
Add xtermCallbacks.clear() and iterate terminals to call terminalBufferManager.dispose(id) for each before clearing state.
🟡 [CMT-003] [MEDIUM] Missing SHELL_EXITED event dispatch when terminal status transitions to 'exited'
📁 apps/frontend/src/renderer/stores/terminal-store.ts:395
setTerminalStatus sends SHELL_READY when status is 'running' (line 397) but never sends SHELL_EXITED when status is 'exited'. The XState machine defines SHELL_EXITED transitions in all relevant states, but the event is never dispatched from renderer code. The machine's 'exited' state is unreachable through normal terminal exit flow, causing XState/Zustand state divergence.
Suggested fix:
Add: if (status === 'exited') { sendTerminalMachineEvent(id, { type: 'SHELL_EXITED' }); } to setTerminalStatus.
🔵 [CMT-001] [LOW] TerminalAPI interface options type missing dangerouslySkipPermissions
📁 apps/frontend/src/preload/api/terminal-api.ts:50
The TerminalAPI interface declares resumeClaudeInTerminal with options typed as { migratedSession?: boolean } but the implementation at line 169 and ElectronAPI type at ipc.ts:255 both include dangerouslySkipPermissions?: boolean. Callers using the TerminalAPI interface cannot pass the flag.
Suggested fix:
Update the interface at line 50 to include dangerouslySkipPermissions?: boolean in the options type.
🟡 [CMT-002] [MEDIUM] [FROM COMMENTS] clearAllTerminals does not clean up xtermCallbacks or terminalBufferManager
📁 apps/frontend/src/renderer/stores/terminal-store.ts:505
From CodeRabbit: clearAllTerminals cleans up XState actors but not xtermCallbacks or terminalBufferManager, unlike removeTerminal which cleans all three. Confirmed valid by finding-validator.
Suggested fix:
Add xtermCallbacks.clear() and terminalBufferManager cleanup to clearAllTerminals.
🟡 [CMT-003] [MEDIUM] [FROM COMMENTS] Missing SHELL_EXITED event when terminal exits
📁 apps/frontend/src/renderer/stores/terminal-store.ts:395
From CodeRabbit: setTerminalStatus sends SHELL_READY for 'running' but not SHELL_EXITED for 'exited', causing XState machine to never reach its exited state. Confirmed valid by finding-validator.
Suggested fix:
Add SHELL_EXITED dispatch when status is 'exited'.
🔵 [CMT-001] [LOW] [FROM COMMENTS] TerminalAPI interface type mismatch
📁 apps/frontend/src/preload/api/terminal-api.ts:50
From CodeRabbit: TerminalAPI interface missing dangerouslySkipPermissions in options type, mismatching the implementation. Confirmed valid by finding-validator.
Suggested fix:
Add dangerouslySkipPermissions?: boolean to the interface options type.
This review was generated by Auto Claude.
…erminal-session-persisten
Security: - Move YOLO mode (dangerouslySkipPermissions) to server-side storage instead of accepting from renderer via IPC. Main process stores flag during profile migration and restores it when resume is called. XState machine fixes: - Add CLAUDE_ACTIVE self-transition in claude_active state so setClaudeSessionId can update context.claudeSessionId (was silently dropped, blocking hasActiveSession guard) - Add SHELL_EXITED dispatch in setTerminalStatus for 'exited' status so XState machine reaches its exited state Dead code removal: - Remove unused swapProfileAndResume method (migration handled directly in IPC handler) and its now-unused migrateSession import - Remove unused deriveTerminalStateFromMachine function and SnapshotFrom import Resource cleanup: - clearAllTerminals now disposes terminalBufferManager entries and xtermCallbacks alongside XState actors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts`:
- Around line 272-285: The post-swap deferred resume call to
ClaudeIntegration.resumeClaudeAsync passes migratedSession: true but drops the
sessionId, so migratedSessionFlags (YOLO mode) are not applied; update the call
site in terminal-manager.ts (the block that computes isPostSwap and calls
ClaudeIntegration.resumeClaudeAsync) to pass the terminal's sessionId (e.g.,
terminal.sessionId) when isPostSwap is true, or alternatively add a fallback in
ClaudeIntegration.resumeClaudeAsync to read sessionId from the terminal object
if the sessionId argument is undefined; ensure the resumeClaudeAsync invocation
includes the migratedSession flag plus the correct sessionId so
migratedSessionFlags are restored.
In `@apps/frontend/src/renderer/stores/terminal-store.ts`:
- Around line 394-405: setClaudeMode currently sends a CLAUDE_ACTIVE event with
no sessionId which can overwrite an existing claudeSessionId in the machine; fix
by reading the actor snapshot's context (via
getOrCreateTerminalActor(id).getSnapshot().context.claudeSessionId) and include
that sessionId in the payload when calling sendTerminalMachineEvent(id, { type:
'CLAUDE_ACTIVE', sessionId: existingId }) only if it exists, otherwise omit the
field; apply the same change to the other CLAUDE_ACTIVE send-site referenced in
the review so neither call clears an existing sessionId.
| // Determine if this is a post-swap deferred resume | ||
| const isPostSwap = terminal.swapState?.isSwapping === true && terminal.swapState?.sessionMigrated === true; | ||
|
|
||
| // Clear the pending flag | ||
| terminal.pendingClaudeResume = false; | ||
|
|
||
| // Clear swap state if this was a post-swap resume | ||
| if (isPostSwap) { | ||
| terminal.swapState = undefined; | ||
| } | ||
|
|
||
| // Now actually resume Claude | ||
| await ClaudeIntegration.resumeClaudeAsync(terminal, undefined, this.getWindow); | ||
| await ClaudeIntegration.resumeClaudeAsync(terminal, undefined, this.getWindow, isPostSwap ? { migratedSession: true } : undefined); | ||
| } |
There was a problem hiding this comment.
Post-swap deferred resume drops the migrated sessionId.
Line 284 calls resume with migratedSession: true but sessionId is undefined, so migratedSessionFlags won’t be applied and YOLO mode won’t be restored for deferred resumes. Pass the terminal’s sessionId (or fallback inside resumeClaudeAsync).
🐛 Suggested fix
- await ClaudeIntegration.resumeClaudeAsync(terminal, undefined, this.getWindow, isPostSwap ? { migratedSession: true } : undefined);
+ const resumeSessionId = terminal.claudeSessionId;
+ await ClaudeIntegration.resumeClaudeAsync(
+ terminal,
+ resumeSessionId,
+ this.getWindow,
+ isPostSwap ? { migratedSession: true } : undefined
+ );🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts` around lines 272 - 285,
The post-swap deferred resume call to ClaudeIntegration.resumeClaudeAsync passes
migratedSession: true but drops the sessionId, so migratedSessionFlags (YOLO
mode) are not applied; update the call site in terminal-manager.ts (the block
that computes isPostSwap and calls ClaudeIntegration.resumeClaudeAsync) to pass
the terminal's sessionId (e.g., terminal.sessionId) when isPostSwap is true, or
alternatively add a fallback in ClaudeIntegration.resumeClaudeAsync to read
sessionId from the terminal object if the sessionId argument is undefined;
ensure the resumeClaudeAsync invocation includes the migratedSession flag plus
the correct sessionId so migratedSessionFlags are restored.
| setClaudeMode: (id: string, isClaudeMode: boolean) => { | ||
| // Send corresponding event to XState machine | ||
| if (isClaudeMode) { | ||
| // Ensure machine has transitioned past idle before sending CLAUDE_ACTIVE | ||
| const actor = getOrCreateTerminalActor(id); | ||
| if (String(actor.getSnapshot().value) === 'idle') { | ||
| sendTerminalMachineEvent(id, { type: 'SHELL_READY' }); | ||
| } | ||
| sendTerminalMachineEvent(id, { type: 'CLAUDE_ACTIVE' }); | ||
| } else { | ||
| sendTerminalMachineEvent(id, { type: 'CLAUDE_EXITED' }); | ||
| } |
There was a problem hiding this comment.
CLAUDE_ACTIVE without a sessionId can erase machine context.
setClaudeMode(true) sends CLAUDE_ACTIVE without a sessionId, which assigns claudeSessionId: undefined in the machine. If the sessionId was already captured, this clears it and can break swap/resume guards. Consider passing the existing sessionId.
🐛 Suggested fix
if (isClaudeMode) {
// Ensure machine has transitioned past idle before sending CLAUDE_ACTIVE
const actor = getOrCreateTerminalActor(id);
if (String(actor.getSnapshot().value) === 'idle') {
sendTerminalMachineEvent(id, { type: 'SHELL_READY' });
}
- sendTerminalMachineEvent(id, { type: 'CLAUDE_ACTIVE' });
+ const existingSessionId = get().terminals.find(t => t.id === id)?.claudeSessionId;
+ sendTerminalMachineEvent(id, { type: 'CLAUDE_ACTIVE', claudeSessionId: existingSessionId });
} else {
sendTerminalMachineEvent(id, { type: 'CLAUDE_EXITED' });
}Also applies to: 423-426
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts` around lines 394 - 405,
setClaudeMode currently sends a CLAUDE_ACTIVE event with no sessionId which can
overwrite an existing claudeSessionId in the machine; fix by reading the actor
snapshot's context (via
getOrCreateTerminalActor(id).getSnapshot().context.claudeSessionId) and include
that sessionId in the payload when calling sendTerminalMachineEvent(id, { type:
'CLAUDE_ACTIVE', sessionId: existingId }) only if it exists, otherwise omit the
field; apply the same change to the other CLAUDE_ACTIVE send-site referenced in
the review so neither call clears an existing sessionId.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 4 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 10 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 6 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 3 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 5 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: TerminalSwapState is defined and read but never populated — dead code
- quality: XState race condition: CLAUDE_ACTIVE silently dropped in pending_resume and swapping states
- quality: Synchronous file I/O in migrateSession() blocks Electron main process
- quality: [FROM COMMENTS] Synchronous file I/O in migrateSession() blocks Electron main process (from CodeRabbit)
Verdict
All 10 previous findings from the last review have been verified as resolved — excellent progress. The fix commit (dea5aeb) successfully addressed the YOLO mode security issue (server-side validation), removed dead code (swapProfileAndResume, deriveTerminalStateFromMachine), added CLAUDE_ACTIVE self-transition in claude_active state, added SHELL_EXITED dispatch, fixed clearAllTerminals cleanup, and aligned TerminalAPI types.
However, 3 MEDIUM severity findings remain confirmed valid after validation:
- TerminalSwapState dead code (NEW-002): terminal.swapState is checked but never populated — TerminalSwapState/TerminalSwapPhase types and the isPostSwap logic in activateDeferredResume are dead code.
- XState race condition in setClaudeMode (NEW-003): setClaudeMode(true) only guards against 'idle' state but CLAUDE_ACTIVE is silently dropped in 'pending_resume' and 'swapping' states. Adding CLAUDE_ACTIVE handling to pending_resume would fix this.
- Synchronous file I/O in migrateSession (CMT-001, from CodeRabbit): mkdirSync, copyFileSync, cpSync block the Electron main process event loop. Should use fs.promises equivalents.
Additionally, CI has 1 check still pending which prevents a READY_TO_MERGE verdict.
3 findings were dismissed as false positives: unused isClaudeMode in the hook (handled correctly via server-side path), extraFlags empty string inconsistency (actually correct for template literal usage), and RESUME_REQUESTED from claude_active (deliberate and tested design).
2 LOW severity findings (migratedSessionFlags cleanup and setPendingClaudeResume desync) are confirmed but non-blocking.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (6 selected of 6 total)
🟡 [NEW-002] [MEDIUM] TerminalSwapState is defined and read but never populated — dead code
📁 apps/frontend/src/main/terminal/terminal-manager.ts:272
terminal.swapState is checked in activateDeferredResume (line 273: isPostSwap = terminal.swapState?.isSwapping === true && terminal.swapState?.sessionMigrated === true) but swapState is never set on any TerminalProcess object anywhere in the codebase. The only assignment is terminal.swapState = undefined (line 280). This means isPostSwap is always false, making the check and conditional block dead code. TerminalSwapState interface and TerminalSwapPhase type in types.ts are also unused.
Suggested fix:
Either remove the swapState check from activateDeferredResume and the TerminalSwapState/TerminalSwapPhase types from types.ts, or implement the code that populates terminal.swapState during profile switching.
🟡 [NEW-003] [MEDIUM] XState race condition: CLAUDE_ACTIVE silently dropped in pending_resume and swapping states
📁 apps/frontend/src/renderer/stores/terminal-store.ts:394
setClaudeMode(true) only guards against the idle state (sending SHELL_READY first) but does not account for pending_resume or swapping states, neither of which handles CLAUDE_ACTIVE. If the data handler detects Claude is active and calls setClaudeMode(true) while the XState machine is in pending_resume (before setPendingClaudeResume(false) sends RESUME_COMPLETE), the CLAUDE_ACTIVE event is silently dropped, creating a desync between Zustand (isClaudeMode=true) and XState (stuck in pending_resume).
Suggested fix:
Add CLAUDE_ACTIVE handling to the pending_resume state in terminal-machine.ts, transitioning to claude_active with setClaudeSessionId action. This would correctly handle the case where Claude becomes active during a pending resume.
🟡 [CMT-001] [MEDIUM] Synchronous file I/O in migrateSession() blocks Electron main process
📁 apps/frontend/src/main/claude-profile/session-utils.ts:121
migrateSession() uses synchronous fs operations (mkdirSync at line 121, copyFileSync at line 127, cpSync with recursive at line 156) on the Electron main process, called from terminal-handlers.ts IPC handler. The cpSync recursively copies the session directory including tool-results/, which could contain many files for long-running sessions. This blocks the event loop and could cause UI freezes. The rest of the codebase follows async patterns (invokeClaudeAsync, resumeClaudeAsync).
Suggested fix:
Convert migrateSession() to use fs.promises (mkdir, copyFile, cp) and await the result in terminal-handlers.ts. This aligns with the async patterns used throughout the codebase.
🔵 [NEW-001] [LOW] migratedSessionFlags Map not cleaned up on terminal destruction or killAll
📁 apps/frontend/src/main/terminal/terminal-manager.ts:31
The migratedSessionFlags Map entries are only deleted when a migrated session is successfully resumed in resumeClaudeAsync. If a terminal is destroyed before the resume happens, or if killAll() is called during app shutdown, stale entries remain. While each entry is small (string key + boolean value), this is a resource cleanup gap.
Suggested fix:
Add this.migratedSessionFlags.clear() to the killAll() method.
🔵 [NEW-004] [LOW] setPendingClaudeResume XState desync when claudeSessionId is missing
📁 apps/frontend/src/renderer/stores/terminal-store.ts:461
setPendingClaudeResume(id, true) only sends RESUME_REQUESTED to the XState machine if terminal.claudeSessionId is truthy. If a restored session had isClaudeMode=true but no captured sessionId, the Zustand store will have pendingClaudeResume=true while the XState machine does not enter pending_resume state. The functional resume flow via IPC still works regardless.
Suggested fix:
Document that this is intentional behavior (XState is for observability, not control flow), or make claudeSessionId optional in RESUME_REQUESTED event type.
🟡 [CMT-001] [MEDIUM] [FROM COMMENTS] Synchronous file I/O in migrateSession() blocks Electron main process (from CodeRabbit)
📁 apps/frontend/src/main/claude-profile/session-utils.ts:121
CodeRabbit correctly identified that migrateSession() uses synchronous fs operations (mkdirSync, copyFileSync, cpSync) on the Electron main process. This blocks the event loop during profile swaps. Should be converted to async fs.promises equivalents.
Suggested fix:
Convert migrateSession() to async using fs.promises and await the result in terminal-handlers.ts.
This review was generated by Auto Claude.
…erminal-session-persisten
| store.setClaudeSessionId(newTerminal.id, sessionId); | ||
| // Set pending resume flag - user can trigger resume from terminal tab | ||
| store.setPendingClaudeResume(newTerminal.id, true); | ||
| // Send a message to the terminal about the session | ||
| window.electronAPI.sendTerminalInput( | ||
|
|
||
| // Auto-resume the Claude session with --continue | ||
| // YOLO mode (dangerouslySkipPermissions) is preserved server-side by the | ||
| // main process during migration (storeMigratedSessionFlag), so resumeClaudeAsync | ||
| // will restore it automatically when migratedSession is true | ||
| window.electronAPI.resumeClaudeInTerminal( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Convert migrateSession() from sync to async fs operations (mkdir, copyFile, cp, unlink from fs/promises) to avoid blocking the Electron main process during session migration - Remove dead TerminalSwapState/TerminalSwapPhase types and swapState field from TerminalProcess (no longer referenced after prior cleanup) - Remove dead swapState logic from activateDeferredResume - Add migratedSessionFlags.clear() to killAll() to prevent memory leaks - Add CLAUDE_ACTIVE handler to pending_resume XState state (fixes race where Claude becomes active before RESUME_COMPLETE fires) - Add test coverage for CLAUDE_ACTIVE in pending_resume state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
apps/frontend/src/main/claude-profile/session-utils.ts (1)
127-142:⚠️ Potential issue | 🟡 Minor
EEXISTbranch is unreachable —copyFileoverwrites by default.
fs.promises.copyFile(src, dest)overwrites the destination by default.EEXISTis only thrown when theCOPYFILE_EXCLflag is passed as the third argument. Without it, lines 136–141 are dead code and the "skip if already exists" behavior described in the comment never triggers — instead the file is silently overwritten.If you intend to skip when the target exists, pass the flag:
🐛 Proposed fix
+import { constants } from 'fs'; // ... try { - await copyFile(sourceFile, targetFile); + await copyFile(sourceFile, targetFile, constants.COPYFILE_EXCL); result.filesCopied++;If overwriting is acceptable, remove the dead
EEXISTbranch to avoid misleading future readers.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts`:
- Around line 29-30: migratedSessionFlags can retain entries if a terminal with
a pending migration flag is destroyed without resuming; update the
Terminal.destroy (or equivalent terminal cleanup) to check for and remove the
session's key from migratedSessionFlags (same Map referenced by
resumeClaudeAsync and killAll) when a terminal is torn down, ensuring any
sessionId associated with this terminal is cleared to avoid stale entries.
In `@apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts`:
- Around line 98-108: Add a test mirroring the existing CLAUDE_EXITED
error-preservation case but starting from the claude_active state: use runEvents
to drive events { type: 'SHELL_READY' }, { type: 'CLAUDE_START', profileId:
'profile-1' }, { type: 'CLAUDE_ACTIVE', claudeSessionId: 'session-1' }, then {
type: 'CLAUDE_EXITED', error: new Error('boom') } and assert snapshot.value ===
'shell_ready' and that snapshot.context.error is the passed error and
snapshot.context.claudeSessionId is cleared and isBusy is false; this will catch
the missing setError in the claude_active → CLAUDE_EXITED transition (reference
runEvents, snapshot, claude_active, CLAUDE_EXITED).
| /** Server-side storage for YOLO mode flags during profile migration (sessionId → flag) */ | ||
| private migratedSessionFlags: Map<string, boolean> = new Map(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: migratedSessionFlags entries may linger if a migrated session is never resumed.
Entries are consumed in resumeClaudeAsync (line 241) and bulk-cleared in killAll (line 119), but if a terminal is destroyed individually before resuming (e.g., user closes it), the stale entry persists until the next killAll. Practically this is a handful of Map entries at most, so low impact, but consider also clearing entries in destroy() if the terminal had a pending flag.
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/terminal-manager.ts` around lines 29 - 30,
migratedSessionFlags can retain entries if a terminal with a pending migration
flag is destroyed without resuming; update the Terminal.destroy (or equivalent
terminal cleanup) to check for and remove the session's key from
migratedSessionFlags (same Map referenced by resumeClaudeAsync and killAll) when
a terminal is torn down, ensuring any sessionId associated with this terminal is
cleared to avoid stale entries.
apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts
Show resolved
Hide resolved
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 4 previous findings addressed
- ❌ Unresolved: 2 previous findings remain
- 🆕 New Issues: 7 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 6 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: RESUME_COMPLETE sent without claudeSessionId wipes machine context
- quality: Missing setError on claude_active → CLAUDE_EXITED drops error info
- quality: [FROM COMMENTS] Missing setError on claude_active → CLAUDE_EXITED (from CodeRabbit)
Verdict
CI Status: ✅ All 20 CI checks passing — no CI blockers.
Good progress: 4 of 6 previous findings fully resolved (dead swapState code removed, CLAUDE_ACTIVE race condition fixed in XState machine, synchronous I/O converted to async). 1 partially resolved (migratedSessionFlags cleared in killAll but not in destroy).
3 MEDIUM severity confirmed findings block merge:
-
NCR-001 (medium):
RESUME_COMPLETEsent withoutclaudeSessionIdinsetPendingClaudeResume(id, false)— the XState machine'ssetClaudeSessionIdaction overwrites the previously-stored session ID withundefined, causing data loss when transitioning toclaude_active. Fix: IncludeclaudeSessionIdin the event. -
CMT-001-new (medium): Missing
setErroraction onclaude_active→CLAUDE_EXITEDtransition (line 83 of terminal-machine.ts).claude_startingcorrectly uses['setError', 'clearSession']butclaude_activeonly uses'clearSession', silently discarding error info when Claude crashes while active. Identified by CodeRabbit and confirmed by code inspection. Fix: AddsetErrorto the actions array. -
PREV-NEW004 (medium, unresolved from previous review):
setPendingClaudeResume(id, true)still guards XState event behindterminal?.claudeSessionIdwhile unconditionally updating Zustand, causing state divergence. Fix: Either skip the Zustand update when XState can't be notified, or send the event regardless.
3 LOW severity findings (non-blocking, address when convenient):
- NCR-003: migratedSessionFlags leak on individual terminal destroy
- NCR-004: Swapping state lacks phase-ordering guards
- CMT-002: EEXIST dead code branch in session-utils.ts
1 finding dismissed as false positive: NCR-002 (setClaudeMode without sessionId is a by-design two-step async pattern).
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (8 selected of 8 total)
🔵 [NEW-004] [LOW] [UNRESOLVED] setPendingClaudeResume XState desync when claudeSessionId is missing
📁 apps/frontend/src/renderer/stores/terminal-store.ts:461
setPendingClaudeResume(id, true) only sends RESUME_REQUESTED to the XState machine if terminal.claudeSessionId is truthy. If a restored session had isClaudeMode=true but no captured sessionId, the Zustand store will have pendingClaudeResume=true while the XState machine does not enter pending_resume state. The functional resume flow via IPC still works regardless.
Resolution note: terminal-store.ts line 465 still guards sendTerminalMachineEvent behind if (terminal?.claudeSessionId). When claudeSessionId is missing, Zustand sets pendingClaudeResume=true (line 476-480) but XState never receives RESUME_REQUESTED, causing state divergence between the two systems.
Suggested fix:
Document that this is intentional behavior (XState is for observability, not control flow), or make claudeSessionId optional in RESUME_REQUESTED event type.
🟡 [NCR-001] [MEDIUM] RESUME_COMPLETE sent without claudeSessionId wipes machine context
📁 apps/frontend/src/renderer/stores/terminal-store.ts:472
setPendingClaudeResume(id, false) sends { type: 'RESUME_COMPLETE' } without claudeSessionId. The machine's setClaudeSessionId action reads event.claudeSessionId (undefined), overwriting the session ID stored during RESUME_REQUESTED. The machine enters claude_active with claudeSessionId=undefined, losing the session. Fix: include terminal's claudeSessionId in the RESUME_COMPLETE event.
Suggested fix:
Include claudeSessionId: const terminal = get().terminals.find(t => t.id === id); sendTerminalMachineEvent(id, { type: 'RESUME_COMPLETE', claudeSessionId: terminal?.claudeSessionId });
🟡 [CMT-001-new] [MEDIUM] Missing setError on claude_active → CLAUDE_EXITED drops error info
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:83
The claude_active state's CLAUDE_EXITED transition uses only 'clearSession' but not 'setError', unlike the identical transition from claude_starting (line 74) which uses ['setError', 'clearSession']. When Claude crashes with an error while active, the error message is silently discarded and the user never sees the crash reason. Identified by CodeRabbit and confirmed by code inspection.
Suggested fix:
Change line 83 from: CLAUDE_EXITED: { target: 'shell_ready', actions: 'clearSession' } to: CLAUDE_EXITED: { target: 'shell_ready', actions: ['setError', 'clearSession'] }
🔵 [NCR-003] [LOW] migratedSessionFlags entries leak on individual terminal destroy
📁 apps/frontend/src/main/terminal/terminal-manager.ts:110
destroy() cleans lastNotifiedRateLimitReset but not migratedSessionFlags. If a terminal is destroyed before resumeClaudeAsync consumes the stored YOLO flag, the entry persists until killAll(). Minor memory leak with small boolean values during rare profile switch operations.
Suggested fix:
Add migratedSessionFlags cleanup in destroy(): capture terminal's sessionId before destruction, then delete from migratedSessionFlags in the cleanup callback.
🔵 [NCR-004] [LOW] Swapping state accepts out-of-order phase events without guards
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:94
The swapping state handles SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, and SWAP_RESUME_COMPLETE without guard conditions checking context.swapPhase. Any event can be sent in any order. While callers currently send in correct order, the machine doesn't enforce the swap phase invariant itself.
Suggested fix:
Add guards like: SWAP_MIGRATED: { guard: ({ context }) => context.swapPhase === 'capturing', actions: 'setSwapPhaseRecreating' }. Or model sub-phases as nested XState states.
🔵 [CMT-002] [LOW] EEXIST error branch is unreachable dead code
📁 apps/frontend/src/main/claude-profile/session-utils.ts:136
copyFile() at line 128 is called without COPYFILE_EXCL flag. Node.js copyFile silently overwrites by default, never throwing EEXIST. The catch branch at lines 136-141 handling EEXIST is unreachable dead code. If intent was to skip existing files, COPYFILE_EXCL must be passed.
Suggested fix:
Either remove the EEXIST branch, or pass fs.constants.COPYFILE_EXCL flag to copyFile if skip-on-exist behavior is intended.
🟡 [CMT-001-new] [MEDIUM] [FROM COMMENTS] Missing setError on claude_active → CLAUDE_EXITED (from CodeRabbit)
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:83
CodeRabbit correctly identified that the claude_active → CLAUDE_EXITED transition at line 83 is missing the setError action that claude_starting has at line 74. Error information from Claude crashes while active is silently discarded. The existing test only covers claude_starting. A test for claude_active error preservation would fail, confirming the bug.
Suggested fix:
Add setError to the transition actions and add test: it('should set error on CLAUDE_EXITED with error from claude_active', () => { ... expect(snapshot.context.error).toBe('crash'); });
🔵 [CMT-002] [LOW] [FROM COMMENTS] EEXIST error branch is unreachable dead code (from CodeRabbit)
📁 apps/frontend/src/main/claude-profile/session-utils.ts:136
CodeRabbit correctly identified that copyFile() without COPYFILE_EXCL flag never throws EEXIST, making lines 136-141 dead code.
Suggested fix:
Remove the EEXIST branch or add COPYFILE_EXCL flag.
This review was generated by Auto Claude.
- Add setError action to claude_active CLAUDE_EXITED transition to preserve error messages - Fix missing await on resumeClaudeInTerminal call with proper error handling fallback - Improve TypeScript type safety in test helper using Partial<TerminalContext> - Add test coverage for CLAUDE_EXITED error preservation from claude_active state All tests pass (52/52). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| await window.electronAPI.resumeClaudeInTerminal( | ||
| newTerminal.id, | ||
| sessionId, | ||
| { migratedSession: true } | ||
| ); | ||
| debugLog('[useTerminalProfileChange] Resume completed for terminal:', newTerminal.id); | ||
| } catch (error) { | ||
| // If auto-resume fails, set pendingClaudeResume flag | ||
| // so the user can manually resume when they view the terminal | ||
| debugError('[useTerminalProfileChange] Resume failed, marking as pending:', error); | ||
| store.setPendingClaudeResume(newTerminal.id, true); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 5 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 5 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 4 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 8 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: [UNRESOLVED] RESUME_COMPLETE sent without claudeSessionId wipes machine context
- quality: setClaudeMode dispatches CLAUDE_ACTIVE without claudeSessionId, wiping session ID
- quality: await on fire-and-forget IPC call makes error handling dead code
- quality: [FROM COMMENTS] try/catch around fire-and-forget IPC call is ineffective (Sentry Bot + CodeRabbit)
Verdict
CI Status: ✅ All 20 CI checks passing — no CI blockers.
Good progress: The commit 5f66bde successfully resolved the CMT-001-new finding (missing setError on claude_active → CLAUDE_EXITED), with both the fix and a corresponding test added. The test helper was also updated to use Partial<TerminalContext> as suggested.
Blocking issues (3 MEDIUM confirmed valid):
-
NEW-008: setClaudeMode wipes claudeSessionId — This is the most impactful finding. In the common code path (
useTerminalEvents.ts),setClaudeSessionId(id, sessionId)correctly sets the session ID in XState, but the immediately followingsetClaudeMode(id, true)dispatchesCLAUDE_ACTIVEwithoutclaudeSessionId, triggering thesetClaudeSessionIdaction which overwrites the session ID toundefined. This breaks thehasActiveSessionguard for subsequent profile swaps. -
NCR-001: RESUME_COMPLETE wipes claudeSessionId —
setPendingClaudeResume(id, false)sends{ type: 'RESUME_COMPLETE' }withoutclaudeSessionId. The machine'ssetClaudeSessionIdaction readsevent.claudeSessionId(undefined), overwriting any tracked session. This breaks the samehasActiveSessionguard. -
NEW-002: Dead error handling on fire-and-forget IPC —
resumeClaudeInTerminalusesipcRenderer.send()(returns void), soawaitresolves immediately and thecatchblock withsetPendingClaudeResumefallback is unreachable for IPC errors. Flagged independently by both Sentry Bot and CodeRabbit.
Dismissed 4 findings as false positives (NCR-004 swap guards, NEW-001 Map TTL, NEW-004 guard mismatch, NEW-006 truthy-only storage) — verified these are not real issues.
5 LOW findings are real but non-blocking polish items that can be addressed later.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (9 selected of 9 total)
🔵 [NEW-004] [LOW] [UNRESOLVED] [UNRESOLVED] setPendingClaudeResume XState desync when claudeSessionId is missing
📁 apps/frontend/src/renderer/stores/terminal-store.ts:461
setPendingClaudeResume(id, true) only sends RESUME_REQUESTED to the XState machine if terminal.claudeSessionId is truthy. If a restored session had isClaudeMode=true but no captured sessionId, the Zustand store will have pendingClaudeResume=true while the XState machine does not enter pending_resume state. The functional resume flow via IPC still works regardless.
Resolution note: terminal-store.ts line 465 still guards sendTerminalMachineEvent behind if (terminal?.claudeSessionId). When claudeSessionId is missing, Zustand sets pendingClaudeResume=true (line 476-480) but XState never receives RESUME_REQUESTED, causing state divergence between the two systems.
Resolution note: setPendingClaudeResume at terminal-store.ts:461-481: when pending=true and terminal.claudeSessionId is falsy, the guard at line 465 prevents sending RESUME_REQUESTED to XState, but Zustand set() at line 476-480 unconditionally sets pendingClaudeResume to true. XState and Zustand desync.
Suggested fix:
Document that this is intentional behavior (XState is for observability, not control flow), or make claudeSessionId optional in RESUME_REQUESTED event type.
🟡 [NCR-001] [MEDIUM] [UNRESOLVED] RESUME_COMPLETE sent without claudeSessionId wipes machine context
📁 apps/frontend/src/renderer/stores/terminal-store.ts:472
setPendingClaudeResume(id, false) sends { type: 'RESUME_COMPLETE' } without claudeSessionId. The machine's setClaudeSessionId action reads event.claudeSessionId (undefined), overwriting the session ID stored during RESUME_REQUESTED. The machine enters claude_active with claudeSessionId=undefined, losing the session. Fix: include terminal's claudeSessionId in the RESUME_COMPLETE event.
Resolution note: terminal-store.ts:472 sends { type: 'RESUME_COMPLETE' } without claudeSessionId. Machine's setClaudeSessionId action (terminal-machine.ts:147) reads event.claudeSessionId which is undefined, overwriting any existing claudeSessionId in context.
Suggested fix:
Include claudeSessionId: const terminal = get().terminals.find(t => t.id === id); sendTerminalMachineEvent(id, { type: 'RESUME_COMPLETE', claudeSessionId: terminal?.claudeSessionId });
🔵 [NCR-003] [LOW] [UNRESOLVED] migratedSessionFlags entries leak on individual terminal destroy
📁 apps/frontend/src/main/terminal/terminal-manager.ts:110
destroy() cleans lastNotifiedRateLimitReset but not migratedSessionFlags. If a terminal is destroyed before resumeClaudeAsync consumes the stored YOLO flag, the entry persists until killAll(). Minor memory leak with small boolean values during rare profile switch operations.
Resolution note: terminal-manager.ts:105-113 destroy() cleanup callback only deletes lastNotifiedRateLimitReset, not migratedSessionFlags. killAll() at line 119 does clear migratedSessionFlags. Entries keyed by sessionId persist if terminal destroyed before resumeClaudeAsync consumes them.
Suggested fix:
Add migratedSessionFlags cleanup in destroy(): capture terminal's sessionId before destruction, then delete from migratedSessionFlags in the cleanup callback.
🔵 [CMT-002] [LOW] [UNRESOLVED] EEXIST error branch is unreachable dead code
📁 apps/frontend/src/main/claude-profile/session-utils.ts:136
copyFile() at line 128 is called without COPYFILE_EXCL flag. Node.js copyFile silently overwrites by default, never throwing EEXIST. The catch branch at lines 136-141 handling EEXIST is unreachable dead code. If intent was to skip existing files, COPYFILE_EXCL must be passed.
Resolution note: session-utils.ts:128: await copyFile(sourceFile, targetFile) called without COPYFILE_EXCL flag. EEXIST catch branch at lines 136-141 is unreachable dead code since copyFile silently overwrites.
Suggested fix:
Either remove the EEXIST branch, or pass fs.constants.COPYFILE_EXCL flag to copyFile if skip-on-exist behavior is intended.
🟡 [NEW-008] [MEDIUM] setClaudeMode dispatches CLAUDE_ACTIVE without claudeSessionId, wiping session ID
📁 apps/frontend/src/renderer/stores/terminal-store.ts:402
setClaudeMode(id, true) dispatches { type: 'CLAUDE_ACTIVE' } without claudeSessionId. In the common useTerminalEvents flow, setClaudeSessionId correctly sets the session ID in XState, but the immediately following setClaudeMode dispatches CLAUDE_ACTIVE without claudeSessionId. The self-transition at line 81 runs setClaudeSessionId action which reads event.claudeSessionId (undefined), overwriting the session ID to undefined. This breaks the hasActiveSession guard for subsequent profile swaps.
Suggested fix:
Include terminal's claudeSessionId when dispatching CLAUDE_ACTIVE from setClaudeMode:
const terminal = get().terminals.find(t => t.id === id);
sendTerminalMachineEvent(id, { type: 'CLAUDE_ACTIVE', claudeSessionId: terminal?.claudeSessionId });
🟡 [NEW-002] [MEDIUM] await on fire-and-forget IPC call makes error handling dead code
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:115
resumeClaudeInTerminal uses ipcRenderer.send() (fire-and-forget, returns void). The await on void resolves immediately. The try/catch block at lines 110-126 can never catch main-process errors. The setPendingClaudeResume fallback at line 125 is unreachable for IPC errors. Flagged independently by both Sentry Bot and CodeRabbit.
Suggested fix:
Either convert resumeClaudeInTerminal to use ipcRenderer.invoke() for bidirectional error propagation, or remove the try/catch and add a comment documenting the fire-and-forget nature.
🔵 [NEW-005] [LOW] YOLO mode flag uses empty string vs undefined inconsistently
📁 apps/frontend/src/main/terminal/claude-integration-handler.ts:1433
resumeClaudeAsync uses empty string for no-flag case while invokeClaudeAsync and invokeClaude use undefined. Both produce correct commands but the inconsistency could cause confusion in maintenance.
Suggested fix:
Change to: const extraFlags = terminal.dangerouslySkipPermissions ? YOLO_MODE_FLAG : undefined; and adjust template literal accordingly.
🔵 [NEW-007] [LOW] dangerouslySkipPermissions sent to renderer but never consumed
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:301
dangerouslySkipPermissions is included in the TERMINAL_PROFILE_CHANGED IPC payload but the renderer in useTerminalProfileChange.ts never reads it. The code comment explicitly states YOLO mode is preserved server-side via storeMigratedSessionFlag. This is dead data in the IPC payload.
Suggested fix:
Remove dangerouslySkipPermissions from the terminalsNeedingRefresh payload and from the TerminalProfileChangedEvent type.
🟡 [CMT-001] [MEDIUM] [FROM COMMENTS] try/catch around fire-and-forget IPC call is ineffective (Sentry Bot + CodeRabbit)
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:110
resumeClaudeInTerminal uses ipcRenderer.send() which returns void. The await resolves immediately and the catch block with setPendingClaudeResume fallback is unreachable for IPC errors. Flagged independently by both Sentry Bot and CodeRabbit, increasing confidence. Merged with NEW-002.
Suggested fix:
Switch to ipcRenderer.invoke() for error propagation, or document the fire-and-forget limitation.
This review was generated by Auto Claude.
…plement-account-aware-terminal-session-persisten
…PC clarity
Address three blocking review findings:
1. NEW-008: setClaudeMode now preserves claudeSessionId when dispatching CLAUDE_ACTIVE
- Include terminal's current claudeSessionId in the event to prevent XState
setClaudeSessionId action from overwriting it to undefined
- Fixes hasActiveSession guard for profile swaps
2. NCR-001: setPendingClaudeResume preserves claudeSessionId in RESUME_COMPLETE event
- Include terminal's claudeSessionId when sending RESUME_COMPLETE to XState
- Prevents machine from losing session ID on resume completion
3. NEW-002: Remove ineffective try/catch around fire-and-forget IPC call
- resumeClaudeInTerminal uses ipcRenderer.send() which returns void immediately
- Removed try/catch and await that could never catch main process errors
- Added comment documenting fire-and-forget nature and error handling via events
All tests pass (52/52).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
apps/frontend/src/renderer/stores/terminal-store.ts (1)
379-461: 🧹 Nitpick | 🔵 TrivialDual state: Zustand
Terminalfields and XState machine context track overlapping concerns.
setTerminalStatus,setClaudeMode,setClaudeSessionId,setClaudeBusy, andsetPendingClaudeResumeall update both the ZustandTerminalobject and the XState machine. This creates two sources of truth forisClaudeMode,claudeSessionId,isBusy, andpendingClaudeResume. If any code path updates one without the other, they diverge silently.If this is an intentional incremental migration (XState added first, UI reads switched later), consider adding a code comment documenting the plan and a TODO for removing the Zustand-side duplicates once the UI reads from machine snapshots.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts`:
- Around line 482-507: Add assertions in the RESET-from-all-states test to
verify resetContext clears swap-specific fields: after running runEvents([{
type: 'RESET' }], state, ...), assert snapshot.context.swapTargetProfileId is
undefined and snapshot.context.swapPhase is undefined (in addition to existing
checks for claudeSessionId, profileId, isBusy, error) to match resetContext
behavior.
In `@apps/frontend/src/shared/state-machines/terminal-machine.ts`:
- Around line 94-115: The swapping state transitions accept swap events
unconditionally; add guards (conds) that check context.swapPhase before allowing
each event to run so events only advance when in the expected phase. For
example, require swapPhase === 'capturing' for SWAP_SESSION_CAPTURED, swapPhase
=== 'migrating' for SWAP_MIGRATED, swapPhase === 'recreating' for
SWAP_TERMINAL_RECREATED, and swapPhase === 'resuming' for SWAP_RESUME_COMPLETE;
keep the existing actions (setCapturedSession, setSwapPhaseMigrating,
setSwapPhaseRecreating, setSwapPhaseResuming, applySwapComplete) but attach the
appropriate cond to each event, and leave SWAP_FAILED/SHELL_EXITED/RESET
unchanged (or ensure they still clearSwapState) so the machine self-protects
against out-of-order progression.
| describe('RESET from all states', () => { | ||
| const states = [ | ||
| 'idle', | ||
| 'shell_ready', | ||
| 'claude_starting', | ||
| 'claude_active', | ||
| 'swapping', | ||
| 'pending_resume', | ||
| 'exited', | ||
| ]; | ||
|
|
||
| for (const state of states) { | ||
| it(`should reset to idle from ${state}`, () => { | ||
| const snapshot = runEvents( | ||
| [{ type: 'RESET' }], | ||
| state, | ||
| { claudeSessionId: 'session-1', profileId: 'profile-1', isBusy: true, error: 'err' } | ||
| ); | ||
| expect(snapshot.value).toBe('idle'); | ||
| expect(snapshot.context.claudeSessionId).toBeUndefined(); | ||
| expect(snapshot.context.profileId).toBeUndefined(); | ||
| expect(snapshot.context.isBusy).toBe(false); | ||
| expect(snapshot.context.error).toBeUndefined(); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Comprehensive RESET matrix is excellent for regression safety.
Iterating all seven states ensures RESET always returns to idle with clean context. Consider also asserting swapTargetProfileId and swapPhase are cleared, since resetContext explicitly sets them to undefined.
♻️ Suggested addition
expect(snapshot.context.profileId).toBeUndefined();
expect(snapshot.context.isBusy).toBe(false);
expect(snapshot.context.error).toBeUndefined();
+ expect(snapshot.context.swapTargetProfileId).toBeUndefined();
+ expect(snapshot.context.swapPhase).toBeUndefined();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('RESET from all states', () => { | |
| const states = [ | |
| 'idle', | |
| 'shell_ready', | |
| 'claude_starting', | |
| 'claude_active', | |
| 'swapping', | |
| 'pending_resume', | |
| 'exited', | |
| ]; | |
| for (const state of states) { | |
| it(`should reset to idle from ${state}`, () => { | |
| const snapshot = runEvents( | |
| [{ type: 'RESET' }], | |
| state, | |
| { claudeSessionId: 'session-1', profileId: 'profile-1', isBusy: true, error: 'err' } | |
| ); | |
| expect(snapshot.value).toBe('idle'); | |
| expect(snapshot.context.claudeSessionId).toBeUndefined(); | |
| expect(snapshot.context.profileId).toBeUndefined(); | |
| expect(snapshot.context.isBusy).toBe(false); | |
| expect(snapshot.context.error).toBeUndefined(); | |
| }); | |
| } | |
| }); | |
| describe('RESET from all states', () => { | |
| const states = [ | |
| 'idle', | |
| 'shell_ready', | |
| 'claude_starting', | |
| 'claude_active', | |
| 'swapping', | |
| 'pending_resume', | |
| 'exited', | |
| ]; | |
| for (const state of states) { | |
| it(`should reset to idle from ${state}`, () => { | |
| const snapshot = runEvents( | |
| [{ type: 'RESET' }], | |
| state, | |
| { claudeSessionId: 'session-1', profileId: 'profile-1', isBusy: true, error: 'err' } | |
| ); | |
| expect(snapshot.value).toBe('idle'); | |
| expect(snapshot.context.claudeSessionId).toBeUndefined(); | |
| expect(snapshot.context.profileId).toBeUndefined(); | |
| expect(snapshot.context.isBusy).toBe(false); | |
| expect(snapshot.context.error).toBeUndefined(); | |
| expect(snapshot.context.swapTargetProfileId).toBeUndefined(); | |
| expect(snapshot.context.swapPhase).toBeUndefined(); | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts`
around lines 482 - 507, Add assertions in the RESET-from-all-states test to
verify resetContext clears swap-specific fields: after running runEvents([{
type: 'RESET' }], state, ...), assert snapshot.context.swapTargetProfileId is
undefined and snapshot.context.swapPhase is undefined (in addition to existing
checks for claudeSessionId, profileId, isBusy, error) to match resetContext
behavior.
| swapping: { | ||
| on: { | ||
| SWAP_SESSION_CAPTURED: { | ||
| actions: ['setCapturedSession', 'setSwapPhaseMigrating'], | ||
| }, | ||
| SWAP_MIGRATED: { | ||
| actions: 'setSwapPhaseRecreating', | ||
| }, | ||
| SWAP_TERMINAL_RECREATED: { | ||
| actions: 'setSwapPhaseResuming', | ||
| }, | ||
| SWAP_RESUME_COMPLETE: { | ||
| target: 'claude_active', | ||
| actions: 'applySwapComplete', | ||
| }, | ||
| SWAP_FAILED: { | ||
| target: 'shell_ready', | ||
| actions: ['setError', 'clearSwapState'], | ||
| }, | ||
| SHELL_EXITED: { target: 'exited', actions: ['clearSession', 'clearSwapState'] }, | ||
| RESET: { target: 'idle', actions: 'resetContext' }, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Swap phase events are not guarded by current swapPhase, allowing out-of-order progression.
The swapping state accepts SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, and SWAP_RESUME_COMPLETE regardless of the current swapPhase. For instance, receiving SWAP_MIGRATED while still in the capturing phase would silently advance to recreating without having captured the session. If the orchestrating code guarantees ordering this is fine, but adding guards would make the machine self-protecting.
♻️ Example: guard swap events by current phase
swapping: {
on: {
SWAP_SESSION_CAPTURED: {
+ guard: ({ context }) => context.swapPhase === 'capturing',
actions: ['setCapturedSession', 'setSwapPhaseMigrating'],
},
SWAP_MIGRATED: {
+ guard: ({ context }) => context.swapPhase === 'migrating',
actions: 'setSwapPhaseRecreating',
},
SWAP_TERMINAL_RECREATED: {
+ guard: ({ context }) => context.swapPhase === 'recreating',
actions: 'setSwapPhaseResuming',
},
SWAP_RESUME_COMPLETE: {
+ guard: ({ context }) => context.swapPhase === 'resuming',
target: 'claude_active',
actions: 'applySwapComplete',
},🤖 Prompt for AI Agents
In `@apps/frontend/src/shared/state-machines/terminal-machine.ts` around lines 94
- 115, The swapping state transitions accept swap events unconditionally; add
guards (conds) that check context.swapPhase before allowing each event to run so
events only advance when in the expected phase. For example, require swapPhase
=== 'capturing' for SWAP_SESSION_CAPTURED, swapPhase === 'migrating' for
SWAP_MIGRATED, swapPhase === 'recreating' for SWAP_TERMINAL_RECREATED, and
swapPhase === 'resuming' for SWAP_RESUME_COMPLETE; keep the existing actions
(setCapturedSession, setSwapPhaseMigrating, setSwapPhaseRecreating,
setSwapPhaseResuming, applySwapComplete) but attach the appropriate cond to each
event, and leave SWAP_FAILED/SHELL_EXITED/RESET unchanged (or ensure they still
clearSwapState) so the machine self-protects against out-of-order progression.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 4 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 5 previous findings addressed
- ❌ Unresolved: 4 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 8 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: No user notification when Claude session migration fails during profile switch
- quality: Dual Zustand/XState state updates lack architectural documentation
Verdict
CI: All 20 checks passing. 4 previous findings resolved (NCR-001, NEW-008, NEW-002, CMT-001), 2 dismissed as false positives (NEW-005, NCR-NEW-005). Good progress on session ID preservation and IPC clarity.
However, 3 MEDIUM-severity confirmed-valid findings remain: (1) NEW-004: XState desync in setPendingClaudeResume when claudeSessionId is missing - Zustand and XState can diverge; (2) NCR-NEW-004: No user notification when Claude session migration fails during profile switch - users silently lose their session; (3) CMT-NEW-003: Dual Zustand/XState state updates across 5 store methods lack architectural documentation explaining synchronization contract and migration plan. Additionally, 5 LOW-severity confirmed valid findings exist (NCR-003 migratedSessionFlags leak, CMT-002 EEXIST dead code, NEW-007 unused payload field, CMT-NEW-001 incomplete RESET tests, CMT-NEW-002 unguarded swap phases).
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (8 selected of 8 total)
🔵 [NEW-004] [LOW] [UNRESOLVED] [UNRESOLVED] [UNRESOLVED] setPendingClaudeResume XState desync when claudeSessionId is missing
📁 apps/frontend/src/renderer/stores/terminal-store.ts:461
setPendingClaudeResume(id, true) only sends RESUME_REQUESTED to the XState machine if terminal.claudeSessionId is truthy. If a restored session had isClaudeMode=true but no captured sessionId, the Zustand store will have pendingClaudeResume=true while the XState machine does not enter pending_resume state. The functional resume flow via IPC still works regardless.
Resolution note: terminal-store.ts line 465 still guards sendTerminalMachineEvent behind if (terminal?.claudeSessionId). When claudeSessionId is missing, Zustand sets pendingClaudeResume=true (line 476-480) but XState never receives RESUME_REQUESTED, causing state divergence between the two systems.
Resolution note: setPendingClaudeResume at terminal-store.ts:461-481: when pending=true and terminal.claudeSessionId is falsy, the guard at line 465 prevents sending RESUME_REQUESTED to XState, but Zustand set() at line 476-480 unconditionally sets pendingClaudeResume to true. XState and Zustand desync.
Resolution note: setPendingClaudeResume: if (pending) { const terminal = get().terminals.find(t => t.id === id); if (terminal?.claudeSessionId) { sendTerminalMachineEvent(id, { type: 'RESUME_REQUESTED', claudeSessionId: terminal.claudeSessionId }); } } — guard on claudeSessionId still exists, no fallback path when missing
Suggested fix:
Document that this is intentional behavior (XState is for observability, not control flow), or make claudeSessionId optional in RESUME_REQUESTED event type.
🔵 [NCR-003] [LOW] [UNRESOLVED] [UNRESOLVED] migratedSessionFlags entries leak on individual terminal destroy
📁 apps/frontend/src/main/terminal/terminal-manager.ts:110
destroy() cleans lastNotifiedRateLimitReset but not migratedSessionFlags. If a terminal is destroyed before resumeClaudeAsync consumes the stored YOLO flag, the entry persists until killAll(). Minor memory leak with small boolean values during rare profile switch operations.
Resolution note: terminal-manager.ts:105-113 destroy() cleanup callback only deletes lastNotifiedRateLimitReset, not migratedSessionFlags. killAll() at line 119 does clear migratedSessionFlags. Entries keyed by sessionId persist if terminal destroyed before resumeClaudeAsync consumes them.
Resolution note: destroy() method: this.lastNotifiedRateLimitReset.delete(terminalId); — migratedSessionFlags not cleaned up. killAll() clears it, resumeClaudeAsync deletes on consumption, but destroy() path leaks entries.
Suggested fix:
Add migratedSessionFlags cleanup in destroy(): capture terminal's sessionId before destruction, then delete from migratedSessionFlags in the cleanup callback.
🔵 [CMT-002] [LOW] [UNRESOLVED] [UNRESOLVED] EEXIST error branch is unreachable dead code
📁 apps/frontend/src/main/claude-profile/session-utils.ts:136
copyFile() at line 128 is called without COPYFILE_EXCL flag. Node.js copyFile silently overwrites by default, never throwing EEXIST. The catch branch at lines 136-141 handling EEXIST is unreachable dead code. If intent was to skip existing files, COPYFILE_EXCL must be passed.
Resolution note: session-utils.ts:128: await copyFile(sourceFile, targetFile) called without COPYFILE_EXCL flag. EEXIST catch branch at lines 136-141 is unreachable dead code since copyFile silently overwrites.
Resolution note: await copyFile(sourceFile, targetFile); — no COPYFILE_EXCL flag passed. The EEXIST catch branch at lines 136-141 remains unreachable dead code.
Suggested fix:
Either remove the EEXIST branch, or pass fs.constants.COPYFILE_EXCL flag to copyFile if skip-on-exist behavior is intended.
🟡 [NCR-NEW-004] [MEDIUM] No user notification when Claude session migration fails during profile switch
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:105
When a terminal had an active Claude session (sessionId truthy) but migration failed (sessionMigrated false), the terminal is recreated as a plain shell without any user notification. The isClaudeMode field was added to the TerminalProfileChangedEvent IPC payload but is never consumed by the renderer. Users silently lose their Claude session on failed migration. An else branch should handle this case with a toast or status message.
Suggested fix:
Add an else branch: if (terminalInfo.isClaudeMode && sessionId && !sessionMigrated) { /* show notification or set terminal status */ }
🔵 [CMT-NEW-001] [LOW] RESET test missing swapTargetProfileId and swapPhase assertions
📁 apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts:498
The RESET tests set initial context with claudeSessionId, profileId, isBusy, and error but omit swapTargetProfileId and swapPhase. The resetContext action explicitly clears all 6 fields, but only 4 are tested. This is especially relevant for the 'swapping' state where swap fields would naturally be set.
Suggested fix:
Add swapTargetProfileId: 'profile-2' and swapPhase: 'migrating' to the initial context, plus expect(snapshot.context.swapTargetProfileId).toBeUndefined() and expect(snapshot.context.swapPhase).toBeUndefined() assertions.
🔵 [CMT-NEW-002] [LOW] Swap phase events lack ordering guards
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:94
Within the 'swapping' state, events SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, and SWAP_RESUME_COMPLETE have no guards checking the current swapPhase. Out-of-order events are silently accepted. Ordering is currently guaranteed by the single caller (profile change handler), but guards would make the machine self-documenting and protect against future changes.
Suggested fix:
Add guards to swap phase transitions, e.g., SWAP_MIGRATED: { guard: ({context}) => context.swapPhase === 'migrating', actions: 'setSwapPhaseRecreating' }
🟡 [CMT-NEW-003] [MEDIUM] Dual Zustand/XState state updates lack architectural documentation
📁 apps/frontend/src/renderer/stores/terminal-store.ts:379
Five store methods (setTerminalStatus, setClaudeMode, setClaudeSessionId, setClaudeBusy, setPendingClaudeResume) update both Zustand state and XState machines simultaneously. While per-method inline comments exist, there is no architectural-level documentation explaining why dual updates are needed, which system is the source of truth, or what the migration plan is. The NEW-004 desync bug demonstrates this pattern is already causing subtle issues.
Suggested fix:
Add a top-level comment block or architecture decision record explaining: (1) XState is being introduced alongside Zustand for lifecycle state management, (2) Zustand remains the source of truth for UI rendering, (3) XState enforces valid transitions and enables future features like swap coordination, (4) eventual goal to derive Zustand fields from XState snapshots.
🔵 [CMT-SENTRY] [LOW] [FROM COMMENTS] Fire-and-forget IPC concern (Sentry Bot) - RESOLVED
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:117
Sentry bot flagged that await-ing resumeClaudeInTerminal (fire-and-forget IPC) rendered the try/catch ineffective. This has been fixed: await and try/catch removed, replaced with direct call and comment explaining fire-and-forget nature and onTerminalPendingResume fallback.
This review was generated by Auto Claude.
Three improvements for production code quality: 1. Use path alias for consistency - Change terminal-store.ts line 7 to use @shared/* instead of relative import - Matches project conventions (CLAUDE.md path alias guidelines) 2. Prevent XState/Zustand state divergence in setPendingClaudeResume - Only set pendingClaudeResume flag in Zustand if RESUME_REQUESTED was sent to XState - When claudeSessionId is missing, skip both XState event and Zustand update - Prevents UI showing "resume pending" when state machine doesn't know about it 3. Clean up migratedSessionFlags on individual terminal destroy - Previously only cleared on killAll(), entries could linger if terminal closed before resume - Now removes entry in destroy() if terminal has claudeSessionId - Prevents Map from accumulating stale session flags All tests pass (128/128 files, 3271/3277 tests). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 5 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 5 previous findings remain
- 🆕 New Issues: 6 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 9 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- quality: [UNRESOLVED] No user notification when Claude session migration fails during profile switch
- quality: setClaudeSessionId sends CLAUDE_ACTIVE without idle-state guard
- quality: Fire-and-forget resume has no error recovery; comment about fallback is incorrect
- quality: [FROM COMMENTS] No user notification when Claude session migration fails during profile switch
Verdict
CI: All 20 checks passing. Good progress — 3 previous findings resolved (NEW-004: XState/Zustand desync fix, NCR-003: migratedSessionFlags cleanup in destroy(), CMT-SENTRY: fire-and-forget IPC fix). The author has been actively addressing feedback with dedicated fix commits.
However, 3 MEDIUM-severity confirmed-valid findings block merge:
- NCR-NEW-004: No user notification when Claude session migration fails during profile switch — user's Claude session silently lost
- NEW-001: setClaudeSessionId sends CLAUDE_ACTIVE without idle-state guard (inconsistent with setClaudeMode which has the guard) — causes XState/Zustand desync
- NEW-003: Fire-and-forget resume has no error recovery, and the comment claiming onTerminalPendingResume will fire on failure is incorrect — no such fallback exists in the error path
Additionally, 5 LOW-severity findings remain unresolved (CMT-002: EEXIST dead code, CMT-NEW-001: RESET test gaps, CMT-NEW-002: swap phase guards, CMT-NEW-003: architectural documentation, NEW-004: CLAUDE_EXITED sent to non-accepting states, NEW-006: isBusy cleared on self-transitions). While individually minor, the MEDIUM findings represent real UX and state-consistency risks that should be addressed before merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (10 selected of 10 total)
🔵 [CMT-002] [LOW] [UNRESOLVED] [UNRESOLVED] [UNRESOLVED] EEXIST error branch is unreachable dead code
📁 apps/frontend/src/main/claude-profile/session-utils.ts:136
copyFile() at line 128 is called without COPYFILE_EXCL flag. Node.js copyFile silently overwrites by default, never throwing EEXIST. The catch branch at lines 136-141 handling EEXIST is unreachable dead code. If intent was to skip existing files, COPYFILE_EXCL must be passed.
Resolution note: session-utils.ts:128: await copyFile(sourceFile, targetFile) called without COPYFILE_EXCL flag. EEXIST catch branch at lines 136-141 is unreachable dead code since copyFile silently overwrites.
Resolution note: await copyFile(sourceFile, targetFile); — no COPYFILE_EXCL flag passed. The EEXIST catch branch at lines 136-141 remains unreachable dead code.
Resolution note: await copyFile(sourceFile, targetFile) at line 128 — no COPYFILE_EXCL flag. The EEXIST catch branch at lines 136-141 remains unreachable dead code. Neither was the flag added nor was the dead branch removed.
Suggested fix:
Either remove the EEXIST branch, or pass fs.constants.COPYFILE_EXCL flag to copyFile if skip-on-exist behavior is intended.
🟡 [NCR-NEW-004] [MEDIUM] [UNRESOLVED] No user notification when Claude session migration fails during profile switch
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:105
When a terminal had an active Claude session (sessionId truthy) but migration failed (sessionMigrated false), the terminal is recreated as a plain shell without any user notification. The isClaudeMode field was added to the TerminalProfileChangedEvent IPC payload but is never consumed by the renderer. Users silently lose their Claude session on failed migration. An else branch should handle this case with a toast or status message.
Resolution note: if (sessionId && sessionMigrated) { ... } at line 105 handles success case only. No else branch exists for when sessionMigrated is false. Terminal is recreated as plain shell with no user notification when migration fails.
Suggested fix:
Add an else branch: if (terminalInfo.isClaudeMode && sessionId && !sessionMigrated) { /* show notification or set terminal status */ }
🔵 [CMT-NEW-001] [LOW] [UNRESOLVED] RESET test missing swapTargetProfileId and swapPhase assertions
📁 apps/frontend/src/shared/state-machines/__tests__/terminal-machine.test.ts:498
The RESET tests set initial context with claudeSessionId, profileId, isBusy, and error but omit swapTargetProfileId and swapPhase. The resetContext action explicitly clears all 6 fields, but only 4 are tested. This is especially relevant for the 'swapping' state where swap fields would naturally be set.
Resolution note: RESET tests at line 498 set initial context with { claudeSessionId, profileId, isBusy, error } but omit swapTargetProfileId and swapPhase. Assertions check only 4 of 6 context fields. The runEvents helper defaults omitted fields to undefined, so no meaningful test of clearing swap state.
Suggested fix:
Add swapTargetProfileId: 'profile-2' and swapPhase: 'migrating' to the initial context, plus expect(snapshot.context.swapTargetProfileId).toBeUndefined() and expect(snapshot.context.swapPhase).toBeUndefined() assertions.
🔵 [CMT-NEW-002] [LOW] [UNRESOLVED] Swap phase events lack ordering guards
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:94
Within the 'swapping' state, events SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, and SWAP_RESUME_COMPLETE have no guards checking the current swapPhase. Out-of-order events are silently accepted. Ordering is currently guaranteed by the single caller (profile change handler), but guards would make the machine self-documenting and protect against future changes.
Resolution note: Swapping state (lines 94-116) has no guards on SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, or SWAP_RESUME_COMPLETE. Out-of-order events are silently accepted. Compare with SWAP_INITIATED which does have guard: 'hasActiveSession'.
Suggested fix:
Add guards to swap phase transitions, e.g., SWAP_MIGRATED: { guard: ({context}) => context.swapPhase === 'migrating', actions: 'setSwapPhaseRecreating' }
🟡 [NEW-001] [MEDIUM] setClaudeSessionId sends CLAUDE_ACTIVE without idle-state guard
📁 apps/frontend/src/renderer/stores/terminal-store.ts:425
setClaudeSessionId sends CLAUDE_ACTIVE to XState without checking if the machine is in idle state (where CLAUDE_ACTIVE is rejected). setClaudeMode has a guard for this (checks idle, sends SHELL_READY first) but setClaudeSessionId does not. During profile change flow, setClaudeSessionId is called on a new terminal whose actor starts in idle, causing XState context to lack the session ID while Zustand stores it.
Suggested fix:
Add the same idle-state guard as setClaudeMode: check if actor is in idle state and send SHELL_READY first before CLAUDE_ACTIVE.
🔵 [NEW-002] [LOW] migratedSessionFlags may be prematurely deleted by destroy()
📁 apps/frontend/src/main/terminal/terminal-manager.ts:106
During profile migration, storeMigratedSessionFlag stores a flag with the OLD session ID. Then the OLD terminal's destroy() correctly matches and deletes this flag. By the time resumeClaudeAsync on the new terminal tries to consume the flag, it may already be gone. This could prevent YOLO mode from being restored after profile switches.
Suggested fix:
Needs human review: verify if the IPC timing guarantees destroy completes before resumeClaudeAsync runs, or restructure to avoid the race condition.
🟡 [NEW-003] [MEDIUM] Fire-and-forget resume has no error recovery; comment about fallback is incorrect
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:111
resumeClaudeInTerminal uses fire-and-forget IPC (ipcRenderer.send). The main process handler catches errors but only logs them — no failure event is emitted back to the renderer. The comment claiming 'the main process will emit onTerminalPendingResume if the resume fails' is incorrect. If resume fails, the terminal has its session ID set but Claude isn't running.
Suggested fix:
Either (1) emit a TERMINAL_RESUME_FAILED event from the main process error path, (2) switch to ipcRenderer.invoke for error detection, or (3) correct the comment to accurately describe the failure behavior.
🔵 [NEW-004-STORE] [LOW] setClaudeMode sends CLAUDE_EXITED to non-accepting XState states
📁 apps/frontend/src/renderer/stores/terminal-store.ts:406
setClaudeMode(id, false) sends CLAUDE_EXITED unconditionally, but XState only accepts it from claude_starting and claude_active. If machine is in pending_resume or swapping, the event is dropped while Zustand always updates, creating a potential desync.
Suggested fix:
Add a state check before sending CLAUDE_EXITED, similar to how setPendingClaudeResume checks machine state.
🔵 [NEW-006] [LOW] setClaudeSessionId action clears isBusy on self-transitions
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:144
The setClaudeSessionId XState action unconditionally sets isBusy: false on every invocation, including CLAUDE_ACTIVE self-transitions in claude_active state. A late session ID arriving while Claude is busy would incorrectly clear the busy indicator.
Suggested fix:
Preserve isBusy during self-transitions: isBusy: ({ context, event }) => event.type === 'CLAUDE_ACTIVE' ? context.isBusy : false
🟡 [CMT-RPT-001] [MEDIUM] [FROM COMMENTS] No user notification when Claude session migration fails during profile switch
📁 apps/frontend/src/renderer/hooks/useTerminalProfileChange.ts:104
When sessionId is truthy but sessionMigrated is false, the terminal is silently recreated as a plain shell. No toast, message, or feedback indicates the user's active Claude session was lost. This was previously flagged (NCR-NEW-004) and remains unaddressed.
Suggested fix:
Add an else branch with a user notification (toast or terminal message) when sessionId exists but sessionMigrated is false.
This review was generated by Auto Claude.
This commit addresses all 10 findings from the latest Auto Claude PR review: **BLOCKING FIXES (3 MEDIUM severity):** 1. NCR-NEW-004: Add user notification when session migration fails - Added i18n key 'terminal:swap.migrationFailed' (EN + FR) - Hook now shows toast notification when isClaudeMode && sessionId && !sessionMigrated - Users are now informed when their Claude session is lost during profile switch 2. NEW-001: Add idle-state guard to setClaudeSessionId - setClaudeSessionId now checks if XState machine is in 'idle' state - Sends SHELL_READY first before CLAUDE_ACTIVE, matching setClaudeMode pattern - Prevents XState/Zustand desync during profile change flow 3. NEW-003: Correct fire-and-forget IPC comment - Removed incorrect claim about onTerminalPendingResume fallback - Updated comment to accurately describe actual failure behavior - Documents that errors are logged but no event is emitted back to renderer **LOW SEVERITY FIXES (5):** 4. CMT-002: Remove EEXIST dead code - copyFile() without COPYFILE_EXCL never throws EEXIST - Removed unreachable catch branch at lines 136-141 - Added comment documenting silent overwrite behavior 5. NEW-004-STORE: Add state check for CLAUDE_EXITED event - setClaudeMode now checks machine state before sending CLAUDE_EXITED - Only sends event if machine is in 'claude_starting' or 'claude_active' - Prevents XState/Zustand desync when event is dropped 6. NEW-006: Preserve isBusy during CLAUDE_ACTIVE self-transitions - setClaudeSessionId action now checks if it's a self-transition - Preserves existing isBusy state during self-transitions - Prevents late session ID updates from incorrectly clearing busy indicator 7. CMT-NEW-001: Add swap state assertions to RESET tests - Updated RESET test to include swapTargetProfileId and swapPhase - Tests now verify all 6 context fields are cleared (not just 4) - Comprehensive test coverage for resetContext action **REMAINING LOW SEVERITY (acknowledged but not blocking):** 8. CMT-NEW-002: Swap phase events lack ordering guards - Acknowledged: ordering currently guaranteed by single caller - Guards would be defensive but not essential for current implementation - Can be addressed in future refactoring if needed 9. CMT-NEW-003: Dual Zustand/XState state updates lack architectural docs - Acknowledged: inline comments exist per method - Architectural-level documentation could be added separately - Does not block merge as per-method comments are clear All tests passing (3271 passed, 6 skipped). TypeScript compiles cleanly. Biome linting clean for all changed files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| // Clean up migrated session flags if this terminal has a pending migrated session | ||
| const terminal = this.terminals.get(id); | ||
| if (terminal?.claudeSessionId) { | ||
| this.migratedSessionFlags.delete(terminal.claudeSessionId); | ||
| } |
There was a problem hiding this comment.
Bug: During a profile swap, destroyTerminal() prematurely deletes the dangerouslySkipPermissions flag from migratedSessionFlags before the new terminal can use it, causing the flag to be lost.
Severity: HIGH
Suggested Fix
The flag should not be deleted from migratedSessionFlags within the terminalManager.destroy method. The cleanup of this flag should be deferred until after the new terminal has successfully resumed the session and consumed the flag, or it should be removed from the destroy logic altogether if another mechanism handles its lifecycle.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/frontend/src/main/terminal/terminal-manager.ts#L106-L110
Potential issue: During a profile swap, a race condition occurs where the
`dangerouslySkipPermissions` flag (YOLO mode) is prematurely deleted. The
`destroyTerminal` function for the old terminal is called and awaited, which cleans up
the flag from the `migratedSessionFlags` map. Immediately after, a new terminal is
created and attempts to resume the session by looking for the same flag in the map.
Because the flag was already deleted, the new terminal fails to restore the YOLO mode,
silently reverting to standard permission behavior and potentially causing unexpected
permission prompts for the user.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 2 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 8 previous findings addressed
- ❌ Unresolved: 2 previous findings remain
- 🆕 New Issues: 7 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 7 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: Claude exit via updateTerminal bypasses XState machine, causing state divergence
- quality: destroy() prematurely deletes migratedSessionFlags, losing YOLO mode during profile swap
- quality: [FROM COMMENTS] destroy() prematurely deletes migratedSessionFlags entry (confirmed by sentry bot and code analysis)
Verdict
CI: All 20 checks passing. Excellent progress: 8 of 10 previous findings resolved in this round. However, 1 HIGH and 1 MEDIUM confirmed-valid finding block merge:
-
[HIGH] NCR-R7-001: The
onTerminalClaudeExithandler in useTerminalEvents.ts usesstore.updateTerminal()(a plain Zustand setter) instead ofstore.setClaudeMode(id, false). This bypasses XState notification entirely, leaving the machine stuck inclaude_activewhile Zustand showsisClaudeMode: false. This Zustand/XState state divergence could cause incorrect behavior in swap operations and other XState-guarded logic. -
[MEDIUM] NEW-002/CMT-001: The
destroy()method in terminal-manager.ts deletesmigratedSessionFlags[terminal.claudeSessionId]when the OLD terminal is destroyed. SinceuseTerminalProfileChange.tscallsdestroyTerminal(line 64) beforeresumeClaudeInTerminal(line 121), the YOLO mode flag is always deleted before the new terminal can consume it. Fix: remove the cleanup fromdestroy()—resumeClaudeAsyncalready cleans up after consumption, andkillAll()clears the entire map.
Additionally, 2 unresolved LOW findings from previous review remain (swap phase ordering guards, plus the migratedSessionFlags issue above which was re-confirmed), and 4 new LOW findings were identified. The LOW findings are suggestions and do not block merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (9 selected of 9 total)
🔵 [CMT-NEW-002] [LOW] [UNRESOLVED] [UNRESOLVED] Swap phase events lack ordering guards
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:94
Within the 'swapping' state, events SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, and SWAP_RESUME_COMPLETE have no guards checking the current swapPhase. Out-of-order events are silently accepted. Ordering is currently guaranteed by the single caller (profile change handler), but guards would make the machine self-documenting and protect against future changes.
Resolution note: Swapping state (lines 94-116) has no guards on SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, or SWAP_RESUME_COMPLETE. Out-of-order events are silently accepted. Compare with SWAP_INITIATED which does have guard: 'hasActiveSession'.
Resolution note: Lines 94-115 of terminal-machine.ts: swapping state has no guard properties on SWAP_SESSION_CAPTURED, SWAP_MIGRATED, SWAP_TERMINAL_RECREATED, or SWAP_RESUME_COMPLETE. Compare to SWAP_INITIATED (line 86) which has 'guard: hasActiveSession'. Out-of-order events are silently accepted without swapPhase checks.
Suggested fix:
Add guards to swap phase transitions, e.g., SWAP_MIGRATED: { guard: ({context}) => context.swapPhase === 'migrating', actions: 'setSwapPhaseRecreating' }
🔵 [NEW-002] [LOW] [UNRESOLVED] migratedSessionFlags may be prematurely deleted by destroy()
📁 apps/frontend/src/main/terminal/terminal-manager.ts:106
During profile migration, storeMigratedSessionFlag stores a flag with the OLD session ID. Then the OLD terminal's destroy() correctly matches and deletes this flag. By the time resumeClaudeAsync on the new terminal tries to consume the flag, it may already be gone. This could prevent YOLO mode from being restored after profile switches.
Resolution note: terminal-manager.ts destroy() at lines 107-109 deletes migratedSessionFlags[terminal.claudeSessionId]. useTerminalProfileChange.ts calls destroyTerminal (line 64) BEFORE resumeClaudeInTerminal (line 121). The flag is stored with old terminal's claudeSessionId as key, and destroy() deletes it before resumeClaudeAsync can consume it.
Suggested fix:
Needs human review: verify if the IPC timing guarantees destroy completes before resumeClaudeAsync runs, or restructure to avoid the race condition.
🟠 [NCR-R7-001] [HIGH] Claude exit via updateTerminal bypasses XState machine, causing state divergence
📁 apps/frontend/src/renderer/components/terminal/useTerminalEvents.ts:150
The onTerminalClaudeExit handler calls store.updateTerminal() with isClaudeMode: false instead of store.setClaudeMode(id, false). updateTerminal is a plain Zustand setter with NO XState notification, so the XState machine remains in claude_active while Zustand shows isClaudeMode=false. This divergence affects XState-guarded logic like SWAP_INITIATED's hasActiveSession guard.
Suggested fix:
Replace store.updateTerminal() with store.setClaudeMode(terminalId, false) followed by separate updates for other fields (claudeSessionId, isClaudeBusy, status). Or send CLAUDE_EXITED to the XState machine before calling updateTerminal.
🟡 [NEW-002] [MEDIUM] destroy() prematurely deletes migratedSessionFlags, losing YOLO mode during profile swap
📁 apps/frontend/src/main/terminal/terminal-manager.ts:106
During profile swap, storeMigratedSessionFlag stores the YOLO mode flag keyed by sessionId. Then destroy() of the OLD terminal deletes migratedSessionFlags[terminal.claudeSessionId] — the same key. Since useTerminalProfileChange.ts calls destroyTerminal (line 64) before resumeClaudeInTerminal (line 121), the flag is always deleted before the new terminal's resumeClaudeAsync can consume it.
Suggested fix:
Remove lines 107-109 from destroy(). The flag is already cleaned up by resumeClaudeAsync after consumption (line 247), and killAll() clears the entire map on shutdown.
🔵 [NCR-R7-003] [LOW] Deprecated sync resumeClaude does not include YOLO mode flag
📁 apps/frontend/src/main/terminal/claude-integration-handler.ts:1214
The deprecated sync resumeClaude constructs the command as just '--continue' without the YOLO_MODE_FLAG, while the async resumeClaudeAsync includes it. The sync version is still exported but has no active TypeScript callers.
Suggested fix:
Add YOLO mode flag to the sync version for consistency, or remove the sync version entirely if no callers remain.
🔵 [NCR-R7-004] [LOW] isBusy preservation heuristic incorrectly detects self-transitions from pending_resume
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:153
The setClaudeSessionId action uses context.claudeSessionId presence to detect self-transitions. When transitioning from pending_resume (where claudeSessionId is set) to claude_active via CLAUDE_ACTIVE, the heuristic incorrectly preserves stale isBusy instead of clearing it.
Suggested fix:
Consider checking event metadata or machine state instead of context.claudeSessionId to distinguish self-transitions.
🔵 [NCR-R7-005] [LOW] setPendingClaudeResume silently drops request when claudeSessionId is missing
📁 apps/frontend/src/renderer/stores/terminal-store.ts:481
When pending=true and the terminal has no claudeSessionId, shouldUpdateZustand is set to false and neither XState nor Zustand is updated. No warning or log is emitted, making debugging difficult.
Suggested fix:
Add a debugLog call when the request is silently dropped.
🔵 [NCR-R7-007] [LOW] Missing CLAUDE_BUSY handling in claude_starting and pending_resume states
📁 apps/frontend/src/shared/state-machines/terminal-machine.ts:71
CLAUDE_BUSY event is only handled in claude_active state. If Claude becomes busy during claude_starting or pending_resume, the event is silently dropped and the busy indicator won't appear until the next event in claude_active.
Suggested fix:
Add CLAUDE_BUSY: { actions: 'setBusy' } to claude_starting and pending_resume states.
🟡 [CMT-001] [MEDIUM] [FROM COMMENTS] destroy() prematurely deletes migratedSessionFlags entry (confirmed by sentry bot and code analysis)
📁 apps/frontend/src/main/terminal/terminal-manager.ts:109
Sentry bot correctly identified that during profile swap, destroy() deletes the migratedSessionFlags entry before the new terminal can consume it. The flag is stored with the old terminal's claudeSessionId as key. When destroy() runs on the old terminal, it deletes that exact key. By the time resumeClaudeAsync runs on the new terminal, the flag is gone and YOLO mode is silently lost.
Suggested fix:
Remove migratedSessionFlags cleanup from destroy() (lines 107-109). The flag is already cleaned up by resumeClaudeAsync after consumption, and killAll() clears the entire map.
This review was generated by Auto Claude.
1 participant
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Implements account-aware terminal session persistence by fixing the Claude Code session resume flow after profile swaps and introducing an XState state machine to formalize terminal lifecycle tracking. When users swap authentication accounts, the terminal is now automatically recreated with the new account's credentials and the previous Claude Code session is seamlessly resumed under the new account, eliminating the need for manual intervention.
Related Issue
Closes #229
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemAI Disclosure
Tool(s) used: Claude Code
Testing level:
Untested -- AI output not yet verified
Lightly tested -- ran the app / spot-checked key paths
Fully tested -- all tests pass, manually verified behavior
I understand what this PR does and how the underlying code works
Checklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksfindExecutable()or platform abstractions)If you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Screenshots
Not applicable — backend feature implementation.
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details: All changes are backward compatible. The XState machine integrates seamlessly with existing Zustand store, and profile swap/resume flows preserve existing CLI behavior while adding automatic session continuation.
Change Context
Branch Information
Summary of Changes
This PR implements account-aware terminal session persistence through:
terminal-machine.ts): Formalized terminal lifecycle states (idle, running, claude-active, swapping, exited) replacing ad-hoc boolean flagsresumeClaude()flow to acceptmigratedSessionflag, enabling automatic resumption after profile swapsswapProfileAndResume()method to capture session → migrate → recreate → auto-resumeisClaudeMode,pendingClaudeResume,isClaudeBusywith machine-driven stateuseTerminalProfileChangeto automatically resume Claude sessions after terminal recreationFiles Changed: 14 files (986 insertions, 28 deletions)
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests