fix(vscode): choose a TypeScript-capable Node for the test worker - #8
fix(vscode): choose a TypeScript-capable Node for the test worker#8fi3ework wants to merge 5 commits into
Conversation
The extension host's PATH is a login-shell snapshot taken at startup, so the bare `node` a worker was spawned with is typically a version manager's global default rather than the version the user's terminal would give them. Measured on one machine: login shell 20.19.4, interactive shell 22.23.1. That matters because a worker has to load the project's config, and an `rstack.config.*` goes through rstack's shim, which calls `@rstackjs/load-config` with `loader: 'native'` and no jiti fallback. On a Node without native type stripping the load fails with a bare `ERR_UNKNOWN_FILE_EXTENSION` per project, no test is ever discovered, and nothing says why — while `rs test` in the terminal works, because the version manager has switched Node there. The worker's Node is now chosen rather than assumed: the `node` on PATH when it satisfies the floor, otherwise whatever the user's interactive shell resolves, otherwise a status-bar mismatch naming both candidates and the setting to override. `rstack.rstest.nodeExecutable` skips the preflight entirely and stays the escape hatch. The extension host's own runtime is deliberately not a candidate. It would silently move the run onto Electron's Node — a different ABI line (NODE_MODULE_VERSION 146 against plain Node 24.18's 137, so non-N-API addons fail to load) on a version chosen by VS Code's release cadence rather than by the project. A green run has to mean the same thing in the editor as in the terminal. The floor is uniform rather than per-project. Specialising it would buy back only Node 20, whose support window ended 2026-04-30, at the cost of a second code path. It lives beside `SUPPORT_MATRIX` so the extension's version requirements have one home, and shares its prerelease and soft-pass rules through the extracted `checkVersion`. Resolution is memoized for the extension host — one PATH, one shell — so a monorepo runs one probe and logs one notice, and is warmed at register() so the probes overlap detection instead of blocking the first spawn.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a32d5c3689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… surface The hover is now a fixed-width card (140px, 250px when notices are present) instead of tracking the widest row: one action per cell so the renderer never finds a break opportunity inside an action, column count derived from the widest row rather than a hardcoded slot count, and notices rendered as rows of the same table under their own divider — prose wrapping is left to the hover's own CSS cap instead of hand-split lines. A version-mismatch keeps the idle glyph and colours the item amber: the mismatch is advisory, the run goes ahead, and swapping the glyph reads as "stopped" — the one thing that has not happened.
Replaces the per-stack requestRestart callback with a declarative restartOnSettings list the shell watches. One settings.json save is one change event, so the two paths are decided per stack: a stack whose own gate moved is the reconcile's to handle (rebuilding it would fight a stack on its way out), while another stack's moved setting still triggers its restart. Regression-tested both ways.
…he project Two holes in the preflight, one per candidate source: - An explicitly configured nodeExecutable was used verbatim and silently. It is still always honoured — the setting is the escape hatch — but it is now probed too, and one that fell below the floor surfaces through the same version-mismatch status, stating that the run goes ahead. - The interactive-shell probe spawned with no cwd, so it inherited the extension host's (typically /). Version managers resolve version files against the shell's cwd and fnm's default strategy never walks upward, so the probe answered with the manager's global default even when the project pins a version. The probe now stands in the first detected folder that does not pin nodeExecutable — one query decides both whether the warm-up has a reader and where the shell stands — and the worker spawn path passes its project's cwd for the case where every folder pinned. E2E fixtures pin Node via .nvmrc so the suites stop depending on the developer machine's version-manager default. The decisions and their rejected alternatives are recorded in ADR 0001; CONTEXT.md gains the Runtimes glossary the ADR speaks in.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f4c4128fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { nodeExecutable, nodeExecArgs } = | ||
| await this.resolveWorkerNodeCommand(); |
There was a problem hiding this comment.
If a restart or nodeExecutable change disposes this API while the runtime preflight is still pending, dispose() cannot kill this worker because it has not yet been added to childProcesses; once the await completes, the retired API continues into spawn() and loads the project config with the stale runtime. The project's cancellation token only suppresses the eventual tree update, so add a disposal/generation check after this await to ensure a full restart actually retires all old workers.
AGENTS.md reference: packages/vscode/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| void configuredNodeBelowFloor(nodeExecutable).then((message) => { | ||
| if (message) { | ||
| status.versionMismatch(message, NODE_RUNTIME_STATUS_SOURCE); | ||
| } |
There was a problem hiding this comment.
When probing a configured executable takes long enough for nodeExecutable to change, the setting-triggered restart unbinds and then rebinds the module-global status, but this old .then() remains live; it can consequently report the obsolete executable's mismatch into the replacement controller after the restart completed. This is distinct from the already-reported uncleared automatic mismatch: here the stale mismatch is first produced after the new registration is active, so the callback needs a disposal or registration-generation guard.
AGENTS.md reference: packages/vscode/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
The runtime-selection criterion names the limit on what a piece of work can end up loading. "Surface" read as API surface — outward — when the concept points inward; "bound" states it literally, and the ADR's "the line is…" sentence stops being a metaphor. Prose uses bounded/unbounded.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f916da941
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "type": "string", | ||
| "scope": "resource", | ||
| "markdownDescription": "Overrides the `node` binary used to spawn the Rstest test worker process. Provide an absolute path to a Node.js executable (for example, a version-manager or custom build). When empty, the `node` binary on `PATH` is used. Supports the `${workspaceFolder}` placeholder." | ||
| "markdownDescription": "Overrides the `node` binary used to spawn the Rstest test worker process. Provide an absolute path to a Node.js executable (for example, a version-manager or custom build). Setting this bypasses the extension's Node.js version check entirely, which makes it the escape hatch when no suitable Node.js can be found automatically. When empty, the extension picks one: the `node` on `PATH` if it is new enough, otherwise the one your interactive shell resolves. Supports the `${workspaceFolder}` placeholder." |
There was a problem hiding this comment.
When a user explicitly configures a below-floor executable, this description promises that the setting bypasses the version check entirely, but resolveWorkerNodeCommand() still calls configuredNodeBelowFloor() and displays an amber mismatch. The configured runtime is honored, so describe the check as advisory rather than claiming it does not occur; otherwise the Settings UI contradicts the behavior users see.
Useful? React with 👍 / 👎.
1 participant
The bug
Open an rstack-cli-style repo (an
rstack.config.tswithdefine.test(), norstest.config.*) and no test is discovered — no gutter icons, an empty Test Explorer — whilers testin the integrated terminal works fine. The output channel shows one bareERR_UNKNOWN_FILE_EXTENSIONper project and nothing that says why.The cause is which Node the worker was spawned with. The extension host's
PATHis a login-shell snapshot taken at startup, so barenoderesolves to a version manager's global default rather than the version the user's terminal would give them. Measured on the reporting machine:A worker has to load the project's config, and an
rstack.config.*goes through rstack's shim, which calls@rstackjs/load-configwithloader: 'native'and no jiti fallback — so the runtime must strip TypeScript itself. Verified: 22.17.1 reportsprocess.features.typescriptfalse, 22.18.0 reportsstrip.The change
The worker's Node is now chosen rather than assumed:
rstack.rstest.nodeExecutable— always honoured: an explicit choice is the escape hatch for everything the preflight can get wrong. It is probed all the same, so a setting pointed at a Node that has since fallen below the floor surfaces as a status naming the version and the setting — while the run still goes ahead.The
nodeonPATH, if it satisfies the floor. Retried briefly when not found — VS Code resolves the shell environment while extensions are already activating.Otherwise whatever the user's interactive shell resolves (
$SHELL -i -c '… command -v node', 5s timeout, skipped on Windows), standing in the project — the shell's cwd is the first detected folder that does not pinnodeExecutable. This is what makes a version manager work end to end: its hooks live in the interactive rc files the login-shell snapshot never ran, and it resolves.nvmrc/.node-versionagainst the shell's cwd without walking upward, so a probe standing in the extension host's cwd (typically/) would still answer with the global default.Otherwise a status-bar version-mismatch naming both candidates and the setting to override:
On the reporting machine step 3 recovers to the project's pinned Node, so the failure path is never reached.
Both failure modes surface through the status bar rather than notifications: the item turns amber (keeping the idle glyph — the mismatch is advisory and the run state has not changed), and the hover card gains a notice section spelling out the consequence, which is what tells the two messages apart — one says tests will not run, the other says the extension is running with the configured Node anyway.
Decisions worth reviewing
The extension host's own runtime is deliberately not a candidate. It would silently move the run onto Electron's Node — a different ABI line (measured
NODE_MODULE_VERSION146 against plain Node 24.18's 137, so non-N-API addons fail to load) on a version chosen by VS Code's release cadence rather than by the project. A green run has to mean the same thing in the editor as in the terminal. Upstream Vitest reaches the same conclusion:process.execPathappears nowhere in its production code, and it throws rather than degrade.The floor is uniform, not per-project. A native
rstest.config.tswould load on older engines (Rsbuild carries a bundled jiti), so specialising the floor would buy back only Node 20 — whose support window ended 2026-04-30 — at the cost of a second code path. The trade-off is that a Node 20 user with a native config who has no newer Node anywhere loses a setup that would otherwise work; the shell probe recovers most of them andnodeExecutablecovers the rest.The shell probe runs once per host and stands in one folder. One PATH, one shell, one interactive-shell startup cost — so a 20-project monorepo runs one probe, and the standpoint is first-caller-wins. The warm-up derives whether to warm and where to stand from one query (the first detected folder without a pinned
nodeExecutable), so the two cannot disagree in a multi-root window where only some folders pin. Per-project probes and walking upward for.git/version files were both considered and rejected — the extension stands where the user's terminal would; how the version manager answers from there is the manager's business. Recorded with the rest of the runtime decisions indocs/adr/0001-node-runtime-selection.md.Notes
NODE_RUNTIME_RANGElives besideSUPPORT_MATRIXinshared/versionCheck.ts, so "what does this extension require?" has one home. It shares the prerelease and soft-pass rules through an extractedcheckVersion, whichcheckPackageVersionnow delegates to — previously the two had diverged on both. Whatunknownmeans stays each caller's choice, documented atcheckVersion: package checks soft-pass it, runtime candidates reject it.register()so the probes overlap detection instead of blocking the first worker spawn.nodeExecutablehere;binPath/customBinPath/trace.serverfor lint) are now declared asrestartOnSettingson the controller and watched by the shell — a stack whose own enable-gate moved in the same settings save is left to the reconcile instead of being rebuilt on its way out..nvmrc: the probe deliberately never walks upward, so without a local pin the suites' pass/fail would depend on the developer machine's version-manager global default.rstack.config.tsfine but segfaults running@rstest/core(bun 1.3.2 × 0.11.5, reproduced). Recorded in AGENTS.md along with the reason never to auto-detect it frombun.lock.AGENTS.mdgains adaptation perf(vscode): pre-spawned rs fmt standby for the active editor #6 and three gotchas; thenodeExecutablesetting description no longer claims PATH is used when it is empty.CONTEXT.mdgains the Runtimes glossary the ADR speaks in.Verification
pnpm lint(0 lint errors, 0 type errors),pnpm test:unit(223 passing), and therstest,lintandvscodeE2E slices all green. The rstest slice doubles as an end-to-end check of the probe's standpoint: it passes on a machine whose version-manager global default is below the floor, which only works because the shell probe stands in the fixture folder and reads its.nvmrc.