Repository navigation
Restore previous sessions and resume agents - #2977
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded a comprehensive manual session restoration feature enabling users to reopen previously saved sessions via menu, keyboard shortcut (⌘⇧O), command palette, or CLI. Integrated restorable agent session (Claude/Codex) state into persistence and snapshot hierarchy with new indexing and restoration logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Menu/<br/>Shortcut/<br/>Palette
participant AppDelegate
participant SessionStore as SessionPersistence<br/>Store
participant AgentIndex as RestorableAgent<br/>SessionIndex
participant Workspace
participant Terminal as Terminal<br/>Panel
User->>UI: Trigger "Reopen Previous Session"
UI->>AppDelegate: reopenPreviousSession()
AppDelegate->>SessionStore: loadReopenSessionSnapshot()
SessionStore-->>AppDelegate: AppSessionSnapshot (with agent data)
AppDelegate->>AgentIndex: load()
AgentIndex-->>AppDelegate: RestorableAgentSessionIndex
AppDelegate->>Workspace: Apply snapshots per-window
Workspace->>Workspace: sessionPanelSnapshot()<br/>(with restorable agent)
Workspace->>Terminal: Create surface with<br/>agent workingDirectory
Terminal-->>Workspace: Panel ready
Workspace->>Terminal: Send resumeCommand<br/>(e.g., `codex resume <id>`)
Terminal-->>Workspace: Resume marker in scrollback
Workspace-->>AppDelegate: Restoration complete
AppDelegate-->>UI: true (success)
UI-->>User: Beep on failure or silent on success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
Sources/AppDelegate.swift (1)
3918-3927:⚠️ Potential issue | 🟠 MajorRestore the primary window before the post-restore save.
createMainWindow(sessionWindowSnapshot:)makes each created window key, whilesortedMainWindowContextsForSessionSnapshot()saves the key window first. During multi-window restore, this makes the last created window become the persisted “primary” window, andreopenPreviousSession()then brings that same last window forward instead of the snapshot’s first window.Move primary-window activation before
completeSessionRestoreOperation()saves, or make the post-restore save use the snapshot order instead of key-window order.Suggested direction
- self.completeSessionRestoreOperation() + self.completeSessionRestoreOperation(primaryWindow: primaryWindow) ... - completeSessionRestoreOperation() + completeSessionRestoreOperation(primaryWindow: primaryWindow) ... - completeSessionRestoreOperation() + completeSessionRestoreOperation(primaryWindow: primaryRestoredWindow) ... - private func completeSessionRestoreOperation() { + private func completeSessionRestoreOperation(primaryWindow: NSWindow? = nil) { + if let primaryWindow { + primaryWindow.makeKeyAndOrderFront(nil) + setActiveMainWindow(primaryWindow) + } startupSessionSnapshot = nil isApplyingSessionRestore = false _ = saveSessionSnapshot(includeScrollback: false) }Also applies to: 3972-3978, 4768-4778
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3918 - 3927, During multi-window session restore the call sequence causes the last created window (made key by createMainWindow(sessionWindowSnapshot:)) to be saved as the “primary” because completeSessionRestoreOperation() reads key-window order; fix by ensuring the intended primary window from sortedMainWindowContextsForSessionSnapshot() is activated before the post-restore save: after creating windows, explicitly make the first snapshot’s restored window key (or call the activation routine used for primary selection) and only then call completeSessionRestoreOperation(); alternatively, change completeSessionRestoreOperation()’s save logic to iterate/save windows in the snapshot order (from sortedMainWindowContextsForSessionSnapshot()) instead of relying on current key-window state so reopenPreviousSession() will bring forward the snapshot’s first window rather than the last-created key window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 3168-3178: Current branch treats the app as restored immediately
after launch when socket.connect() fails; instead, after calling launchApp() and
activateApp(), retry connecting or invoke the same restore flow used for the
running-app path (e.g. call session.restore_previous or reuse the socket restore
endpoint) and only print jsonString(["restored": true, "launched": true]) or
"OK" after that restore call succeeds; update the block around SocketClient/try?
client.connect(), client.close(), launchApp(), activateApp(),
jsonOutput/jsonString to perform the restore confirmation and handle/report any
failure from that restore before emitting success.
In `@Sources/AppDelegate.swift`:
- Around line 3949-3955: The startup restore can run inside
registerMainWindow(...) during createMainWindow(sessionWindowSnapshot:) and
overwrite the manual reopen; to prevent this, mark the startup restore as
"attempted" (or otherwise disable attemptStartupSessionRestoreIfNeeded()) before
creating windows when applying a session restore: in the block where
isApplyingSessionRestore is set and you iterate snapshotWindows (use
sortedMainWindowContextsForSessionSnapshot(), isApplyingSessionRestore,
snapshotWindows, createMainWindow(sessionWindowSnapshot:)), set the flag or call
the method that records the startup restore has been attempted so
registerMainWindow(...) won't call attemptStartupSessionRestoreIfNeeded(...) and
clobber the manual snapshot.
In `@Sources/RestorableAgentSession.swift`:
- Around line 27-39: The resumeCommand currently ignores the captured
workingDirectory; update SessionRestorableAgentSnapshot.resumeCommand to prepend
a guarded cd when workingDirectory is non-nil by using shellSingleQuoted to
quote it, e.g. return "cd <quoted cwd> && <originalResume>" so restored agents
run in the persisted cwd; use the existing private static func
shellSingleQuoted(_:) to quote workingDirectory and keep the original
kind.resumeCommand(sessionId:) as the command tail.
In `@Sources/Workspace.swift`:
- Around line 269-275: The current call to
restorableAgentIndex?.snapshot(workspaceId: id, panelId: panelId) conflates
"index not loaded" and "index loaded but no agent" into nil, causing
sessionPanelSnapshot to fall back to restoredAgentSnapshotsByPanelId and
re-persist stale agent data; change the logic so you first detect whether
restorableAgentIndex is present (e.g. capture the optional index in a variable),
and then: if the index is nil, allow sessionPanelSnapshot to use
restoredAgentSnapshotsByPanelId; if the index is non-nil but snapshot(...)
returns nil, treat that as an authoritative miss and pass an explicit “no agent”
marker (or nil but accompanied by a flag) to sessionPanelSnapshot so it clears
any cached restoredAgentSnapshotsByPanelId for that panel; update
sessionPanelSnapshot (and its callers) to accept and honor this distinction.
- Around line 692-693: The resume logic currently sends the raw resumeCommand
(snapshot.terminal?.agent?.resumeCommand) into sendInputWhenReady which can land
in the wrong directory if shell RC files cd elsewhere; update the Workspace code
that triggers sending the resume command to wrap the command with the restored
working directory using a helper like resumeCommandWithCwd(_:workingDirectory:)
(or reuse the existing session-entry cwd guard), and implement
shellSingleQuoted(_:) to safely single-quote and escape the cwd; then call
sendInputWhenReady(resumeCommandWithCwd(resumeCommand, workingDirectory:
snapshot.terminal?.agent?.workingDirectory) + "\n", to: terminalPanel) so all
resume call sites use the guarded form.
In `@tests/test_restore_session_relaunches_codex_resume.py`:
- Around line 176-180: The test directly computes and deletes real user files
(bundle_id via _bundle_id, socket_path, snapshot/previous_snapshot via
_snapshot_path, and hook_state path) which risks destroying a developer's
session state; change the test to isolate its filesystem operations by creating
a temporary home/app-support environment (e.g., use tempfile.TemporaryDirectory
or a tmp_path fixture), stub or override _bundle_id/_snapshot_path to return
paths inside that temp directory, and for hook_state either point it to a temp
file or back up the real ~/.cmuxterm/codex-hook-sessions.json before the test
and restore it after—ensure all cleanup happens in a finally/teardown so real
user files are never deleted.
In `@tests/test_session_relaunch_resumes_agent_sessions.py`:
- Around line 175-180: The test constructs real user file paths (via _bundle_id,
_snapshot_path) and variables bundle_id, socket_path, snapshot,
previous_snapshot, codex_hook_state, claude_hook_state and then deletes them,
risking wiping a developer's real session state; modify the test to isolate
these paths by using a temporary HOME or app-support fixture (e.g., set
Path.home() patch or use tmp_path to create a temp directory) or implement
explicit backup-and-restore around those files so the test operates only on temp
copies and restores any pre-existing
~/.cmuxterm/{codex,claude}-hook-sessions.json and per-bundle snapshots after the
test, making sure every reference to _bundle_id/_snapshot_path in this test (and
the similar blocks at 193-197 and 273-279) points to the temp fixture instead of
the real home directory.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3918-3927: During multi-window session restore the call sequence
causes the last created window (made key by
createMainWindow(sessionWindowSnapshot:)) to be saved as the “primary” because
completeSessionRestoreOperation() reads key-window order; fix by ensuring the
intended primary window from sortedMainWindowContextsForSessionSnapshot() is
activated before the post-restore save: after creating windows, explicitly make
the first snapshot’s restored window key (or call the activation routine used
for primary selection) and only then call completeSessionRestoreOperation();
alternatively, change completeSessionRestoreOperation()’s save logic to
iterate/save windows in the snapshot order (from
sortedMainWindowContextsForSessionSnapshot()) instead of relying on current
key-window state so reopenPreviousSession() will bring forward the snapshot’s
first window rather than the last-created key window.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f846ec46-170a-491c-a53f-6aec289a9515
📒 Files selected for processing (17)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swifttests/test_restore_session_relaunches_codex_resume.pytests/test_session_relaunch_resumes_agent_sessions.pyweb/data/cmux-settings.schema.jsonweb/data/cmux-shortcuts.ts
| let client = SocketClient(path: socketPath) | ||
| if (try? client.connect()) == nil { | ||
| client.close() | ||
| try launchApp() | ||
| try activateApp() | ||
| if jsonOutput { | ||
| print(jsonString(["restored": true, "launched": true])) | ||
| } else { | ||
| print("OK") | ||
| } | ||
| return |
There was a problem hiding this comment.
Don’t report restore success before the app confirms it.
When the socket is unavailable, this branch prints OK / "restored": true immediately after open -a cmux. That can mask launch failures and returns success for the same “no previous snapshot” case that the running-app path correctly surfaces via session.restore_previous.
Proposed fix: launch if needed, then use the same restore endpoint
- let client = SocketClient(path: socketPath)
- if (try? client.connect()) == nil {
- client.close()
- try launchApp()
- try activateApp()
- if jsonOutput {
- print(jsonString(["restored": true, "launched": true]))
- } else {
- print("OK")
- }
- return
- }
-
- defer { client.close() }
- try authenticateClientIfNeeded(
- client,
- explicitPassword: explicitPassword,
- socketPath: socketPath
- )
-
- let response = try client.sendV2(method: "session.restore_previous")
+ let probeClient = SocketClient(path: socketPath)
+ let alreadyRunning = (try? probeClient.connect()) != nil
+ probeClient.close()
+
+ let client = try connectClient(
+ socketPath: socketPath,
+ explicitPassword: explicitPassword,
+ launchIfNeeded: true
+ )
+ defer { client.close() }
+
+ let response = try client.sendV2(method: "session.restore_previous")
+ try activateApp()
if jsonOutput {
- print(jsonString(response))
+ var output = response
+ output["launched"] = !alreadyRunning
+ print(jsonString(output))
} else {
print("OK")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 3168 - 3178, Current branch treats the app as
restored immediately after launch when socket.connect() fails; instead, after
calling launchApp() and activateApp(), retry connecting or invoke the same
restore flow used for the running-app path (e.g. call session.restore_previous
or reuse the socket restore endpoint) and only print jsonString(["restored":
true, "launched": true]) or "OK" after that restore call succeeds; update the
block around SocketClient/try? client.connect(), client.close(), launchApp(),
activateApp(), jsonOutput/jsonString to perform the restore confirmation and
handle/report any failure from that restore before emitting success.
| let existingContexts = sortedMainWindowContextsForSessionSnapshot() | ||
| isApplyingSessionRestore = true | ||
|
|
||
| if existingContexts.isEmpty { | ||
| for windowSnapshot in snapshotWindows { | ||
| _ = createMainWindow(sessionWindowSnapshot: windowSnapshot) | ||
| } |
There was a problem hiding this comment.
Prevent startup restore from clobbering manual reopen.
When there are no registered main contexts, createMainWindow(sessionWindowSnapshot:) registers the new window, and registerMainWindow(...) then calls attemptStartupSessionRestoreIfNeeded(...) at Line 4917. If startup restore has not been marked attempted yet, the cached startup snapshot can overwrite the manual reopen snapshot being applied here.
Suggested fix
let existingContexts = sortedMainWindowContextsForSessionSnapshot()
+ startupSessionSnapshot = nil
+ didAttemptStartupSessionRestore = true
isApplyingSessionRestore = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 3949 - 3955, The startup restore can
run inside registerMainWindow(...) during
createMainWindow(sessionWindowSnapshot:) and overwrite the manual reopen; to
prevent this, mark the startup restore as "attempted" (or otherwise disable
attemptStartupSessionRestoreIfNeeded()) before creating windows when applying a
session restore: in the block where isApplyingSessionRestore is set and you
iterate snapshotWindows (use sortedMainWindowContextsForSessionSnapshot(),
isApplyingSessionRestore, snapshotWindows,
createMainWindow(sessionWindowSnapshot:)), set the flag or call the method that
records the startup restore has been attempted so registerMainWindow(...) won't
call attemptStartupSessionRestoreIfNeeded(...) and clobber the manual snapshot.
| private static func shellSingleQuoted(_ value: String) -> String { | ||
| "'" + value.replacingOccurrences(of: "'", with: "'\\''") + "'" | ||
| } | ||
| } | ||
|
|
||
| struct SessionRestorableAgentSnapshot: Codable, Sendable { | ||
| var kind: RestorableAgentKind | ||
| var sessionId: String | ||
| var workingDirectory: String? | ||
|
|
||
| var resumeCommand: String { | ||
| kind.resumeCommand(sessionId: sessionId) | ||
| } |
There was a problem hiding this comment.
Guard agent resumes with the persisted cwd.
workingDirectory is captured but ignored when generating resumeCommand, so restored Claude/Codex panels can resume in the wrong project directory. Compose the command as cd <quoted cwd> && <agent resume> when a cwd is available.
🐛 Proposed fix
- private static func shellSingleQuoted(_ value: String) -> String {
+ fileprivate static func shellSingleQuoted(_ value: String) -> String {
"'" + value.replacingOccurrences(of: "'", with: "'\\''") + "'"
}
}
@@
var resumeCommand: String {
- kind.resumeCommand(sessionId: sessionId)
+ let command = kind.resumeCommand(sessionId: sessionId)
+ guard let workingDirectory else { return command }
+ return "cd \(RestorableAgentKind.shellSingleQuoted(workingDirectory)) && \(command)"
}
}Based on learnings, session resume commands must use a guaranteed cwd guard: cd <shell-quoted cwd> && <resumeCommand>.
📝 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.
| private static func shellSingleQuoted(_ value: String) -> String { | |
| "'" + value.replacingOccurrences(of: "'", with: "'\\''") + "'" | |
| } | |
| } | |
| struct SessionRestorableAgentSnapshot: Codable, Sendable { | |
| var kind: RestorableAgentKind | |
| var sessionId: String | |
| var workingDirectory: String? | |
| var resumeCommand: String { | |
| kind.resumeCommand(sessionId: sessionId) | |
| } | |
| fileprivate static func shellSingleQuoted(_ value: String) -> String { | |
| "'" + value.replacingOccurrences(of: "'", with: "'\\''") + "'" | |
| } | |
| } | |
| struct SessionRestorableAgentSnapshot: Codable, Sendable { | |
| var kind: RestorableAgentKind | |
| var sessionId: String | |
| var workingDirectory: String? | |
| var resumeCommand: String { | |
| let command = kind.resumeCommand(sessionId: sessionId) | |
| guard let workingDirectory else { return command } | |
| return "cd \(RestorableAgentKind.shellSingleQuoted(workingDirectory)) && \(command)" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/RestorableAgentSession.swift` around lines 27 - 39, The resumeCommand
currently ignores the captured workingDirectory; update
SessionRestorableAgentSnapshot.resumeCommand to prepend a guarded cd when
workingDirectory is non-nil by using shellSingleQuoted to quote it, e.g. return
"cd <quoted cwd> && <originalResume>" so restored agents run in the persisted
cwd; use the existing private static func shellSingleQuoted(_:) to quote
workingDirectory and keep the original kind.resumeCommand(sessionId:) as the
command tail.
| .compactMap { panelId in | ||
| sessionPanelSnapshot( | ||
| panelId: panelId, | ||
| includeScrollback: includeScrollback, | ||
| restorableAgent: restorableAgentIndex?.snapshot(workspaceId: id, panelId: panelId) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid re-persisting stale agent resume metadata.
restorableAgentIndex?.snapshot(...) collapses “index unavailable” and “no active/restorable agent for this panel” into nil. Because sessionPanelSnapshot then falls back to restoredAgentSnapshotsByPanelId, a once-restored panel can keep writing its old agent forever after the live index no longer reports it, causing future restores to relaunch a stale Claude/Codex session.
Treat a loaded index miss as authoritative and clear the cached snapshot; only fall back to restoredAgentSnapshotsByPanelId when the index itself is unavailable.
Proposed direction
- restorableAgent: restorableAgentIndex?.snapshot(workspaceId: id, panelId: panelId)
+ restorableAgent: restorableAgentIndex?.snapshot(workspaceId: id, panelId: panelId),
+ restorableAgentLookupAvailable: restorableAgentIndex != nil- restorableAgent: SessionRestorableAgentSnapshot?
+ restorableAgent: SessionRestorableAgentSnapshot?,
+ restorableAgentLookupAvailable: Bool- let effectiveRestorableAgent = restorableAgent ?? restoredAgentSnapshotsByPanelId[panelId]
- if let restorableAgent {
- restoredAgentSnapshotsByPanelId[panelId] = restorableAgent
- }
+ let effectiveRestorableAgent: SessionRestorableAgentSnapshot?
+ if restorableAgentLookupAvailable {
+ effectiveRestorableAgent = restorableAgent
+ if let restorableAgent {
+ restoredAgentSnapshotsByPanelId[panelId] = restorableAgent
+ } else {
+ restoredAgentSnapshotsByPanelId.removeValue(forKey: panelId)
+ }
+ } else {
+ effectiveRestorableAgent = restoredAgentSnapshotsByPanelId[panelId]
+ }Also applies to: 449-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 269 - 275, The current call to
restorableAgentIndex?.snapshot(workspaceId: id, panelId: panelId) conflates
"index not loaded" and "index loaded but no agent" into nil, causing
sessionPanelSnapshot to fall back to restoredAgentSnapshotsByPanelId and
re-persist stale agent data; change the logic so you first detect whether
restorableAgentIndex is present (e.g. capture the optional index in a variable),
and then: if the index is nil, allow sessionPanelSnapshot to use
restoredAgentSnapshotsByPanelId; if the index is non-nil but snapshot(...)
returns nil, treat that as an authoritative miss and pass an explicit “no agent”
marker (or nil but accompanied by a flag) to sessionPanelSnapshot so it clears
any cached restoredAgentSnapshotsByPanelId for that panel; update
sessionPanelSnapshot (and its callers) to accept and honor this distinction.
| if let resumeCommand = snapshot.terminal?.agent?.resumeCommand { | ||
| sendInputWhenReady(resumeCommand + "\n", to: terminalPanel) |
There was a problem hiding this comment.
Guard restored agent resumes with the intended cwd.
This sends the bare claude --resume / codex resume command after the new shell starts. If shell rc files cd elsewhere before sendInputWhenReady fires, the resumed agent lands in the wrong directory. Wrap the resume with cd <shell-quoted cwd> && ... using the restored terminal/agent working directory.
Proposed direction
- if let resumeCommand = snapshot.terminal?.agent?.resumeCommand {
- sendInputWhenReady(resumeCommand + "\n", to: terminalPanel)
+ if let agent = snapshot.terminal?.agent {
+ let resumeCommand = Self.resumeCommandWithCwd(
+ agent.resumeCommand,
+ workingDirectory: workingDirectory
+ )
+ sendInputWhenReady(resumeCommand + "\n", to: terminalPanel)
}Add the helper in this Workspace extension or reuse the existing session-entry cwd guard helper:
private static func resumeCommandWithCwd(_ command: String, workingDirectory: String?) -> String {
guard let cwd = workingDirectory?.trimmingCharacters(in: .whitespacesAndNewlines),
!cwd.isEmpty else {
return command
}
return "cd \(shellSingleQuoted(cwd)) && \(command)"
}
private static func shellSingleQuoted(_ value: String) -> String {
"'" + value.replacingOccurrences(of: "'", with: "'\"'\"'") + "'"
}Based on learnings, “all call sites that generate ‘resume’ commands … should use this form so fresh shells and rc files cannot land outside the intended directory.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 692 - 693, The resume logic currently
sends the raw resumeCommand (snapshot.terminal?.agent?.resumeCommand) into
sendInputWhenReady which can land in the wrong directory if shell RC files cd
elsewhere; update the Workspace code that triggers sending the resume command to
wrap the command with the restored working directory using a helper like
resumeCommandWithCwd(_:workingDirectory:) (or reuse the existing session-entry
cwd guard), and implement shellSingleQuoted(_:) to safely single-quote and
escape the cwd; then call sendInputWhenReady(resumeCommandWithCwd(resumeCommand,
workingDirectory: snapshot.terminal?.agent?.workingDirectory) + "\n", to:
terminalPanel) so all resume call sites use the guarded form.
| bundle_id = _bundle_id(app_path) | ||
| socket_path = Path(f"/tmp/cmux-restore-session-codex-{bundle_id.replace('.', '-')}.sock") | ||
| snapshot = _snapshot_path(bundle_id) | ||
| previous_snapshot = _snapshot_path(bundle_id, suffix="-previous") | ||
| hook_state = Path.home() / ".cmuxterm" / "codex-hook-sessions.json" |
There was a problem hiding this comment.
Avoid deleting real user session state from the test.
This test removes the actual cmux snapshot files and ~/.cmuxterm/codex-hook-sessions.json. That can destroy a developer’s saved restore state when the test is run against a local app bundle. Please isolate the app’s home/app-support paths for the test or backup and restore these files.
Also applies to: 189-192, 272-277
🧰 Tools
🪛 Ruff (0.15.10)
[error] 177-177: Probable insecure usage of temporary file or directory: "/tmp/cmux-restore-session-codex-"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_restore_session_relaunches_codex_resume.py` around lines 176 -
180, The test directly computes and deletes real user files (bundle_id via
_bundle_id, socket_path, snapshot/previous_snapshot via _snapshot_path, and
hook_state path) which risks destroying a developer's session state; change the
test to isolate its filesystem operations by creating a temporary
home/app-support environment (e.g., use tempfile.TemporaryDirectory or a
tmp_path fixture), stub or override _bundle_id/_snapshot_path to return paths
inside that temp directory, and for hook_state either point it to a temp file or
back up the real ~/.cmuxterm/codex-hook-sessions.json before the test and
restore it after—ensure all cleanup happens in a finally/teardown so real user
files are never deleted.
| bundle_id = _bundle_id(app_path) | ||
| socket_path = Path(f"/tmp/cmux-session-relaunch-agents-{bundle_id.replace('.', '-')}.sock") | ||
| snapshot = _snapshot_path(bundle_id) | ||
| previous_snapshot = _snapshot_path(bundle_id, suffix="-previous") | ||
| codex_hook_state = Path.home() / ".cmuxterm" / "codex-hook-sessions.json" | ||
| claude_hook_state = Path.home() / ".cmuxterm" / "claude-hook-sessions.json" |
There was a problem hiding this comment.
Avoid deleting real user session state from the test.
This test unlinks the actual per-bundle cmux snapshots and ~/.cmuxterm/{codex,claude}-hook-sessions.json. Running it locally can wipe a developer’s saved session/resume metadata. Please isolate these paths under a temp home/app-support fixture, or backup and restore the existing files around the test.
Also applies to: 193-197, 273-279
🧰 Tools
🪛 Ruff (0.15.10)
[error] 176-176: Probable insecure usage of temporary file or directory: "/tmp/cmux-session-relaunch-agents-"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_session_relaunch_resumes_agent_sessions.py` around lines 175 -
180, The test constructs real user file paths (via _bundle_id, _snapshot_path)
and variables bundle_id, socket_path, snapshot, previous_snapshot,
codex_hook_state, claude_hook_state and then deletes them, risking wiping a
developer's real session state; modify the test to isolate these paths by using
a temporary HOME or app-support fixture (e.g., set Path.home() patch or use
tmp_path to create a temp directory) or implement explicit backup-and-restore
around those files so the test operates only on temp copies and restores any
pre-existing ~/.cmuxterm/{codex,claude}-hook-sessions.json and per-bundle
snapshots after the test, making sure every reference to
_bundle_id/_snapshot_path in this test (and the similar blocks at 193-197 and
273-279) points to the temp fixture instead of the real home directory.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d19411b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try launchApp() | ||
| try activateApp() | ||
| if jsonOutput { | ||
| print(jsonString(["restored": true, "launched": true])) |
There was a problem hiding this comment.
When cmux restore-session can't connect, this branch only launches/activates the app and returns success, but never invokes session.restore_previous. That breaks the recovery case where session-<bundle>.json is already blank and only session-<bundle>-previous.json has the desired state: startup immediately runs SessionPersistenceStore.syncManualRestoreSnapshotCache() (Sources/AppDelegate.swift), which copies the blank snapshot over -previous, so the old session is lost before any restore call happens. In this scenario the command reports OK/{"restored":true} even though nothing was restored.
Useful? React with 👍 / 👎.
|
Closing and reopening via gh CLI per request. |
Greptile SummaryThis PR adds "Reopen Previous Session" functionality — a The architecture is well-designed, but the regression test was committed after the fix rather than before it, which directly violates the project's two-commit regression test policy. Confidence Score: 4/5Safe to merge with minor follow-ups; the P1 is a process violation that doesn't affect runtime correctness. The core implementation is solid — agent data is correctly embedded in session snapshots at quit time, shell quoting is correct, Sources/SessionPersistence.swift (fallback logic), CLI/cmux.swift (restore-session asymmetry), tests/test_session_relaunch_resumes_agent_sessions.py (commit order + workspace index) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User triggers Reopen Previous Session]) --> B{App running?}
B -- No --> C[cmux restore-session launches app normally]
C --> D[Startup restore reads default snapshot]
B -- Yes --> F{Via socket?}
F -- Yes --> G[session.restore_previous socket command]
F -- No --> H[reopenPreviousSession directly]
G --> H
H --> I[loadReopenSessionSnapshot]
I --> J{-previous file exists?}
J -- Yes --> K[Load -previous snapshot]
J -- No --> L[Fallback: load default snapshot]
K --> M[isApplyingSessionRestore = true]
L --> M
M --> N{Existing windows?}
N -- No --> O[createMainWindow for each snapshot window]
N -- Yes --> P[restoreSessionSnapshot: tear down existing tabs, rebuild]
O --> Q[completeSessionRestoreOperation]
P --> Q
Q --> R[makeKeyAndOrderFront + NSApp activate]
style L fill:#ffddaa,stroke:#cc8800
Reviews (1): Last reviewed commit: "Add relaunch regression coverage for ses..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #!/usr/bin/env python3 | ||
| """ | ||
| Regression: normal relaunch should resume saved Claude/Codex sessions. | ||
|
|
||
| Repro for issue #2923: | ||
| 1) Launch cmux and seed workspaces with tracked Claude/Codex sessions. | ||
| 2) Quit the app normally so the session snapshot is saved. | ||
| 3) Relaunch cmux the next day. | ||
| 4) Verify the restored panels automatically run the saved resume commands. | ||
| """ |
There was a problem hiding this comment.
The CLAUDE.md regression test policy requires: Commit 1 = failing test only (CI goes red), Commit 2 = the fix (CI goes green). In this PR the order is reversed — 7f4d8c38 ships the fix first, and this test is tacked on in the follow-up commit 09d19411. CI therefore never ran this test in a state where it could fail, so there's no proof the test actually catches the bug. This also makes it harder to revert just the fix while keeping the test as a guard.
Context Used: CLAUDE.md (source)
| static func loadReopenSessionSnapshot(fileURL: URL? = nil) -> AppSessionSnapshot? { | ||
| if let manualRestoreSnapshot = load(fileURL: fileURL ?? manualRestoreSnapshotFileURL()) { | ||
| return manualRestoreSnapshot | ||
| } | ||
| return load() | ||
| } |
There was a problem hiding this comment.
When no -previous snapshot file exists (first-ever run, or after a fresh install), loadReopenSessionSnapshot() falls through to load() and returns the current running session's snapshot. reopenPreviousSession() then calls restoreSessionSnapshot, which is destructive — it tears down existing workspaces and re-creates them from the snapshot. If the user triggers ⌘⇧O before ever quitting once, they get the current session replaced with itself (minus any work done since the last autosave tick). Consider returning nil when the previous-session file is absent so reopenPreviousSession can beep instead.
| static func loadReopenSessionSnapshot(fileURL: URL? = nil) -> AppSessionSnapshot? { | |
| if let manualRestoreSnapshot = load(fileURL: fileURL ?? manualRestoreSnapshotFileURL()) { | |
| return manualRestoreSnapshot | |
| } | |
| return load() | |
| } | |
| static func loadReopenSessionSnapshot(fileURL: URL? = nil) -> AppSessionSnapshot? { | |
| load(fileURL: fileURL ?? manualRestoreSnapshotFileURL()) | |
| } |
| let client = SocketClient(path: socketPath) | ||
| if (try? client.connect()) == nil { | ||
| client.close() | ||
| try launchApp() | ||
| try activateApp() | ||
| if jsonOutput { | ||
| print(jsonString(["restored": true, "launched": true])) | ||
| } else { | ||
| print("OK") | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
restore-session (app-not-running path) skips the -previous snapshot
When the app is not running, runRestoreSession launches cmux normally and returns immediately. The startup restore reads the default (current) snapshot — not the -previous one. But when the app IS running, session.restore_previous reads from the -previous file (state before the current session). A running-app restore gives "yesterday's" state while a cold-launch restore gives the most recent autosave. Consider documenting this asymmetry in the help text, as it directly affects the stated overnight-relaunch use-case.
| def workspace_contains(index: int, expected: str) -> bool: | ||
| if len(client.list_workspaces()) <= index: | ||
| return False | ||
| client.select_workspace(index) | ||
| return expected in _read_scrollback(client) |
There was a problem hiding this comment.
workspace_contains selects by positional index (0, 1) inside the _wait_for_condition polling loop. If restored workspaces arrive in a different order the predicate will check the wrong workspace and the test could pass spuriously. Consider recording workspace IDs during setup and selecting by ID instead.
| def workspace_contains(index: int, expected: str) -> bool: | |
| if len(client.list_workspaces()) <= index: | |
| return False | |
| client.select_workspace(index) | |
| return expected in _read_scrollback(client) | |
| def workspace_contains(ws_id: str, expected: str) -> bool: | |
| client.select_workspace(ws_id) | |
| return expected in _read_scrollback(client) |
This branch was successfully deployed
2 participants
Summary
File > Reopen Previous Session,⌘⇧O, orcmux restore-sessionCloses #2923.
Testing
python3 -m py_compile tests/test_session_relaunch_resumes_agent_sessions.pytests/test_restore_session_relaunches_codex_resume.py./scripts/reload.sh --tag issue-2923-reopen-sessionsbut localxcodebuildstalled in the Xcode environment before producing a compile resultSummary by cubic
Restore the last saved session and auto-resume Claude/Codex panels on relaunch or on demand. Adds a quick “Reopen Previous Session” flow to bring back windows, panes, and agent sessions.
cmux restore-sessionCLI; restores into a running app or launches and restores if not running.session.restore_previousfor programmatic restores.Written for commit 09d1941. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
cmux restore-sessionCLI command for manual session restorationDocumentation
Localization