Repository navigation
Restoring a remote workspace terminal hands it this machine's working directory - #8634
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemote agent resume now selects trusted runtime or launch directories, removes conflicting recorded working-directory arguments, and passes exact directory selections into startup command generation. Regression tests cover remote trust, agent-specific paths, option formats, canonicalization, and focused CI execution. ChangesRemote resume working-directory handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workspace
participant RestorableAgentWorkingDirectorySelection
participant RestorableAgentSession
participant AgentLaunchSanitizer
Workspace->>RestorableAgentWorkingDirectorySelection: resolve trusted runtime or launch directory
RestorableAgentWorkingDirectorySelection->>RestorableAgentSession: provide exact working-directory selection
RestorableAgentSession->>AgentLaunchSanitizer: remove recorded working-directory options
AgentLaunchSanitizer-->>RestorableAgentSession: return sanitized resume arguments
RestorableAgentSession-->>Workspace: produce resume startup input
Possibly related PRs
Suggested reviewers: Merge Risk: 🟡 Moderate · up to The PR hardens remote terminal restoration by requiring trusted remote directories and removing captured cwd arguments, but deferred restores can still fall back to persisted snapshot or launch directories when no trusted remote cwd exists. This could start a remote agent in a local or incorrect directory, so the deferred path needs correction or explicit owner acceptance before merge. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 9 files. (2 skipped: 1 unsupported, 1 too large.) Full details: Cmux Swift Package BoundariesExplanation The diff adds Resolution Move Full details: Cmux Architecture RethinkExplanation The PR introduces two remote-agent resume paths with different working-directory policies. The immediate path in Resolution Use one shared remote-agent startup-input path for both immediate and deferred restore. Preserve the selection mode, including exact Full details: Cmux No Ambient Global StateExplanation The diff adds a new static helper to an existing caseless namespace. Resolution Move the resume-command behavior from the static
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
d935d20 to
7cfee7d
Compare
testTypingRepairForwardsKeyDown asserted that the repaired keyDown reached libghostty, but forwarding only happens once the runtime surface is live, and the headless xctest host does not always spin one up. The assertion then fails for a reason that has nothing to do with typing repair. Gate the forward observation on a live surface, matching the constraint the neighbouring tests in this file already respect. This commit previously also carried a fix for remote-workspace restore handing the terminal a local working directory. That fix is manaflow-ai#8634, which restructures the same logic behind a named predicate and gates both paths rather than one, so it is dropped here to avoid two competing changes to the same function.
testTypingRepairForwardsKeyDown asserted that the repaired keyDown reached libghostty, but forwarding only happens once the runtime surface is live, and the headless xctest host does not always spin one up. The assertion then fails for a reason that has nothing to do with typing repair. Gate the forward observation on a live surface, matching the constraint the neighbouring tests in this file already respect. This commit previously also carried a fix for remote-workspace restore handing the terminal a local working directory. That fix is manaflow-ai#8634, which restructures the same logic behind a named predicate and gates both paths rather than one, so it is dropped here to avoid two competing changes to the same function.
Greptile SummaryThis PR fixes a bug in session-restore logic where a remote-workspace terminal would be handed the local machine's working directory (typically
Confidence Score: 5/5Safe to merge — the change is a minimal, targeted fix to a single restore-path condition with no new state, no concurrency changes, and a pre-existing test confirming the correct outcome. The diff is a six-line refactor of scalar boolean logic. Files Needing Attention: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Restore terminal snapshot] --> B{restoresLocalTerminalSurface?}
B -- No / remote surface --> C[localWorkingDirectory = nil]
C --> D[requestedWorkingDirectory = nil]
B -- Yes / local surface --> E{startupHandlesWorkingDirectory?}
E -- No --> F[localWorkingDirectory = savedWorkingDirectory or workingDirectory]
F --> G[requestedWorkingDirectory = localWorkingDirectory]
E -- Yes --> H[localWorkingDirectory = nil]
H --> I[requestedWorkingDirectory = workingDirectory via fallback]
Reviews (2): Last reviewed commit: "Restoring a remote workspace terminal ha..." | Re-trigger Greptile |
7cfee7d to
16d5e0e
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Workspace.swift`:
- Around line 1460-1471: Update the working-directory selection around
restoresLocalTerminalSurface and requestedWorkingDirectory to use
snapshot.terminal?.isRemoteTerminal as the authoritative remote-surface
indicator, rather than inferring local ownership from missing
effectiveRemoteStartupCommand or restoredRemotePTYAttachCommand. Ensure
explicitly remote restored terminals never receive the workspace-local fallback
cwd, and fail closed with nil when no safe local working-directory source
exists.
🪄 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 Plus
Run ID: 43912b14-903f-4df4-81af-b25c0f611789
📒 Files selected for processing (1)
Sources/Workspace.swift
16d5e0e to
b5294c4
Compare
* cmuxTests: gate the keyDown-forward assertion on a live surface testTypingRepairForwardsKeyDown asserted that the repaired keyDown reached libghostty, but forwarding only happens once the runtime surface is live, and the headless xctest host does not always spin one up. The assertion then fails for a reason that has nothing to do with typing repair. Gate the forward observation on a live surface, matching the constraint the neighbouring tests in this file already respect. This commit previously also carried a fix for remote-workspace restore handing the terminal a local working directory. That fix is #8634, which restructures the same logic behind a named predicate and gates both paths rather than one, so it is dropped here to avoid two competing changes to the same function. * cmuxTests: skip the keyDown-forward check visibly instead of silently dropping it The forward assertion was wrapped in `if terminalPanel.surface.hasLiveSurface`, which means on a host without a live libghostty surface the oracle simply disappears and the test still reports green. The comment even said the headless host does not always provide one, so the coverage was not just conditional, it was silently conditional, and nothing distinguished a run that checked the forwarding from a run that did not. XCTSkipUnless says it out loud. The repair-routing assertions above run first and are checked on every host either way, so nothing that was verified before is verified less now. --------- Co-authored-by: ejc3 <ejc3@users.noreply.github.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@greptile-apps review |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
# Conflicts: # .github/workflows/ci.yml # Sources/RestorableAgentSession.swift
… directory Restoring a remote-workspace terminal builds a resume command that cds into this machine's cwd, so the far host is asked to enter a path that only exists locally. Two gaps line up to cause it: - updatePanelDirectory dropped every generic live report on a trust-required remote panel, so a remote-workspace terminal that never got a report through the remote control channel had no recorded directory at all in the session snapshot. Accept the first report into an empty slot; overwrites stay refused and the value gains no remote-report provenance. - On restore, the remote branch of resumeSessionWorkingDirectory fell back to workingDirectory, which degrades to the local cwd whenever remote trust rules blank the saved directory. Use the panel's recorded directory instead: the resume command runs on the remote host, where that path is the right target. The pre-existing regression test AgentSessionAutoResumeSettingsTests.testRemoteWorkspaceAutoResumeKeepsRemoteStartupCommand was failing on main (the resume cd prefix and requestedWorkingDirectory both landed on the local home directory) and now passes, along with the restored panelDirectories assert in TabManagerSessionSnapshotTests.testSessionSnapshotRestoresSSHWorkspaceDescriptor.
…tions removingSavedWorkingDirectoryOptions documents that it preserves content after `--`, but with removeAllWorkingDirectoryOptions the value branch accepted any next token -- including `--` itself. `--work-dir -- --cwd /x prompt` consumed the delimiter as the option's value, and everything after it was then sanitized as options rather than passed through. A cwd option immediately before `--`, or at the very end, has no value to take: drop the option alone. Two cases added, both red without the change.
…me restores a remote terminal
0249baa to
bdcaa45
Compare
Remote (exact) restores removed -w, -C and --workspace for every agent, which dropped Claude's -w/--worktree, Qoder's --workspace profile selector, and attached tokens such as -Continue that main keeps on remote resume. Remove-all now drops an option regardless of value only when its cwd meaning is known for the kind (AgentWorkingDirectoryOptionPolicy, ported from manaflow-ai#9855): --cd/--cwd/--work-dir always; -C for codex; -w for kimi and qoder; --workspace for cursor. Attached short forms follow the same scoping. Custom registrations pass no kind. Value-matched removal is unchanged. Also moves RestorableAgentWorkingDirectorySelection into CMUXAgentLaunch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI failure attributionCI passes on Written by |
Exact remote restores passed no agent kind whenever a registration was present, so Kimi (restored through its exact built-in registration) kept an attached -w/dir cwd option. Use managedProviderKind, which maps kimi and other exact built-in registrations to their kind; user-authored templates never reach this sanitizing branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged, thanks @ejc3! I added one change on top: exact restores now remove only the cwd flags each agent actually owns (codex |
|
Merge receipt for |
c490fb4 Add an opt-in subtle sidebar selection highlight (manaflow-ai#14890) b78dae5 ci: evict parked pull request builds oldest first instead of a free-disk floor (manaflow-ai#15039) 31b6fc9 test: close the TabManagers the session snapshot tests create (manaflow-ai#15038) e64eb93 ci: send every retry of an owned-mini job to Blacksmith, once (manaflow-ai#15035) 17f0f90 Test: pin persisted Kimi custom kind on exact remote restore (manaflow-ai#15034) f9d5bb5 Switch between cmux and cmux NIGHTLY from inside the app (manaflow-ai#14995) 756847a sidebar: keep the selected row clear of the footer (manaflow-ai#15013) 1431a1e One dialog per close, and Feed banner clicks open the agent (manaflow-ai#14960) 74d4719 ci: route cli-product-tests to the gui runners like the shards (manaflow-ai#15011) b78e8aa ci: park each pull request's build on its root and route its re-push back to it (manaflow-ai#15020) 60e45e7 web tests: run every child process asynchronously (manaflow-ai#15001) 55724f0 ci: side lanes take the light minis' side runners first (manaflow-ai#15019) 3be8bdd Restoring a remote workspace terminal hands it this machine's working directory (manaflow-ai#8634) a9e555c Reattach 0.64.25 tmux-profile SSH workspaces after upgrading (manaflow-ai#14938) fe20e20 fix: mark workspace-level notifications read on workspace focus (manaflow-ai#12427) # Conflicts: # .github/workflows/app-host-test-rerun.yml # .github/workflows/auth-refresh-tests.yml # .github/workflows/ci-macos.yml # .github/workflows/ci.yml # .github/workflows/cloud-command-deadlines.yml # .github/workflows/cloud-machine-tests.yml # .github/workflows/cloud-task-local-tests.yml # .github/workflows/cmux-tui.yml # .github/workflows/iroh-v2.yml # .github/workflows/relay-tls.yml # .github/workflows/reload-build.yml # .github/workflows/remote-daemon.yml # .github/workflows/terminal-hang-diagnostics.yml
Summary
CMUX_REMOTE_INITIAL_CWD, which is how main starts remote terminals; the regression tests check that variable.Testing
AgentSessionAutoResumeSwiftTestsandRemoteAgentRestoreWorkingDirectoryTestspass (39 tests), and theCMUXAgentLaunchpackage's suite passes (453 tests).SessionPersistenceTests/testAgentHookSurfaceResumeStartupInputRunsWhenSavedWorkingDirectoryWasDeletedfails on this branch and identically on main in a host where a realcodexis onPATH; it is not affected by this change.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Note
High Risk
Changes session-restore and remote auto-resume cwd trust boundaries and command construction; incorrect behavior could run agents in the wrong directory on a remote host or drop cwd when it should apply.
Overview
Fixes remote workspace terminal restore leaking the local machine's cwd into remote agent resume commands.
Working directory policy: Introduces
RestorableAgentWorkingDirectorySelectionwithrecordedFallbackvsexactmodes. Exact selection stops silent fallback to agent/launch snapshot cwd and forces stripping all captured cwd argv flags (--work-dir, attached-C/w, etc.) viaAgentLaunchSanitizer.removeAllWorkingDirectoryOptions.Remote restore path: When restoring a remote workspace terminal snapshot, resume cwd is chosen only from trusted remote directory provenance (
directoryIsTrustedRemoteReport), thenAgentResumeWorkingDirectoryresolves per agent kind (runtime vs launch namespace). Remote auto-resume uses.exact(...)so untrusted or missing remote cwd cannot reapply recorded local paths.CI: Adds a non-tolerant focused app-host run for
RemoteAgentRestoreWorkingDirectoryTestsso assertion failures on this boundary are not masked by the broad sharded suite's tolerance.Tests: Broad regressions for exact cwd stripping, remote authoritative directory updates, directory-keyed vs id-keyed agents, and untrusted remote panels; hook binding duplicate Kimi
--work-dirdrop.Reviewed by Cursor Bugbot for commit 146a348. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests
Changelog
Fixed: Restoring a remote workspace terminal starts it in the remote directory instead of this Mac's working directory, and keeps agent flags like Claude's
-won resume