Repository navigation
Fix agent resume post-exit cwd - #5421
austinywang wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughThreads a normalized optional returnWorkingDirectory through workspace startup, surface/agent resume launcher generation, and return-shell script creation; the return script now emits a prioritized cd chain (primary → fallback → HOME). Tests and Xcode project entries validate the new behavior. ChangesResume Launcher Working Directory Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ 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 |
Greptile SummaryFixes issue #5391 by decoupling the resumed agent command's
Confidence Score: 5/5Safe to merge — changes are confined to launcher script generation and the return-cwd wiring, fully covered by real zsh execution tests. The fix correctly splits the resumed command's own cd prefix from the outer shell's post-exit directory, normalization handles nil/empty/whitespace-only paths, and the always-emit-HOME fallback closes the landing regression. Integration tests execute real generated scripts and assert outer-pwd-before-exec. No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let temp = try TemporaryScriptFixture() | ||
| defer { temp.cleanup() } | ||
| let binding = SurfaceResumeBindingSnapshot( | ||
| name: "Issue 5391 Binding", | ||
| kind: "codex", | ||
| command: #"/bin/zsh -fc 'printf "child-pwd=%s\n" "$PWD"'"#, | ||
| cwd: " ", | ||
| source: "agent-hook", | ||
| autoResume: true, | ||
| updatedAt: 123 | ||
| ) | ||
|
|
||
| let startupCommand = try #require(binding.startupCommandWithLauncherScript( | ||
| fileManager: temp.fileManager, | ||
| temporaryDirectory: temp.root | ||
| )) | ||
| let scriptContents = try String( | ||
| contentsOfFile: launcherScriptPath(from: startupCommand), | ||
| encoding: .utf8 | ||
| ) | ||
|
|
||
| try expectGeneratedScript(scriptContents, returnsTo: .home) | ||
| let output = try temp.runInstrumentedScript(scriptContents) | ||
|
|
||
| #expect(output.lines.contains("child-pwd=/")) | ||
| #expect(output.lines.contains("outer-pwd-before-exec=\(temp.homeDirectory.path)")) | ||
| #expect(!output.lines.contains("outer-pwd-before-exec=/")) | ||
| } |
There was a problem hiding this comment.
returnWorkingDirectory fallback path
surfaceResumeBindingLauncherFallsBackToHomeForEmptyCwd calls startupCommandWithLauncherScript without passing returnWorkingDirectory, so fallbackWorkingDirectory inside commandThenReturnLines is always nil. The test only confirms the pure $HOME branch. The production path through Workspace.surfaceResumeStartupLaunch always passes bindingReturnWorkingDirectory (e.g. snapshot.terminal?.workingDirectory), meaning that when binding.cwd is empty but a snapshot directory exists, the outer shell should navigate there — not to $HOME. A complementary test asserting returnsTo: .literal(snapshotDir) when returnWorkingDirectory is non-nil would give full branch coverage for normalized(workingDirectory) ?? normalized(fallbackWorkingDirectory).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/RestorableAgentSession.swift`:
- Line 717: The savedReturnWorkingDirectory selection uses workingDirectory ??
launchCommand?.workingDirectory which treats whitespace-only strings as valid;
normalize workingDirectory first (trim whitespace/newlines and treat empty after
trimming as nil) before falling back to launchCommand?.workingDirectory so a
blank persisted cwd does not suppress launchCommand?.workingDirectory — update
the assignment for savedReturnWorkingDirectory in RestorableAgentSession.swift
to use the trimmed/emptiness-checked workingDirectory when deciding to fall back
to launchCommand?.workingDirectory.
In `@Sources/SessionPersistence.swift`:
- Around line 1303-1308: The generated shell snippet currently only tries to cd
into the preferred directory and gives up on failure; update the branch that
builds the line (the block using normalized(workingDirectory) ??
normalized(fallbackWorkingDirectory), TerminalStartupShellQuoting.singleQuoted
and lines.append) so the appended shell expression chains runtime fallbacks:
attempt cd to the quoted primary returnWorkingDirectory, if that fails try the
quoted fallbackWorkingDirectory, and if that also fails fall back to cd --
"${HOME}" (all failures suppressed with 2>/dev/null and || to continue); in
short, change the single cd command produced by
TerminalStartupShellQuoting.singleQuoted into a chained cd sequence that tries
primary -> fallback -> $HOME at runtime.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8be45352-785a-4ed9-b980-2726bc75569a
📒 Files selected for processing (6)
Sources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentSessionAutoResumeSettingsTests.swiftcmuxTests/TerminalStartupReturnShellScriptTests.swift
cc2658d to
c5580c5
Compare
| let bindingReturnWorkingDirectory = | ||
| effectiveResumeBindingForStartup?.cwd | ||
| ?? snapshot.terminal?.workingDirectory | ||
| ?? restorableAgent?.workingDirectory | ||
| ?? snapshot.directory |
There was a problem hiding this comment.
bindingReturnWorkingDirectory cascade uses the raw, un-normalized value of effectiveResumeBindingForStartup?.cwd. When cwd is a whitespace-only string (e.g. " ") — a case the new tests explicitly exercise — Swift's ?? operator sees it as non-nil and short-circuits, so snapshot.terminal?.workingDirectory, restorableAgent?.workingDirectory, and snapshot.directory are never considered. normalizedTerminalWorkingDirectory then trims the whitespace value to nil inside commandThenReturnLines, and the outer shell falls through to $HOME rather than the terminal's saved directory.
| let bindingReturnWorkingDirectory = | |
| effectiveResumeBindingForStartup?.cwd | |
| ?? snapshot.terminal?.workingDirectory | |
| ?? restorableAgent?.workingDirectory | |
| ?? snapshot.directory | |
| let bindingReturnWorkingDirectory = | |
| normalizedTerminalWorkingDirectory(effectiveResumeBindingForStartup?.cwd) | |
| ?? snapshot.terminal?.workingDirectory | |
| ?? restorableAgent?.workingDirectory | |
| ?? snapshot.directory |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/RestorableAgentSession.swift (1)
717-737:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
resumeStartupCommandstill can't take a distinct return cwd.
workingDirectoryonSessionRestorableAgentSnapshotis the resume/fork cwd, but this method also reuses it (orlaunchCommand?.workingDirectory) as the post-exit return target. That means the agent launcher path still cannot restore the outer shell to the saved panel/session cwd when those differ — most obviously forcwd == .ignore, where the correct return dir must come from the surface snapshot, not the resume snapshot.Suggested direction
- func resumeStartupCommand( - fileManager: FileManager = .default, - temporaryDirectory: URL = FileManager.default.temporaryDirectory - ) -> String? { - let savedReturnWorkingDirectory = normalizedTerminalWorkingDirectory(workingDirectory) + func resumeStartupCommand( + fileManager: FileManager = .default, + temporaryDirectory: URL = FileManager.default.temporaryDirectory, + returnWorkingDirectory: String? = nil + ) -> String? { + let savedReturnWorkingDirectory = normalizedTerminalWorkingDirectory(returnWorkingDirectory) + ?? normalizedTerminalWorkingDirectory(workingDirectory) ?? normalizedTerminalWorkingDirectory(launchCommand?.workingDirectory)Then thread the saved surface/panel cwd into this API from the restore path instead of recomputing it locally.
Based on learnings:
SessionEntry.resumeWorkingDirectoryis the single source of truth for the working directory used in registered-agent resume commands, and registrations withcwd: .ignoreset it to nil, suppressing both the cwd guard in the resume command and the terminal working directory at placement time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RestorableAgentSession.swift` around lines 717 - 737, The resumeStartupCommand currently recomputes the "return" cwd from SessionRestorableAgentSnapshot. Replace that recomputation by accepting/using the single source of truth (the saved surface/panel cwd provided by the restore path — e.g. SessionEntry.resumeWorkingDirectory or an explicit parameter) and pass that value into AgentResumeScriptStore.writeLauncherScript's workingDirectory/fallbackWorkingDirectory arguments instead of using normalizedTerminalWorkingDirectory(workingDirectory) or launchCommand?.workingDirectory; ensure registrations with cwd == .ignore map to nil so the resume command's internal cd guard behavior remains unchanged (update resumeStartupCommand signature to take the returnWorkingDirectory or read SessionEntry.resumeWorkingDirectory and use resumeCommand/registration?.cwd logic accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/TerminalStartupReturnShellScriptTests.swift`:
- Around line 192-197: Replace the non-fatal assertions in
shellSingleQuotedTokenValue(_:) with a fatal precondition: change the two
`#expect` checks for token.hasPrefix("'") and token.hasSuffix("'") to `#require` (or
another precondition API) so the method aborts on violation before calling
token.dropFirst().dropLast(); include a short message in each `#require` to
indicate the expectation (e.g., "token must start with '\''" and "token must end
with '\''") to make failures clear.
---
Outside diff comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 717-737: The resumeStartupCommand currently recomputes the
"return" cwd from SessionRestorableAgentSnapshot. Replace that recomputation by
accepting/using the single source of truth (the saved surface/panel cwd provided
by the restore path — e.g. SessionEntry.resumeWorkingDirectory or an explicit
parameter) and pass that value into AgentResumeScriptStore.writeLauncherScript's
workingDirectory/fallbackWorkingDirectory arguments instead of using
normalizedTerminalWorkingDirectory(workingDirectory) or
launchCommand?.workingDirectory; ensure registrations with cwd == .ignore map to
nil so the resume command's internal cd guard behavior remains unchanged (update
resumeStartupCommand signature to take the returnWorkingDirectory or read
SessionEntry.resumeWorkingDirectory and use resumeCommand/registration?.cwd
logic accordingly).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5beab6f-372e-47d3-b8ad-dd492062c9fa
📒 Files selected for processing (6)
Sources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentSessionAutoResumeSettingsTests.swiftcmuxTests/TerminalStartupReturnShellScriptTests.swift
c5580c5 to
97c069e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
97c069e to
f3d7713
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f3d7713. Configure here.
f3d7713 to
cf17278
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SessionPersistence.swift (1)
1285-1356: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the return-shell/launcher code into its own Swift file.
This change adds more restore-shell behavior to a file that already owns snapshot schema, approval persistence, and crypto/keychain logic. Keeping
TerminalStartupReturnShellScriptandSurfaceResumeBindingScriptStorehere makes an already over-budget file harder to reason about and test.As per coding guidelines, “Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility” and “One major type per file.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/SessionPersistence.swift` around lines 1285 - 1356, The file has grown beyond a single responsibility; extract the resume-shell/launcher logic into a new Swift source (e.g., TerminalStartupReturnShellScript.swift) by moving the TerminalStartupReturnShellScript type and the SurfaceResumeBindingScriptStore type + their private helpers (returnWorkingDirectoryLine, normalized, commandThenReturnLines, zshIntegrationReentryLines, safeFilenamePrefix references used) into the new file; preserve existing access control (private/internal) or raise to internal if they are referenced outside, add any needed imports (Foundation/FileManager), update any call sites to the moved types (no API changes), and run tests to ensure symbols resolve and no visibility regressions occur.
♻️ Duplicate comments (1)
Sources/RestorableAgentSession.swift (1)
846-852:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the return-directory list before deciding whether to fall back.
returnWorkingDirectories.isEmptyonly catches a literally empty array. A value like[nil]or[" "]still bypassesworkingDirectory, andTerminalStartupReturnShellScriptthen normalizes those entries away and falls straight through to$HOME. That makes the new “ignore whitespace-only paths” behavior inconsistent for callers that pass placeholder entries.Suggested fix
if returnToLoginShell { + let effectiveReturnWorkingDirectories = + firstNormalizedTerminalWorkingDirectory(returnWorkingDirectories) == nil + ? returnWorkingDirectories + [workingDirectory] + : returnWorkingDirectories lines.append(contentsOf: TerminalStartupReturnShellScript.commandThenReturnLines( command: command, - returnWorkingDirectories: returnWorkingDirectories.isEmpty - ? [workingDirectory] - : returnWorkingDirectories + returnWorkingDirectories: effectiveReturnWorkingDirectories )) } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RestorableAgentSession.swift` around lines 846 - 852, Normalize and filter returnWorkingDirectories before checking emptiness: compute a normalized array by compactMap-ing/removing nils, trimming whitespace/newlines, and filtering out empty strings, then use that normalized array in the conditional (i.e. use [workingDirectory] when the normalized array is empty, otherwise pass the normalized array into TerminalStartupReturnShellScript.commandThenReturnLines). Update the code around the returnToLoginShell block that references returnWorkingDirectories and workingDirectory (RestorableAgentSession.swift / TerminalStartupReturnShellScript.commandThenReturnLines) to use this normalized list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/SessionPersistence.swift`:
- Around line 1285-1356: The file has grown beyond a single responsibility;
extract the resume-shell/launcher logic into a new Swift source (e.g.,
TerminalStartupReturnShellScript.swift) by moving the
TerminalStartupReturnShellScript type and the SurfaceResumeBindingScriptStore
type + their private helpers (returnWorkingDirectoryLine, normalized,
commandThenReturnLines, zshIntegrationReentryLines, safeFilenamePrefix
references used) into the new file; preserve existing access control
(private/internal) or raise to internal if they are referenced outside, add any
needed imports (Foundation/FileManager), update any call sites to the moved
types (no API changes), and run tests to ensure symbols resolve and no
visibility regressions occur.
---
Duplicate comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 846-852: Normalize and filter returnWorkingDirectories before
checking emptiness: compute a normalized array by compactMap-ing/removing nils,
trimming whitespace/newlines, and filtering out empty strings, then use that
normalized array in the conditional (i.e. use [workingDirectory] when the
normalized array is empty, otherwise pass the normalized array into
TerminalStartupReturnShellScript.commandThenReturnLines). Update the code around
the returnToLoginShell block that references returnWorkingDirectories and
workingDirectory (RestorableAgentSession.swift /
TerminalStartupReturnShellScript.commandThenReturnLines) to use this normalized
list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6cb7497-ee2c-4026-8586-226cff62bc4e
📒 Files selected for processing (5)
Sources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/Workspace.swiftcmuxTests/AgentSessionAutoResumeSettingsTests.swiftcmuxTests/TerminalStartupReturnShellScriptTests.swift
cf17278 to
7fb06d7
Compare
…-5391-exit-root-cwd # Conflicts: # cmux.xcodeproj/project.pbxproj
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #5391
Summary
$HOME, beforeexec -lTwo-commit proof
test: cover agent resume return cwdadds the failing regression onlyfix: restore cwd after agent resume exitsimplements the shared launcher fixVerification
/beforeexec -l.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes generated zsh startup/restore scripts and cwd selection across workspace restore and agent resume paths; behavior is well-covered by new tests but affects real shell state on exit.
Overview
Fixes post-exit shell placement after agent/surface resume by splitting the cwd used inside the resumed command from the cwd chain used when the outer login shell returns before
exec -l.Return-shell scripts now take
returnWorkingDirectoriesinstead of a singleworkingDirectory.TerminalStartupReturnShellScriptemits a chainedcd(normalized, deduped candidates, then$HOME) rather than one optional directory or silence. Shared helpersnormalizedTerminalWorkingDirectory/firstNormalizedTerminalWorkingDirectorycentralize trimming and “first non-empty” selection.Call sites thread ordered candidates through agent resume launchers, surface binding launchers, and
Workspacerestore (binding → terminal → agent → snapshot). Agents with.ignorecwd still resume without forcing projectcdin the child, but the outer shell can still return via the explicit candidate list.Tests: new
TerminalStartupReturnShellScriptTests(instrumented zsh scripts,.ignoreagents, empty/stale binding cwd); existing auto-resume test updated for$HOMEfallback.Reviewed by Cursor Bugbot for commit b71d247. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes a regression where the outer login shell could stay at
/or the launcher cwd after an agent resume. We now always return the outer shell to the saved session directory via an ordered, normalized cwd list with a$HOMEfallback, without changing the resumed command’s cwd. Fixes #5391.TerminalStartupReturnShellScriptnow takesreturnWorkingDirectories, builds a normalized, de-dupedcdchain, and always appends$HOMEbeforeexec -l..ignoreagents: the resumed command runs in the current dir; the outer shell returns via the candidate chain/$HOMEor explicit overrides.Workspaceand binding launchers pass an ordered return list (binding → terminal → agent → snapshot); helpersnormalizedTerminalWorkingDirectory/firstNormalizedTerminalWorkingDirectorycentralize trimming/selection.TerminalStartupReturnShellScriptTestsand updated tests to cover$HOMEdefault when no cwd is saved, runtime fallback when preferred cwd is missing,.ignorebehavior, and workspace restore when binding cwd is empty/stale.Written for commit b71d247. Summary will update on new commits.
Summary by CodeRabbit
New Features
cd, falling back to HOME; agent ignore policy can suppress returning to a directory.Tests
Chores