fix(vscode): choose a TypeScript-capable Node for the test worker - #8
Merged
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".
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".
The nodeExecutable description claimed the setting bypasses the version check entirely; it is honoured unconditionally but still probed, and a below-floor choice surfaces an advisory status while runs proceed.
…ispose A settings-triggered restart can dispose an RstestApi while an await is still pending inside it; three sites could then act for a master that no longer exists: - createChildProcess spawned its worker after the Node preflight settled, producing the one process dispose() cannot kill (it was never added to childProcesses), running on the very resolution the restart exists to replace. - The configured-node verdict and the preflight failure both latched their version-mismatch into what is by then the replacement registration's status, with nothing left to clear it. Both now report through one disposed-aware helper. - resolveRstestPath re-latched a core version mismatch after the project's status was forgotten — sticky for a root that never comes back. A disposed master also fast-fails createChildProcess at entry, sparing it the package resolution and probe costs of a doomed spawn.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fff21f6ce
ℹ️ 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".
…ive shell paths Two review findings on the preflight: - The configured-node advisory latched under the host key, though nodeExecutable is resource-scoped: removing the folder that pinned a below-floor Node left its warning stuck for the whole window. The advisory now latches under a per-project node-runtime: key that dispose() forgets — its own namespace, because the bare project key is erased by the core version check's versionOk on every spawn. - The shell probe rejected any answer not starting with '/', discarding the legitimate relative paths a relative PATH entry produces. A relative answer now anchors where the shell stood; only a bare name (a shell function, not a path) is still rejected. This also removes a latent ambiguity: an unanchored relative executable would resolve against a different directory at probe time than at worker spawn.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c4cb61f98
ℹ️ 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".
Native type stripping was unflagged in 23.6.0 and backported to 22.18.0, so 23.0-23.5 satisfy a plain >=22.18.0 floor yet cannot load an rstack.config.ts. Encode the discontinuity as ^22.18.0 || >=23.6.0, and interpolate a human-readable NODE_RUNTIME_LABEL into the user-facing messages, since the raw disjunction reads as npm jargon in a status bar.
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.
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.