[codex] fix remote agent ssh working directories#1119
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSSH remote working-directory overrides now flow through renderer forms, session save/reset logic, process spawning, SSH wrapper normalization, command building, and file-tree root selection. Tests were added or updated across the same paths. ChangesSSH remote working directory flow
Sequence Diagram(s)sequenceDiagram
participant EditAgentModal
participant HandleSaveEditAgent
participant ProcessSpawn
participant WrapSpawnWithSsh
participant BuildSshCommandWithStdin
EditAgentModal->>HandleSaveEditAgent: Save SSH workingDirOverride
HandleSaveEditAgent->>ProcessSpawn: Session state with remoteCwd
ProcessSpawn->>WrapSpawnWithSsh: Spawn config and SSH settings
WrapSpawnWithSsh->>BuildSshCommandWithStdin: remoteArgs and remoteCwd
BuildSshCommandWithStdin->>ProcessSpawn: sshStdinScript with remote cd
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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 |
jtalborough
force-pushed
the
codex/fix-ssh-remote-cwd
branch
from
8ac3b0b to
f5cacd9
Compare
jtalborough
force-pushed
the
codex/fix-ssh-remote-cwd
branch
from
f5cacd9 to
fbba3d1
Compare
![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
🧹 Nitpick comments (1)
src/main/utils/ssh-spawn-wrapper.ts (1)
20-29: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHandle inline working-dir flag forms in SSH arg normalization.
normalizeSshWorkingDirArgs()only rewrites two-token forms (e.g.,--cwd /path). Inline forms like--cwd=/pathor--working-directory=/pathbypass this and can still leak local paths into remote commands.Suggested patch
const WORKING_DIR_VALUE_FLAGS = new Set(['-C', '--cwd', '--working-directory', '--directory']); +const WORKING_DIR_INLINE_PREFIXES = ['--cwd=', '--working-directory=', '--directory=']; function normalizeSshWorkingDirArgs(args: string[], localCwd: string): string[] { return args.map((arg, index) => { if (index > 0 && WORKING_DIR_VALUE_FLAGS.has(args[index - 1]) && arg === localCwd) { return '.'; } + for (const prefix of WORKING_DIR_INLINE_PREFIXES) { + if (arg.startsWith(prefix) && arg.slice(prefix.length) === localCwd) { + return `${prefix}.`; + } + } return arg; }); }🤖 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 `@src/main/utils/ssh-spawn-wrapper.ts` around lines 20 - 29, The normalizeSshWorkingDirArgs function currently only handles two-token forms (flag and value as separate arguments) but misses inline forms where the flag and value are combined with an equals sign (e.g., --cwd=/path). Update the function to also detect inline flag-value pairs by checking if the current argument contains one of the WORKING_DIR_VALUE_FLAGS followed by an equals sign, extract the path portion after the equals sign, and if it matches localCwd, replace it with a dot while preserving the flag portion of the argument.
🤖 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 `@src/__tests__/main/ipc/handlers/process.test.ts`:
- Around line 2181-2184: The test assertion for the SSH stdin script is checking
for the old behavior where tilde paths remain as written (cd '~/git-projects'),
but the SSH command builder has been updated to expand tilde to the $HOME
environment variable. Update the .toContain() assertion that checks
spawnCall.sshStdinScript to expect the new format where ~/git-projects is
rendered as "$HOME"/git-projects instead of the literal tilde path.
In `@src/main/ipc/handlers/process.ts`:
- Around line 920-933: The code silently falls through when getSshRemoteConfig
returns a null config, allowing the spawn to run locally despite SSH being
explicitly enabled at the session level. After calling getSshRemoteConfig with
the sshStoreAdapter and sessionSshRemoteConfig parameters, add a validation
check to ensure sshResult.config is not null. If the config is null while SSH is
enabled (isSshRemoteSpawn is true and sessionSshRemoteConfig is present), throw
an error or log a fatal error and exit the process to fail loudly as per the
coding guidelines, rather than silently falling back to local execution.
---
Nitpick comments:
In `@src/main/utils/ssh-spawn-wrapper.ts`:
- Around line 20-29: The normalizeSshWorkingDirArgs function currently only
handles two-token forms (flag and value as separate arguments) but misses inline
forms where the flag and value are combined with an equals sign (e.g.,
--cwd=/path). Update the function to also detect inline flag-value pairs by
checking if the current argument contains one of the WORKING_DIR_VALUE_FLAGS
followed by an equals sign, extract the path portion after the equals sign, and
if it matches localCwd, replace it with a dot while preserving the flag portion
of the argument.
🪄 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: CHILL
Plan: Pro
Run ID: 83988e9c-727d-463c-9c2d-7f06913443ac
📒 Files selected for processing (12)
src/__tests__/main/ipc/handlers/process.test.tssrc/__tests__/main/utils/ssh-command-builder.test.tssrc/__tests__/main/utils/ssh-spawn-wrapper.test.tssrc/__tests__/renderer/components/NewInstanceModal.test.tsxsrc/__tests__/renderer/components/NewInstanceModal/EditAgentModal.test.tsxsrc/__tests__/renderer/hooks/useFileTreeManagement.test.tssrc/main/ipc/handlers/process.tssrc/main/utils/ssh-command-builder.tssrc/main/utils/ssh-spawn-wrapper.tssrc/renderer/components/NewInstanceModal/EditAgentModal.tsxsrc/renderer/components/NewInstanceModal/NewInstanceModal.tsxsrc/renderer/hooks/git/useFileTreeManagement.ts
Greptile SummaryThis PR fixes SSH remote-agent working directory handling across the spawn IPC handler, the generic spawn wrapper, and the new/edit agent UI, so that
Confidence Score: 3/5The spawn-path and builder fixes are correct and well-tested, but the new editable Remote Working Directory field in EditAgentModal introduces a save path where clearing the input silently persists the locally-expanded projectRoot as workingDirOverride, causing every subsequent SSH agent spawn to fail with a cd error on the remote host. The IPC handler and ssh-command-builder changes are solid. The EditAgentModal save logic has a gap: clearing the remote dir field produces a blank workingDirOverride that silently falls back to session.projectRoot (a locally-expanded path), which breaks the next agent spawn on the remote. Until that path is guarded or the empty-field case is handled explicitly, a user editing an SSH agent could unknowingly put it in a broken state with no warning in the UI. src/renderer/components/NewInstanceModal/EditAgentModal.tsx — the handleSave fallback from empty workingDirOverride to session.projectRoot needs attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as NewInstanceModal / EditAgentModal
participant IPC as process:spawn handler
participant Wrapper as wrapSpawnWithSsh
participant Builder as ssh-command-builder
participant SSH as SSH process
UI->>IPC: "spawn cwd=localPath workingDirOverride=~/git-projects"
IPC->>IPC: "agentWorkingDirArgCwd = dot (SSH spawn)"
IPC->>IPC: "buildAgentArgs with cwd=dot produces args with -C dot"
IPC->>IPC: "remoteCwd = workingDirOverride.trim() OR config.cwd"
IPC->>Builder: "buildSshCommandWithStdin cwd=remoteCwd"
Builder->>Builder: shellEscapeRemotePath produces dollar HOME slash git-projects
Builder-->>IPC: stdinScript with cd dollar HOME git-projects OR exit 1
IPC->>SSH: spawn ssh args stdinScript
SSH->>SSH: bash -s reads stdinScript expands HOME executes agent
Note over UI,IPC: wrapSpawnWithSsh path used by Group Chat etc.
UI->>Wrapper: "wrapSpawnWithSsh args include -C localPath workingDirOverride=/remote/path"
Wrapper->>Wrapper: normalizeSshWorkingDirArgs replaces localPath with dot
Wrapper->>Wrapper: "remoteCwd = workingDirOverride"
Wrapper->>Builder: "buildSshCommand cwd=remoteCwd args include -C dot"
Builder-->>Wrapper: SSH command with cd /remote/path
Wrapper-->>UI: wrapped spawn config
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as NewInstanceModal / EditAgentModal
participant IPC as process:spawn handler
participant Wrapper as wrapSpawnWithSsh
participant Builder as ssh-command-builder
participant SSH as SSH process
UI->>IPC: "spawn cwd=localPath workingDirOverride=~/git-projects"
IPC->>IPC: "agentWorkingDirArgCwd = dot (SSH spawn)"
IPC->>IPC: "buildAgentArgs with cwd=dot produces args with -C dot"
IPC->>IPC: "remoteCwd = workingDirOverride.trim() OR config.cwd"
IPC->>Builder: "buildSshCommandWithStdin cwd=remoteCwd"
Builder->>Builder: shellEscapeRemotePath produces dollar HOME slash git-projects
Builder-->>IPC: stdinScript with cd dollar HOME git-projects OR exit 1
IPC->>SSH: spawn ssh args stdinScript
SSH->>SSH: bash -s reads stdinScript expands HOME executes agent
Note over UI,IPC: wrapSpawnWithSsh path used by Group Chat etc.
UI->>Wrapper: "wrapSpawnWithSsh args include -C localPath workingDirOverride=/remote/path"
Wrapper->>Wrapper: normalizeSshWorkingDirArgs replaces localPath with dot
Wrapper->>Wrapper: "remoteCwd = workingDirOverride"
Wrapper->>Builder: "buildSshCommand cwd=remoteCwd args include -C dot"
Builder-->>Wrapper: SSH command with cd /remote/path
Wrapper-->>UI: wrapped spawn config
Reviews (1): Last reviewed commit: "fix remote agent ssh working directories" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| workingDirOverride: | ||
| sshRemoteConfig.workingDirOverride || session?.projectRoot || undefined, | ||
| sshRemoteConfig.workingDirOverride?.trim() || session?.projectRoot || undefined, |
There was a problem hiding this comment.
When a user opens this modal for an SSH session, edits the Remote Working Directory field, and then clears it, setSshRemoteConfig stores workingDirOverride: ''. At save time ''.trim() is '' (falsy), so the expression falls through to session?.projectRoot. For any session created with a tilde path such as ~/git-projects, session.projectRoot holds the locally-expanded value (e.g., /Users/jta/git-projects). That path is then stored as workingDirOverride, and on the next spawn the SSH script runs cd '/Users/jta/git-projects' || exit 1 on the remote host, which exits 1 and kills the agent.
Consider treating an empty field as a validation error (disable Save) or rejecting an empty submission rather than silently saving with the wrong path.
There was a problem hiding this comment.
Addressed in ecf0c7690. EditAgentModal now treats an empty SSH remote working directory as invalid, disables Save, shows Remote working directory is required, and handleSave returns before calling onSave. When valid, it stores the trimmed remote directory directly instead of falling back to session.projectRoot, so clearing the field cannot persist a local Mac path. Added regression coverage in EditAgentModal.test.tsx; validation passed with focused tests, npm run lint, npm run lint:eslint, and git diff --check.
| }); | ||
| mockProcessManager.spawn.mockReturnValue({ pid: 12345, success: true }); | ||
|
|
||
| const handler = handlers.get('process:spawn'); |
There was a problem hiding this comment.
shellEscapeRemotePath output
The buildSshCommandWithStdin mock (line 113) generates cd '${remoteOptions.cwd}' || exit 1 — bare single-quoted path — rather than calling shellEscapeRemotePath. For a tilde path the real implementation produces cd "$HOME"/'git-projects' || exit 1, not cd '~/git-projects' || exit 1. This test verifies that the handler correctly routes workingDirOverride to the builder (good), but the assertion as written would still pass even if shellEscapeRemotePath were removed or broken. Tilde-expansion correctness is already covered in ssh-command-builder.test.ts.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed in ecf0c7690. The buildSshCommandWithStdin mock now matches the real shellEscapeRemotePath tilde output for ~/git-projects, and the assertion expects cd "$HOME"/'git-projects' || exit 1. The production tilde contract remains covered in ssh-command-builder.test.ts; this handler test now verifies routing while staying faithful to the builder output.
|
@jtalborough thanks for the contribution, this is a clean, well-scoped fix for a genuinely annoying SSH remote-cwd bug. I reviewed the PR alongside the Greptile and CodeRabbit reviews, and verified each finding against the current code rather than just the addressed-in replies:
I also ran the focused suite locally ( Happy with this. Approving and tagging |
|
@codex review please |
|
In the mean time @jtalborough please retarget to rc branch. We give everything cook time in RC before we merge to main to avoid any wide spread issues |
![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: 48c4f82856
ℹ️ 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".
| // projectRoot (which is the remote path the user originally configured). | ||
| workingDirOverride: | ||
| sshRemoteConfig.workingDirOverride || session?.projectRoot || undefined, | ||
| workingDirOverride: trimmedRemoteWorkingDir, |
There was a problem hiding this comment.
When an SSH agent has syncHistory enabled and the user changes this new Remote Working Directory field, saving only updates sessionSshRemoteConfig.workingDirOverride; handleSaveEditAgent leaves session.cwd unchanged, and buildSharedHistoryContext() still uses session.cwd as the remote history path. The next turn will run the agent in trimmedRemoteWorkingDir, but .maestro/history is read and written under the old cwd, so synced history disappears or goes to the wrong remote project after an edit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in affc58f62. When an enabled SSH agent saves a changed workingDirOverride, handleSaveEditAgent now updates cwd, fullPath, and remoteCwd to the saved remote cwd. That keeps buildSharedHistoryContext() pointed at the same remote directory the agent will run in, including syncHistory sessions. Added lifecycle regression coverage for this path.
| // projectRoot (which is the remote path the user originally configured). | ||
| workingDirOverride: | ||
| sshRemoteConfig.workingDirOverride || session?.projectRoot || undefined, | ||
| workingDirOverride: trimmedRemoteWorkingDir, |
There was a problem hiding this comment.
When an SSH session already has a loaded file tree or git refs, changing this new remote working directory only saves the override. The existing fileTree, fileTreeStats, isGitRepo, branches, and tags stay attached to the session, and useFileTreeManagement skips its initial load once fileTreeStats is present, so the Files/Git UI keeps showing the old remote directory until a manual refresh or restart. Clear those cached fields or trigger a refresh when the saved remote cwd changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in affc58f62. The same SSH remote cwd change path now clears stale project state: fileTree, file-tree stats/loading/error/retry fields, expanded/scroll state, git repo status, cached branches, cached tags, and git ref cache time. It also clears stale runtime SSH identity so file/git operations re-resolve from sessionSshRemoteConfig. Added lifecycle regression coverage for the cache reset.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: affc58f623
ℹ️ 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".
| const sshProjectContextChanged = | ||
| !!nextRemoteCwd && | ||
| (nextRemoteCwd !== currentSavedRemoteCwd || | ||
| nextRemoteCwd !== s.cwd || | ||
| sessionSshRemoteConfig?.remoteId !== s.sessionSshRemoteConfig?.remoteId); |
There was a problem hiding this comment.
When an SSH agent whose cwd was moved by this new edit path is later switched to Local Execution, nextRemoteCwd is undefined, so sshProjectContextChanged is false and the reset block below never runs. That leaves the remote cwd/remoteCwd and any existing sshRemoteId on the session while sessionSshRemoteConfig.enabled is false; the next local spawn uses a remote-only cwd and file/git helpers can still dispatch through the stale remote id. Clear the SSH fields and restore a local cwd, or prevent disabling without choosing a local path.
Useful? React with 👍 / 👎.
| // Use it as workingDirOverride so SSH terminals cd to the right place. | ||
| workingDirOverride: | ||
| sshRemoteConfig.workingDirOverride || expandedWorkingDir || undefined, | ||
| sshRemoteConfig.workingDirOverride?.trim() || remoteWorkingDir || undefined, |
There was a problem hiding this comment.
When creating an SSH agent with syncHistory enabled and a home-relative remote path such as ~/repo, this line now preserves the raw remote path in workingDirOverride, but onCreate below still receives expandedWorkingDir for session.cwd. The agent spawn uses the override, while buildSharedHistoryContext() uses session.cwd, so shared history is read and written under the locally expanded path on the remote instead of the project the agent actually runs in. Pass the remote path through as the session cwd for SSH creation or make shared history use the override.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function getTreeRoot(session: Session, sshContext?: SshContext): string { | ||
| return sshContext?.remoteCwd || session.projectRoot || session.cwd; |
There was a problem hiding this comment.
For an SSH agent, session.remoteCwd is also updated when the user runs cd in the Command Terminal, so prioritizing it here makes the Files panel scan whatever terminal directory was last visited instead of the configured project root. For example, after cd /tmp, the next refresh loads /tmp over SSH and the project files disappear until another cwd change resets remoteCwd. Use the configured working directory for the tree root, not the mutable terminal cwd.
Useful? React with 👍 / 👎.
| Object.assign(updatedFields, { | ||
| cwd: nextRemoteCwd, | ||
| fullPath: nextRemoteCwd, | ||
| remoteCwd: nextRemoteCwd, |
There was a problem hiding this comment.
When this block moves an SSH agent to a new remote working directory, it updates cwd and remoteCwd but leaves autoRunFolderPath pointing at the old project. The Right Bar passes that saved folder together with the SSH remote id, so after a remote cwd edit the Auto Run panel continues reading and writing playbooks in the previous remote project instead of the directory the agent now runs in. Update or clear the Auto Run folder path when the project context changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/renderer/hooks/useSessionLifecycle.test.ts (1)
195-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for SSH disable /
remoteId: nulltransitions.Line 195 currently covers only “SSH stays enabled + workingDir changes”. Please add a case where SSH is turned off (or
remoteIdis cleared) to assert SSH/file-tree/git state is also reset.🤖 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 `@src/__tests__/renderer/hooks/useSessionLifecycle.test.ts` around lines 195 - 291, Add a regression test in useSessionLifecycle.test.ts for the SSH-disable transition by covering handleSaveEditAgent when the sessionSshRemoteConfig switches from enabled to disabled or remoteId becomes null. Reuse createMockSession, useSessionStore, and useSessionLifecycle to set up an SSH session, then assert the updated session clears sshRemote and sshRemoteId, resets cwd/fullPath/remoteCwd appropriately, and wipes stale fileTree, explorer, and git cache fields just like the existing SSH edit test does for handleSaveEditAgent.
🤖 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 `@src/renderer/hooks/session/useSessionLifecycle.ts`:
- Around line 192-221: The SSH cleanup in useSessionLifecycle should not depend
on nextRemoteCwd being truthy, because disabling SSH or clearing remoteId can
leave stale session state. Update the sshProjectContextChanged check and the
surrounding reset logic so it also runs when sessionSshRemoteConfig changes to
null/disabled or its remoteId changes, and ensure the updatedFields reset branch
still clears sshRemote, sshRemoteId, and related file tree state in those
transitions.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useSessionLifecycle.test.ts`:
- Around line 195-291: Add a regression test in useSessionLifecycle.test.ts for
the SSH-disable transition by covering handleSaveEditAgent when the
sessionSshRemoteConfig switches from enabled to disabled or remoteId becomes
null. Reuse createMockSession, useSessionStore, and useSessionLifecycle to set
up an SSH session, then assert the updated session clears sshRemote and
sshRemoteId, resets cwd/fullPath/remoteCwd appropriately, and wipes stale
fileTree, explorer, and git cache fields just like the existing SSH edit test
does for handleSaveEditAgent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca7eaf71-6d4b-4960-aa89-4727a283ff13
📒 Files selected for processing (3)
src/__tests__/main/utils/ssh-command-builder.test.tssrc/__tests__/renderer/hooks/useSessionLifecycle.test.tssrc/renderer/hooks/session/useSessionLifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/main/utils/ssh-command-builder.test.ts
| const sshProjectContextChanged = | ||
| !!nextRemoteCwd && | ||
| (nextRemoteCwd !== currentSavedRemoteCwd || | ||
| nextRemoteCwd !== s.cwd || | ||
| sessionSshRemoteConfig?.remoteId !== s.sessionSshRemoteConfig?.remoteId); | ||
|
|
||
| if (sshProjectContextChanged) { | ||
| Object.assign(updatedFields, { | ||
| cwd: nextRemoteCwd, | ||
| fullPath: nextRemoteCwd, | ||
| remoteCwd: nextRemoteCwd, | ||
| sshRemote: undefined, | ||
| sshRemoteId: undefined, | ||
| fileTree: [], | ||
| fileExplorerExpanded: [], | ||
| fileExplorerScrollPos: 0, | ||
| fileTreeStats: undefined, | ||
| fileTreeTruncated: undefined, | ||
| fileTreeLoadedCap: undefined, | ||
| fileTreeLoading: undefined, | ||
| fileTreeLoadingProgress: undefined, | ||
| fileTreeLastScanTime: undefined, | ||
| fileTreeError: undefined, | ||
| fileTreeRetryAt: undefined, | ||
| isGitRepo: false, | ||
| gitBranches: undefined, | ||
| gitTags: undefined, | ||
| gitRefsCacheTime: undefined, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SSH context reset is skipped when SSH is disabled or remoteId is cleared.
Line 193 gates the reset on !!nextRemoteCwd, so transitions like enabled: true -> false never enter the cache/SSH cleanup branch, leaving stale remote session state.
Suggested fix
-const sshProjectContextChanged =
- !!nextRemoteCwd &&
- (nextRemoteCwd !== currentSavedRemoteCwd ||
- nextRemoteCwd !== s.cwd ||
- sessionSshRemoteConfig?.remoteId !== s.sessionSshRemoteConfig?.remoteId);
+const nextSshEnabled = !!(sessionSshRemoteConfig?.enabled && sessionSshRemoteConfig.remoteId);
+const prevSshEnabled = !!(s.sessionSshRemoteConfig?.enabled && s.sessionSshRemoteConfig.remoteId);
+const sshProjectContextChanged =
+ nextSshEnabled !== prevSshEnabled ||
+ sessionSshRemoteConfig?.remoteId !== s.sessionSshRemoteConfig?.remoteId ||
+ nextRemoteCwd !== currentSavedRemoteCwd ||
+ nextRemoteCwd !== s.cwd;
if (sshProjectContextChanged) {
Object.assign(updatedFields, {
- cwd: nextRemoteCwd,
- fullPath: nextRemoteCwd,
- remoteCwd: nextRemoteCwd,
+ ...(nextRemoteCwd
+ ? { cwd: nextRemoteCwd, fullPath: nextRemoteCwd, remoteCwd: nextRemoteCwd }
+ : { remoteCwd: undefined }),
sshRemote: undefined,
sshRemoteId: undefined,
fileTree: [],📝 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.
| const sshProjectContextChanged = | |
| !!nextRemoteCwd && | |
| (nextRemoteCwd !== currentSavedRemoteCwd || | |
| nextRemoteCwd !== s.cwd || | |
| sessionSshRemoteConfig?.remoteId !== s.sessionSshRemoteConfig?.remoteId); | |
| if (sshProjectContextChanged) { | |
| Object.assign(updatedFields, { | |
| cwd: nextRemoteCwd, | |
| fullPath: nextRemoteCwd, | |
| remoteCwd: nextRemoteCwd, | |
| sshRemote: undefined, | |
| sshRemoteId: undefined, | |
| fileTree: [], | |
| fileExplorerExpanded: [], | |
| fileExplorerScrollPos: 0, | |
| fileTreeStats: undefined, | |
| fileTreeTruncated: undefined, | |
| fileTreeLoadedCap: undefined, | |
| fileTreeLoading: undefined, | |
| fileTreeLoadingProgress: undefined, | |
| fileTreeLastScanTime: undefined, | |
| fileTreeError: undefined, | |
| fileTreeRetryAt: undefined, | |
| isGitRepo: false, | |
| gitBranches: undefined, | |
| gitTags: undefined, | |
| gitRefsCacheTime: undefined, | |
| }); | |
| } | |
| const nextSshEnabled = !!(sessionSshRemoteConfig?.enabled && sessionSshRemoteConfig.remoteId); | |
| const prevSshEnabled = !!(s.sessionSshRemoteConfig?.enabled && s.sessionSshRemoteConfig.remoteId); | |
| const sshProjectContextChanged = | |
| nextSshEnabled !== prevSshEnabled || | |
| sessionSshRemoteConfig?.remoteId !== s.sessionSshRemoteConfig?.remoteId || | |
| nextRemoteCwd !== currentSavedRemoteCwd || | |
| nextRemoteCwd !== s.cwd; | |
| if (sshProjectContextChanged) { | |
| Object.assign(updatedFields, { | |
| ...(nextRemoteCwd | |
| ? { cwd: nextRemoteCwd, fullPath: nextRemoteCwd, remoteCwd: nextRemoteCwd } | |
| : { remoteCwd: undefined }), | |
| sshRemote: undefined, | |
| sshRemoteId: undefined, | |
| fileTree: [], | |
| fileExplorerExpanded: [], | |
| fileExplorerScrollPos: 0, | |
| fileTreeStats: undefined, | |
| fileTreeTruncated: undefined, | |
| fileTreeLoadedCap: undefined, | |
| fileTreeLoading: undefined, | |
| fileTreeLoadingProgress: undefined, | |
| fileTreeLastScanTime: undefined, | |
| fileTreeError: undefined, | |
| fileTreeRetryAt: undefined, | |
| isGitRepo: false, | |
| gitBranches: undefined, | |
| gitTags: undefined, | |
| gitRefsCacheTime: undefined, | |
| }); | |
| } |
🤖 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 `@src/renderer/hooks/session/useSessionLifecycle.ts` around lines 192 - 221,
The SSH cleanup in useSessionLifecycle should not depend on nextRemoteCwd being
truthy, because disabling SSH or clearing remoteId can leave stale session
state. Update the sshProjectContextChanged check and the surrounding reset logic
so it also runs when sessionSshRemoteConfig changes to null/disabled or its
remoteId changes, and ensure the updatedFields reset branch still clears
sshRemote, sshRemoteId, and related file tree state in those transitions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2c31ca1a5
ℹ️ 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".
| const isSshRemoteSpawn = | ||
| config.toolType !== 'terminal' && !!config.sessionSshRemoteConfig?.enabled; | ||
| const agentWorkingDirArgCwd = isSshRemoteSpawn ? '.' : config.cwd; |
There was a problem hiding this comment.
This fixes the main process:spawn path by passing . into agent working-directory args for SSH, but the automatic tab-naming SSH path still builds args with config.cwd before wrapping, as checked in src/main/ipc/handlers/tabNaming.ts. For SSH Codex or Factory Droid agents whose workingDirOverride is ~/repo but cwd is the locally expanded /Users/..., chat turns now run in the right remote directory while tab naming still invokes the remote agent with -C /Users/... or --cwd /Users/..., so naming fails or runs against the wrong remote path.
Useful? React with 👍 / 👎.
3 participants
Summary
Fixes SSH remote-agent working directory handling so remote agents use the configured remote cwd instead of a locally expanded path. This covers both the generic SSH spawn wrapper and the desktop
process:spawnSSH path, plus the new/edit agent UI paths that persist remote cwd overrides.Root Cause
The desktop spawn handler still passed the local
cwdinto the SSH command builder even whensessionSshRemoteConfig.workingDirOverridewas set. New remote-agent creation also stored the locally expanded working directory asworkingDirOverride, so inputs like~/git-projectscould become/Users/.../git-projectsand fail on the remote host.Changes
workingDirOverrideinprocess:spawnandwrapSpawnWithSsh.~/...paths through$HOMEwhen building SSH commands.EditAgentModal.Validation
git diff --checknpm test -- NewInstanceModal.test.tsx process.test.ts ssh-command-builder.test.ts ssh-spawn-wrapper.test.tsnpm run build:main && npm run build:preload && npm run build:renderer && npm run build:cliNotes
npm run build:rendererstill emits the existing Vite CSS/chunk warnings; the build exits successfully.Summary by CodeRabbit
New Features
~) paths.Bug Fixes
process:spawnso remote execution uses the resolved SSH working directory (correct~/$HOMEexpansion) and avoids leaking the localcwd.-C/--cwd=<path>handling to preserve directory semantics (.when appropriate).Tests