Repository navigation
feat(pty): make the Rust PTY host a checked opt-in with a Node fallback - #1829
Conversation
The Rust PTY host's PtySession checks skip without a build, and the self-hosted runners have no Rust toolchain. The Rust PTY host job runs on GitHub's Ubuntu image, which has one: it builds the host, runs cargo test, then host.test.ts and session.test.ts with the Rust host in place.
OVIS_PTY_SIDECAR=rust used the Rust host whenever its file existed. Now the server checks the binary once per path before using it (at startup when Node can run the sidecar): run with no arguments it must print its usage line and exit 2, and `1 1 true` must exit 0 within 3 s. A binary that is missing, not executable, not for this CPU, another program, or that crashes, hangs or cannot open a pty is reported once with the reason, and every attach uses Node. PtySession starts an attach on Node when the Rust binary cannot be started after passing (and only once Node did start), and a Rust host that ends by a signal the server did not send makes every later attach use Node until the server restarts. A session the server kills, even right after spawning it, never counts. PtySession reports the host it runs on (`host`), which the parity tests and the bench assert. A leading ~/ in OVIS_PTY_SIDECAR_BIN is expanded, as herdr's plugin env file is not a shell, and a value of OVIS_PTY_SIDECAR other than rust or node is reported once. Whether a pane attaches still depends on Node: with the Rust host asked for on a PC without Node, the server says that panes are mirrored. Node stays the default.
… it cannot run The plugin env file lines, why an installed app needs an absolute or ~/ binary path (in-app updates run from ~/.config/ovis/updates), the checks before first use, the fallbacks, and that attach versus mirror still depends on Node.
The check's runs had a 3 s timeout, but Bun ends a timed-out run with SIGTERM, which a binary that ignores it outlives: the check, which runs at startup, then held the server's start for good. Each run past its time is now killed with SIGKILL; a test runs a fake host that ignores SIGTERM. Also from review: a value of OVIS_PTY_SIDECAR other than node is now reported at startup, not on the first attach; the no-Node warning is not given on Windows, where the Rust host never runs; ptyHost, which nothing used any more, is gone; one helper says why a spawn threw; and the tests check the cause in a refusal (exit code, signal, errno, timeout) instead of its wording.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
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 |
|
Merge pass review-work for b112b23: REVIEW FAILED on one stated behaviour. Everything else holds up well. Blocker: an invalid Checked and passing:
Notes, non-blocking:
CodeRabbit was rate-limited, so this review stands in for it. Its findings and their resolution go on this PR before the merge. |
|
Fixed the blocker in b84227b: an invalid
I left the MINOR notes for later: asserting |
|
Codex review of b84227b ( |
|
Merge pass review-work report. Overall: PASSED. This PR adds a CI workflow job and CodeRabbit was rate-limited, so per AGENTS.md the independent review and how each finding was resolved are recorded here and in the earlier comment.
Manual QA
Code review
The
Round 1's finding, fixed in b84227b: Follow-ups (MINOR)
All required checks are green, and no review thread is open. CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it. |
#1829 added a rust-pty-host job to ci.yml with actions/checkout@v7. It now uses the same v7.0.1 commit SHA as every other checkout; it already kept no Git credential. Without the pin, the action audit in scripts/ci-setup.test.ts names that step and fails.
…ecked (#1859) Step 1 of making the Rust PTY host the default on Linux and macOS (Node becomes optional): ship the binaries with every release and install them, checked. ## What changes - **New workflow `pty-sidecar.yml` (Rust PTY host binaries)**: builds `ovis-pty-sidecar` for linux-x64 and linux-arm64 (static musl, so no glibc floor) and a universal (arm64 + x86_64), ad-hoc signed macOS binary. It runs `cargo test` on each and, on Linux, `host.test.ts` + `session.test.ts` on the built binary. It writes `ovis-pty-sidecar.json` (version, the `crates/pty-sidecar` git tree, SHA-256 per platform) and `SHA256SUMS`. It runs on PRs touching the host/PTY code, is called by Release, and can be dispatched with a tag to attach the binaries to an existing release. - **Release** builds them in parallel with validation and attaches them in `gh release create`, so no tag exists without them. - **`scripts/pty-sidecar.ts install`**: fetches the manifest for `v<package.json version>`. It installs into `bin/ovis-pty-sidecar` only when the crate tree matches this checkout, the SHA-256 matches, and the binary answers as the host. The move is one rename, so a failure leaves nothing behind. Otherwise it says why and exits 0: that install keeps the Node sidecar. - It runs as a herdr plugin `[[build]]` step (Linux/macOS), after each update the app installs (`server/updater.ts`, failures logged and ignored), and from `install.sh` for installs that predate it. - **Remote PC bundles** carry the host in `bin/`; the smoke checks that it runs. `scripts/pty-sidecar.ts` and `crates/pty-sidecar` are bundle inputs for the release guards. - **`server/pty/host.ts`** looks at `bin/ovis-pty-sidecar` before `target/release`. This does **not** change the default: the Rust host still needs `OVIS_PTY_SIDECAR=rust`. The default flip comes after #1829, which this PR does not touch (the only overlap is `host.ts`'s default path, a one-line rebase either way). ## Verification - `bun run check fast`: all green (unit 6392 pass, 0 fail). - A static musl build passes `session.test.ts` + `host.test.ts` with `OVIS_PTY_SIDECAR=rust` (32 pass, 0 fail). - `install` against a local fake release: it installs the binary and keeps it on a rerun. A tampered binary is refused with a SHA-256 mismatch and leaves no `bin/`. A manifest from other crate sources is refused. An unreachable release leaves Node in use, with exit 0 (exit 1 with `--strict`). `scripts/pty-sidecar.test.ts` covers these cases. - This PR's own **Rust PTY host binaries** run exercises the three builds and the manifest. **Needs a real review**: this changes workflows (Release, Remote PC bundles, a new one) and the installer path. --------- Co-authored-by: mp <mp@local>
#1829 added a rust-pty-host job to ci.yml with actions/checkout@v7. It now uses the same v7.0.1 commit SHA as every other checkout; it already kept no Git credential. Without the pin, the action audit in scripts/ci-setup.test.ts names that step and fails.
…s the fallback (#1908) Step 2 of removing Node from Linux and macOS installs. It builds on #1829 (the Rust PTY host as a checked opt-in) and #1859 (release binaries installed into `bin/ovis-pty-sidecar`, checked against the release's SHA-256). ## What changes - **Rust by default.** On Linux and macOS, `server/pty/host.ts` uses the Rust PTY host whenever it passes its startup check. That host is the installed release's (its record vouches for it), else a local build, else `OVIS_PTY_SIDECAR_BIN`. `OVIS_PTY_SIDECAR=node` asks for the Node sidecar. Unknown values are reported once and mean Node. A checkout without a host says so once, as a log line. - **A PC without Node attaches.** `sidecarAvailable()` is true when the Rust host passes or Node with node-pty can run. With neither, panes are mirrored, and the server logs which way they go. The production check and the test check share one rule. - **The Node fallback never loads node-pty inside Bun** (AGENTS.md): - `PtySession` probes the `node` it will spawn on the spawn's own PATH, requires Node 18 or newer, and spawns that absolute path. - `pty-host.mjs` exits 70 before importing node-pty when it runs under Bun. - **Breaker.** A Rust host ended by a signal turns the Rust host off only where Node can take over. On a PC without Node it is kept and logged, so one crash does not leave every later attach without a host. - **Installer.** - `scripts/preflight.sh` no longer requires Node; it only notes an old one. - `install.sh` installs Node 22 only when the Rust host does not run here, using the server's own two-step check of the binary. - A failed Node install is a warning, not an unfinished install. - For an older release whose build step still asks for Node, `install.sh` installs Node and retries once. - **Docs.** INSTALL.md (including what a source checkout needs), README in English, Korean, Japanese and Chinese, `docs/guide.md`, AGENTS.md, CONTRIBUTING.md, the crate README and a changelog fragment now say Node is optional. ## Verification - `bun run check fast` on this head: see the comment below. - PTY, sidecar and installer tests on this head: 166 pass, 0 fail. The new tests simulate a PC without Node by putting a Bun-as-`node` on PATH, and each fails when its fix is reverted: - the breaker keeps the Rust host; - Bun is never run as the Node host; - the server attaches on Rust alone and mirrors with neither. - **Real checks on this code** (step 3, before any release). These used #1859's CI binaries in `bin/` with no Node on PATH, in a fresh Ubuntu 24.04 container and on eamac (macOS, fresh `/tmp` HOME, isolated herdr session). Each machine passes three cases: - **Rust host:** attach, typing, resize, about 4 MB of output, and close with no host left. First paint was 285 ms on Linux and 139 ms on eamac. - **Broken binary, no Node:** the pane is mirrored, and the log says so. - **Broken binary, with Node:** the Node sidecar attaches. - **Reviews.** omo-native-code-reviewer found one blocker (the breaker on a PC without Node) and four should-fix items. Codex (gpt-6.1-sol, fast, xhigh) found four more: the node probed versus the node spawned, Node 18 or newer, an older release's preflight, and `install.sh`'s check of the binary. All are fixed in this commit. The code is the one reviewed, rebased onto main with no conflicts. ## Not covered here Only a published release can prove the `install.sh` and `herdr plugin install` paths with no Node. Those fetch the host by tag, so they need a release cut with #1859 and this PR. Removing the Node sidecar itself is a later step. **Needs a real review:** this changes the installer, the plugin build's preflight and the PTY host default. --------- Co-authored-by: mp <mp@local>
What
The Rust PTY host from #1785 (
crates/pty-sidecar) becomes a usable opt-in on Linux and macOS. The Node sidecar stays the default, and the server falls back to it whenever the Rust host cannot do the job. Before this,OVIS_PTY_SIDECAR=rustused the Rust host whenever its file existed. A binary that was present but broken failed the attach. A crash ended the terminal with nothing learned from it.Turning it on
OVIS_PTY_SIDECAR=rustandOVIS_PTY_SIDECAR_BIN=<binary>go in the environment, or for the plugin install in herdr's pluginenvfile. There is no in-app switch, so i18n and the Settings UI are untouched.~/paths: a leading~/inOVIS_PTY_SIDECAR_BINis expanded, because the pluginenvfile is not a shell.~/.config/ovis/updates/, where the default relative build path does not exist.OVIS_PTY_SIDECARother thanrustornodeis reported once and means Node.When the Rust host cannot run (
server/pty/host.ts,server/pty/session.ts)OVIS_PTY_SIDECARthannodeis looked at then too.pty-sidecar: usage:and exit 2. This checks it exists, executes, matches this CPU/OS and is this program.<bin> 1 1 truemust exit 0, so a pty can be opened, forked into and reaped here.ENOENT,EACCES,ENOEXEC,not the Rust PTY host (exit 0), a signal name, or a timeout), and every attach uses Node.terminal-process.tsas today and nothing is disabled.closedis set beforeproc.kill().onExit, becauseonExitcan start the next session synchronously.PtySession.hostsays which host a session runs on. The parity tests and the bench assert it, so a disabled Rust host can never silently turn the Rust checks into Node runs.Not changed
sidecarAvailable()). That answer is cached for the server's life inserver/core/server-core.ts, and this PR leaves that file alone. A breaker that could falsify a Rust-only answer would leave a Node-less PC with failing attaches until restart.CI
A new Rust PTY host job runs on GitHub's Ubuntu image, which has a Rust toolchain; the self-hosted runners do not have one. It builds the crate with
--locked, runscargo test(the frame parser's tests never ran in CI before), runsserver/pty/host.test.ts(so the Rust parity cases run instead of skipping), and runsserver/pty/session.test.tswithOVIS_PTY_SIDECAR=rust. The build takes about 6 s cold. The job is not a required check.Tests
server/pty/host.test.ts: 17 new tests, using fake hosts written to amkdtempdirectory so they run without cargo.ptyHost, which nothing called any more, is gone, and its tests now callrustSidecar./bin/trueand another program's usage line.EACCES,ENOEXECandENOENT. Refusals are checked by their cause (exit code, signal, errno, timeout), not their wording.~and~/expand, while~userand absolute paths stay as they are.node, empty and unset are silent.PtySession:onExiton a signal death the server did not send.Mutation check: each mechanism was broken on purpose at the final commit, by a script in a scratch worktree, and its test failed (14 mutations):
~expansion;onExit;With everything restored, 41 tests pass and 3 skip.
Verification
bun test ./server/pty/host.test.tsgave 41 pass, 3 skip (the Rust-only cases in the Node block), 0 fail.bun test ./server/pty/session.test.tspassed 9/9 on Node and 9/9 withOVIS_PTY_SIDECAR=rust.bun test ./server/machines.test.ts, the conventions tests andbun run typecheckpass.cargo testpasses for the crate.createServeron loopback, headless Chromium on?pane=):sleep 30, and two resizes (smaller, then larger) each checked withstty sizeinside the pane. The attach's parent process wasovis-pty-sidecar.sttyagain until the size arrives (at most 15 tries; Node needed a second ask in 2 of 3 runs).pty-exitcode 137, one breaker line, and after a reload the attach runs onnode pty-host.mjsand takes input.OVIS_PTY_SIDECAR_BIN=/bin/true, a missing path and a non-executable file each produce one startup warning (not the Rust PTY host (exit 0),ENOENT,EACCES), and the attach runs on Node.OVIS_PTY_SIDECAR_BIN=~/.cache/…/ovis-pty-sidecarruns on Rust.nodeon PATH, withOVIS_PTY_SIDECAR=rust:/api/healthreportsterminal_attach: false, terminal_mirror: true, plus the one warning.attach-held,take-over,keys-lifecycle,outputandreattachgave 46/46 on Node. WithOVIS_PTY_SIDECAR=rustthey gave 44/46. The two are theoutputcases that removenodefrom PATH to make the attach's spawn fail; the Rust host starts by its path and does not fail there, as noted in feat(pty): add an experimental Rust PTY host behind OVIS_PTY_SIDECAR #1785. CI runs them on Node.bun run check fast: all steps ok on the final commit (workflow syntax with actionlint, generated files, typecheck, build, unit tests 6380 pass / 0 fail), exit 0. The browser runs were repeated after the last commit: Rust attach,/bin/truefalling back with its startup warning, andOVIS_PTY_SIDECAR=RUSTreported at startup.cargo test(15/15) and the no-Node check, and read every mutation log.Review
This changes how the server spawns terminal attaches when the opt-in is set, and adds a CI job, so it waits for a real review. It touches no file another open PR touches.