[codex] fix remote agent ssh working directories#1119
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds tilde-aware path escaping ( ChangesSSH Remote Working Directory Override
Sequence Diagram(s)sequenceDiagram
participant User
participant EditAgentModal
participant Handler as process:spawn<br/>Handler
participant SshSpawn as wrapSpawnWithSsh
participant Builder as buildSshCommandWithStdin
User->>EditAgentModal: Enter remote working dir
EditAgentModal->>EditAgentModal: Trim & validate input
EditAgentModal->>Handler: Save with workingDirOverride
Handler->>Handler: Compute remoteCwd from override
Handler->>SshSpawn: Pass remoteArgs, remoteCwd
SshSpawn->>Builder: Build SSH command with remoteCwd
Builder->>Builder: Apply shellEscapeRemotePath
Builder->>SshSpawn: Return sshStdinScript with $HOME expansion
SshSpawn->>Handler: Return SSH-wrapped spawn config
Handler->>Handler: Execute via SSH with remote cd
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 👍 / 👎.
| // 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 👍 / 👎.
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:spawnworking-directory handling so remote commands/cdscripts use the resolved SSH working directory (with correct~/$HOMEexpansion) and avoid leaking the localcwd.-C/--cwd=<path>behavior to preserve remote semantics.Tests