fix(cli): route repo add through SSH hostId, not desktop paths - #9920
fix(cli): route repo add through SSH hostId, not desktop paths#9920connfy wants to merge 1 commit into
Conversation
Windows desktop probes were turning remote POSIX paths into C:\home\... and --environment was confused with SSH connectionIds. Accept --host ssh:<id> (and bare connectionIds), validate on the SSH host, and keep absolute remote paths intact for Linux-relay/Windows-desktop imports. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ee54eb7-6ec5-4559-b389-c4525d9385de
📒 Files selected for processing (13)
src/cli/handlers/repo.tssrc/cli/help.tssrc/cli/index.test.tssrc/cli/repo-add-host.test.tssrc/cli/repo-add-host.tssrc/cli/repo-path-arguments.test.tssrc/cli/repo-path-arguments.tssrc/cli/specs/core.tssrc/main/ipc/repos.tssrc/main/repos/add-remote-repo-from-path.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/repo.ts
| const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) | ||
| const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) | ||
| const priorExitCode = process.exitCode | ||
|
|
||
| await main( | ||
| ['repo', 'add', '--path', '/home/me/orca', '--host', 'env:not-a-host', '--json'], | ||
| '/tmp/repo' | ||
| ) | ||
|
|
||
| expect(callMock).not.toHaveBeenCalled() | ||
| expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain( | ||
| '--environment selects a paired Orca server' | ||
| ) | ||
| expect(process.exitCode).toBe(1) | ||
|
|
||
| process.exitCode = priorExitCode |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore process.exitCode in a finally block.
A failed assertion skips Line 2766 and leaks exit code 1 into later tests.
Proposed fix
const priorExitCode = process.exitCode
-
- await main(
- ['repo', 'add', '--path', '/home/me/orca', '--host', 'env:not-a-host', '--json'],
- '/tmp/repo'
- )
-
- expect(callMock).not.toHaveBeenCalled()
- expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain(
- '--environment selects a paired Orca server'
- )
- expect(process.exitCode).toBe(1)
-
- process.exitCode = priorExitCode
+ try {
+ await main(
+ ['repo', 'add', '--path', '/home/me/orca', '--host', 'env:not-a-host', '--json'],
+ '/tmp/repo'
+ )
+
+ expect(callMock).not.toHaveBeenCalled()
+ expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain(
+ '--environment selects a paired Orca server'
+ )
+ expect(process.exitCode).toBe(1)
+ } finally {
+ process.exitCode = priorExitCode
+ }📝 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 logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) | |
| const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) | |
| const priorExitCode = process.exitCode | |
| await main( | |
| ['repo', 'add', '--path', '/home/me/orca', '--host', 'env:not-a-host', '--json'], | |
| '/tmp/repo' | |
| ) | |
| expect(callMock).not.toHaveBeenCalled() | |
| expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain( | |
| '--environment selects a paired Orca server' | |
| ) | |
| expect(process.exitCode).toBe(1) | |
| process.exitCode = priorExitCode | |
| const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) | |
| const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) | |
| const priorExitCode = process.exitCode | |
| try { | |
| await main( | |
| ['repo', 'add', '--path', '/home/me/orca', '--host', 'env:not-a-host', '--json'], | |
| '/tmp/repo' | |
| ) | |
| expect(callMock).not.toHaveBeenCalled() | |
| expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain( | |
| '--environment selects a paired Orca server' | |
| ) | |
| expect(process.exitCode).toBe(1) | |
| } finally { | |
| process.exitCode = priorExitCode | |
| } |
| export async function addRemoteRepoFromPath( | ||
| store: Store, | ||
| args: AddRemoteRepoFromPathArgs | ||
| ): Promise<{ repo: Repo; alreadyExisted: boolean } | { error: string }> { | ||
| const gitProvider = getSshGitProvider(args.connectionId) | ||
| if (!gitProvider) { | ||
| return { error: `SSH connection "${args.connectionId}" not found or not connected` } | ||
| } | ||
|
|
||
| let repoKind: 'git' | 'folder' = args.kind ?? 'git' | ||
| let resolvedPath = await resolveRemoteHomePath(args.connectionId, args.remotePath) | ||
|
|
||
| const existing = store | ||
| .getRepos() | ||
| .find( | ||
| (repo) => | ||
| repo.connectionId === args.connectionId && | ||
| normalizeRuntimePathForComparison(repo.path) === | ||
| normalizeRuntimePathForComparison(resolvedPath) | ||
| ) | ||
| if (existing) { | ||
| return { repo: existing, alreadyExisted: true } | ||
| } | ||
|
|
||
| if (args.kind !== 'folder') { | ||
| try { | ||
| const check = await gitProvider.isGitRepoAsync(resolvedPath) | ||
| if (check.isRepo) { | ||
| repoKind = 'git' | ||
| if (check.rootPath) { | ||
| resolvedPath = check.rootPath | ||
| } | ||
| } else { | ||
| return { error: `Not a valid git repository: ${args.remotePath}` } | ||
| } | ||
| } catch (err) { | ||
| if (err instanceof Error && err.message.includes('Not a valid git repository')) { | ||
| return { error: err.message } | ||
| } | ||
| return { error: `Not a valid git repository: ${args.remotePath}` } | ||
| } | ||
| } | ||
|
|
||
| const existingAfterRootResolve = store | ||
| .getRepos() | ||
| .find( | ||
| (repo) => | ||
| repo.connectionId === args.connectionId && | ||
| normalizeRuntimePathForComparison(repo.path) === | ||
| normalizeRuntimePathForComparison(resolvedPath) | ||
| ) | ||
| if (existingAfterRootResolve) { | ||
| return { repo: existingAfterRootResolve, alreadyExisted: true } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Dedup check is a TOCTOU race for concurrent adds of the same path.
Two await points (resolveRemoteHomePath, isGitRepoAsync, and later detectRepoIconAndUpstream) sit between each dedup lookup and store.addRepo. Two concurrent calls (e.g. CLI invoked twice, or CLI + UI) targeting the same connectionId/path can both pass both dedup checks before either calls addRepo, producing duplicate Repo entries. cloneRemoteRepo in src/main/ipc/repos.ts guards this exact scenario with remoteCloneInFlightByPath; this shared helper (used directly for CLI repo add, no directory-creation atomicity to fall back on) has no equivalent guard.
🔒 Suggested fix: in-flight guard keyed by connectionId+resolved path
+const inFlightByKey = new Set<string>()
+
export async function addRemoteRepoFromPath(
store: Store,
args: AddRemoteRepoFromPathArgs
): Promise<{ repo: Repo; alreadyExisted: boolean } | { error: string }> {
const gitProvider = getSshGitProvider(args.connectionId)
if (!gitProvider) {
return { error: `SSH connection "${args.connectionId}" not found or not connected` }
}
+
+ const lockKey = `${args.connectionId}:${normalizeRuntimePathForComparison(args.remotePath)}`
+ if (inFlightByKey.has(lockKey)) {
+ return { error: 'An add operation is already in progress for this path' }
+ }
+ inFlightByKey.add(lockKey)
+ try {
... existing body ...
+ } finally {
+ inFlightByKey.delete(lockKey)
+ }
}| if (args.kind !== 'folder') { | ||
| try { | ||
| const check = await gitProvider.isGitRepoAsync(resolvedPath) | ||
| if (check.isRepo) { | ||
| repoKind = 'git' | ||
| if (check.rootPath) { | ||
| resolvedPath = check.rootPath | ||
| } | ||
| } else { | ||
| return { error: `Not a valid git repository: ${args.remotePath}` } | ||
| } | ||
| } catch (err) { | ||
| if (err instanceof Error && err.message.includes('Not a valid git repository')) { | ||
| return { error: err.message } | ||
| } | ||
| return { error: `Not a valid git repository: ${args.remotePath}` } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Catch-all swallows the real error behind a generic "not a valid git repository" message.
Any thrown error from isGitRepoAsync (network failure, permission denied, SSH disconnect) that doesn't literally contain the string 'Not a valid git repository' is replaced with a generic message, hiding the actual cause from the user/CLI output.
🐛 Preserve the original error detail
} catch (err) {
if (err instanceof Error && err.message.includes('Not a valid git repository')) {
return { error: err.message }
}
- return { error: `Not a valid git repository: ${args.remotePath}` }
+ const detail = err instanceof Error ? err.message : String(err)
+ return { error: `Not a valid git repository: ${args.remotePath} (${detail})` }
}📝 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.
| if (args.kind !== 'folder') { | |
| try { | |
| const check = await gitProvider.isGitRepoAsync(resolvedPath) | |
| if (check.isRepo) { | |
| repoKind = 'git' | |
| if (check.rootPath) { | |
| resolvedPath = check.rootPath | |
| } | |
| } else { | |
| return { error: `Not a valid git repository: ${args.remotePath}` } | |
| } | |
| } catch (err) { | |
| if (err instanceof Error && err.message.includes('Not a valid git repository')) { | |
| return { error: err.message } | |
| } | |
| return { error: `Not a valid git repository: ${args.remotePath}` } | |
| } | |
| if (args.kind !== 'folder') { | |
| try { | |
| const check = await gitProvider.isGitRepoAsync(resolvedPath) | |
| if (check.isRepo) { | |
| repoKind = 'git' | |
| if (check.rootPath) { | |
| resolvedPath = check.rootPath | |
| } | |
| } else { | |
| return { error: `Not a valid git repository: ${args.remotePath}` } | |
| } | |
| } catch (err) { | |
| if (err instanceof Error && err.message.includes('Not a valid git repository')) { | |
| return { error: err.message } | |
| } | |
| const detail = err instanceof Error ? err.message : String(err) | |
| return { error: `Not a valid git repository: ${args.remotePath} (${detail})` } | |
| } |
| const mux = getActiveMultiplexer(connectionId) | ||
| if (!mux) { | ||
| return path | ||
| } | ||
| try { | ||
| const result = (await mux.request('session.resolveHome', { path })) as { resolvedPath: string } | ||
| return result.resolvedPath | ||
| } catch { | ||
| // Why: older relays may not support this; return the original path so callers surface their own validation error. | ||
| return path | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked cast on the SSH relay response.
mux.request('session.resolveHome', ...) result is force-cast to { resolvedPath: string } without validating the shape. A relay returning an unexpected/malformed payload (without throwing) would silently propagate a non-string value into resolvedPath, breaking downstream normalizeRuntimePathForComparison/getRemoteRepoFolderName calls.
🛡️ Validate the response shape
- const result = (await mux.request('session.resolveHome', { path })) as { resolvedPath: string }
- return result.resolvedPath
+ const result = await mux.request('session.resolveHome', { path })
+ const resolvedPath = (result as { resolvedPath?: unknown } | undefined)?.resolvedPath
+ return typeof resolvedPath === 'string' && resolvedPath ? resolvedPath : path📝 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 mux = getActiveMultiplexer(connectionId) | |
| if (!mux) { | |
| return path | |
| } | |
| try { | |
| const result = (await mux.request('session.resolveHome', { path })) as { resolvedPath: string } | |
| return result.resolvedPath | |
| } catch { | |
| // Why: older relays may not support this; return the original path so callers surface their own validation error. | |
| return path | |
| } | |
| } | |
| const mux = getActiveMultiplexer(connectionId) | |
| if (!mux) { | |
| return path | |
| } | |
| try { | |
| const result = await mux.request('session.resolveHome', { path }) | |
| const resolvedPath = (result as { resolvedPath?: unknown } | undefined)?.resolvedPath | |
| return typeof resolvedPath === 'string' && resolvedPath ? resolvedPath : path | |
| } catch { | |
| // Why: older relays may not support this; return the original path so callers surface their own validation error. | |
| return path | |
| } | |
| } |
| // Local and legacy runtime repos both have null executionHostId/connectionId, so only a runtime host may backfill; local imports leave it untouched. | ||
| const repos: Record<string, unknown>[] = [ | ||
| { | ||
| id: 'repo-local-1', | ||
| path: '/workspace', | ||
| displayName: 'workspace', | ||
| badgeColor: 'blue', | ||
| addedAt: 1, | ||
| kind: 'folder' | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the actual legacy repo shape and returned value.
The comment says legacy repos contain executionHostId/connectionId: null, but the fixture omits both fields. Also, only the stored object is checked; a stamped returned copy would pass. Seed the null fields and assert that repo preserves them as well.
Also applies to: 6644-6649
| it('routes SSH hostId repo.add through the remote connection path without desktop path probing', async () => { | ||
| const isGitRepoAsync = vi.fn().mockResolvedValue({ | ||
| isRepo: true, | ||
| rootPath: '/home/minhun/dev/orca-r11-reconnect' | ||
| }) | ||
| sshGitProviders.set('ssh-1784350275544-cv3c0t', { isGitRepoAsync }) | ||
| const repos: Record<string, unknown>[] = [] | ||
| const runtimeStore = { | ||
| ...store, | ||
| getRepos: () => [...repos] as never, | ||
| addRepo: (repo: Record<string, unknown>) => { | ||
| repos.push(repo) | ||
| }, | ||
| getRepo: (id: string) => repos.find((repo) => repo.id === id) as never, | ||
| getSshTarget: vi.fn().mockReturnValue({ label: 'relay-host' }) | ||
| } | ||
| const runtime = new OrcaRuntimeService(runtimeStore as never) | ||
|
|
||
| const repo = await runtime.addRepo( | ||
| '/home/minhun/dev/orca-r11-reconnect', | ||
| 'git', | ||
| 'ssh:ssh-1784350275544-cv3c0t' | ||
| ) | ||
|
|
||
| expect(isGitRepoAsync).toHaveBeenCalledWith('/home/minhun/dev/orca-r11-reconnect') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify that SSH imports skip desktop probing.
The remote-provider assertion proves the SSH path is used, but it does not prove that the desktop filesystem probe was not performed first. Spy on the local repository/path detector and assert it is not called for this POSIX SSH path.
| const parsedHost = parseExecutionHostId(executionHostId ?? null) | ||
| if (parsedHost?.kind === 'ssh') { | ||
| const result = await addRemoteRepoFromPath(this.store, { | ||
| connectionId: parsedHost.targetId, | ||
| remotePath: path, | ||
| kind | ||
| }) | ||
| if ('error' in result) { | ||
| throw new Error(result.error) | ||
| } | ||
| this.invalidateResolvedWorktreeCache() | ||
| this.invalidateWorktreeScanCacheForRepo(result.repo.id) | ||
| this.notifyReposChanged() | ||
| return this.store.getRepo(result.repo.id) ?? result.repo | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the shared remote repo helper's path validation
fd add-remote-repo-from-path
ast-grep outline src/main/repos/add-remote-repo-from-path.ts --items all 2>/dev/nullRepository: stablyai/orca
Length of output: 1015
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/main/repos/add-remote-repo-from-path.ts | cat -nRepository: stablyai/orca
Length of output: 5433
Validate SSH paths before importing
addRemoteRepoFromPath still accepts relative remotePath values: resolveRemoteHomePath returns non-~/ inputs unchanged, and the helper proceeds to isGitRepoAsync/detectRepoIconAndUpstream without an absolute-path guard. If this branch is meant to import only absolute POSIX paths, add that check here.
3 participants
Summary
orca repo addnow accepts--host ssh:<connectionId>(or a bare connectionId fromrepo list) and validates/imports the path on that SSH host instead of the Windows desktop filesystem.C:\home\...mangling) and clarifies that--environmentis for paired Orca servers, not SSH connectionIds.addRemoteRepoFromPathfor runtime RPC + IPC, with deterministic CLI/runtime regression coverage for the Linux-relay / Windows-desktop ORCH-R12 repro.Test plan
npx vitest run src/cli/repo-add-host.test.ts src/cli/repo-path-arguments.test.ts src/cli/index.test.ts src/main/runtime/orca-runtime.test.ts src/main/ipc/repos-remote.test.ts(1087 passed)orca repo add --path /absolute/remote/repo --host <connectionId> --jsonand confirm the repo registers with thatconnectionId--host, confirm a POSIX/home/...path errors with the--host ssh:<connectionId>hint instead of probingC:\home\...--environment <connectionId>still fails as unknown environment (expected), while--host <connectionId>succeedsMade with Cursor
ELI5
orca repo add with an SSH host used to resolve paths on your desktop filesystem and mangle Linux paths. It now validates and imports on the SSH host itself, keeping remote absolute paths intact.