Repository navigation
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⛔ Files ignored due to path filters (1)
⚙️ Run configuration
⛔ Files ignored due to path filters (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Paired clients can see an incorrect filename when the last listed file ends in a space. This is a narrow issue with a localized fix; the change is otherwise mergeable with owner awareness. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 19 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
-
-
-
Check name Status Explanation Title check The title clearly identifies the primary change: capability-negotiated SSH host management for paired clients. It is specific and concise. Description check The description covers the required sections, including the user impact, implementation, rationale, linked issues, testing, visual proof, compatibility notes, and checklist. It clearly notes that manu… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
-
-
✨ Finishing Touches 💡 2
-
-
-
-
📝 Generate docstrings 💡
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
🛠️ Fix failing CI checks 💡
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9e664587-889f-4d7f-a7b7-262a773a755e
⛔ Files ignored due to path filters (1)
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (19)
src/main/ipc/repos/repos-changed-notification.tssrc/main/ipc/ssh-browse.tssrc/main/ipc/ssh-target-crud-handlers-ownership.test.tssrc/main/ipc/ssh-target-crud-handlers.tssrc/main/ipc/ssh.tssrc/main/runtime/rpc/methods/repo.test.tssrc/main/runtime/rpc/methods/repo.tssrc/main/runtime/rpc/methods/ssh-target-management.test.tssrc/main/runtime/rpc/methods/ssh.tssrc/main/runtime/runtime-project-host-setup-controller.test.tssrc/main/runtime/runtime-project-host-setup-controller.tssrc/main/runtime/runtime-repository-command-surface.tssrc/main/ssh/ssh-remote-directory-browse.tssrc/main/ssh/ssh-target-registry.tssrc/renderer/src/runtime/runtime-ssh-target-management.test.tssrc/renderer/src/runtime/runtime-ssh-target-management.tssrc/shared/protocol-version.tssrc/shared/rpc-contract/ssh-params.tssrc/shared/ssh-target-management.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| const lines = stdout.trim().split(/\r?\n/) | ||
| if (lines.length === 0) { | ||
| rejectOnce(new Error('Empty response from remote')) | ||
| return | ||
| } | ||
|
|
||
| const resolvedPath = lines[0] | ||
| const entries: RemoteDirEntry[] = [] | ||
|
|
||
| for (let i = 1; i < lines.length; i++) { | ||
| const line = lines[i] | ||
| if (!line || line === './' || line === '../') { | ||
| continue | ||
| } | ||
| if (line.endsWith('/')) { | ||
| entries.push({ name: line.slice(0, -1), isDirectory: true }) | ||
| } else { | ||
| entries.push({ name: line, isDirectory: false }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
git rev-parse --verify eaf5dbed9e5fec6046eb8b4835303c40125b8012
git show eaf5dbed9e5fec6046eb8b4835303c40125b8012:src/main/ipc/ssh-browse.ts | rg -n -C 5 'trim\(|split\(|readStdout|sortDirEntries'
git diff eaf5dbed9e5fec6046eb8b4835303c40125b8012 7fa838a866e29bfed5f5b0e1ef9f3f3c9301ac2a -- src/main/ipc/ssh-browse.ts src/main/ssh/ssh-remote-directory-browse.tsRepository: stablyai/orca
Length of output: 24495
🏁 Script executed:
set -eu
printf '%s\n' '--- head RPC browse references ---'
rg -n -F --glob '*.ts' -- 'browseSshDirectory' src/main src/renderer || test "$?" -eq 1
printf '%s\n' '--- head ssh RPC method context ---'
git show 7fa838a866e29bfed5f5b0e1ef9f3f3c9301ac2a:src/main/runtime/rpc/methods/ssh.ts | nl -ba | sed -n '145,185p'
printf '%s\n' '--- merge-base ssh RPC browse references ---'
git grep -n -F 'browseSshDirectory' eaf5dbed9e5fec6046eb8b4835303c40125b8012 -- src/main src/renderer || test "$?" -eq 1
printf '%s\n' '--- RPC diff for browse-related additions ---'
git diff --unified=12 eaf5dbed9e5fec6046eb8b4835303c40125b8012 7fa838a866e29bfed5f5b0e1ef9f3f3c9301ac2 -- src/main/runtime/rpc/methods/ssh.ts src/main/runtime/rpcRepository: stablyai/orca
Length of output: 26711
Preserve trailing spaces in RPC browse results.
The extracted parser already used stdout.trim(), but this PR adds ssh.browseDir, which exposes the parser to paired clients. If the final POSIX entry is a regular file named file , trimming changes it to file. The RPC response then contains and displays the wrong filename.
Suggested fix
--- "a/src/main/ssh/ssh-remote-directory-browse.ts"
+++ "b/src/main/ssh/ssh-remote-directory-browse.ts"
@@ -198,7 +198,10 @@
}
// Why: Windows OpenSSH exec emits CRLF; split on \r?\n so a trailing \r doesn't defeat the endsWith('/') dir check or leave a stray CR in names.
- const lines = stdout.trim().split(/\r?\n/)
+ const lines = stdout.split(/\r?\n/)
+ while (lines.length > 0 && lines[lines.length - 1] === '') {
+ lines.pop()
+ }
if (lines.length === 0) {
rejectOnce(new Error('Empty response from remote'))
return📝 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 lines = stdout.trim().split(/\r?\n/) | |
| if (lines.length === 0) { | |
| rejectOnce(new Error('Empty response from remote')) | |
| return | |
| } | |
| const resolvedPath = lines[0] | |
| const entries: RemoteDirEntry[] = [] | |
| for (let i = 1; i < lines.length; i++) { | |
| const line = lines[i] | |
| if (!line || line === './' || line === '../') { | |
| continue | |
| } | |
| if (line.endsWith('/')) { | |
| entries.push({ name: line.slice(0, -1), isDirectory: true }) | |
| } else { | |
| entries.push({ name: line, isDirectory: false }) | |
| const lines = stdout.split(/\r?\n/) | |
| while (lines.length > 0 && lines[lines.length - 1] === '') { | |
| lines.pop() | |
| } | |
| if (lines.length === 0) { | |
| rejectOnce(new Error('Empty response from remote')) | |
| return | |
| } | |
| const resolvedPath = lines[0] | |
| const entries: RemoteDirEntry[] = [] | |
| for (let i = 1; i < lines.length; i++) { | |
| const line = lines[i] | |
| if (!line || line === './' || line === '../') { | |
| continue | |
| } | |
| if (line.endsWith('/')) { | |
| entries.push({ name: line.slice(0, -1), isDirectory: true }) | |
| } else { | |
| entries.push({ name: line, isDirectory: false }) |
9f1a4dc to
f361c91
Compare
f361c91 to
0d11eb9
Compare
This branch has not been deployed
1 participant
ELI5
An Orca server can keep its own list of SSH hosts and run projects on them. Until now, a computer or browser paired with that server could only see those hosts. It could not add, edit or remove them, look inside their folders, or add a project on them. This PR gives the server a safe way to do those things when a paired client asks. The next PR builds the screens on top.
What Changed
orca servehad none at all.ssh.target-management.v1answers these methods, all with theworkspacepermission:ssh.listEditableTargets,ssh.addTarget,ssh.updateTarget,ssh.removeTargetssh.browseDir: lists a folder on one of the server's SSH hosts.repo.addRemote: adds an existing folder on one of the server's SSH hosts as a project.repo.addRemoteuses the registration the server already uses forprojectHostSetup.setupExistingFolderon SSH hosts.docs/reference/remote-wire-compatibility.md. The new methods are distinct, so an old server can never strip an unknown field and act locally. For example,repo.addwith aconnectionIdon an old server would have registered a path on the server's own disk. The renderer client module checks the capability before every call. An old server gives a typed "update this server" error and nothing falls back to the client's own SSH hosts.workspaceper the owner's decision that a paired client counts as "shell as the user" (docs(rpc): a paired client runs commands as you; say so when pairing #27317). Phones are refused because the methods are not on the mobile allowlist. An SSH host'sorcaCLI is refused unless that host has remote control enabled.Why
The owner chose that paired
workspaceaccess equals shell access (D2 Q1 = b). A client that can open a terminal can already edit~/.ssh/config, so a narrower tier here would protect nothing. Reusing the desktop's own handlers keeps one set of rules for local and remote management. Distinct methods behind a capability are the only shape that is safe when the client and server are on different versions.Considered: adding an optional
connectionIdtorepo.add. Rejected, because an old server strips the field and registers a path on its own disk instead. Also considered: accepting a password and keeping it in memory for the next connect. Rejected, because the server has no secret store today, and a write path with nothing behind it would only mislead. Remote credential prompts are separate work.Linked Issue
Server half of #8489 and #25887. Both close with the UI in the follow-up PR.
Ideas and tests harvested from the community PR #8492 by @jae-heo: a strict, bounded target schema, a reusable
browseSshDirectory, and therepo.addRemoteRPC with its test. Thank you.Visual Proof
N/A: server and RPC only, no UI in this PR.
Testing
pnpm tc,pnpm lint(owner-routing ratchet stays at 0, runtime-Electron ratchet stays at 0, RPC params catalog regenerated),pnpm run check:code-quality:changedNew:
src/main/runtime/rpc/methods/ssh-target-management.test.ts, which covers:New:
src/renderer/src/runtime/runtime-ssh-target-management.test.ts, a mixed-version test. An old server refuses every operation with "update" and nothing is called locally; a current server is called by environment.Updated:
ssh-target-crud-handlers-ownership.test.ts(paired clients get the same managed-host rules),repo.test.ts,runtime-project-host-setup-controller.test.ts.Focused suites:
src/main/runtime/rpc,src/main/ipc,src/main/ssh,src/main/startup,src/renderer/src/runtime.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
P0/P1 review by two independent reviewers; see comments.
Agent skill upstream boundary
Notes
connectionStatusonly when the server has a state for the target; absence stays absent, never "disconnected". Browse on a target that isn't connected fails with "not found" and makes no verdict. Remove tears down through the existing lifecycle queue.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)