Repository navigation
feat(orcad): items 2–7 — degradation, natives, daemon, ops, deploy - #16398
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds Orcad build and deployment support. It introduces native ABI detection, node-pty prebuilds, startup preflight checks, daemon supervision, bind validation, instance locking, health reporting, and terminal degradation status. It adds transactional remote installation, activation records, state snapshots, rollback, garbage collection, and activation gating. It also generalizes remote install handling for relay and Orcad namespaces and adds extensive unit and integration coverage. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed scope, rationale, testing summary, and trade-off, but it does not follow the required template. It omits the required Linked Issue, Visual Proof or explicit N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections. Resolution Restructure the description using the repository template. Add a linked issue in the required format, provide visual proof or write N/A with a reason, complete the Testing checkboxes and platform details, complete AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections. Full details: Docstring CoverageExplanation Docstring coverage is 50.93% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 108 functions across 48 files. (36 skipped: 4 unsupported, 2 too large, 30 over the file limit.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
src/shared/orcad-artifacts.ts (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the TypeScript header comments. Keep the required contract in one concise comment. Move detailed implementation history to the linked documentation.
src/shared/orcad-artifacts.ts#L1-L10: retain the artifact-manifest contract and remove build implementation detail.src/main/orcad/node-pty-prebuilt-slot.ts#L1-L15: retain the slot-installation reason and move patch history to the glibc compatibility document.As per coding guidelines:
**/*.{ts,tsx,js,jsx}requires concise comments and says “1 LINE if possible.”Source: Coding guidelines
src/main/orcad/native-host-abi.test.ts (1)
102-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLine 108 asserts nothing on Linux. The expected value is
abi.libcitself, so the check is a tautology on the platform it targets.♻️ Suggested assertion
- expect(abi.libc).toBe(process.platform === 'linux' ? abi.libc : 'none') + if (process.platform === 'linux') { + expect(['glibc', 'musl']).toContain(abi.libc) + } else { + expect(abi.libc).toBe('none') + }src/main/orcad/node-pty-precondition.ts (1)
305-322: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the spawn-helper path when the probe printed no directory. Line 310 falls back to
join(nodePtyDir, 'build', 'Release')whenloadedDiris empty, but the binary may have loaded fromprebuilds/<platform>-<arch>. In that case the helper check tests a directory that was never used and reportsspawn_helper_missingfor a healthy host. The fallback only matters if stdout parsing fails, so the practical impact is limited to a false degradation report.src/main/ssh/orcad-activation-record-store.ts (1)
19-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert a POSIX host, and reuse the shared quoting helper.
The command is POSIX-only, yet
wrapCommand: options.host.commandDialect !== 'powershell'implies a powershell host can reach this path. CallassertPosixOrcadHostfrom./orcad-remote-host-support, asorcad-remote-launch.tsdoes, so an unsupported host fails explicitly. Also replace the localshellQuotewithshellEscapefrom./ssh-connection-utilsto keep one quoting implementation.src/main/ssh/orcad-remote-launch.ts (1)
136-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
sawCandidatebranch.Both branches of the ternary at Line 161 return
{ state: 'pending' }, sosawCandidatehas no effect. Either drop the variable or return the distinct state you intended for a truncated JSON line.src/main/ssh/orcad-remote-gc.ts (1)
46-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winForward
signalthroughRemoteInstallGcOptionsand all GC commands.gcOldRemoteInstallVersionsdoes not accept a signal, and its listing, lock probes, move, and removal commands callexecCommandwithout one. An aborted deploy can therefore continue remote GC operations until the pass completes.src/main/ssh/orcad-state-snapshot.ts (1)
159-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFall back on empty output, not on exit status.
findexits non-zero when any listed member is absent, andorca-data.jsonis a legacy path that is normally absent. So on a GNU host the firstfindprints valid mtimes, exits non-zero, and the BSD branch runs too with astat -f %mdirective that GNUstatdoes not accept for--file-system.sort -n | tail -1still selects the largest numeric line, so the parsed result stays correct, but the command does double work and mixes in junk lines.♻️ Proposed refactor
return [ - `newest=$(find ${paths} -type f -exec stat -c %Y {} + 2>/dev/null ||`, - `find ${paths} -type f -exec stat -f %m {} + 2>/dev/null);`, + `newest=$(find ${paths} -type f -exec stat -c %Y {} + 2>/dev/null);`, + `if [ -z "$newest" ]; then`, + `newest=$(find ${paths} -type f -exec stat -f %m {} + 2>/dev/null); fi;`, 'if [ -z "$newest" ]; then echo UNKNOWN; else', `echo "$newest" | sort -n | tail -1; fi` ].join(' ')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49a7fb93-0b40-421d-9545-e091f55c50de
📒 Files selected for processing (91)
.gitignoreconfig/scripts/build-orcad-prebuilds.mjsconfig/scripts/build-orcad-prebuilds.test.mjsconfig/scripts/build-orcad.mjsdocs/reference/linux-glibc-compatibility.mddocs/reference/orcad-operations.mdpackage.jsonsrc/main/daemon/daemon-entry-path-layouts.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-host-relocation.test.tssrc/main/daemon/daemon-host-relocation.tssrc/main/daemon/daemon-init-child-readiness.test.tssrc/main/daemon/daemon-init-child-startup-failure.test.tssrc/main/daemon/daemon-init-dependency-mocks.tssrc/main/daemon/daemon-init-endpoint-adoption.test.tssrc/main/daemon/daemon-init-fresh-import.tssrc/main/daemon/daemon-init-live-session-preservation.test.tssrc/main/daemon/daemon-init-mock-types.tssrc/main/daemon/daemon-init-packaged-bundle-staleness.test.tssrc/main/daemon/daemon-init-provider-installation.test.tssrc/main/daemon/daemon-init-replacement-reporting.test.tssrc/main/daemon/daemon-init-restart-sequence.test.tssrc/main/daemon/daemon-init-wedged-daemon-grace.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-respawn-throttle.test.tssrc/main/daemon/daemon-respawn-throttle.tssrc/main/daemon/daemon-spawner.tssrc/main/observability/logs-directory.tssrc/main/orcad/main-preflight-order.test.tssrc/main/orcad/main.tssrc/main/orcad/native-host-abi.test.tssrc/main/orcad/native-host-abi.tssrc/main/orcad/node-pty-prebuilt-slot.test.tssrc/main/orcad/node-pty-prebuilt-slot.tssrc/main/orcad/node-pty-precondition.test.tssrc/main/orcad/node-pty-precondition.tssrc/main/orcad/orcad-bind-address.test.tssrc/main/orcad/orcad-bind-address.tssrc/main/orcad/orcad-bundle-native-load-order.test.tssrc/main/orcad/orcad-daemon-supervision.test.tssrc/main/orcad/orcad-daemon-supervision.tssrc/main/orcad/orcad-entry.tssrc/main/orcad/orcad-health.test.tssrc/main/orcad/orcad-health.tssrc/main/orcad/orcad-instance-lock.test.tssrc/main/orcad/orcad-instance-lock.tssrc/main/orcad/orcad-launch-contract.test.tssrc/main/orcad/orcad-native-preflight.test.tssrc/main/orcad/orcad-native-preflight.tssrc/main/ports/port-scan-command-client.test.tssrc/main/ports/port-scan-command-client.tssrc/main/runtime/native-terminal-availability.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/runtime-rpc-websocket-bind-host.test.tssrc/main/runtime/runtime-rpc.tssrc/main/server/serve-readiness.test.tssrc/main/server/serve-readiness.tssrc/main/ssh/build-toolchain-diagnosis.tssrc/main/ssh/orcad-activation-gate.test.tssrc/main/ssh/orcad-activation-gate.tssrc/main/ssh/orcad-activation-record-store.tssrc/main/ssh/orcad-activation-record.test.tssrc/main/ssh/orcad-activation-record.tssrc/main/ssh/orcad-local-build-hash.tssrc/main/ssh/orcad-remote-deploy.test.tssrc/main/ssh/orcad-remote-deploy.tssrc/main/ssh/orcad-remote-gc.test.tssrc/main/ssh/orcad-remote-gc.tssrc/main/ssh/orcad-remote-host-support.tssrc/main/ssh/orcad-remote-launch.test.tssrc/main/ssh/orcad-remote-launch.tssrc/main/ssh/orcad-remote-process-control.tssrc/main/ssh/orcad-remote-rollback.test.tssrc/main/ssh/orcad-remote-rollback.tssrc/main/ssh/orcad-remote-shell-commands.integration.test.tssrc/main/ssh/orcad-state-snapshot.test.tssrc/main/ssh/orcad-state-snapshot.tssrc/main/ssh/orcad-update-plan.test.tssrc/main/ssh/orcad-update-plan.tssrc/main/ssh/remote-install-coexistence.test.tssrc/main/ssh/remote-install-coexistence.tssrc/main/ssh/remote-install-gc.tssrc/main/ssh/remote-install-model.test.tssrc/main/ssh/remote-install-model.tssrc/main/ssh/ssh-relay-build-toolchain.tssrc/main/ssh/ssh-relay-install-namespace.tssrc/main/ssh/ssh-relay-versioned-install.tssrc/main/ssh/ssh-remote-commands.tssrc/shared/orcad-artifacts.tssrc/shared/runtime-types.ts
💤 Files with no reviewable changes (8)
- src/main/daemon/daemon-init-packaged-bundle-staleness.test.ts
- src/main/daemon/daemon-init-child-readiness.test.ts
- src/main/daemon/daemon-init-live-session-preservation.test.ts
- src/main/daemon/daemon-init-restart-sequence.test.ts
- src/main/daemon/daemon-init-wedged-daemon-grace.test.ts
- src/main/daemon/daemon-init-replacement-reporting.test.ts
- src/main/daemon/daemon-init-child-startup-failure.test.ts
- src/main/daemon/daemon-init-endpoint-adoption.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
Comment on lines
+65
to
+71
| export function slotName(argv = process.argv, platform = process.platform, arch = process.arch) { | ||
| const forced = argv.find((arg) => arg.startsWith('--slot=')) | ||
| if (forced) { | ||
| return forced.slice('--slot='.length) | ||
| } | ||
| const libc = detectLibc(platform) | ||
| return libc === 'none' ? `${platform}-${arch}` : `${platform}-${arch}-${libc}` |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect every release invocation. Each forced slot must match its runner platform
# and architecture, and all matrix jobs must use one Node ABI and node-pty version.
rg -n --hidden -C 3 \
--glob '!node_modules/**' \
'(build-orcad-prebuilds\.mjs|build:orcad-prebuilds|--slot=)' .Repository: stablyai/orca
Length of output: 7251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- build script: slot, manifest, merge, and entrypoint ---'
cat -n config/scripts/build-orcad-prebuilds.mjs | sed -n '1,235p'
printf '%s\n' '--- focused tests ---'
cat -n config/scripts/build-orcad-prebuilds.test.mjs | sed -n '1,180p'
printf '%s\n' '--- workflow and manifest consumers ---'
rg -n --hidden -C 5 \
--glob '!node_modules/**' \
--glob '.github/**' \
--glob 'config/**' \
'(require-slots|nodeAbi|node-pty|manifest|orcad-prebuilds)' .github config package.jsonRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- build script definitions and entrypoint ---'
cat -n config/scripts/build-orcad-prebuilds.mjs | sed -n '26,125p'
cat -n config/scripts/build-orcad-prebuilds.mjs | sed -n '125,225p'
printf '%s\n' '--- focused tests ---'
cat -n config/scripts/build-orcad-prebuilds.test.mjs | sed -n '1,145p'
printf '%s\n' '--- only matching workflow/package lines ---'
rg -n --hidden \
--glob '!node_modules/**' \
--glob '.github/workflows/**' \
--glob 'package.json' \
'(build-orcad-prebuilds|require-slots|--slot=|nodeAbi|node-pty)' .github package.jsonRepository: stablyai/orca
Length of output: 16823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all Orca prebuild references in release configuration ---'
rg -n --hidden -C 8 \
--glob '.github/workflows/**' \
--glob 'config/scripts/build-orcad-prebuilds.mjs' \
--glob 'docs/reference/linux-glibc-compatibility.md' \
'(orcad|prebuild|require-slots|slot=)' .github config docs/reference/linux-glibc-compatibility.md
printf '%s\n' '--- release workflow job boundaries around matching references ---'
sed -n '1,220p' .github/workflows/release-cut.yml
sed -n '220,520p' .github/workflows/release-cut.ymlRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Orca native loader and slot selection ---'
cat -n src/main/orcad/node-pty-prebuilt-slot.ts | sed -n '1,220p'
rg -n --hidden -C 5 \
--glob '!node_modules/**' \
'(node-pty-prebuilt-slot|prebuilds/|manifest\.json|pty\.node|nodeAbi)' \
src/main/orcad src/main config package.json
printf '%s\n' '--- repository-tracked release/build references ---'
git ls-files | xargs rg -n -C 3 '(build-orcad-prebuilds|require-slots|--slot=|out/orcad/prebuilds)' 2>/dev/nullRepository: stablyai/orca
Length of output: 50372
Validate forced slot metadata before merging.
slotName() accepts a complete arbitrary slot label, so an arm64 build can populate linux-x64-glibc. A later x64 host can copy that binary because the slot and Node ABI checks pass, then fail to load it. Derive platform and architecture from the build host, allow only the libc override, and reject mergeManifest() inputs with different nodeAbi or node-pty versions.
|
|
||
| The readiness payload carries a `health` object: | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fence language.
Use text on this fence. markdownlint reports MD040 for the untyped block.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Source: Linters/SAST tools
Comment on lines
+125
to
+135
| it.runIf(process.platform !== 'win32')('refuses a data root owned by another uid', () => { | ||
| const root = makeRoot() | ||
| // /tmp itself is root-owned and sticky on every supported platform, so it stands in for | ||
| // "a data root this process does not own" without needing privileges to create one. | ||
| expect(() => acquireOrcadInstanceLock('/tmp', hooks())).toThrow( | ||
| expect.objectContaining({ code: 'orcad_data_root_wrong_owner' }) | ||
| ) | ||
| // And the private root this test made is still acceptable, so the refusal is about | ||
| // ownership rather than a blanket rejection. | ||
| expect(acquireOrcadInstanceLock(root, hooks()).record.identity).toBe('uid-1000') | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Check whether CI runs vitest as root (container user) and whether other tests already guard on getuid.
fd -H -t f -e yml -e yaml . .github/workflows --exec rg -n 'container|image:|runs-on|user:' {} \;
rg -n 'getuid\(\)\s*[!=]==?\s*0' --type=tsRepository: stablyai/orca
Length of output: 2269
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test and implementation ---'
cat -n src/main/orcad/orcad-instance-lock.test.ts | sed -n '110,170p'
cat -n src/main/orcad/orcad-instance-lock.ts | sed -n '1,190p'
printf '%s\n' '--- relevant workflow section ---'
fd -H -t f -e yml -e yaml . .github/workflows --exec sh -c '
if rg -q -n "container: ubuntu:20\\.04|orcad-instance-lock|vitest" "$1"; then
echo "FILE: $1"
cat -n "$1" | sed -n "950,1005p"
fi
' sh {}Repository: stablyai/orca
Length of output: 12866
🏁 Script executed:
#!/bin/bash
set -eu
cat -n src/main/orcad/orcad-instance-lock.ts | sed -n '180,285p'
cat -n src/main/orcad/orcad-instance-lock.test.ts | sed -n '1,45p'Repository: stablyai/orca
Length of output: 6058
Guard this test against running as root.
When process.getuid() === 0, /tmp passes the ownership check. acquireOrcadInstanceLock can then change its shared permissions to 0700 and create an unreleased /tmp/orcad.lock. Run this case only for non-root processes.
Comment on lines
+100
to
+110
| const daemon = health.terminalDaemon | ||
| if (daemon.state === 'absent') { | ||
| return { | ||
| decision: 'reject', | ||
| code: 'orcad_activation_daemon_absent', | ||
| reason: | ||
| 'The candidate has no terminal daemon. Every terminal on this host would run in the ' + | ||
| 'orcad process and die with it, which is the exact regression the daemon exists to ' + | ||
| 'prevent. Nothing was activated.' | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Does readiness parsing validate the health/terminalDaemon shape, or only JSON.parse + cast?
rg -n -C10 'parseOrcadReadinessOutput' --type=ts src/main/ssh/orcad-remote-launch.ts
rg -n -C4 'terminalDaemon' --type=ts src/main/orcad/orcad-health.ts src/main/server/serve-readiness.tsRepository: stablyai/orca
Length of output: 2669
🏁 Script executed:
#!/bin/bash
sed -n '134,190p' src/main/ssh/orcad-remote-launch.ts
sed -n '1,135p' src/main/ssh/orcad-activation-gate.ts
rg -n -C12 'deployOrcad|evaluateOrcadActivation|orcad_activation_daemon_absent' --type=ts src/mainRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
sed -n '261,325p' src/main/ssh/orcad-remote-deploy.tsRepository: stablyai/orca
Length of output: 2222
Guard health.terminalDaemon before reading state.
If a candidate publishes a matching health.buildHash without terminalDaemon, the readiness parser accepts it and evaluateOrcadActivation throws at daemon.state. deployOrcad calls this after stopping the incumbent, so the exception bypasses rejection and restoration. Use a nullish guard before reading state.
Comment on lines
+283
to
+330
| if (record.active) { | ||
| const outgoingDir = computeRemoteInstallDir( | ||
| ORCAD_INSTALL_MODEL, | ||
| options.remoteHome, | ||
| record.active | ||
| ) | ||
| const stopped = parseOrcadStopOutcome( | ||
| await exec(options, stopOrcadCommand(options.host, outgoingDir, { | ||
| waitSeconds: STOP_WAIT_SECONDS | ||
| })) | ||
| ) | ||
| if (!orcadStopFreedTheHost(stopped)) { | ||
| return { | ||
| outcome: 'installed-not-activated', | ||
| fullVersion, | ||
| code: 'orcad_outgoing_stop_incomplete', | ||
| reason: | ||
| `orcad ${record.active} did not exit within ${STOP_WAIT_SECONDS}s of SIGTERM ` + | ||
| `(${stopped}). It is still holding the data root and the port, so the candidate ` + | ||
| 'cannot start. Not escalating to SIGKILL: that skips the shutdown that releases ' + | ||
| 'the instance lock, and the successor would then refuse to start.' | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const parsed = await launchAndAwaitReadiness(options, { | ||
| remoteInstallDir: remoteDir, | ||
| nodePath: options.nodePath, | ||
| fullVersion, | ||
| userDataDir: options.userDataDir, | ||
| bindHost: options.bindHost, | ||
| port: options.port | ||
| }) | ||
| const verdict = evaluateOrcadActivation(parsed.state === 'ready' ? parsed.readiness : null, { | ||
| buildHash: computeLocalOrcadBuildHash(options.localOrcadDir), | ||
| fullVersion | ||
| }) | ||
| if (verdict.decision === 'reject') { | ||
| const restored = await restoreIncumbent(options, record, remoteDir) | ||
| return { | ||
| outcome: 'installed-not-activated', | ||
| fullVersion, | ||
| code: verdict.code, | ||
| reason: | ||
| `${verdict.reason} Candidate stderr is at ` + | ||
| `${joinRemotePath(options.host, remoteDir, ORCAD_LOG_FILENAME)}. ${restored}` | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Restore the incumbent when the candidate path throws, not only when the gate rejects.
Once the incumbent is stopped at Lines 289-293, four later steps can throw instead of returning a verdict: the launch exec, the readiness-poll exec, options.signal?.throwIfAborted() at Line 190, and computeLocalOrcadBuildHash at Line 317 (readFileSync on the local bundle). Each of those propagates out of deployOrcad with the outgoing orcad already stopped and no relaunch attempt, so the host serves nothing until an operator intervenes. Only verdict.decision === 'reject' calls restoreIncumbent.
Wrap the launch-and-gate section so any failure runs the same restore path, and compute the local build hash before the host is mutated.
🛠️ Proposed change
export async function deployOrcad(options: OrcadDeployOptions): Promise<OrcadDeployResult> {
const now = options.now ?? ((): Date => new Date())
const fullVersion = readLocalFullVersion(options.localOrcadDir)
+ const localBuildHash = computeLocalOrcadBuildHash(options.localOrcadDir)
const remoteDir = computeRemoteInstallDir(ORCAD_INSTALL_MODEL, options.remoteHome, fullVersion)- const parsed = await launchAndAwaitReadiness(options, {
- remoteInstallDir: remoteDir,
- nodePath: options.nodePath,
- fullVersion,
- userDataDir: options.userDataDir,
- bindHost: options.bindHost,
- port: options.port
- })
- const verdict = evaluateOrcadActivation(parsed.state === 'ready' ? parsed.readiness : null, {
- buildHash: computeLocalOrcadBuildHash(options.localOrcadDir),
- fullVersion
- })
+ let verdict: OrcadActivationVerdict
+ try {
+ const parsed = await launchAndAwaitReadiness(options, {
+ remoteInstallDir: remoteDir,
+ nodePath: options.nodePath,
+ fullVersion,
+ userDataDir: options.userDataDir,
+ bindHost: options.bindHost,
+ port: options.port
+ })
+ verdict = evaluateOrcadActivation(parsed.state === 'ready' ? parsed.readiness : null, {
+ buildHash: localBuildHash,
+ fullVersion
+ })
+ } catch (error) {
+ // The incumbent is already stopped; put it back before surfacing the failure.
+ const restored = await restoreIncumbent(options, record, remoteDir).catch(
+ (restoreError: unknown) => `Restoring the previous version also failed: ${String(restoreError)}`
+ )
+ throw new Error(
+ `Activating orcad ${fullVersion} failed before a verdict was reached: ${String(error)} ${restored}`
+ )
+ }📝 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.
Suggested change
| if (record.active) { | |
| const outgoingDir = computeRemoteInstallDir( | |
| ORCAD_INSTALL_MODEL, | |
| options.remoteHome, | |
| record.active | |
| ) | |
| const stopped = parseOrcadStopOutcome( | |
| await exec(options, stopOrcadCommand(options.host, outgoingDir, { | |
| waitSeconds: STOP_WAIT_SECONDS | |
| })) | |
| ) | |
| if (!orcadStopFreedTheHost(stopped)) { | |
| return { | |
| outcome: 'installed-not-activated', | |
| fullVersion, | |
| code: 'orcad_outgoing_stop_incomplete', | |
| reason: | |
| `orcad ${record.active} did not exit within ${STOP_WAIT_SECONDS}s of SIGTERM ` + | |
| `(${stopped}). It is still holding the data root and the port, so the candidate ` + | |
| 'cannot start. Not escalating to SIGKILL: that skips the shutdown that releases ' + | |
| 'the instance lock, and the successor would then refuse to start.' | |
| } | |
| } | |
| } | |
| const parsed = await launchAndAwaitReadiness(options, { | |
| remoteInstallDir: remoteDir, | |
| nodePath: options.nodePath, | |
| fullVersion, | |
| userDataDir: options.userDataDir, | |
| bindHost: options.bindHost, | |
| port: options.port | |
| }) | |
| const verdict = evaluateOrcadActivation(parsed.state === 'ready' ? parsed.readiness : null, { | |
| buildHash: computeLocalOrcadBuildHash(options.localOrcadDir), | |
| fullVersion | |
| }) | |
| if (verdict.decision === 'reject') { | |
| const restored = await restoreIncumbent(options, record, remoteDir) | |
| return { | |
| outcome: 'installed-not-activated', | |
| fullVersion, | |
| code: verdict.code, | |
| reason: | |
| `${verdict.reason} Candidate stderr is at ` + | |
| `${joinRemotePath(options.host, remoteDir, ORCAD_LOG_FILENAME)}. ${restored}` | |
| } | |
| } | |
| if (record.active) { | |
| const outgoingDir = computeRemoteInstallDir( | |
| ORCAD_INSTALL_MODEL, | |
| options.remoteHome, | |
| record.active | |
| ) | |
| const stopped = parseOrcadStopOutcome( | |
| await exec(options, stopOrcadCommand(options.host, outgoingDir, { | |
| waitSeconds: STOP_WAIT_SECONDS | |
| })) | |
| ) | |
| if (!orcadStopFreedTheHost(stopped)) { | |
| return { | |
| outcome: 'installed-not-activated', | |
| fullVersion, | |
| code: 'orcad_outgoing_stop_incomplete', | |
| reason: | |
| `orcad ${record.active} did not exit within ${STOP_WAIT_SECONDS}s of SIGTERM ` + | |
| `(${stopped}). It is still holding the data root and the port, so the candidate ` + | |
| 'cannot start. Not escalating to SIGKILL: that skips the shutdown that releases ' + | |
| 'the instance lock, and the successor would then refuse to start.' | |
| } | |
| } | |
| } | |
| let verdict: OrcadActivationVerdict | |
| try { | |
| const parsed = await launchAndAwaitReadiness(options, { | |
| remoteInstallDir: remoteDir, | |
| nodePath: options.nodePath, | |
| fullVersion, | |
| userDataDir: options.userDataDir, | |
| bindHost: options.bindHost, | |
| port: options.port | |
| }) | |
| verdict = evaluateOrcadActivation(parsed.state === 'ready' ? parsed.readiness : null, { | |
| buildHash: localBuildHash, | |
| fullVersion | |
| }) | |
| } catch (error) { | |
| const restored = await restoreIncumbent(options, record, remoteDir).catch( | |
| (restoreError: unknown) => `Restoring the previous version also failed: ${String(restoreError)}` | |
| ) | |
| throw new Error( | |
| `Activating orcad ${fullVersion} failed before a verdict was reached: ${String(error)} ${restored}` | |
| ) | |
| } | |
| if (verdict.decision === 'reject') { | |
| const restored = await restoreIncumbent(options, record, remoteDir) | |
| return { | |
| outcome: 'installed-not-activated', | |
| fullVersion, | |
| code: verdict.code, | |
| reason: | |
| `${verdict.reason} Candidate stderr is at ` + | |
| `${joinRemotePath(options.host, remoteDir, ORCAD_LOG_FILENAME)}. ${restored}` | |
| } | |
| } |
Comment on lines
+58
to
+70
| return [ | ||
| `cd ${dir} &&`, | ||
| // Why truncate: a re-launch into a dir that already holds a previous readiness line would | ||
| // otherwise let the deploy activate on the OLD process's health payload. | ||
| `: > ${readiness} &&`, | ||
| 'umask 077 &&', | ||
| `ORCA_VERSION=${shellEscape(spec.fullVersion)}`, | ||
| `ORCA_USER_DATA=${shellEscape(spec.userDataDir)}`, | ||
| `nohup ${shellEscape(spec.nodePath)} ${entry}`, | ||
| `--json --bind ${shellEscape(spec.bindHost)} --port ${String(spec.port)}`, | ||
| `> ${readiness} 2>> ${log} < /dev/null &`, | ||
| `echo $! > ${pidFile} && cat ${pidFile}` | ||
| ].join(' ') |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set umask 077 before creating the readiness file.
Line 62 creates the readiness file before the umask applies, so it gets the login shell's default mode. The redirect at Line 68 reuses that inode, so the readiness payload stays group- and world-readable on a shared host. The log and PID files are created after the umask and are restricted.
🔒 Proposed fix
return [
`cd ${dir} &&`,
+ 'umask 077 &&',
// Why truncate: a re-launch into a dir that already holds a previous readiness line would
// otherwise let the deploy activate on the OLD process's health payload.
`: > ${readiness} &&`,
- 'umask 077 &&',
`ORCA_VERSION=${shellEscape(spec.fullVersion)}`,📝 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.
Suggested change
| return [ | |
| `cd ${dir} &&`, | |
| // Why truncate: a re-launch into a dir that already holds a previous readiness line would | |
| // otherwise let the deploy activate on the OLD process's health payload. | |
| `: > ${readiness} &&`, | |
| 'umask 077 &&', | |
| `ORCA_VERSION=${shellEscape(spec.fullVersion)}`, | |
| `ORCA_USER_DATA=${shellEscape(spec.userDataDir)}`, | |
| `nohup ${shellEscape(spec.nodePath)} ${entry}`, | |
| `--json --bind ${shellEscape(spec.bindHost)} --port ${String(spec.port)}`, | |
| `> ${readiness} 2>> ${log} < /dev/null &`, | |
| `echo $! > ${pidFile} && cat ${pidFile}` | |
| ].join(' ') | |
| return [ | |
| `cd ${dir} &&`, | |
| 'umask 077 &&', | |
| // Why truncate: a re-launch into a dir that already holds a previous readiness line would | |
| // otherwise let the deploy activate on the OLD process's health payload. | |
| `: > ${readiness} &&`, | |
| `ORCA_VERSION=${shellEscape(spec.fullVersion)}`, | |
| `ORCA_USER_DATA=${shellEscape(spec.userDataDir)}`, | |
| `nohup ${shellEscape(spec.nodePath)} ${entry}`, | |
| `--json --bind ${shellEscape(spec.bindHost)} --port ${String(spec.port)}`, | |
| `> ${readiness} 2>> ${log} < /dev/null &`, | |
| `echo $! > ${pidFile} && cat ${pidFile}` | |
| ].join(' ') |
Comment on lines
+31
to
+39
| const host = getRemoteHostPlatform('linux-x64') | ||
| let root = '' | ||
| let dataDir = '' | ||
| let snapshotDir = '' | ||
| let versionDir = '' | ||
|
|
||
| function sh(command: string): string { | ||
| return execFileSync('/bin/sh', ['-c', command], { encoding: 'utf8' }) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Skip this file on Windows.
Every case runs /bin/sh, and the liveness cases also use ps and kill. On a Windows host execFileSync('/bin/sh', ...) fails with ENOENT, so the whole file fails instead of skipping. Gate the suites on the platform.
🧪 Proposed fix
const host = getRemoteHostPlatform('linux-x64')
+const posixOnly = process.platform !== 'win32'Then gate each suite, for example describe.skipIf(!posixOnly)('state snapshot commands, run for real', ...).
Comment on lines
+44
to
+48
| export function selectRemoteInstallModel(input: { | ||
| registration: RemoteHostRegistration | ||
| /** Raw directory names under `~/.orca-remote/`, as listed on the host. */ | ||
| installedDirNames: readonly string[] | ||
| }): RemoteInstallSelection { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Find non-test callers of the selection entry point and of the refusal codes.
rg -nP '\bselectRemoteInstallModel\s*\(' -g '!**/*.test.ts'
rg -nP 'remote_host_registered_under_both_models|remote_host_not_registered' -g '!**/*.test.ts'
rg -nP '\bRemoteHostRegistration\b' -g '!**/*.test.ts'Repository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- outline ---'
ast-grep outline src/main/ssh/remote-install-coexistence.ts
printf '%s\n' '--- implementation ---'
sed -n '1,220p' src/main/ssh/remote-install-coexistence.ts
printf '%s\n' '--- all references ---'
rg -n -P '\b(selectRemoteInstallModel|RemoteHostRegistration|remote_host_registered_under_both_models|remote_host_not_registered)\b' .Repository: stablyai/orca
Length of output: 5563
Wire selectRemoteInstallModel into the production connection path. The function has no non-test callers, so its refusal decisions are not enforced at runtime.
Comment on lines
+24
to
+30
| export const ORCAD_ARTIFACTS: readonly OrcadArtifact[] = [ | ||
| { filename: 'orcad.js' }, | ||
| // Forked so a native @parcel/watcher fault kills the child, not the server. | ||
| { filename: 'parcel-watcher-process-entry.js' }, | ||
| // Forked so PTYs outlive the runtime process; its absence makes every restart destructive. | ||
| { filename: 'daemon-entry.js' } | ||
| ] |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Include native prebuilds in the deployment artifact contract.
orcadArtifactFilenames() returns only the JavaScript bundles. src/main/ssh/remote-install-model.ts:50-59 uses this list for required remote artifacts, but resolveOrcadPrebuildsDir() requires prebuilds beside orcad.js. Fresh remote installs will therefore omit every shipped node-pty slot. Stage the prebuild tree and include its manifest and bytes in the deployment identity hash.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88a8e893-803e-4325-94ab-cf52c291731e
📒 Files selected for processing (1)
src/main/orcad/node-pty-precondition.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
Comment on lines
+32
to
+35
| const probe = spawnSync(process.execPath, ['-e', `require(${JSON.stringify(REAL_PTY_NODE)})`], { | ||
| encoding: 'utf8', | ||
| timeout: 30_000 | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the shared child-process wrapper.
This direct spawnSync call violates the Windows child-process rule. Use the repository process wrapper for this probe.
As per coding guidelines: Windows child processes must start through src/shared/child-process/, never child_process directly.
Source: Coding guidelines
Comment on lines
+217
to
+222
| // Why not a fixed status: a prepared host gives 'ok', CI's unprepared shard gives | ||
| // 'degraded', and a corrupt binding gives 'blocked' — all three are honest. What is | ||
| // invariant is that anything other than 'ok' names an established cause, so the | ||
| // host can never decline a terminal for a reason it did not work out. | ||
| expect(['ok', 'degraded', 'blocked', 'unverifiable']).toContain(verdict.status) | ||
| if (verdict.status !== 'ok') { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accept the documented unverifiable reason.
checkNodePtyPrecondition returns status: 'unverifiable' with reason: 'unknown' when the child probe times out or cannot start. The new assertion rejects this valid result. Require a defined reason, or allow unknown specifically for unverifiable.
… host `resolveWorkerEntryPath` branched on `isPackaged` alone and joined `process.resourcesPath`. orcad reports `isPackaged` true — correctly, it is a production build, and ~15 consumers read it that way to gate HTTPS-only skill downloads and the real CLI name — but `process.resourcesPath` is Electron-only and `undefined` under plain Node. So the packaged branch threw `TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string` where a clean "worker unavailable" was the honest outcome. The type said `resourcesPath: string`, which is how it went unnoticed; it is now `string | undefined`, so the compiler carries the fact. A host with no Electron resources tree has no asar to look in, so it falls back to the module directory and lets the caller report a missing worker. Found by the item 1 agent while auditing the same `isPackaged` defect class in the watcher. Verified in both directions: reverting the guard reproduces the TypeError.
Of the two ways node-pty fails, only one is catchable. A missing module throws MODULE_NOT_FOUND. A module built against the wrong libc or Node ABI is refused by the dynamic loader, and in the worst case takes the process down before any handler exists — that is #9902, which crashed the desktop app on Ubuntu 20.04 before a window appeared. There was no libc or ABI precondition anywhere in the tree. So orcad now proves the load in a CHILD process, from main.ts, before anything requires node-pty. Whatever the child does — throw, abort, die on a signal — is data rather than our own death, and the operator gets a sentence naming the host's libc, Node ABI and prebuild slot plus the command to run. Proven-unloadable exits 78 (EX_CONFIG), so a supervisor does not restart an unequippable host forever. A probe that never answered is unverifiable, not blocked: refusing to boot on an inconclusive signal would take down hosts that work. The child dlopens the file node-pty would have chosen, before requiring the package. node-pty's loader walks several directories and rethrows only the LAST error, so a refused binary reads as "Cannot find module ./prebuilds/..." — which sends the operator to install a module that is already there. It also reports through stdout: node echoes the whole -e source above a stack trace, and matching tokens against stderr made the probe's own source text answer for the verdict. Verdicts reach clients as a terminal_unavailable degradation alongside the existing browser_unavailable one, through the same cause-registry shape. degradations[].code is now an open vocabulary; clients already render only `message`. Prebuilds are compiled from PATCHED sources — the patch IS the glibc-floor fix, so an upstream tarball reproduces #9902 — into linux-{x64,arm64}-{glibc,musl} and darwin-{x64,arm64} slots. libc is in the slot name because node-pty's loader falls back to prebuilds/<platform>-<arch> and cannot tell glibc from musl. orcad installs the matching slot at boot, so a host with no compiler serves terminals. The relay's five pure toolchain-diagnosis functions moved to a transport-free module so the Node bundle can reuse them without dragging ssh2 in behind them; the relay keeps its API by re-export. macOS gets `xcode-select --install` rather than the cross-distro apt/dnf/pacman/apk menu, every line of which is wrong there.
…pared host CI's test shard runs `vitest` directly, so `ensure-native-runtime --runtime=node` never prepares node-pty for the Node ABI — `degraded` is the correct verdict there, and asserting 'ok' encoded an environment the shard does not have. Asserting whatever it returned would be vacuous, so the expectation is now derived from an independent require() of node-pty. Verified it still bites: forcing the precondition to always report 'ok' fails the suite.
orcad declared `canRecoverPersistentLocalPtys: () => false` because it did not run the terminal daemon, so every restart, update and rollback SIGKILLed every running terminal — on the host whose selling point is that work survives the client going away. That is the one property `ssh-execution-boundary.md` recommends the peer model for. Item 4 — the daemon: - Port the launch path off electron: `daemon-init.ts`, `daemon-host-relocation.ts` and `observability/logs-directory.ts` now read the `AppEnvironment` port. Relocation additionally asks whether the app root is an asar archive rather than whether the build is packaged, so a Node host answering `isPackaged() === true` no longer walks into an Electron-only NSIS-escape path (same precedent as `parcel-watcher-entry-path.ts`). - `build-orcad.mjs` emits `daemon-entry.js` beside `orcad.js`, scans the forked children's metafiles for electron/node:sqlite, and load-checks the child under plain Node. - orcad spawns and adopts the daemon; shutdown disconnects and never kills it. `canRecoverPersistentLocalPtys` now reads the live provider and is false under degraded routing, where fresh terminals would die with the process. Item 3 — the ops contract (docs/reference/orcad-operations.md): - Bind policy: `--bind`, default loopback, pinned so neither `orca serve`'s wide default nor the connected-device widen can override it, and so a paired client cannot rebind the listener from outside. - Instance lock on the data root before profile load, scoped to the runtime role so it never refuses a restart that a live daemon makes worthwhile. - Supervision: exit codes a supervisor can act on (78 = do not retry), second-signal escalation, a shutdown deadline, and crash-loop containment on daemon respawn. - Health in the readiness payload: build hash, Node ABI, and a PTY self-test that spans both processes — the daemon spawns a real PTY in its own process and the verdict crosses its socket. Both bundle load-checks now assert on exit codes: these bundles are minified onto one line, so Node's uncaught-exception report echoes every string literal in the bundle and the previous message match passed against a bundle that never loaded.
Plan items 6 and 7 from docs/design/shipping-orcad.html. Install reuses the relay's transaction verbatim — per-version lock, staged SFTP write, .install-complete sentinel, stale-lock recovery — under a parameterized namespace, so orcad-<v>/ sits beside relay-<v>/ permanently (§06). Parameterizing GC is the trap that creates: each model now collects only its own directories, enforced twice (prefix-scoped remote listing plus a local ownership re-check), and a client picks its model from how the host is registered, never from what it finds on disk. Activation is separate from installation, because a versioned directory selects nothing. A candidate is launched, publishes orca_server_ready, and only becomes active if its cross-process health payload passes: right build hash, listening, daemon live, PTY self-test green. A rejected candidate is stopped and the incumbent restarted, so a careful deploy cannot cause the outage it was being careful about. Update and rollback are shaped by the daemon. An update restarts orcad, the daemon outlives it, and the surviving daemon was forked from the outgoing bundle — so live terminals defer the update rather than proceed, and GC pins the active version, the rollback target and the live daemon's bundle. Orca's persisted state carries no schema version, so rollback restores a pre-activation snapshot rather than trusting backward-readability; the point past which it is unsafe is the first terminal created after activation, which the snapshot cannot describe and the surviving daemon still owns. Running the generated shell for real found two bugs the text assertions missed: tar members re-quoted inside a shell variable captured nothing, and kill -0 reports a zombie as alive.
…ent-shaped
The real-host case cannot predict a status: CI's shard runs vitest directly, so
node-pty is never built for the Node ABI and 'degraded' is correct there, while a
prepared checkout gives 'ok'.
The previous attempt used require('node-pty') as ground truth, which resolves the
JS wrapper while the native binding loads lazily — it proved strictly less than
the precondition checks, and failed CI for exactly that reason.
What is invariant on a host with node-pty installed: never 'blocked', and never a
degraded verdict carrying an unestablished reason. The injected-input tests keep
the logic coverage.
Both remaining CI failures were the same shape: tests reaching into node_modules for a pty.node that only exists after `ensure-native-runtime --runtime=node`, which CI's shard never runs because it invokes vitest directly. Slot *placement* is the logic worth checking on every host, so it now uses a synthetic payload and asserts the verdict stays honest about not loading. The three assertions that genuinely need a Node-ABI binding are gated on it existing. Verified: breaking slot installation fails both placement tests; with the real pty.node hidden the file is 17 passed / 3 skipped instead of ENOENT.
… file existing CI ships a pty.node built for Electron's ABI, so existsSync was true while require still failed — the gate ran exactly the tests that host can never satisfy. It now probes the binding in a child process, so a bad one cannot take the runner down. The self-consistency assertion also allowed too little: 'blocked' is the honest verdict for a corrupt binding, alongside 'ok' on a prepared host and 'degraded' on an unprepared one. What stays invariant is that anything other than 'ok' names an established cause, so a terminal is never declined for a reason nobody worked out. Verified against all three host states: prepared (19 passed), unprepared, and a corrupt binding (17 passed / 3 skipped, no failures).
CI has a loadable pty.node but no spawn-helper, and a slot without the helper is
legitimately 'degraded'. So the previous gate let a test run whose premise ('a
complete slot yields ok') that host cannot satisfy.
Verified in both states: with the helper present 19 pass; with it removed the
load-dependent cases skip (17 passed / 3 skipped) instead of failing.
nwparker
force-pushed
the
nwparker/orcad-full-stack
branch
from
August 27, 2026 07:07
0190891 to
b2ff8cc
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41f930a5-c503-4499-a146-9c246468eeea
📒 Files selected for processing (92)
.gitignoreconfig/scripts/build-orcad-prebuilds.mjsconfig/scripts/build-orcad-prebuilds.test.mjsconfig/scripts/build-orcad.mjsdocs/reference/linux-glibc-compatibility.mddocs/reference/orcad-operations.mdpackage.jsonsrc/main/daemon/daemon-entry-path-layouts.test.tssrc/main/daemon/daemon-entry.tssrc/main/daemon/daemon-host-relocation.test.tssrc/main/daemon/daemon-host-relocation.tssrc/main/daemon/daemon-init-child-readiness.test.tssrc/main/daemon/daemon-init-child-startup-failure.test.tssrc/main/daemon/daemon-init-dependency-mocks.tssrc/main/daemon/daemon-init-endpoint-adoption.test.tssrc/main/daemon/daemon-init-fresh-import.tssrc/main/daemon/daemon-init-live-session-preservation.test.tssrc/main/daemon/daemon-init-mock-types.tssrc/main/daemon/daemon-init-packaged-bundle-staleness.test.tssrc/main/daemon/daemon-init-provider-installation.test.tssrc/main/daemon/daemon-init-replacement-reporting.test.tssrc/main/daemon/daemon-init-restart-sequence.test.tssrc/main/daemon/daemon-init-wedged-daemon-grace.test.tssrc/main/daemon/daemon-init.tssrc/main/daemon/daemon-respawn-throttle.test.tssrc/main/daemon/daemon-respawn-throttle.tssrc/main/daemon/daemon-spawner.tssrc/main/observability/logs-directory.tssrc/main/orcad/main-preflight-order.test.tssrc/main/orcad/main.tssrc/main/orcad/native-host-abi.test.tssrc/main/orcad/native-host-abi.tssrc/main/orcad/node-pty-prebuilt-slot.test.tssrc/main/orcad/node-pty-prebuilt-slot.tssrc/main/orcad/node-pty-precondition.test.tssrc/main/orcad/node-pty-precondition.tssrc/main/orcad/orcad-bind-address.test.tssrc/main/orcad/orcad-bind-address.tssrc/main/orcad/orcad-bundle-native-load-order.test.tssrc/main/orcad/orcad-daemon-supervision.test.tssrc/main/orcad/orcad-daemon-supervision.tssrc/main/orcad/orcad-entry.tssrc/main/orcad/orcad-health.test.tssrc/main/orcad/orcad-health.tssrc/main/orcad/orcad-instance-lock.test.tssrc/main/orcad/orcad-instance-lock.tssrc/main/orcad/orcad-launch-contract.test.tssrc/main/orcad/orcad-native-preflight.test.tssrc/main/orcad/orcad-native-preflight.tssrc/main/ports/port-scan-command-client.test.tssrc/main/ports/port-scan-command-client.tssrc/main/runtime/native-terminal-availability.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/runtime-rpc-websocket-bind-host.test.tssrc/main/runtime/runtime-rpc.tssrc/main/server/serve-readiness.test.tssrc/main/server/serve-readiness.tssrc/main/ssh/build-toolchain-diagnosis.tssrc/main/ssh/orcad-activation-gate.test.tssrc/main/ssh/orcad-activation-gate.tssrc/main/ssh/orcad-activation-record-store.tssrc/main/ssh/orcad-activation-record.test.tssrc/main/ssh/orcad-activation-record.tssrc/main/ssh/orcad-local-build-hash.tssrc/main/ssh/orcad-remote-deploy.test.tssrc/main/ssh/orcad-remote-deploy.tssrc/main/ssh/orcad-remote-gc.test.tssrc/main/ssh/orcad-remote-gc.tssrc/main/ssh/orcad-remote-host-support.tssrc/main/ssh/orcad-remote-launch.test.tssrc/main/ssh/orcad-remote-launch.tssrc/main/ssh/orcad-remote-process-control.tssrc/main/ssh/orcad-remote-rollback.test.tssrc/main/ssh/orcad-remote-rollback.tssrc/main/ssh/orcad-remote-shell-commands.integration.test.tssrc/main/ssh/orcad-state-snapshot.test.tssrc/main/ssh/orcad-state-snapshot.tssrc/main/ssh/orcad-update-plan.test.tssrc/main/ssh/orcad-update-plan.tssrc/main/ssh/remote-install-coexistence.test.tssrc/main/ssh/remote-install-coexistence.tssrc/main/ssh/remote-install-gc.tssrc/main/ssh/remote-install-model.test.tssrc/main/ssh/remote-install-model.tssrc/main/ssh/ssh-relay-build-toolchain.tssrc/main/ssh/ssh-relay-install-namespace.tssrc/main/ssh/ssh-relay-versioned-install.tssrc/main/ssh/ssh-remote-commands.tssrc/shared/orcad-artifacts.tssrc/shared/runtime-session-contracts.tssrc/shared/runtime-types.ts
💤 Files with no reviewable changes (8)
- src/main/daemon/daemon-init-restart-sequence.test.ts
- src/main/daemon/daemon-init-packaged-bundle-staleness.test.ts
- src/main/daemon/daemon-init-child-startup-failure.test.ts
- src/main/daemon/daemon-init-live-session-preservation.test.ts
- src/main/daemon/daemon-init-child-readiness.test.ts
- src/main/daemon/daemon-init-wedged-daemon-grace.test.ts
- src/main/daemon/daemon-init-endpoint-adoption.test.ts
- src/main/daemon/daemon-init-replacement-reporting.test.ts
🚧 Files skipped from review as they are similar to previous changes (78)
- src/main/daemon/daemon-respawn-throttle.test.ts
- src/main/daemon/daemon-init-mock-types.ts
- src/main/ssh/orcad-activation-record.test.ts
- src/main/daemon/daemon-init-fresh-import.ts
- src/main/runtime/orca-runtime.ts
- src/main/orcad/orcad-launch-contract.test.ts
- src/main/daemon/daemon-init-dependency-mocks.ts
- src/main/ssh/remote-install-coexistence.test.ts
- src/main/orcad/main-preflight-order.test.ts
- src/main/ssh/orcad-remote-launch.test.ts
- src/main/daemon/daemon-host-relocation.test.ts
- src/main/ssh/orcad-remote-gc.ts
- src/main/daemon/daemon-entry.ts
- src/main/orcad/orcad-native-preflight.test.ts
- src/main/ssh/ssh-relay-build-toolchain.ts
- config/scripts/build-orcad-prebuilds.test.mjs
- src/main/ssh/orcad-update-plan.test.ts
- src/main/ssh/orcad-activation-gate.test.ts
- src/main/orcad/orcad-bundle-native-load-order.test.ts
- src/main/runtime/runtime-rpc-websocket-bind-host.test.ts
- src/main/ssh/orcad-update-plan.ts
- src/main/ssh/orcad-remote-rollback.ts
- .gitignore
- src/shared/orcad-artifacts.ts
- src/main/observability/logs-directory.ts
- src/main/ssh/orcad-remote-deploy.test.ts
- src/main/orcad/orcad-daemon-supervision.ts
- src/main/ssh/orcad-activation-record-store.ts
- config/scripts/build-orcad.mjs
- src/main/orcad/node-pty-prebuilt-slot.ts
- src/main/orcad/main.ts
- src/main/ssh/orcad-remote-process-control.ts
- src/main/ssh/build-toolchain-diagnosis.ts
- src/main/orcad/orcad-health.ts
- src/main/runtime/orca-runtime.test.ts
- src/main/daemon/daemon-entry-path-layouts.test.ts
- src/main/ssh/remote-install-model.test.ts
- src/main/ssh/orcad-state-snapshot.ts
- src/main/daemon/daemon-respawn-throttle.ts
- src/main/daemon/daemon-spawner.ts
- src/main/orcad/orcad-instance-lock.test.ts
- src/main/ssh/ssh-relay-install-namespace.ts
- src/main/ssh/orcad-activation-record.ts
- src/main/orcad/orcad-bind-address.test.ts
- src/main/ssh/orcad-activation-gate.ts
- config/scripts/build-orcad-prebuilds.mjs
- src/main/ssh/remote-install-coexistence.ts
- package.json
- src/main/orcad/orcad-instance-lock.ts
- src/main/orcad/node-pty-prebuilt-slot.test.ts
- src/main/ssh/orcad-local-build-hash.ts
- src/main/runtime/runtime-rpc.ts
- src/main/daemon/daemon-host-relocation.ts
- src/main/daemon/daemon-init.ts
- src/main/orcad/native-host-abi.test.ts
- src/main/ports/port-scan-command-client.test.ts
- src/main/ssh/orcad-remote-host-support.ts
- src/main/orcad/node-pty-precondition.ts
- src/main/runtime/native-terminal-availability.ts
- src/main/ssh/orcad-remote-gc.test.ts
- src/main/server/serve-readiness.test.ts
- src/main/orcad/native-host-abi.ts
- src/main/ssh/ssh-relay-versioned-install.ts
- src/main/ssh/orcad-remote-launch.ts
- src/main/orcad/orcad-health.test.ts
- src/main/ssh/remote-install-gc.ts
- src/main/orcad/orcad-daemon-supervision.test.ts
- src/main/orcad/orcad-bind-address.ts
- src/main/orcad/orcad-native-preflight.ts
- src/main/ssh/ssh-remote-commands.ts
- src/main/server/serve-readiness.ts
- docs/reference/linux-glibc-compatibility.md
- src/main/ssh/orcad-state-snapshot.test.ts
- src/main/orcad/orcad-entry.ts
- src/main/ports/port-scan-command-client.ts
- src/main/ssh/orcad-remote-rollback.test.ts
- src/main/ssh/orcad-remote-deploy.ts
- src/main/daemon/daemon-init-provider-installation.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
Comment on lines
+25
to
+28
| **Consequence for supervision:** orcad's shutdown path calls `disconnectDaemon()`, never | ||
| `shutdownDaemon()`. A supervisor that reaps orcad's whole process group — systemd's | ||
| `KillMode=control-group` — kills the daemon too and turns every restart back into data loss. | ||
| Use `KillMode=mixed` (the default) or `process`, and never `--send-sigkill` on the group. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
for f in /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/*/*.md; do
printf '\n--- %s ---\n' "$f"
head -80 "$f"
done
printf '%s\n' '--- target document ---'
cat -n docs/reference/orcad-operations.md | sed -n '1,55p'
printf '%s\n' '--- related shutdown terms ---'
rg -n -C 3 'disconnectDaemon|shutdownDaemon|KillMode|send-sigkill|daemon|orcad' docs src .github systemd 2>/dev/null | head -240 || trueRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
log=/tmp/coderabbit-shell-logs/shell-output-ipiDFu
printf '%s\n' '--- target document from captured output ---'
rg -n -A 8 -B 5 'Consequence for supervision|disconnectDaemon|shutdownDaemon|KillMode|send-sigkill' "$log" || true
printf '%s\n' '--- repository occurrences ---'
rg -n -A 8 -B 8 'disconnectDaemon|shutdownDaemon|KillMode|send-sigkill' docs src .github 2>/dev/null | head -220 || true
printf '%s\n' '--- unit/config files ---'
find . -type f \( -iname '*systemd*' -o -iname '*.service' -o -iname '*.unit' \) -printRepository: stablyai/orca
Length of output: 1515
🌐 Web query:
systemd KillMode default control-group mixed SIGKILL remaining processes process official man page
💡 Result:
The systemd KillMode setting determines how processes within a unit's control group are terminated when the unit is stopped [1][2]. The default value is control-group [3][1]. When KillMode is set to mixed, the following behavior occurs: 1. The initial stop signal (defaulting to SIGTERM, as configured by KillSignal=) is sent only to the main process of the unit [1][2]. 2. Any remaining processes in the unit's control group (other than the main process) are immediately sent SIGKILL once the main process has exited [1][2]. For the control-group mode, all processes in the control group receive the initial stop signal [1][2]. If processes remain after the configured TimeoutStopSec= has elapsed, systemd then sends SIGKILL to all remaining processes [3][2]. In summary, the mixed mode is specifically designed to send the initial stop signal only to the main process, while ensuring all other processes in the control group are terminated with SIGKILL once the main process exits [1][2].
Citations:
- 1: https://manpages.ubuntu.com/manpages/noble/man5/systemd.kill.5.html
- 2: https://manpages.org/systemdkill/5
- 3: https://man7.org/linux/man-pages/man5/systemd.kill.5.html
Use KillMode=process, not KillMode=mixed.
KillMode=control-group is systemd’s default. With KillMode=mixed, systemd sends SIGKILL to remaining control-group processes when orcad exits. This can kill the daemon that must outlive orcad. State KillMode=process explicitly and remove the default claim.
🧰 Tools
🪛 LanguageTool
[style] ~26-~26: Consider using “who” when you are referring to a person instead of an object.
Context: ... never shutdownDaemon(). A supervisor that reaps orcad's whole process group — sys...
(THAT_WHO)
Comment on lines
+123
to
+130
| code: typeof BROWSER_UNAVAILABLE_ERROR_CODE | typeof TERMINAL_UNAVAILABLE_ERROR_CODE | ||
| capability: 'browser.headless.v1' | typeof TERMINAL_PTY_DEGRADATION_CAPABILITY | ||
| message: string | ||
| /** | ||
| * Machine-readable cause. Optional for mixed-version peers: absence means the host | ||
| * predates structured causes, NOT that the cause is 'unconfigured'. | ||
| */ | ||
| reason?: RuntimeBrowserUnavailableReason | ||
| reason?: RuntimeBrowserUnavailableReason | RuntimeTerminalUnavailableReason |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make RuntimeDegradation a discriminated union.
The current type accepts a terminal code with a browser capability or browser reason. Correlate code, capability, and reason in separate union members so invalid degradation payloads are not type-correct.
9 tasks done
9 tasks done
paidaxingyo666
pushed a commit
to paidaxingyo666/Manta
that referenced
this pull request
Aug 30, 2026
…tablyai#16398) * fix(ports): stop joining an undefined resourcesPath on a non-Electron host `resolveWorkerEntryPath` branched on `isPackaged` alone and joined `process.resourcesPath`. orcad reports `isPackaged` true — correctly, it is a production build, and ~15 consumers read it that way to gate HTTPS-only skill downloads and the real CLI name — but `process.resourcesPath` is Electron-only and `undefined` under plain Node. So the packaged branch threw `TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string` where a clean "worker unavailable" was the honest outcome. The type said `resourcesPath: string`, which is how it went unnoticed; it is now `string | undefined`, so the compiler carries the fact. A host with no Electron resources tree has no asar to look in, so it falls back to the module directory and lets the caller report a missing worker. Found by the item 1 agent while auditing the same `isPackaged` defect class in the watcher. Verified in both directions: reverting the guard reproduces the TypeError. * feat(orcad): prove node-pty loads before anything requires it Of the two ways node-pty fails, only one is catchable. A missing module throws MODULE_NOT_FOUND. A module built against the wrong libc or Node ABI is refused by the dynamic loader, and in the worst case takes the process down before any handler exists — that is stablyai#9902, which crashed the desktop app on Ubuntu 20.04 before a window appeared. There was no libc or ABI precondition anywhere in the tree. So orcad now proves the load in a CHILD process, from main.ts, before anything requires node-pty. Whatever the child does — throw, abort, die on a signal — is data rather than our own death, and the operator gets a sentence naming the host's libc, Node ABI and prebuild slot plus the command to run. Proven-unloadable exits 78 (EX_CONFIG), so a supervisor does not restart an unequippable host forever. A probe that never answered is unverifiable, not blocked: refusing to boot on an inconclusive signal would take down hosts that work. The child dlopens the file node-pty would have chosen, before requiring the package. node-pty's loader walks several directories and rethrows only the LAST error, so a refused binary reads as "Cannot find module ./prebuilds/..." — which sends the operator to install a module that is already there. It also reports through stdout: node echoes the whole -e source above a stack trace, and matching tokens against stderr made the probe's own source text answer for the verdict. Verdicts reach clients as a terminal_unavailable degradation alongside the existing browser_unavailable one, through the same cause-registry shape. degradations[].code is now an open vocabulary; clients already render only `message`. Prebuilds are compiled from PATCHED sources — the patch IS the glibc-floor fix, so an upstream tarball reproduces stablyai#9902 — into linux-{x64,arm64}-{glibc,musl} and darwin-{x64,arm64} slots. libc is in the slot name because node-pty's loader falls back to prebuilds/<platform>-<arch> and cannot tell glibc from musl. orcad installs the matching slot at boot, so a host with no compiler serves terminals. The relay's five pure toolchain-diagnosis functions moved to a transport-free module so the Node bundle can reuse them without dragging ssh2 in behind them; the relay keeps its API by re-export. macOS gets `xcode-select --install` rather than the cross-distro apt/dnf/pacman/apk menu, every line of which is wrong there. * test(orcad): pin the node-pty precondition to ground truth, not a prepared host CI's test shard runs `vitest` directly, so `ensure-native-runtime --runtime=node` never prepares node-pty for the Node ABI — `degraded` is the correct verdict there, and asserting 'ok' encoded an environment the shard does not have. Asserting whatever it returned would be vacuous, so the expectation is now derived from an independent require() of node-pty. Verified it still bites: forcing the precondition to always report 'ok' fails the suite. * feat(orcad): run the terminal daemon, and the ops contract around it orcad declared `canRecoverPersistentLocalPtys: () => false` because it did not run the terminal daemon, so every restart, update and rollback SIGKILLed every running terminal — on the host whose selling point is that work survives the client going away. That is the one property `ssh-execution-boundary.md` recommends the peer model for. Item 4 — the daemon: - Port the launch path off electron: `daemon-init.ts`, `daemon-host-relocation.ts` and `observability/logs-directory.ts` now read the `AppEnvironment` port. Relocation additionally asks whether the app root is an asar archive rather than whether the build is packaged, so a Node host answering `isPackaged() === true` no longer walks into an Electron-only NSIS-escape path (same precedent as `parcel-watcher-entry-path.ts`). - `build-orcad.mjs` emits `daemon-entry.js` beside `orcad.js`, scans the forked children's metafiles for electron/node:sqlite, and load-checks the child under plain Node. - orcad spawns and adopts the daemon; shutdown disconnects and never kills it. `canRecoverPersistentLocalPtys` now reads the live provider and is false under degraded routing, where fresh terminals would die with the process. Item 3 — the ops contract (docs/reference/orcad-operations.md): - Bind policy: `--bind`, default loopback, pinned so neither `orca serve`'s wide default nor the connected-device widen can override it, and so a paired client cannot rebind the listener from outside. - Instance lock on the data root before profile load, scoped to the runtime role so it never refuses a restart that a live daemon makes worthwhile. - Supervision: exit codes a supervisor can act on (78 = do not retry), second-signal escalation, a shutdown deadline, and crash-loop containment on daemon respawn. - Health in the readiness payload: build hash, Node ABI, and a PTY self-test that spans both processes — the daemon spawns a real PTY in its own process and the verdict crosses its socket. Both bundle load-checks now assert on exit codes: these bundles are minified onto one line, so Node's uncaught-exception report echoes every string literal in the bundle and the previous message match passed against a bundle that never loaded. * feat(orcad): deploy, activate and roll back a versioned orcad install Plan items 6 and 7 from docs/design/shipping-orcad.html. Install reuses the relay's transaction verbatim — per-version lock, staged SFTP write, .install-complete sentinel, stale-lock recovery — under a parameterized namespace, so orcad-<v>/ sits beside relay-<v>/ permanently (§06). Parameterizing GC is the trap that creates: each model now collects only its own directories, enforced twice (prefix-scoped remote listing plus a local ownership re-check), and a client picks its model from how the host is registered, never from what it finds on disk. Activation is separate from installation, because a versioned directory selects nothing. A candidate is launched, publishes orca_server_ready, and only becomes active if its cross-process health payload passes: right build hash, listening, daemon live, PTY self-test green. A rejected candidate is stopped and the incumbent restarted, so a careful deploy cannot cause the outage it was being careful about. Update and rollback are shaped by the daemon. An update restarts orcad, the daemon outlives it, and the surviving daemon was forked from the outgoing bundle — so live terminals defer the update rather than proceed, and GC pins the active version, the rollback target and the live daemon's bundle. Orca's persisted state carries no schema version, so rollback restores a pre-activation snapshot rather than trusting backward-readability; the point past which it is unsafe is the first terminal created after activation, which the snapshot cannot describe and the surviving daemon still owns. Running the generated shell for real found two bugs the text assertions missed: tar members re-quoted inside a shell variable captured nothing, and kill -0 reports a zombie as alive. * test(orcad): assert the precondition is self-consistent, not environment-shaped The real-host case cannot predict a status: CI's shard runs vitest directly, so node-pty is never built for the Node ABI and 'degraded' is correct there, while a prepared checkout gives 'ok'. The previous attempt used require('node-pty') as ground truth, which resolves the JS wrapper while the native binding loads lazily — it proved strictly less than the precondition checks, and failed CI for exactly that reason. What is invariant on a host with node-pty installed: never 'blocked', and never a degraded verdict carrying an unestablished reason. The injected-input tests keep the logic coverage. * fix(orcad): drop an eslint-disable the rule no longer needs * test(orcad): separate slot placement from the load verdict Both remaining CI failures were the same shape: tests reaching into node_modules for a pty.node that only exists after `ensure-native-runtime --runtime=node`, which CI's shard never runs because it invokes vitest directly. Slot *placement* is the logic worth checking on every host, so it now uses a synthetic payload and asserts the verdict stays honest about not loading. The three assertions that genuinely need a Node-ABI binding are gated on it existing. Verified: breaking slot installation fails both placement tests; with the real pty.node hidden the file is 17 passed / 3 skipped instead of ENOENT. * test(orcad): gate the load-dependent cases on a real load, not on the file existing CI ships a pty.node built for Electron's ABI, so existsSync was true while require still failed — the gate ran exactly the tests that host can never satisfy. It now probes the binding in a child process, so a bad one cannot take the runner down. The self-consistency assertion also allowed too little: 'blocked' is the honest verdict for a corrupt binding, alongside 'ok' on a prepared host and 'degraded' on an unprepared one. What stays invariant is that anything other than 'ok' names an established cause, so a terminal is never declined for a reason nobody worked out. Verified against all three host states: prepared (19 passed), unprepared, and a corrupt binding (17 passed / 3 skipped, no failures). * test(orcad): gate on the whole premise — binding AND spawn-helper CI has a loadable pty.node but no spawn-helper, and a slot without the helper is legitimately 'degraded'. So the previous gate let a test run whose premise ('a complete slot yields ok') that host cannot satisfy. Verified in both states: with the helper present 19 pass; with it removed the load-dependent cases skip (17 passed / 3 skipped) instead of failing. * fix(orcad): preserve degradation types after rebase (cherry picked from commit 5631aa0)
This was referenced Oct 11, 2026
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…tablyai#16398) * fix(ports): stop joining an undefined resourcesPath on a non-Electron host `resolveWorkerEntryPath` branched on `isPackaged` alone and joined `process.resourcesPath`. orcad reports `isPackaged` true — correctly, it is a production build, and ~15 consumers read it that way to gate HTTPS-only skill downloads and the real CLI name — but `process.resourcesPath` is Electron-only and `undefined` under plain Node. So the packaged branch threw `TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string` where a clean "worker unavailable" was the honest outcome. The type said `resourcesPath: string`, which is how it went unnoticed; it is now `string | undefined`, so the compiler carries the fact. A host with no Electron resources tree has no asar to look in, so it falls back to the module directory and lets the caller report a missing worker. Found by the item 1 agent while auditing the same `isPackaged` defect class in the watcher. Verified in both directions: reverting the guard reproduces the TypeError. * feat(orcad): prove node-pty loads before anything requires it Of the two ways node-pty fails, only one is catchable. A missing module throws MODULE_NOT_FOUND. A module built against the wrong libc or Node ABI is refused by the dynamic loader, and in the worst case takes the process down before any handler exists — that is stablyai#9902, which crashed the desktop app on Ubuntu 20.04 before a window appeared. There was no libc or ABI precondition anywhere in the tree. So orcad now proves the load in a CHILD process, from main.ts, before anything requires node-pty. Whatever the child does — throw, abort, die on a signal — is data rather than our own death, and the operator gets a sentence naming the host's libc, Node ABI and prebuild slot plus the command to run. Proven-unloadable exits 78 (EX_CONFIG), so a supervisor does not restart an unequippable host forever. A probe that never answered is unverifiable, not blocked: refusing to boot on an inconclusive signal would take down hosts that work. The child dlopens the file node-pty would have chosen, before requiring the package. node-pty's loader walks several directories and rethrows only the LAST error, so a refused binary reads as "Cannot find module ./prebuilds/..." — which sends the operator to install a module that is already there. It also reports through stdout: node echoes the whole -e source above a stack trace, and matching tokens against stderr made the probe's own source text answer for the verdict. Verdicts reach clients as a terminal_unavailable degradation alongside the existing browser_unavailable one, through the same cause-registry shape. degradations[].code is now an open vocabulary; clients already render only `message`. Prebuilds are compiled from PATCHED sources — the patch IS the glibc-floor fix, so an upstream tarball reproduces stablyai#9902 — into linux-{x64,arm64}-{glibc,musl} and darwin-{x64,arm64} slots. libc is in the slot name because node-pty's loader falls back to prebuilds/<platform>-<arch> and cannot tell glibc from musl. orcad installs the matching slot at boot, so a host with no compiler serves terminals. The relay's five pure toolchain-diagnosis functions moved to a transport-free module so the Node bundle can reuse them without dragging ssh2 in behind them; the relay keeps its API by re-export. macOS gets `xcode-select --install` rather than the cross-distro apt/dnf/pacman/apk menu, every line of which is wrong there. * test(orcad): pin the node-pty precondition to ground truth, not a prepared host CI's test shard runs `vitest` directly, so `ensure-native-runtime --runtime=node` never prepares node-pty for the Node ABI — `degraded` is the correct verdict there, and asserting 'ok' encoded an environment the shard does not have. Asserting whatever it returned would be vacuous, so the expectation is now derived from an independent require() of node-pty. Verified it still bites: forcing the precondition to always report 'ok' fails the suite. * feat(orcad): run the terminal daemon, and the ops contract around it orcad declared `canRecoverPersistentLocalPtys: () => false` because it did not run the terminal daemon, so every restart, update and rollback SIGKILLed every running terminal — on the host whose selling point is that work survives the client going away. That is the one property `ssh-execution-boundary.md` recommends the peer model for. Item 4 — the daemon: - Port the launch path off electron: `daemon-init.ts`, `daemon-host-relocation.ts` and `observability/logs-directory.ts` now read the `AppEnvironment` port. Relocation additionally asks whether the app root is an asar archive rather than whether the build is packaged, so a Node host answering `isPackaged() === true` no longer walks into an Electron-only NSIS-escape path (same precedent as `parcel-watcher-entry-path.ts`). - `build-orcad.mjs` emits `daemon-entry.js` beside `orcad.js`, scans the forked children's metafiles for electron/node:sqlite, and load-checks the child under plain Node. - orcad spawns and adopts the daemon; shutdown disconnects and never kills it. `canRecoverPersistentLocalPtys` now reads the live provider and is false under degraded routing, where fresh terminals would die with the process. Item 3 — the ops contract (docs/reference/orcad-operations.md): - Bind policy: `--bind`, default loopback, pinned so neither `orca serve`'s wide default nor the connected-device widen can override it, and so a paired client cannot rebind the listener from outside. - Instance lock on the data root before profile load, scoped to the runtime role so it never refuses a restart that a live daemon makes worthwhile. - Supervision: exit codes a supervisor can act on (78 = do not retry), second-signal escalation, a shutdown deadline, and crash-loop containment on daemon respawn. - Health in the readiness payload: build hash, Node ABI, and a PTY self-test that spans both processes — the daemon spawns a real PTY in its own process and the verdict crosses its socket. Both bundle load-checks now assert on exit codes: these bundles are minified onto one line, so Node's uncaught-exception report echoes every string literal in the bundle and the previous message match passed against a bundle that never loaded. * feat(orcad): deploy, activate and roll back a versioned orcad install Plan items 6 and 7 from docs/design/shipping-orcad.html. Install reuses the relay's transaction verbatim — per-version lock, staged SFTP write, .install-complete sentinel, stale-lock recovery — under a parameterized namespace, so orcad-<v>/ sits beside relay-<v>/ permanently (§06). Parameterizing GC is the trap that creates: each model now collects only its own directories, enforced twice (prefix-scoped remote listing plus a local ownership re-check), and a client picks its model from how the host is registered, never from what it finds on disk. Activation is separate from installation, because a versioned directory selects nothing. A candidate is launched, publishes orca_server_ready, and only becomes active if its cross-process health payload passes: right build hash, listening, daemon live, PTY self-test green. A rejected candidate is stopped and the incumbent restarted, so a careful deploy cannot cause the outage it was being careful about. Update and rollback are shaped by the daemon. An update restarts orcad, the daemon outlives it, and the surviving daemon was forked from the outgoing bundle — so live terminals defer the update rather than proceed, and GC pins the active version, the rollback target and the live daemon's bundle. Orca's persisted state carries no schema version, so rollback restores a pre-activation snapshot rather than trusting backward-readability; the point past which it is unsafe is the first terminal created after activation, which the snapshot cannot describe and the surviving daemon still owns. Running the generated shell for real found two bugs the text assertions missed: tar members re-quoted inside a shell variable captured nothing, and kill -0 reports a zombie as alive. * test(orcad): assert the precondition is self-consistent, not environment-shaped The real-host case cannot predict a status: CI's shard runs vitest directly, so node-pty is never built for the Node ABI and 'degraded' is correct there, while a prepared checkout gives 'ok'. The previous attempt used require('node-pty') as ground truth, which resolves the JS wrapper while the native binding loads lazily — it proved strictly less than the precondition checks, and failed CI for exactly that reason. What is invariant on a host with node-pty installed: never 'blocked', and never a degraded verdict carrying an unestablished reason. The injected-input tests keep the logic coverage. * fix(orcad): drop an eslint-disable the rule no longer needs * test(orcad): separate slot placement from the load verdict Both remaining CI failures were the same shape: tests reaching into node_modules for a pty.node that only exists after `ensure-native-runtime --runtime=node`, which CI's shard never runs because it invokes vitest directly. Slot *placement* is the logic worth checking on every host, so it now uses a synthetic payload and asserts the verdict stays honest about not loading. The three assertions that genuinely need a Node-ABI binding are gated on it existing. Verified: breaking slot installation fails both placement tests; with the real pty.node hidden the file is 17 passed / 3 skipped instead of ENOENT. * test(orcad): gate the load-dependent cases on a real load, not on the file existing CI ships a pty.node built for Electron's ABI, so existsSync was true while require still failed — the gate ran exactly the tests that host can never satisfy. It now probes the binding in a child process, so a bad one cannot take the runner down. The self-consistency assertion also allowed too little: 'blocked' is the honest verdict for a corrupt binding, alongside 'ok' on a prepared host and 'degraded' on an unprepared one. What stays invariant is that anything other than 'ok' names an established cause, so a terminal is never declined for a reason nobody worked out. Verified against all three host states: prepared (19 passed), unprepared, and a corrupt binding (17 passed / 3 skipped, no failures). * test(orcad): gate on the whole premise — binding AND spawn-helper CI has a loadable pty.node but no spawn-helper, and a slot without the helper is legitimately 'degraded'. So the previous gate let a test run whose premise ('a complete slot yields ok') that host cannot satisfy. Verified in both states: with the helper present 19 pass; with it removed the load-dependent cases skip (17 passed / 3 skipped) instead of failing. * fix(orcad): preserve degradation types after rebase
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates plan items 2, 5, 3+4, 6+7 from
docs/design/shipping-orcad.htmlontomain. Items 0 and 1 are already merged (#16368, #16369).Superseding the per-item stack (#16370, #16374, #16375, #16384) — squash-merges kept stranding each PR into its base branch instead of
main, three times. One PR againstmainremoves that failure mode.What lands
Item 2 — the degradation contract's remainder.
resolveWorkerEntryPathjoinedprocess.resourcesPath, which is Electron-only andundefinedunder Node whileisPackagedis true on orcad. That threwTypeError [ERR_INVALID_ARG_TYPE]where a clean "worker unavailable" was correct. The type saidstring; it is nowstring | undefinedso the compiler carries the fact.Item 5 — the uncatchable native failure. Missing node-pty is
MODULE_NOT_FOUNDand degradable; wrong libc dies atdlopenbefore any handler — a shipped incident (#9902). There was no libc/ABI precondition anywhere. Now: detection, an out-of-process load probe that runs before anything in-process requires node-pty,terminal_unavailableinstatus.get, and a prebuild matrix that refuses to compile unpatched sources (the repo's node-pty patch is the glibc-floor fix).Items 3+4 — the daemon and the ops contract. orcad now spawns and supervises the terminal daemon, so
canRecoverPersistentLocalPtysreports real state instead of a hardcodedfalse. Verified end to end: with a live PTY, stopping orcad leaves the daemon running. Bind policy is now loopback by default (was0.0.0.0), plus instance lock, supervision, and health that spans both processes with a realpty-spawnself-test.Items 6+7 — deploy, activate, roll back. Reuses the relay's upload/lock transaction, parameterizes the versioned namespace, forks launch/readiness. Each model GCs only its own namespace permanently — deleting the sibling's tree would reach across the execution boundary. Activation refuses on
daemon_absent/daemon_degradedand distinguishes a realpty-spawnfrom a no-ophandshake. The update path uses SIGTERM twice, never a kill, because a kill skips orcad's release of the daemon and would destroy every terminal on the host.Proof
build-orcadgreen —0.1.0+963cae5090b8, 4.73 MB, zero electron and zeronode:sqlitesmoke:orcad-terminalPASSpnpm run lint, and changed-code quality all cleanFailure injection was run per item and is recorded in each superseded PR — notably: short-circuiting the activation gate fails 10 of its 11 tests, and a static
import 'node-pty'in the graph is caught by the load-order test.Trade-offs
One deliberate, a security improvement: orcad binds loopback instead of every interface. Anyone relying on the old wide bind must pass
--bind. An unattended server should not publish an authenticated endpoint on every interface without someone asking.Everything else is behaviour-identical. Nothing ships or runs orcad yet — item 8 (retiring Electron
orca serve) is deliberately out of scope, so this can be E2E tested hard before any swap.