Repository navigation
Provide Orca CLI on managed Linux and macOS servers - #26539
Conversation
bd28ba5 to
3e96501
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe build now produces and packages a Node-based CLI bundle. Orcad prepares a host-scoped Unix launcher and exposes its path through the app environment. Child processes can use that launcher with an updated PATH and CLI environment. Runtime smoke and end-to-end tests exercise CLI commands in managed terminals. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The Orca CLI launcher change looks sound. The only open concern is that the SSH upgrade end-to-end test can pass without checking the CLI when the host fixture has no 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
422958ce-2631-4bfb-9633-5d2230f858e3
📒 Files selected for processing (14)
config/scripts/build-orcad.mjsconfig/scripts/orcad-entry-build.mjsconfig/scripts/runtime-serve-terminal-smoke.mjssrc/main/cli/cli-command-privileged-transaction.test.tssrc/main/cli/cli-dev-launcher.tssrc/main/cli/orca-cli-child-path.test.tssrc/main/cli/orca-cli-child-path.tssrc/main/orcad/orcad-cli-launcher.test.tssrc/main/orcad/orcad-cli-launcher.tssrc/main/orcad/orcad-entry.tssrc/main/runtime/structured-session-child-identity-env.test.tssrc/shared/app-environment.tssrc/shared/orcad-artifacts.tstests/e2e/ssh-orcad-auto-convert.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| if (!host.exec) { | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '200,300p' tests/e2e/ssh-orcad-auto-convert.spec.ts
rg -n 'host\.exec|test\.skip|hostCanExecute|canExecute' tests/e2e/ssh-orcad-auto-convert.spec.tsRepository: stablyai/orca
Length of output: 4892
🏁 Script executed:
rg -n -F --glob '*.ts' -- 'function startHost' tests
rg -n -F --glob '*.ts' -- 'startHost(' tests/e2e tests
rg -n -F --glob '*.ts' -- 'exec?:' tests
rg -n -F --glob '*.ts' -- 'exec:' tests/e2e tests
rg -n -F --glob '*.ts' -- 'defineConfig' playwright.config.ts tests 2>/dev/null || true
rg -n -F --glob '*.ts' -- 'test.skip' tests/e2e/ssh-orcad-auto-convert.spec.ts tests/e2e/helpersRepository: stablyai/orca
Length of output: 3728
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- startHost definitions and uses ---'
rg -n -F --glob '*.ts' -- 'function startHost' tests || true
rg -n -F --glob '*.ts' -- 'startHost(' tests/e2e tests || true
printf '%s\n' '--- exec declarations and fixture definitions ---'
rg -n -F --glob '*.ts' -- 'exec?:' tests || true
rg -n -F --glob '*.ts' -- 'exec:' tests/e2e tests || true
printf '%s\n' '--- runner configuration and skip usage ---'
rg -n -F --glob '*.ts' -- 'defineConfig' playwright.config.ts tests 2>/dev/null || true
rg -n -F --glob '*.ts' -- 'test.skip' tests/e2e/ssh-orcad-auto-convert.spec.ts tests/e2e/helpers || trueRepository: stablyai/orca
Length of output: 3861
🏁 Script executed:
printf '%s\n' '--- startHost definitions and uses ---'
rg -n -F --glob '*.ts' -- 'function startHost' tests || true
rg -n -F --glob '*.ts' -- 'startHost(' tests/e2e tests || true
printf '%s\n' '--- exec declarations and fixture definitions ---'
rg -n -F --glob '*.ts' -- 'exec?:' tests || true
rg -n -F --glob '*.ts' -- 'exec:' tests/e2e tests || true
printf '%s\n' '--- runner configuration and skip usage ---'
rg -n -F --glob '*.ts' -- 'defineConfig' playwright.config.ts tests 2>/dev/null || true
rg -n -F --glob '*.ts' -- 'test.skip' tests/e2e/ssh-orcad-auto-convert.spec.ts tests/e2e/helpers || trueRepository: stablyai/orca
Length of output: 3861
🏁 Script executed:
printf '%s\n' '--- ssh-orcad-auto-convert.spec.ts imports, constants, startHost ---'
nl -ba tests/e2e/ssh-orcad-auto-convert.spec.ts | sed -n '1,115p'
printf '%s\n' '--- orcad-convert-host helper ---'
nl -ba tests/e2e/helpers/orcad-convert-host.ts | sed -n '1,115p'
printf '%s\n' '--- Playwright config ---'
nl -ba tests/playwright.config.ts | sed -n '1,120p'
printf '%s\n' '--- package runner declarations/scripts ---'
rg -n -F --glob 'package.json' -- 'playwright' . || true
rg -n -F --glob '*.ts' -- 'test(' tests/playwright.config.ts tests/e2e/helpers | head -80 || trueRepository: stablyai/orca
Length of output: 16192
Make the missing CLI coverage explicit.
The Windows host fixture does not provide exec, so this test returns before it runs the CLI assertions. The runner then records the test as passed because the test body returns without an error. Mark this condition as skipped with a reason, or fail it if CLI verification is required.
🐛 Suggested fix
--- "a/tests/e2e/ssh-orcad-auto-convert.spec.ts"
+++ "b/tests/e2e/ssh-orcad-auto-convert.spec.ts"
@@ -250,9 +250,10 @@
app = upgraded.app
await waitForSessionReady(upgraded.page)
await convertAndRetain(upgraded.page, session.userDataDir, seeded)
+ test.skip(!host.exec, 'This host fixture cannot read files to verify CLI output')
if (!host.exec) {
return
}
const environment = (
await upgraded.page.evaluate(() => window.api.runtimeEnvironments.list())
).find((entry) => entry.orcadDeployment?.sshTargetId === seeded.targetId)📝 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 (!host.exec) { | |
| return | |
| } | |
| test.skip(!host.exec, 'This host fixture cannot read files to verify CLI output') | |
| if (!host.exec) { | |
| return | |
| } |
# Conflicts: # tests/e2e/ssh-orcad-auto-convert.spec.ts
Main now bundles the CLI into the server slot and writes a profile launcher on Unix. Drop this branch's parallel CLI packaging, slot launchers, and build-time version; keep the owning-host binding (now exported by main's launcher), the Windows native launcher on main's layout, the Claude command order, and the refreshed shell environment.
2 participants
ELI5
After an SSH host moves to Remote, agents on that machine still need the
orcacommand. The managed server did not include it. This gives Linux and macOS servers their own CLI so agents can inspect their work and use Orca commands on the machine doing the work.What Changed
Bundle the existing CLI and its version metadata into the server's verified artifact inventory. After acquiring the host profile lock, create an atomic, profile-scoped Unix launcher using the same pinned Node that runs the server. Reuse the child PATH function for terminals and structured agents, pinning the launcher and profile to the execution host.
The normal upgrade test now creates a real managed-host terminal, runs
orca statusandorca worktree ps, checks rendered terminal output, and verifies the returned runtime ID matches the remote host. The headless-server terminal smoke test checks CLI commands too.Why
The real conversion left a fresh Linux host with no
orcaon PATH. A desktop resources directory does not exist on a Node server. Reusing the full CLI preserves command behavior and local host authentication; forwarding these commands back to the desktop would target the wrong execution owner. The launcher stays under the profile rather than claiming a global command or Linux's screen-reader name.Linked Issue
Independently reproduced the managed-server CLI gap recorded as F3 in #26483.
Visual Proof
Before, an actual managed terminal reports
QA_CLI_ABSENT. After, the CLI runs both commands and printsQA_CLI_READY. No network or command-handler fault injection was used.Testing
macOS client → real Linux arm64 Docker SSH server: all five conversion scenarios pass, including relay-era migration, first upgrade with CLI use, refused TCP forwarding, occupied port 6768, and server update. Before reproduction, five passing real-host tests.
A separately built macOS arm64 headless server passes real terminal creation and CLI commands with an isolated profile. macOS runtime smoke log.
82 affected unit tests pass, including profile isolation, quoted paths and multiline arguments, clearing inherited Node preloads, slot refresh, old slots without CLI files, and terminal/structured-child PATH behavior.
pnpm tc,pnpm lint, changed-code quality, Electron e2e build, andpnpm run build:orcadpass. The server build checks the CLI's import graph for Electron and node:sqlite too.The Linux fixture combines the successfully built source JavaScript with the adhoc package's unchanged Linux native slot and pinned Node reference; every manifest hash was recomputed and normal candidate preflight verified it. Native modules were not rebuilt for Linux on macOS.
Review
Self-reviewed host ownership, profile-lock ordering, atomic launcher refresh, inherited environment isolation, packaging hashes, and reuse of the existing CLI/PATH implementation.
Agent skill upstream boundary
Notes
This fixes Unix managed servers. Windows still needs a native launcher that preserves argument boundaries; this change deliberately does not add a cmd.exe message proxy, and Windows runtime behavior was not exercised here. No RPC or stream protocol changes. Old server slots without CLI files remain usable. The bundled CLI adds about 1.9 MB before compression.
Checklist