Repository navigation
Keep native contracts on Node and wait for Git upgrade completion - #26034
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Node runtime include list adds the OpenCode launch-model context supervision test and the Hermes remote config adapter test. The symlinked-parent test now polls for the expected repository update with Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ELI5
Keep the real registry watcher, OpenCode process supervision and Hermes file permissions on the application's Node runtime. Make the symlinked Git upgrade test wait for its exact result instead of assuming filesystem work finishes in 50ms.
What Changed
Add the watcher, OpenCode and Hermes suites to the existing Node routing list. All 22 contract cases, process signals, real filesystem operations and their deadlines remain. Other suites continue using Bun.
The Codex handshake-cleanup fixture now reuses the existing cleanup helper. It observes the stub's forced kill before advancing the final 1,000ms exit-proof timer, and keeps stream/process I/O real. The original error, retained connection, subsequent observed exit and production deadlines remain. An existing checked error guard replaces the fixture's unchecked cast.
The Git case replaces its 50ms observation sleep with a completion wait capped at 5s, matching the fixture's existing bounded wait helper. Use
vi.waitForwith the same 25ms polling interval. The exact canonical path and update arguments remain unchanged; production polling does not change.Why
A complete combined run failed the registry watcher, OpenCode supervision and symlinked Git upgrade assertions during a competing full test run. The Git assertion had zero update calls when its 50ms sleep ended. A controlled probe delayed the real stat result by 150ms: the original assertion failed with zero calls, and the completion wait passed with the same path assertion. Waiting for dispatch alone would still run ahead of completed filesystem work.
The registry and OpenCode suites use actual native filesystem notifications, subprocess events, signals and process liveness. The Hermes suite changes real POSIX umask, installs through the local filesystem implementation of the remote adapter and verifies exact 0600/0640 permissions. The threaded trial failed both mode cases because worker threads cannot change umask; Node documents the same restriction. Production runs that host code in Node/Electron, so they fit the agreed Node runtime contract boundary. The watcher and OpenCode suites also pass under Bun when run alone; this PR does not diagnose a Bun or OS bug. Qualification also covers the existing SQLite routing guard and main's new 38-case ledger fixture; their Node routing is already present on main.
6ccaf421; all nine Hermes cases then passed in three Bun/Node pairs onefea4f4d0c96550On current main base
64bb9373dafd, all 71 cases in the three runtime-contract files, Git-upgrade file and Codex connection file pass under both the Bun coordinator and stock Node. Node typecheck, changed-code quality and formatting pass. The delayed-capture probe uses only the fictional child PID and mocked child signals; it changes no production code or deadlines. Its original fixture stalls in both runtimes, so this is a fixture ordering fix, not evidence of a Bun defect or a speed gain.The prior complete run took 959.1470s and failed four cases, including a separate large-file fixture cleanup timeout. It is unsuitable for a speed comparison because another workspace ran a full suite concurrently. This change claims runtime fidelity and completion-based assertions, not a measured speed gain. The separate 2× full-suite goal remains unmet.
Linked Issue
Maintainer-requested qualification follow-up to #25967, #26000 and #26015.
Visual Proof
N/A — test configuration and a test assertion; no rendered interaction changes.
Testing
On macOS arm64, Bun 1.4.2, Node 24.20.0 and Vitest 5.0.3: the first 13 contracts and the nine Hermes cases each pass six before runs; all 37 cases pass six combined after runs with original contract names and outcomes preserved. Per-file runtime identity, Node flags, exposed gc and background launch were recorded. Under the Bun coordinator only the three named contracts move to Node; all 15 Git upgrade cases still execute in Bun. The stock Node fallback passes the same 37 cases. Separately, the six infrastructure files plus the new ledger fixture pass all 72 cases under the Bun coordinator (including the actual Node ledger). Final checks on rebased head
f4f2687bde8cpass all 11 affected/infrastructure files and 109 cases under the Bun coordinator; the stock-Node check also passes all 109 cases. A complete current-head run through standardpnpm test --maxWorkers=16also passes all 11,339 physical files in the balanced unit manifest: 110,366 passing cases, one expected failure and 860 existing skips, with zero failures or worker lifecycle errors. The exact discovered-file list matches the report, source remained unchanged and all process scans succeeded. Raw process wall time was 606.6698s; the current workload differs from the original baseline, and another Vitest coordinator appeared in a process sample. This is complete current-suite correctness evidence, not a matched before/after speed claim. Full typecheck, changed-code quality and formatting pass; CI covers remaining platforms and the full repository.Reproduce through the standard pnpm entry points:
ORCA_BACKGROUND_LAUNCH=1 pnpm test src/main/ipc/runtime-environment-preference.test.ts src/main/opencode/opencode-launch-model-context-supervision.test.ts src/main/ipc/folder-repo-git-upgrade.test.ts src/main/hermes/hermes-config-remote-adapter.test.ts src/main/runtime/rpc/methods/agent-launch-instant-tab.test.ts --maxWorkers=1 ORCA_BACKGROUND_LAUNCH=1 pnpm test:node src/main/ipc/runtime-environment-preference.test.ts src/main/opencode/opencode-launch-model-context-supervision.test.ts src/main/ipc/folder-repo-git-upgrade.test.ts src/main/hermes/hermes-config-remote-adapter.test.ts src/main/runtime/rpc/methods/agent-launch-instant-tab.test.ts --maxWorkers=1Final current-main qualification
CI run 37578417608 passes all five full unit shards, relay, lint/typecheck, selection verification and the aggregate. Its executed source
a1d875e0eb14has the qualified main988f87e743ecand this PR headc5d8fbe876e0as parents. The five reports contain all 11,405 selected physical files exactly once: 11,270 PASS and 135 existing SKIP, zero missing/duplicate files, failures or unhandled errors. Every file in the previous qualified 11,404-file run remains; the one upstream Windows contract is added. Raw logs report 110,671 passing cases, one expected failure and 1,035 existing skips. The artifacts contain per-file status maps, so this is complete file coverage plus case totals, not an individual case-name map. Local affected-suite checks also pass all 71 cases under both standard Bun-coordinator and Node fallback commands; exact-head Node typecheck and changed-code quality pass.CodeRabbit reviewed the fixture changes with no actionable findings and explicitly skipped the final merge-only update because it contained no additional reviewable changes. There are no unresolved review threads. The before/after table above records reliability/runtime-contract improvements. No full-suite speed improvement is claimed; the separate 2× goal remains unmet.
Review
Reviewed actual native-runtime coverage, exact case preservation, real stat completion and the unchanged canonical path assertion. No application code or test removal. Fresh full CI on head
c5d8fbe876e0passed. Earlier CI had the now-fixed runner-memory incident plus the handshake timer race; a later attempt stopped on a separate pre-existing main lint error, resolved by #26079 before this qualification.Agent skill upstream boundary
Notes
Existing cross-platform runtime routing applies; no UI, SSH protocol, remote wire, native binary or application behavior changes.
Checklist