Skip to content

feat(ssh): remote orcad primitives on the pinned Node runtime (#16741 T6-1) - #24419

Merged
OrcaWin merged 1 commit into
mainfrom
OrcaWin/node-rt-p3-t6-1
Oct 1, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
OrcaWin/node-rt-p3-t6-1

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​220 $\color{#cf222e}{\Huge{\mathbf{−}}}$​18 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​202
Prod 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​397 $\color{#cf222e}{\Huge{\mathbf{−}}}$​98 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​299

ELI5

Orca can, in principle, install its server (orcad) on an SSH host and switch to it safely. The code for that already exists on main, but nothing in the app calls it yet. This PR adds the small building blocks the next steps need, and fixes a dangerous edge case in the existing code: a failed read of the host's "which version is active" record was treated as "no record". That could let a later deploy run over a live install.

This is slice T6-1 of the T6 exit-gate track (porting #16741: one remote server for SSH and paired transports). It is inert: nothing in the app calls managed orcad deploy yet.

What Changed

  • orcad-remote-runtime-control.ts (new): the one "launch a slot, then poll its readiness file" loop, plus execOrcadRemote and withoutAbortSignal. deployOrcad and rollbackOrcad each had their own copy of the loop; both now use this one.
  • orcad-remote-record-file.ts (new): reads and writes of small records on the host.
    • Reads are bounded and print an explicit "absent" or "present" marker, so an empty or failed answer can't be mistaken for "no record".
    • Writes go to a partial file that is then renamed into place. If the SSH command ended without confirmation, the partial file is kept, because the write may still be running.
  • Activation record store:
    • It reads through the new record file. A read that can't be verified now rejects instead of reading as empty.
    • New writeOrcadActivationRecord refuses to replace a record this client can't read. A newer schema, for example, stays untouched. Deploy and rollback write through it.
  • orcad-active-readiness.ts (new): proves that a recorded-active or relaunched slot is the expected build, using the existing activation gate (build hash and version). Failures say which of three it was: exited (the process is gone), unverifiable (the host couldn't say) or rejected (it answered with the wrong build).
  • orcad-remote-build-hash.ts and orcad-remote-context.ts (new):
    • Context resolves the host, the home directory, the server target (through main's orcad-deployment-target) and the activation record.
    • Both refuse Windows hosts first; assertPosixOrcadHost stays.
  • Readiness parsing:
    • Reads are capped at 256 KiB.
    • A finished line that isn't valid JSON is now malformed straight away. Before, it stayed pending until the deploy timed out. A half-written last line is still pending.

Why

  • Unverifiable is not absent. The old store read used .catch(() => ''), which turned a lost read into an empty record. An empty record means "nothing is active", which admits a fresh deploy over a live install and loses its rollback target. That's the "loss of contact is never death" rule from docs/reference/ssh-execution-boundary.md.
  • Unknown future schemas are read as unreadable and never overwritten. This keeps an older client from destroying a record a newer one wrote.
  • Adapted from feat(orcad): consolidate Bun runtime and SSH/Remote migration #16741 (a68b6f3): main's pinned-Node deploy core already replaces several of the branch's files:
    • orcad-remote-target-detection → main's orcad-deployment-target;
    • orcad-slot-runtime-eligibility (Bun versus legacy Node) → main's orcad-remote-runtime slot marker;
    • orcad-bundle-installation → main's orcad-remote-install.
  • Not ported, and why:
    • The branch's orcad-remote-host-support delta. It would remove OrcadRemoteLaunchUnsupportedError and the refuseUnverifiable liveness probe, which is a regression.
    • The orcad-remote-process-control delta, which switches stop from SIGTERM to a stop-request file. It only works once orcad itself consumes that file, which is T6-3. Shipping it before then would mean stops never complete on current builds.
    • The Windows and Bun launch paths.
    • ssh-upload-session-lifetime. It comes with feat(ssh): wire SshConnection through the work and transport close ledgers (#16741 T2 P2) #24401, and nothing here needs it.
    • The branch's zod schema for readiness, which carries Bun-specific fields. It will be revisited with the orcad health work.
  • Health coverage: readiness keeps main's activation gate (build hash and version). Requiring the terminal daemon's coverage: 'pty-spawn' from feat(daemon): idle retirement, session census and recovery-only provider (#16741 T2 P4b) #24409 is a follow-up once that merges.

Linked Issue

Part of the Node runtime plan (Phase 3), T6 exit-gate track, porting #16741.

Visual Proof

N/A. There's no UI change, and this code has no production caller yet.

Testing

  • New orcad-remote-primitives.test.ts:

    • an absent record is told apart from a read with no answer;
    • the partial file is kept on an unconfirmed write and removed on a confirmed failure;
    • Windows hosts are refused;
    • a lost activation read rejects;
    • a newer-schema record is unreadable and never overwritten;
    • the build hash is parsed;
    • a liveness probe of DEAD, UNKNOWN or empty maps to exited or unverifiable;
    • a wrong build is rejected;
    • the readiness bounds hold.
  • Updated: the deploy and rollback test hosts, for the marker-delimited record read and the head -c readiness read. Record writes are now checked through writeAtomicOrcadRemoteRecord.

  • Regression: every src/main/ssh/orcad-*, src/main/ssh/ssh-* and src/main/orcad/* test. 216 files: 214 passed, 2 skipped. 2900 tests passed, 22 skipped.

  • Static checks: node config/scripts/check-changed-code-quality.mjs origin/main is clean.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

…T6-1)

- orcad-remote-runtime-control: the one launch-and-poll-readiness loop that
  deployOrcad and rollbackOrcad now share, plus exec and recovery helpers.
- orcad-remote-record-file: bounded, marker-delimited reads and atomic writes
  for host records. A read that returns no verifiable answer rejects instead
  of reading as "no record".
- The activation record store reads through it (a lost read no longer becomes
  an empty record) and writeOrcadActivationRecord refuses to replace a record
  this client cannot read, such as a newer schema; deploy and rollback use it.
- orcad-active-readiness: prove a recorded-active or relaunched slot against
  the activation gate, reporting exited / unverifiable / rejected.
- orcad-remote-build-hash and orcad-remote-context (host, home, server target
  via orcad-deployment-target, activation record; POSIX-only).
- Readiness reads are capped at 256 KiB, and a finished invalid JSON line is
  malformed rather than pending forever.

Inert: nothing in the app calls managed orcad deploy yet. Ported from #16741
(a68b6f3) onto main's pinned-Node deploy core; the branch's Bun runtime,
slot eligibility, target detection and bundle installation are superseded by
main's orcad-remote-runtime, orcad-deployment-target and orcad-remote-install.
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 2/5

[High risk] Refactors remote orcad deployment and runtime control on SSH hosts.

The PR is not ready to merge because activation-record interpretation and publication can lose the host’s active or rollback state.

Findings

  1. P1 Empty record appears absent ▶
  2. P1 Concurrent writes replace records ▶
  3. P1 Rollback can finish unrecorded ▶
  4. P2 Missing readiness appears rejected ▶

Summary

The PR extracts shared remote launch control, adds bounded record I/O and build/context primitives, and routes activation writes through a guarded atomic writer.

  • Failed activation reads now reject rather than silently becoming an empty record, but an existing empty file still reaches that state.
  • Record publication remains vulnerable to concurrent replacement, and rollback’s new final reread can leave host state and the record divergent.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Read activation record] --> B[Install and preflight]
  B --> C[Stop and snapshot incumbent]
  C --> D[Launch and verify slot]
  D --> E[Reread activation record]
  E --> F[Write partial file]
  F --> G[Rename over activation record]
  E -->|Read fails| H[Serving slot and record can diverge]
Loading

Reviews (1) · Last reviewed commit: "feat(ssh): remote orcad primitives on th..."

orcadActivationPath(options.host, options.remoteHome),
ORCAD_ACTIVATION_RECORD_MAX_BYTES
)
return read.state === 'absent' ? read : parseOrcadActivationRecord(read.raw)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Empty record appears absent When orcad-active.json exists but is empty, the file read reports it as present, but parsing its contents turns it back into an absent record. Deploy can then proceed as though nothing is active, even if an installation is live, and lose its rollback target. An existing empty file needs to be treated as unreadable.

Knowledge Base Used: Remote connectivity and execution

Comment on lines +69 to +73
const existing = await readActivationRecordState(options)
if (existing.state === 'unreadable') {
throw new Error(`Refusing to overwrite this host's orcad activation record: ${existing.reason}`)
}
await writeAtomicOrcadRemoteRecord(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Concurrent writes replace records The schema check and the final rename happen in separate SSH commands. If another client writes a newer-schema record between them, mv -f still replaces it. Two deployments can also overwrite each other's active version and rollback target using records read earlier. The check needs to remain valid through publication.

Knowledge Base Used: Remote connectivity and execution

serializeOrcadActivationRecord(withRolledBackVersion(options.record, now())),
{ signal: options.signal }
)
await writeOrcadActivationRecord(options, withRolledBackVersion(options.record, now()))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Rollback can finish unrecorded This write now rereads the activation record after rollback has stopped the incumbent, restored its data, and launched the older build. If that read fails or finds an unreadable record, the function throws without recording the rollback or returning recovery guidance. The old build can be serving while the record still names the stopped build.

Knowledge Base Used: Remote connectivity and execution

Comment on lines +36 to +41
const readiness = parsed.state === 'ready' ? parsed.readiness : null
const verdict = evaluateOrcadActivation(readiness, expectation)
if (verdict.decision === 'reject') {
throw new OrcadActiveReadinessError(
'rejected',
`${label} failed its readiness check: ${verdict.reason}`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing readiness appears rejected When the process is live but its readiness file has not answered, parsing returns pending. Passing that as null to the activation gate labels the result rejected rather than unverifiable. This makes the new verdict less useful for recovery decisions because unavailable evidence looks like a failed build check.

Knowledge Base Used: Remote connectivity and execution

@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: e93eddc0-9dfa-4304-a019-94fe45aac60f

📥 Commits

Reviewing files that changed from the base of the PR and between c991893 and 3258771.

📒 Files selected for processing (12)
  • src/main/ssh/orcad-activation-record-store.ts
  • src/main/ssh/orcad-active-readiness.ts
  • src/main/ssh/orcad-remote-build-hash.ts
  • src/main/ssh/orcad-remote-context.ts
  • src/main/ssh/orcad-remote-deploy.test.ts
  • src/main/ssh/orcad-remote-deploy.ts
  • src/main/ssh/orcad-remote-launch.ts
  • src/main/ssh/orcad-remote-primitives.test.ts
  • src/main/ssh/orcad-remote-record-file.ts
  • src/main/ssh/orcad-remote-rollback.test.ts
  • src/main/ssh/orcad-remote-rollback.ts
  • src/main/ssh/orcad-remote-runtime-control.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The changes add bounded remote-record reads and atomic writes, remote context resolution, build-hash verification, and active readiness checks. Deployment and rollback now use shared launch-and-readiness handling and the activation-record store. Readiness reads have a size limit, and parsing distinguishes incomplete final output from completed invalid output.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 32587

The change adds guarded atomic activation-record writes and bounded readiness handling without an established merge-blocking regression. It is mergeable subject to normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 32587

The changes strengthen record handling and preserve existing launch and readiness controls. Current exposure is limited because managed deployment is not called by the application. One recovery gap remains: the new final record-read check can reject after the service has already changed, leaving the recorded and running versions inconsistent.

Retained concerns

  • Medium · reliability · inferred: The new activation-record writer rereads the record after deploy or rollback has launched and validated the target. A failed read or unreadable-schema rejection then propagates without a defined reconciliation outcome, leaving the service changed while active/previous still describe the earlier state. Base code already lacked compensation for final write failures; this PR adds read failures to that recovery window. No application caller was found, limiting present exposure.
Security review details

Security Blast Radius

  • inferred — The inspected operations exercise the existing SSH account's authority on one supplied host: starting and stopping OrCAD and replacing its user-data snapshot and activation record. No newly reachable unauthenticated or cross-tenant path was demonstrated; application deployment callers were not found in the scoped search.

Trust Boundaries and Controls

  • observed — Remote record output must carry an explicit absent/present marker. The reader checks file type and size before reading, and command paths and write contents are shell-escaped. Publication uses umask 077 and a unique partial file; uncertain command termination preserves that partial rather than racing a possibly ongoing write.

Resilience and Maintainability Implications

  • observed — Atomic record replacement is not a host-wide activation transaction. The existing install lock ends before activation, rollback has no corresponding lifecycle lock, and the writer does not compare record generations. Concurrent stale decisions and final-write recovery gaps predate this PR; the new guard adds a further final-commit rejection path rather than resolving those limitations.

Hardening Proposals

  • proposed — Before wiring managed deployment into application flows, define an explicit indeterminate/reconciliation outcome for post-launch commit failure and a host-scoped transition owner or generation check. Preserve fail-closed schema handling; do not blindly overwrite the record or restart an older build against potentially migrated state.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.93% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 12 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the SSH Orcad primitives added for the pinned Node runtime and includes the related issue reference.
Description check ✅ Passed The description is detailed and covers the user impact, changes, rationale, scope, linked issue reference, visual proof, testing, and agent-skill boundary. Some template sections, including Review, No…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one coverage gap and a minor cleanup note below.

Reviewed changes

  • Shared launch/readiness loop — new orcad-remote-runtime-control.ts extracts the one "launch a slot, then poll its readiness file" loop; deploy and rollback now delegate to launchOrcadAndAwaitReadiness, and the duplicated loops/READINESS_POLL_MS copies are gone.
  • Bounded marker-based record IO — new orcad-remote-record-file.ts reads small host records behind explicit absent/present markers (with wc -c size cap and exit 65 on not-regular/oversize), and writes them atomically via a unique partial + mv -f.
  • Activation store hardening — a failed record read now rejects instead of .catch(() => '') reading it as absent; writeOrcadActivationRecord re-reads and refuses to overwrite a record the client cannot parse (e.g. a newer schema). Deploy and rollback write through it.
  • Readiness cap + malformed detection — readOrcadReadinessCommand caps reads at 256 KiB via head -c; a finished invalid JSON line is now malformed immediately instead of staying pending until timeout.
  • New primitives — orcad-active-readiness.ts, orcad-remote-build-hash.ts, orcad-remote-context.ts (all inert; no production caller yet).

ℹ️ The new record shell commands have no real-shell coverage

readBoundedOrcadRemoteRecord and writeAtomicOrcadRemoteRecord build non-trivial shell strings — a marker/exit 65 read and a umask 077; printf %s … > partial && mv -f … write — but they are only exercised through a mocked execCommand. This repo maintains orcad-remote-shell-commands.integration.test.ts for exactly this reason: its header states command-text tests "stays green while the shell it produces does not work — a quoting slip, a case pattern that never matches." I ran both generated strings against a real /bin/sh (absent / present / oversize / directory / empty, plus quote-and-backslash content) and they behave correctly on Linux, so this is a guard against future regressions rather than a current bug.

Technical details
# Add real-shell coverage for the record read/write commands

## Affected sites
- `src/main/ssh/orcad-remote-record-file.ts:20` — `readBoundedOrcadRemoteRecord` marker/`wc -c`/`exit 65` shell string is never run by a real shell.
- `src/main/ssh/orcad-remote-record-file.ts:46` — `writeAtomicOrcadRemoteRecord` `umask 077; printf %s … > partial && mv -f` command is never run by a real shell.
- `src/main/ssh/orcad-remote-shell-commands.integration.test.ts` — the existing suite that runs generated POSIX commands through `/bin/sh`; does not import the record-file module.

## Required outcome
- The read command distinguishes a missing record (`absent`, exit 0) from a present one (`present` + raw bytes), and exits nonzero (never `absent`) for an oversize/not-regular/dangling-symlink target.
- The write command lands the exact byte content and is atomic (partial does not remain at the final path).

## Suggested approach (optional)
- Extend `orcad-remote-shell-commands.integration.test.ts` to `sh(...)` both command builders against a temp dir, covering: absent, present, oversize, directory, dangling symlink, and content containing a single quote/backslash/newline.

ℹ️ Nitpicks

  • writeAtomicOrcadRemoteRecord keeps its .partial.<pid>.<uuid> file on every timeout/abort (isUnconfirmedSshCommandTermination is also true for the 30s timeout), and nothing sweeps ~/.orca-remote/ partials. Repeated aborted deploys accumulate uniquely named orphans; a periodic sweep (or reusing a stable partial name per target) would bound it.
  • withoutAbortSignal in orcad-remote-runtime-control.ts:34 is exported but has no caller or test in this PR — fine as an inert building block, just flagging that it is currently unreferenced.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@OrcaWin
OrcaWin merged commit dd87ae5 into main Oct 1, 2026
59 checks passed
OrcaWin added a commit that referenced this pull request Oct 2, 2026
* Revert "feat(orcad): source-side dormant export of a relay-hosted SSH target (#16741 T6-8) (#24519)"

This reverts commit 783101b.

* Revert "feat(ssh): update, roll back, recover and stop a managed orcad server (#16741 T6-5 follow-up) (#24463)"

This reverts commit 38c2d1d.

* Revert "feat(ssh): deploy and pair an empty managed orcad server over SSH (#16741 T6-5) (#24453)"

This reverts commit 8b76683.

* Revert "fix(ssh): orcad GC honors the activation journal; readiness requires proven daemon coverage (#16741 T6 follow-up) (#24451)"

This reverts commit d3f8c50.

* Revert "feat(ssh): remote orcad stop by request file and journaled decommission (#16741 T6-4) (#24449)"

This reverts commit 43d9b43.

* Revert "feat(orcad): supervisable server: stop requests, managed stop receipts and a lifetime that keeps its lock on failed teardown (#16741 T6-3) (#24433)"

This reverts commit b093d3a.

* Revert "feat(ssh): crash-safe orcad activation, rollback and recovery (#16741 T6-2) (#24423)"

This reverts commit 1a9ac0e.

* Revert "feat(runtime): SSH access links for paired servers in a downgrade-safe sidecar (#16741 T5-1+T5-2) (#24420)"

This reverts commit 99db2bf.

* Revert "feat(relay): capability-gated owner reset with a durable preparation journal (#16741 T3 R1) (#24418)"

This reverts commit 34a582b.

* Revert "feat(ssh): track connection-manager drains, test probes and provider continuations (#16741 T2 P3+P8a) (#24407)"

This reverts commit d53063d.

* Revert "feat(daemon): idle retirement, session census and recovery-only provider (#16741 T2 P4b) (#24409)"

This reverts commit ff212db.

* Revert "feat(ssh): add pty.resumeClient and split SSH PTY process listing (#16741 T2 P5+P6) (#24414)"

This reverts commit 92cb717.

* Revert "feat(relay): await owned watcher and agent children on shutdown (#16741 T2 P1) (#24400)"

This reverts commit 6b36e4f.

* Revert "feat(session): retry failed renderer session writes and verify local folder PTYs (#16741 T2 P9) (#24406)"

This reverts commit d23ecef.

* Revert "feat(ssh): remote orcad primitives on the pinned Node runtime (#16741 T6-1) (#24419)"

This reverts commit dd87ae5.

* Revert "fix(runtime): fence runtime-environment subscriptions and status probes by identity (#16741 T5-3) (#24421)"

This reverts commit ece9e4d.

* Revert "feat(orcad): migration manifest and dormant-state contracts (#16741 T6-7) (#24422)"

This reverts commit 3fbdaba.

* Revert "feat(ssh): wire SshConnection through the work and transport close ledgers (#16741 T2 P2) (#24401)"

This reverts commit 4e8edc8.

* Revert "feat(profiles): carry markdown frontmatter visibility in project transfers (#16741 T2 P7) (#24405)"

This reverts commit 60c9326.

* Revert "fix(runtime): project the PTY incarnation onto mobile session tabs (#24413)"

This reverts commit 99e0303.

* Revert "feat(daemon): tag daemon stream data with the PTY incarnation id (#16741 T2 P4a) (#24402)"

This reverts commit 817af76.

* Revert "feat(ssh): port the SSH connection work ledger and transport close ledger (#16741 T2) (#24210)"

This reverts commit c991893.

* Revert "feat(relay): fence and drain file and git response streams on shutdown (#24185)"

This reverts commit dc08ffe.

* Revert "refactor(runtime-rpc): extract the Node WebSocket lifecycle; opt-in pinned port (#24186)"

This reverts commit a789233.

* Revert "feat(relay): route relay handlers through work admission; producer publication drain (#24181)"

This reverts commit 0b812bd.

* Revert "feat(relay): land the #16741 T1 seam (work drain, publication drain, release gate) (#24156)"

This reverts commit 3aa2d3a.

---------

Co-authored-by: m4air <m4air@Mac.localdomain>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant