From a32d5c36893996872a863f97700fe8c924cd2d1e Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 10 Aug 2026 15:19:18 +0800 Subject: [PATCH 1/5] fix(vscode): choose a TypeScript-capable Node for the test worker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/vscode/AGENTS.md | 8 +- packages/vscode/package.json | 2 +- packages/vscode/src/shared/versionCheck.ts | 31 +- packages/vscode/src/stacks/test/index.ts | 13 +- packages/vscode/src/stacks/test/master.ts | 68 +++- .../vscode/src/stacks/test/nodeResolution.ts | 284 ++++++++++++++ .../tests/stacks/test/nodeResolution.test.ts | 351 ++++++++++++++++++ 7 files changed, 744 insertions(+), 13 deletions(-) create mode 100644 packages/vscode/src/stacks/test/nodeResolution.ts create mode 100644 packages/vscode/tests/stacks/test/nodeResolution.test.ts diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index da096e1..9216f64 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -5,15 +5,16 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten ## The copies are intentional - `stacks/lint` and `stacks/test` are deliberate near-verbatim copies of the upstream extensions, kept close to upstream so changes can be synced by diffing. Do NOT deduplicate or refactor across the two stacks — the duplication is the point; consolidation is a later, explicit phase. -- The copies diverge from upstream in exactly five ways (the "adaptations" below). When syncing upstream, preserve them. A sixth divergence is either a bug or must be added to this list. +- The copies diverge from upstream in exactly six ways (the "adaptations" below). When syncing upstream, preserve them. A seventh divergence is either a bug or must be added to this list. -## The five adaptations +## The six adaptations 1. **Shell activation** — stacks never self-activate; `register()` returns fast and never blocks on starting a server/worker. 2. **Namespace** — everything user-visible is `rstack.*`. Legacy `rslint.*` / `rstest.*` names appear only in the migration mapping. Command IDs were renamed without aliases (breaking old keybindings was an accepted cost). 3. **Resolve-from-project** — no tool binaries or tool packages in the VSIX; everything resolves from the user's project so the editor runs the CLI's exact versions. Version floors surface as a status, never a crash. All cooperating lint pieces (binary, config loader, plugin host) must come from one resolution root. 4. **Status aggregation** — stacks own no UI chrome; they report to the shell's single status bar item, which always exists. 5. **Worker-cwd decoupling** (test) — a project's cwd is explicit, not derived from the config file path; for native configs behavior stays byte-identical to upstream. +6. **Worker runtime** (test) — the worker's Node is chosen by the extension against one uniform floor, never assumed from PATH; the recovery path is the user's own shell, and the extension host's own runtime is never a candidate. ## Rules @@ -34,6 +35,9 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The fmt stack is a spawn-per-request `rs fmt --stdin-filepath` MVP. Its cwd is the governing config directory because rs fmt resolves config from cwd only, and formatting errors are log-only by design. A single pre-spawned standby that tracks the active editor (see CONTEXT.md) is the accepted, bounded exception to "no warm tier". Do not grow it into a daemon: no long-lived protocol, no process pool, no cross-request state. The endgame is an upstream LSP; the standby retires with it. - `projectModules.ts` has no cache-invalidation hook and restart must not grow one. Node's ESM registry is keyed by resolved URL and process-lifetime, so clearing the local memo hands back the identical module object (verified); a `?epoch=` query does reload the entry but relative specifiers inside it do not inherit the query, yielding a fresh entry over stale dependencies. In-place reinstalls under an unchanged path need a window reload — say so, don't fake it. - The VSIX is platform-targeted for exactly one reason: the test stack's AST collection loads a native parser binding. Do not add another native dependency — it multiplies the release matrix. +- `stacks/test/nodeResolution.ts` takes its shell and its notify callback as options instead of importing `vscode` and the stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveWorkerNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. Move it to `shared/` when a second stack has to run user code on a project-side Node — but not for a caller that only runs _our_ code on the extension host runtime (fmt, the lint plugin host), which has no candidate to choose between and only needs `nativeTypeStrippingAvailable()`. +- The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`): the strictest thing a worker does is load an `rstack.config.*` through rstack's shim, which hardcodes `loader: 'native'` with no jiti fallback and therefore needs native type stripping (22.18+). Specialising the floor per project was considered and rejected — it buys back only Node 20, whose support window ended 2026-04-30, at the cost of a second code path. +- Bun is not a supported worker runtime: `bun run` on `@rstest/core` segfaults (verified, bun 1.3.2 × @rstest/core 0.11.5), even though bun loads `rstack.config.ts` fine. If that is ever revisited, gate it on an explicit setting — never on `bun.lock`, since bun-as-package-manager still runs the `rs` bin through its `#!/usr/bin/env node` shebang. ## Testing diff --git a/packages/vscode/package.json b/packages/vscode/package.json index a501013..688a202 100644 --- a/packages/vscode/package.json +++ b/packages/vscode/package.json @@ -237,7 +237,7 @@ "order": 5, "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." }, "rstack.rstest.nodeExecArgs": { "order": 6, diff --git a/packages/vscode/src/shared/versionCheck.ts b/packages/vscode/src/shared/versionCheck.ts index 9470b20..3c654dc 100644 --- a/packages/vscode/src/shared/versionCheck.ts +++ b/packages/vscode/src/shared/versionCheck.ts @@ -45,22 +45,43 @@ export const readPackageVersion = ( return typeof version === 'string' ? version : undefined; }; -export const checkPackageVersion = ( - packageName: SupportedPackage, +/** + * The floor for the Node.js a test worker runs on. Not part of + * `SUPPORT_MATRIX`, which is keyed by npm package name, but the same kind of + * fact and deliberately kept in the same file so "what does this extension + * require?" has one answer. + * + * Verified: 22.17.1 reports `process.features.typescript` false, 22.18.0 + * reports `strip`. Native type stripping is what lets a worker load an + * `rstack.config.*` — rstack's shim loads it with `loader: 'native'` and no + * jiti fallback. See `stacks/test/nodeResolution.ts` for how it is probed. + */ +export const NODE_RUNTIME_RANGE = '>=22.18.0'; + +/** + * The whole version policy in one place: an unreadable or unparseable version + * is `unknown` (a soft pass — it must never cost a feature), and prereleases of + * a supported range (e.g. `1.0.0-beta.1`) are accepted, because the ecosystem + * ships them and refusing them would strand early adopters. + */ +export const checkVersion = ( version: string | undefined, + required: string, ): VersionCheckResult => { if (!version || !semver.valid(semver.coerce(version) ?? '')) { return { kind: 'unknown', version }; } - const required = SUPPORT_MATRIX[packageName]; - // Prereleases of a supported range (e.g. `1.0.0-beta.1`) are accepted: the - // ecosystem ships them and refusing them would strand early adopters. if (semver.satisfies(version, required, { includePrerelease: true })) { return { kind: 'ok', version }; } return { kind: 'mismatch', version, required }; }; +export const checkPackageVersion = ( + packageName: SupportedPackage, + version: string | undefined, +): VersionCheckResult => checkVersion(version, SUPPORT_MATRIX[packageName]); + export const formatVersionMismatch = ( packageName: SupportedPackage, result: Extract, diff --git a/packages/vscode/src/stacks/test/index.ts b/packages/vscode/src/stacks/test/index.ts index b12005a..b1ff072 100644 --- a/packages/vscode/src/stacks/test/index.ts +++ b/packages/vscode/src/stacks/test/index.ts @@ -7,7 +7,8 @@ import type { import { RstestDiagnostics } from './diagnostics'; import { TestErrorStore, testMessageText } from './errorStore'; import { logger } from './logger'; -import { runningWorkers } from './master'; +import { runningWorkers, workerNodeOptions } from './master'; +import { resetWorkerNodeCache, resolveWorkerNodeOnce } from './nodeResolution'; import { Project, WorkspaceManager } from './project'; import { status } from './status'; import { disposeTerminal } from './terminal'; @@ -525,12 +526,22 @@ class RstestController implements StackController { status.unbind(); throw error; } + // Warm the host-level node preflight while detection and the config-glob + // scan are still running, so the first worker spawn awaits a settled + // promise instead of paying the probes on the critical path. Deliberately + // not awaited — `register()` must return fast (adaptation #1) — and the + // rejection is handled by whoever actually needs the resolution. + void resolveWorkerNodeOnce(workerNodeOptions()).catch(() => {}); return this.#rstest.buildExports(); } dispose(): void { this.#rstest?.dispose(); this.#rstest = undefined; + // The third module singleton with this exact lifetime, alongside the two + // binds above: a re-registered stack re-probes, which is what makes the + // restart command pick up a toolchain change. + resetWorkerNodeCache(); status.unbind(); logger.unbind(); } diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index f1b5c91..d896f3d 100644 --- a/packages/vscode/src/stacks/test/master.ts +++ b/packages/vscode/src/stacks/test/master.ts @@ -22,6 +22,12 @@ import type { RstestDiagnostics } from './diagnostics'; import type { TestErrorStore } from './errorStore'; import { logger } from './logger'; import { nodeRequire } from './nodeRequire'; +import { + NODE_RUNTIME_STATUS_SOURCE, + NodePreflightError, + type ResolveWorkerNodeOptions, + resolveWorkerNodeOnce, +} from './nodeResolution'; import type { Project } from './project'; import { status } from './status'; import { runInTerminal as sendToTerminal, shellQuote } from './terminal'; @@ -31,6 +37,16 @@ import type { Worker } from './worker'; export const runningWorkers = new Set>(); +/** + * The host-level inputs to the worker-node preflight. `notify` must not close + * over `this`: the resolution is memoized for the extension host's lifetime, so + * a callback capturing a `Project` would pin it and its whole test tree. + */ +export const workerNodeOptions = (): ResolveWorkerNodeOptions => ({ + shell: vscode.env.shell || undefined, + notify: (message) => logger.info(message), +}); + // Default host for a fixed debug port. The spawn (`--inspect-wait`), the port // preflight, and the attach config must all use the same host: on a dual-stack // machine `localhost` can resolve to `::1` while the worker listens on IPv4, so @@ -101,12 +117,17 @@ export class RstestApi { return `^${regexpEscape(testCaseNamePath.join(' '))}${isSuite ? ' ' : '$'}`; } - // The node executable + exec args used to run a worker or the CLI, honoring - // the `nodeExecutable` / `nodeExecArgs` settings (`${workspaceFolder}` - // expanded). + // The node executable + exec args honoring the `nodeExecutable` / + // `nodeExecArgs` settings (`${workspaceFolder}` expanded). Used verbatim by + // the terminal CLI, which deliberately skips the worker preflight below: the + // command runs inside the user's own shell, which is the very thing the + // preflight exists to emulate. `configured` reports whether the executable is + // the user's explicit choice or the bare `node` default, so the preflight + // does not have to read the setting a second time. private resolveNodeCommand(): { nodeExecutable: string; nodeExecArgs: string[]; + configured: boolean; } { const configuredExecutable = getConfigValue( 'nodeExecutable', @@ -116,12 +137,50 @@ export class RstestApi { nodeExecutable: configuredExecutable ? this.expandWorkspaceFolder(configuredExecutable) : 'node', + configured: Boolean(configuredExecutable), nodeExecArgs: getConfigValue('nodeExecArgs', this.workspace).map((arg) => this.expandWorkspaceFolder(arg), ), }; } + /** + * The node command for worker spawns — the node-preflight adaptation. Why the + * PATH `node` cannot be trusted, and why the floor is uniform rather than + * per-project, lives in `nodeResolution.ts`. What is decided here is only + * that an explicitly configured `nodeExecutable` skips the preflight + * entirely: an explicit choice is the escape hatch for everything the + * preflight can get wrong, and a wrong one already surfaces through the + * spawn-error notification. + * + * `vscode.env.shell` is read here so `nodeResolution.ts` needs no VS Code + * import. + */ + private async resolveWorkerNodeCommand(): Promise<{ + nodeExecutable: string; + nodeExecArgs: string[]; + }> { + const { nodeExecutable, nodeExecArgs, configured } = + this.resolveNodeCommand(); + if (configured) { + return { nodeExecutable, nodeExecArgs }; + } + try { + const resolution = await resolveWorkerNodeOnce(workerNodeOptions()); + return { nodeExecutable: resolution.executable, nodeExecArgs }; + } catch (error) { + if (error instanceof NodePreflightError) { + // The status-aggregation adaptation: no usable runtime anywhere is the + // same "fix your toolchain" state as an unsupported package version. + // Latched under the host key, not `this.statusSource`: the failure + // belongs to the extension host, so N projects must not file N copies, + // and this project's own recovery must not clear it. + status.versionMismatch(error.message, NODE_RUNTIME_STATUS_SOURCE); + } + throw error; + } + } + // The validated absolute path to the package.json a `rstestPackagePath` // setting points at, or `undefined` when the setting is unset and the bare // `CORE_PACKAGE_JSON` specifier applies. Shared by the worker resolution and @@ -495,7 +554,8 @@ export class RstestApi { ); } const workerPath = path.resolve(__dirname, 'worker.js'); - const { nodeExecutable, nodeExecArgs } = this.resolveNodeCommand(); + const { nodeExecutable, nodeExecArgs } = + await this.resolveWorkerNodeCommand(); const nodeEnv = getConfigValue('nodeEnv', this.workspace); const debugNodeEnv = startDebugging ? getConfigValue('debugNodeEnv', this.workspace) diff --git a/packages/vscode/src/stacks/test/nodeResolution.ts b/packages/vscode/src/stacks/test/nodeResolution.ts new file mode 100644 index 0000000..d66d77e --- /dev/null +++ b/packages/vscode/src/stacks/test/nodeResolution.ts @@ -0,0 +1,284 @@ +import { execFile, spawn } from 'node:child_process'; +import { checkVersion, NODE_RUNTIME_RANGE } from '../../shared/versionCheck'; + +/** + * Choosing the Node.js a test worker runs on. + * + * The bare `node` a GUI extension host inherits is a *login-shell* snapshot + * taken at startup — typically a version manager's global default rather than + * the version the user's terminal would give them. Measured on one developer + * machine: login shell `node` was 20.19.4 while the interactive shell was + * 22.23.1. So "what is on PATH" is not a reliable answer, and asking the user's + * own shell is the recovery path. + * + * The floor (`NODE_RUNTIME_RANGE`, in `shared/versionCheck`) is uniform rather + * than per-project. It is set by the strictest thing a worker does — load an + * `rstack.config.*` — and applying it to every project, including a native + * `rstest.config.*` that Rsbuild's bundled jiti would load on older engines, is + * a deliberate simplification: it drops only Node 20, whose support window + * ended 2026-04-30 (`nodejs/Release`), and keeps one code path instead of two. + * + * There is deliberately NO fallback to the extension host's own runtime + * (`process.execPath` + `ELECTRON_RUN_AS_NODE`). It would silently move the + * test run onto Electron's Node — a different ABI line (measured: Electron + * reports NODE_MODULE_VERSION 146 where plain Node 24.18 reports 137, so + * non-N-API addons fail to load) and 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 status-latch key for a failed preflight. The reporting site's identity is + * the *host*, not a project: there is one PATH and one shell, so filing this + * under a project's source URI would write N entries for one fact and let any + * one project's unrelated recovery (`status.versionOk` after the `@rstest/core` + * package check) or disposal clear it. The `host:` prefix cannot collide with + * the URI and filesystem-path namespaces `status.ts` documents. + */ +export const NODE_RUNTIME_STATUS_SOURCE = 'host:node-runtime'; + +/** `node --version` is instant when it works; a slow answer is a broken one. */ +const VERSION_PROBE_TIMEOUT_MS = 3_000; +const SHELL_PROBE_TIMEOUT_MS = 5_000; + +/** + * VS Code resolves the shell environment asynchronously while extensions are + * already activating, so a `node` that is genuinely installed can be missing + * from `process.env.PATH` for the first moments of a session. Upstream Vitest + * waits the same way. Only a `not-found` is retried — an executable that ran + * and failed will fail again, and paying the probe timeout six times over is + * how a pathological case turns into a minute of silence. + */ +const NOT_FOUND_RETRIES = 5; +const NOT_FOUND_RETRY_DELAY_MS = 200; + +const delay = (ms: number) => + new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * What became of running ` --version`. `ok` with no version is the + * soft pass: an unparseable version must never cost the feature (the rule lives + * in `checkVersion`). The three cases are a union rather than a boolean plus an + * optional field so that "not found, but here is a version" is unrepresentable. + */ +export type NodeProbe = + | { readonly kind: 'ok'; readonly version?: string } + /** Nothing to execute — the one case worth retrying. */ + | { readonly kind: 'not-found' } + /** It exists but did not answer: non-zero exit, or hung past the timeout. */ + | { readonly kind: 'unusable' }; + +export type WorkerNodeResolution = { + readonly executable: string; + /** Which candidate won. Drives the fallback notice, nothing else. */ + readonly source: 'path' | 'shell'; + /** Absent when the version could not be parsed (the soft pass). */ + readonly version?: string; +}; + +/** What each candidate turned out to be, in the order they were tried. */ +export type NodeAttempts = { + /** The PATH `node`'s version, or `undefined` when none answered. */ + readonly path?: string; + /** The shell's `node` version, or `undefined` when it found none. */ + readonly shell?: string; + /** True when the shell was never asked: Windows, or no known shell. */ + readonly shellSkipped: boolean; +}; + +const describeCandidate = (label: string, version: string | undefined) => + version === undefined ? `${label}: none found` : `${label}: ${version}`; + +const formatNodePreflightFailure = (attempts: NodeAttempts): string => { + const candidates = [ + describeCandidate('PATH', attempts.path), + attempts.shellSkipped + ? 'interactive shell: not probed' + : describeCandidate('interactive shell', attempts.shell), + ].join(', '); + return `No Node.js ${NODE_RUNTIME_RANGE} is available to run tests (${candidates}). Rstest needs it to load TypeScript config files. Install a newer Node.js, or set "rstack.rstest.nodeExecutable" to one.`; +}; + +/** No candidate satisfied `NODE_RUNTIME_RANGE`. */ +export class NodePreflightError extends Error { + constructor(readonly attempts: NodeAttempts) { + super(formatNodePreflightFailure(attempts)); + this.name = 'NodePreflightError'; + } +} + +export const probeNodeVersion = (executable: string): Promise => + new Promise((resolve) => { + execFile( + executable, + ['--version'], + { timeout: VERSION_PROBE_TIMEOUT_MS }, + (error, stdout) => { + if (error) { + // `ExecFileException.code` widens to `string | number`: a spawn + // failure carries the errno string, an exit carries the status. + resolve({ kind: error.code === 'ENOENT' ? 'not-found' : 'unusable' }); + return; + } + const version = stdout.trim().replace(/^v/, ''); + resolve({ kind: 'ok', version: version === '' ? undefined : version }); + }, + ); + }); + +const START_TOKEN = '__RSTACK_NODE_START__'; +const END_TOKEN = '__RSTACK_NODE_END__'; + +/** + * Asks the user's own shell where its `node` is, the way their terminal would + * answer. This is what makes a version manager work: its hooks live in the + * interactive rc files that the extension host's login-shell snapshot never + * ran. + * + * `node --version` runs first and its output is discarded — lazily-loading + * version managers define `node` as a shell function that only materializes a + * real binary once called, so without it `command -v` would find nothing. + * `command -v` rather than `which`, and no `[[ ]]`, so the script parses in + * fish as well as bash/zsh. The shell is spawned argv-style rather than through + * an outer `sh -c`, so no quoting is involved and the timeout kills the shell + * itself rather than a wrapper. + * + * Returns `undefined` on any failure; a shell that hangs on a slow rc file must + * cost a bounded wait, not the Test Explorer. + */ +export const probeShellNodePath = ( + shell: string, +): Promise => + new Promise((resolve) => { + const script = `node --version >/dev/null 2>&1; echo ${START_TOKEN}; command -v node; echo ${END_TOKEN}`; + const child = spawn(shell, ['-i', '-c', script], { + stdio: ['ignore', 'pipe', 'ignore'], + timeout: SHELL_PROBE_TIMEOUT_MS, + }); + let output = ''; + child.stdout.on('data', (chunk: Buffer) => { + output += chunk.toString(); + }); + child.on('error', () => resolve(undefined)); + child.on('close', () => { + const start = output.indexOf(START_TOKEN); + const end = output.indexOf(END_TOKEN); + if (start === -1 || end === -1) { + resolve(undefined); + return; + } + const found = output.slice(start + START_TOKEN.length, end).trim(); + // A bare name means the shell resolved `node` to something that is not a + // path; spawning it would only retry PATH. + resolve(found === '' || !found.startsWith('/') ? undefined : found); + }); + }); + +/** `undefined` when the probe could not produce a usable version at all. */ +const versionIfUsable = (probe: NodeProbe): string | undefined => + probe.kind === 'ok' ? probe.version : undefined; + +const satisfiesFloor = (probe: NodeProbe): boolean => + probe.kind === 'ok' && + checkVersion(probe.version, NODE_RUNTIME_RANGE).kind !== 'mismatch'; + +export type ResolveWorkerNodeOptions = { + /** The user's shell, for the interactive probe. Omit to skip that step. */ + readonly shell?: string; + readonly probe?: (executable: string) => Promise; + readonly probeShellPath?: (shell: string) => Promise; + readonly platform?: NodeJS.Platform; + /** Called once, only when the shell candidate had to stand in for PATH. */ + readonly notify?: (message: string) => void; +}; + +/** + * Picks the executable for a test worker. Callers holding an explicit + * `nodeExecutable` setting must not call this at all — an explicit choice is + * honored verbatim, unchecked, because it is the escape hatch for everything + * this function can get wrong. + * + * Throws `NodePreflightError` when no candidate satisfies the floor. + */ +export async function resolveWorkerNode({ + shell, + probe = probeNodeVersion, + probeShellPath = probeShellNodePath, + platform = process.platform, +}: ResolveWorkerNodeOptions = {}): Promise { + let onPath = await probe('node'); + for ( + let attempt = 0; + attempt < NOT_FOUND_RETRIES && onPath.kind === 'not-found'; + attempt++ + ) { + await delay(NOT_FOUND_RETRY_DELAY_MS); + onPath = await probe('node'); + } + if (satisfiesFloor(onPath)) { + return { + executable: 'node', + source: 'path', + version: versionIfUsable(onPath), + }; + } + + // Upstream Vitest skips the shell probe on Windows: there is no `-i -c` + // equivalent that reliably evaluates a user's profile across cmd/PowerShell. + const shellSkipped = platform === 'win32' || shell === undefined; + let fromShell: string | undefined; + if (!shellSkipped) { + const shellPath = await probeShellPath(shell); + if (shellPath !== undefined) { + const probed = await probe(shellPath); + if (satisfiesFloor(probed)) { + return { + executable: shellPath, + source: 'shell', + version: versionIfUsable(probed), + }; + } + fromShell = versionIfUsable(probed); + } + } + throw new NodePreflightError({ + path: versionIfUsable(onPath), + shell: fromShell, + shellSkipped, + }); +} + +/** + * The resolution is a property of the extension host, not of a project: there + * is one PATH and one shell, so a monorepo with N projects must not run N + * identical probes — nor announce the outcome N times, which is why `notify` is + * called from inside the memo and only on the pass that actually probes. The + * shell probe in particular costs a whole interactive shell startup. + * + * The rejection is memoized too, so a pathological host pays the probe timeouts + * once rather than once per project. + * + * `shell` and `notify` are options rather than imported singletons so this + * module stays free of any one stack's — and of VS Code's — globals, which is + * also what lets the tests drive it as a pure decision table. See the note in + * AGENTS.md before moving it to `shared/`. + */ +let cached: Promise | undefined; + +export const resolveWorkerNodeOnce = ( + options: ResolveWorkerNodeOptions = {}, +): Promise => + (cached ??= resolveWorkerNode(options).then((resolution) => { + if (resolution.source === 'shell') { + options.notify?.( + `Node.js on the extension host PATH cannot run tests (needs ${NODE_RUNTIME_RANGE}); using ${resolution.executable}${ + resolution.version ? ` (${resolution.version})` : '' + } from your shell instead`, + ); + } + return resolution; + })); + +export const resetWorkerNodeCache = (): void => { + cached = undefined; +}; diff --git a/packages/vscode/tests/stacks/test/nodeResolution.test.ts b/packages/vscode/tests/stacks/test/nodeResolution.test.ts new file mode 100644 index 0000000..c25903d --- /dev/null +++ b/packages/vscode/tests/stacks/test/nodeResolution.test.ts @@ -0,0 +1,351 @@ +import { afterEach, describe, expect, it } from '@rstest/core'; +import { NODE_RUNTIME_RANGE } from '../../../src/shared/versionCheck'; +import { + type NodeProbe, + NodePreflightError, + probeNodeVersion, + probeShellNodePath, + type ResolveWorkerNodeOptions, + resetWorkerNodeCache, + resolveWorkerNode, + resolveWorkerNodeOnce, +} from '../../../src/stacks/test/nodeResolution'; + +// Every case injects both probes: the point is the decision table, not the +// spawning, which the two real-process describes at the bottom cover. +const versionsOf = + (table: Record) => + (executable: string): Promise => + Promise.resolve(table[executable] ?? { kind: 'not-found' }); + +const shellFinds = + (path: string | undefined) => (): Promise => + Promise.resolve(path); + +const never = () => { + throw new Error('the shell probe should not have been called'); +}; + +const ok = (version: string): NodeProbe => ({ kind: 'ok', version }); + +const base = { shell: '/bin/zsh' } as const; + +/** Asserts the call rejects, and hands back the error already narrowed. */ +const preflightError = ( + options: ResolveWorkerNodeOptions, +): Promise => + resolveWorkerNode(options).then( + () => { + throw new Error('expected a NodePreflightError'); + }, + (error: unknown) => error as NodePreflightError, + ); + +describe('resolveWorkerNode', () => { + it('uses the PATH node when it satisfies the floor', async () => { + const resolution = await resolveWorkerNode({ + ...base, + probe: versionsOf({ node: ok('22.18.0') }), + probeShellPath: never, + }); + expect(resolution).toEqual({ + executable: 'node', + source: 'path', + version: '22.18.0', + }); + }); + + it('soft-passes a PATH node whose version cannot be parsed', async () => { + const resolution = await resolveWorkerNode({ + ...base, + probe: versionsOf({ node: { kind: 'ok' } }), + probeShellPath: never, + }); + expect(resolution.source).toBe('path'); + }); + + it('accepts a prerelease of a satisfying version', async () => { + // The shared `checkVersion` passes `includePrerelease: true`; this pins + // that the node floor now follows the same rule as every package floor. + const resolution = await resolveWorkerNode({ + ...base, + probe: versionsOf({ node: ok('23.0.0-nightly20260101') }), + probeShellPath: never, + }); + expect(resolution.source).toBe('path'); + }); + + it('falls back to the shell node when the PATH node is too old', async () => { + const resolution = await resolveWorkerNode({ + ...base, + probe: versionsOf({ + node: ok('20.19.4'), + '/versions/24/bin/node': ok('24.0.0'), + }), + probeShellPath: shellFinds('/versions/24/bin/node'), + }); + expect(resolution).toEqual({ + executable: '/versions/24/bin/node', + source: 'shell', + version: '24.0.0', + }); + }); + + it('falls back to the shell node when no PATH node runs at all', async () => { + const resolution = await resolveWorkerNode({ + ...base, + probe: versionsOf({ '/versions/24/bin/node': ok('24.0.0') }), + probeShellPath: shellFinds('/versions/24/bin/node'), + }); + expect(resolution.source).toBe('shell'); + }); + + it('retries a not-found PATH node before giving up on it', async () => { + // VS Code resolves the shell env while extensions are already activating, + // so an installed node can be absent for the first moments of a session. + let calls = 0; + const resolution = await resolveWorkerNode({ + ...base, + probe: (executable) => { + if (executable !== 'node') + return Promise.resolve({ kind: 'not-found' }); + calls++; + return Promise.resolve( + calls < 3 ? { kind: 'not-found' } : ok('24.0.0'), + ); + }, + probeShellPath: never, + }); + expect(resolution.source).toBe('path'); + expect(calls).toBe(3); + }); + + it('does not retry a node that ran and failed', async () => { + // Retrying only helps a PATH that is not populated yet. An executable that + // exists but hangs would otherwise cost the probe timeout six times over. + let calls = 0; + const error = await preflightError({ + ...base, + probe: (executable) => { + if (executable !== 'node') + return Promise.resolve({ kind: 'not-found' }); + calls++; + return Promise.resolve({ kind: 'unusable' }); + }, + probeShellPath: shellFinds(undefined), + }); + expect(calls).toBe(1); + expect(error.attempts.path).toBeUndefined(); + }); + + it('skips the shell probe on Windows', async () => { + const error = await preflightError({ + ...base, + platform: 'win32', + probe: versionsOf({ node: ok('20.19.4') }), + probeShellPath: never, + }); + expect(error.attempts).toEqual({ + path: '20.19.4', + shell: undefined, + shellSkipped: true, + }); + }); + + it('skips the shell probe when no shell is known', async () => { + const error = await preflightError({ + probe: versionsOf({ node: ok('20.19.4') }), + probeShellPath: never, + }); + expect(error.attempts.shellSkipped).toBe(true); + }); + + it('reports both candidates when the shell node is also too old', async () => { + const error = await preflightError({ + ...base, + probe: versionsOf({ + node: ok('20.19.4'), + '/versions/22/bin/node': ok('22.14.0'), + }), + probeShellPath: shellFinds('/versions/22/bin/node'), + }); + expect(error.attempts).toEqual({ + path: '20.19.4', + shell: '22.14.0', + shellSkipped: false, + }); + }); + + it('reports a shell that found nothing', async () => { + const error = await preflightError({ + ...base, + probe: versionsOf({ node: ok('20.19.4') }), + probeShellPath: shellFinds(undefined), + }); + expect(error.attempts).toEqual({ + path: '20.19.4', + shell: undefined, + shellSkipped: false, + }); + }); +}); + +describe('NodePreflightError', () => { + it('names the floor, both candidates and the setting to change', () => { + const { message } = new NodePreflightError({ + path: '20.19.4', + shell: '22.14.0', + shellSkipped: false, + }); + expect(message).toContain(NODE_RUNTIME_RANGE); + expect(message).toContain('PATH: 20.19.4'); + expect(message).toContain('interactive shell: 22.14.0'); + expect(message).toContain('rstack.rstest.nodeExecutable'); + }); + + it('does not invent versions for candidates that found nothing', () => { + const { message } = new NodePreflightError({ + path: undefined, + shell: undefined, + shellSkipped: false, + }); + expect(message).toContain('PATH: none found'); + expect(message).not.toContain('undefined'); + }); + + it('says the shell was not probed without blaming Windows', () => { + // `shellSkipped` also covers "no known shell" on macOS/Linux, so the text + // must not claim a platform it cannot know. + const { message } = new NodePreflightError({ + path: '20.19.4', + shell: undefined, + shellSkipped: true, + }); + expect(message).toContain('interactive shell: not probed'); + expect(message).not.toContain('Windows'); + }); +}); + +describe('resolveWorkerNodeOnce', () => { + afterEach(() => { + resetWorkerNodeCache(); + }); + + const options = { + ...base, + probe: versionsOf({ node: ok('24.0.0') }), + probeShellPath: never, + }; + + it('probes once for the whole extension host and re-probes after a reset', async () => { + const first = await resolveWorkerNodeOnce(options); + const second = await resolveWorkerNodeOnce(options); + expect(second).toBe(first); + + resetWorkerNodeCache(); + const third = await resolveWorkerNodeOnce(options); + expect(third).not.toBe(first); + }); + + it('memoizes the rejection, so a broken host pays the probes once', async () => { + let calls = 0; + const failing = { + ...base, + probe: () => { + calls++; + return Promise.resolve(ok('20.19.4')); + }, + probeShellPath: shellFinds(undefined), + }; + await resolveWorkerNodeOnce(failing).catch(() => {}); + await resolveWorkerNodeOnce(failing).catch(() => {}); + expect(calls).toBe(1); + }); + + it('announces the shell fallback exactly once', async () => { + // The notice lives inside the memo, so "probed once" and "announced once" + // are the same guarantee — this is what keeps a 20-project monorepo from + // logging the fallback 20 times. + const notices: string[] = []; + const shellOptions = { + ...base, + probe: versionsOf({ + node: ok('20.19.4'), + '/versions/24/bin/node': ok('24.0.0'), + }), + probeShellPath: shellFinds('/versions/24/bin/node'), + notify: (message: string) => notices.push(message), + }; + + await resolveWorkerNodeOnce(shellOptions); + await resolveWorkerNodeOnce(shellOptions); + expect(notices).toHaveLength(1); + expect(notices[0]).toContain('/versions/24/bin/node'); + }); + + it('stays silent when the PATH node was good enough', async () => { + const notices: string[] = []; + await resolveWorkerNodeOnce({ + ...options, + notify: (message) => notices.push(message), + }); + expect(notices).toHaveLength(0); + }); + + it('shares one in-flight probe across concurrent callers', async () => { + // N projects are constructed in one synchronous loop, so they all reach + // the memo before the first probe settles. + let calls = 0; + const slow = { + ...base, + probe: () => { + calls++; + return new Promise((resolve) => + setTimeout(() => resolve(ok('24.0.0')), 10), + ); + }, + probeShellPath: never, + }; + const all = await Promise.all( + Array.from({ length: 20 }, () => resolveWorkerNodeOnce(slow)), + ); + expect(calls).toBe(1); + expect(new Set(all).size).toBe(1); + }); +}); + +describe('probeNodeVersion', () => { + it('reads the version of a real node executable', async () => { + // The test run's own node is the one binary guaranteed to exist. + const probe = await probeNodeVersion(process.execPath); + expect(probe).toEqual({ kind: 'ok', version: process.versions.node }); + }); + + it('reports a nonexistent executable as not-found, so it is retried', async () => { + const probe = await probeNodeVersion('/nonexistent/definitely-not-node'); + expect(probe).toEqual({ kind: 'not-found' }); + }); + + it('reports an executable that exits non-zero as unusable, not retried', async () => { + const probe = await probeNodeVersion('/usr/bin/false'); + expect(probe.kind === 'unusable' || probe.kind === 'not-found').toBe(true); + }); +}); + +describe('probeShellNodePath', () => { + it('returns undefined when the shell itself cannot be run', async () => { + const found = await probeShellNodePath( + '/nonexistent/definitely-not-a-shell', + ); + expect(found).toBeUndefined(); + }); + + it('slices the answer out of a real shell run', async () => { + if (process.platform === 'win32') return; + // `sh` runs the script and `command -v node` resolves against the test + // run's own PATH, which necessarily has a node on it. + const found = await probeShellNodePath('/bin/sh'); + expect(found).toBeDefined(); + expect(found?.startsWith('/')).toBe(true); + }); +}); From 61a67fa04cba07f993667199cf3d6caffd0bc176 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 10 Aug 2026 20:12:39 +0800 Subject: [PATCH 2/5] feat(vscode): rework the status hover card and give version notices a surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/vscode/src/statusBar.ts | 360 ++++++++++++++++++------ packages/vscode/tests/statusBar.test.ts | 347 +++++++++++++++++++++++ 2 files changed, 622 insertions(+), 85 deletions(-) create mode 100644 packages/vscode/tests/statusBar.test.ts diff --git a/packages/vscode/src/statusBar.ts b/packages/vscode/src/statusBar.ts index e4d88be..a471d2e 100644 --- a/packages/vscode/src/statusBar.ts +++ b/packages/vscode/src/statusBar.ts @@ -8,54 +8,195 @@ import { stackCommand, } from './types'; +/** The item's resting look: no stack is in a state worth colouring for. */ +const IDLE_ITEM_TEXT = '$(zap) Rstack'; + /** - * Icon and hover colour per state. `color` is a theme colour id with `.` - * replaced by `-`, the form VS Code exposes as a CSS variable; the markdown - * sanitizer accepts `var(--vscode-*)` on a `` and nothing else, so the - * hover picks up the user's theme instead of hard-coded hexes. + * Icon, hover colour and detail policy per state. `color` is a theme colour id + * with `.` replaced by `-`, the form VS Code exposes as a CSS variable; the + * markdown sanitizer accepts `var(--vscode-*)` on a `` and nothing else, + * so the hover picks up the user's theme instead of hard-coded hexes. + * + * `spellsOutDetail` decides whether the state's detail gets its own row in the + * hover body. It is a property of the state kind rather than of the message, + * so a state added here has to answer the question once and no call site + * special-cases a particular status. Say yes for the states the user cannot + * act on without reading the words ("crashed — ", "no Node.js + * >=22.18.0 is available"); say no for the ones whose detail is progress + * bookkeeping ("running — 2 folders"), which would otherwise put a second row + * under every stack in the healthy case and double the card's height for + * nothing. + * + * `severity` ranks the states so the item can show the worst one across stacks, + * and `item` is how that winner paints itself — absent means the idle look. + * Both live here rather than in `render` so that a seventh state cannot be + * added without ranking itself: a nested-ternary ladder over the kinds, which + * is what this replaced, silently rendered anything it had not heard of as + * idle. */ const STATE_STYLES: Readonly< - Record + Record< + StackState['kind'], + { + readonly icon: string; + readonly color: string; + readonly spellsOutDetail: boolean; + readonly severity: number; + readonly item?: { + readonly text: string; + readonly background?: string; + }; + } + > > = { // The two off-states share a glyph but not a colour: nothing found here (the // weakest thing on the row) versus somebody turned it off on purpose, which // is worth reading. Keep every state on the plain codicon set — glyphs from // the debug sets are drawn at their own optical size and stick out. - 'not-detected': { icon: '$(circle-slash)', color: 'disabledForeground' }, - disabled: { icon: '$(circle-slash)', color: 'descriptionForeground' }, - starting: { icon: '$(loading~spin)', color: 'descriptionForeground' }, - running: { icon: '$(check)', color: 'testing-iconPassed' }, - crashed: { icon: '$(error)', color: 'testing-iconFailed' }, + 'not-detected': { + icon: '$(circle-slash)', + color: 'disabledForeground', + spellsOutDetail: false, + severity: 0, + }, + disabled: { + icon: '$(circle-slash)', + color: 'descriptionForeground', + // "why is nothing happening" is exactly what this row is asked, and the + // reason (Restricted Mode, a kill switch) is the answer. + spellsOutDetail: true, + severity: 0, + }, + starting: { + icon: '$(loading~spin)', + color: 'descriptionForeground', + spellsOutDetail: false, + severity: 2, + item: { text: '$(loading~spin) Rstack' }, + }, + running: { + icon: '$(check)', + color: 'testing-iconPassed', + spellsOutDetail: false, + // Outranks the off-states so a window with one live stack does not look + // idle, but carries no `item`: healthy is the idle look. + severity: 1, + }, + crashed: { + icon: '$(error)', + color: 'testing-iconFailed', + spellsOutDetail: true, + severity: 4, + item: { + text: '$(error) Rstack', + background: 'statusBarItem.errorBackground', + }, + }, 'version-mismatch': { icon: '$(warning)', color: 'editorWarning-foreground', + spellsOutDetail: true, + severity: 3, + item: { + // Keeps the idle glyph, unlike every other state that colours the item. + // A version mismatch is advisory — the run goes ahead — so the amber + // background is the whole signal; swapping the glyph too reads as + // "stopped", which is the one thing that has not happened. + text: IDLE_ITEM_TEXT, + background: 'statusBarItem.warningBackground', + }, }, }; -const stateText = (state: StackState): string => { +/** + * Columns in the hover table: the state icon, the label, and one per action + * slot. The actions get a column each rather than sharing a cell — see the row + * builder for why that is structural, not cosmetic. The slot count is derived + * from the widest row of the render in hand rather than pinned to a constant, + * so an action added to the row builder cannot be dropped off the end of a + * fixed-width table — a failure whose whole symptom is an icon that never + * appears. + */ +const tableColumns = (slots: number): number => 2 + slots; + +/** + * The card's width, in CSS px: compact while every stack is quiet, wider when + * a notice needs room to read. The hover is shrink-to-fit with a 500px cap, so + * left alone it renders however wide its longest line happens to be — a fixed + * `width` on the table pins it per state instead. The pin is also what lets + * the notices live *in* the table: a fixed-width table cannot be stretched by + * a wide cell (text wraps instead), which is what forced them out of it + * before. `width` is on the markdown sanitizer's attribute allow-list; a + * stray unbreakable token (an absolute path) can still widen the table past + * this, which is accepted — a broken path cannot be copied. + */ +const CARD_WIDTH = 140; +const CARD_WIDTH_WITH_NOTICES = 250; + +/** + * The free text a state carries, if any — a crash message, a version + * complaint, a disable reason. It is its own function because the hover renders + * the two halves of a state in different places — the kind is the icon, the + * detail is prose — and the switch is exhaustive, so a state kind added to the + * union has to say here whether it carries words. + */ +const stateDetail = (state: StackState): string | undefined => { switch (state.kind) { case 'not-detected': - return 'not detected'; + return undefined; case 'disabled': - return state.reason ? `disabled — ${state.reason}` : 'disabled'; + return state.reason; case 'starting': - return state.detail ? `starting — ${state.detail}` : 'starting'; case 'running': - return state.detail ? `running — ${state.detail}` : 'running'; case 'crashed': - return `crashed — ${state.detail}`; case 'version-mismatch': - return `version mismatch — ${state.detail}`; + return state.detail; } }; -const escapeAttribute = (value: string): string => +/** + * The one-line form: the state's kind, plus its detail when it has one. This + * is what the log records and what the icon's native tooltip says, so its + * prefix has to stay distinct per kind — `setState` discriminates transitions + * by this string alone. + */ +const stateText = (state: StackState): string => { + // The ids read as prose once their hyphen is a space ('version-mismatch' → + // 'version mismatch'); no kind has a second one. + const kind = state.kind.replace('-', ' '); + const detail = stateDetail(state); + return detail ? `${kind} — ${detail}` : kind; +}; + +/** + * One escaper for both positions text can land in — an attribute value and a + * cell's contents. Quoting `"` is redundant in a cell and escaping `<`/`>` is + * redundant in an attribute, but a message is attacker-adjacent free text (a + * tool's stderr, a user's paths) landing in a *trusted* `supportHtml` + * MarkdownString, so there is exactly one function to get right rather than a + * pair to pick between under pressure. + */ +const escapeHtml = (value: string): string => value .replace(/&/g, '&') .replace(//g, '>') .replace(/"/g, '"'); +/** + * A state's glyph in its state colour — the row's leading cell, and the + * heading of that state's notice, so the two read as one status rather than as + * a second opinion. One builder because the colour has to arrive as a `style` + * on a ``: the markdown sanitizer drops `style` on every other element, + * and keeps only `color` even here. + */ +const stateIcon = ( + style: (typeof STATE_STYLES)[StackState['kind']], + title?: string, +): string => + `${style.icon}`; + /** * The one anchor builder, so escaping is a property of the markup rather than * something each call site has to remember. `title` becomes the icon's native @@ -63,7 +204,7 @@ const escapeAttribute = (value: string): string => */ const anchor = (command: string, body: string, title?: string): string => `${body}`; /** @@ -71,9 +212,29 @@ const anchor = (command: string, body: string, title?: string): string => * the stack rows'. Icon and label are separate cells and therefore separate * anchors to the same command — one `` cannot span two cells. */ -const actionRow = (command: string, icon: string, label: string): string => +const actionRow = ( + columns: number, + command: string, + icon: string, + label: string, +): string => `${anchor(command, icon)}` + - `${anchor(command, ` ${label}`)}`; + `` + + `${anchor(command, ` ${label}`)}`; + +/** + * A rule and the gap under it, between two halves of the card. The gap is an + * empty spacer row with an explicit `height`, the one pixel-precise spacing + * lever sanitized html has left: cell padding is unreachable (`style` survives + * only on a span, colours only) and everything line-based is quantized to a + * whole row. An empty cell has no line box, so its `height` is what it says. + * Above the rule the hover's own `hr { margin-top: 4px }` is enough — and its + * `margin-bottom: -4px` is why the underside needs the spacer at all. + */ +const sectionBreak = (columns: number): string[] => [ + `
`, + ``, +]; /** * The single always-present status bar item. It is visible @@ -159,39 +320,18 @@ export class StatusBar implements vscode.Disposable { } private render(): void { + // `STACK_IDS` is a non-empty tuple, so the seedless reduce cannot throw. + // Ties keep the earliest stack, matching the `find`-per-kind ladder this + // replaced. const states = STACK_IDS.map((stack) => this.stateOf(stack)); - const worst = states.find((state) => state.kind === 'crashed') - ? 'crashed' - : states.find((state) => state.kind === 'version-mismatch') - ? 'version-mismatch' - : states.find((state) => state.kind === 'starting') - ? 'starting' - : states.find((state) => state.kind === 'running') - ? 'running' - : 'idle'; - - switch (worst) { - case 'crashed': - this.#item.text = '$(error) Rstack'; - this.#item.backgroundColor = new vscode.ThemeColor( - 'statusBarItem.errorBackground', - ); - break; - case 'version-mismatch': - this.#item.text = '$(warning) Rstack'; - this.#item.backgroundColor = new vscode.ThemeColor( - 'statusBarItem.warningBackground', - ); - break; - case 'starting': - this.#item.text = '$(loading~spin) Rstack'; - this.#item.backgroundColor = undefined; - break; - default: - this.#item.text = '$(zap) Rstack'; - this.#item.backgroundColor = undefined; - break; - } + const worst = states.reduce((a, b) => + STATE_STYLES[b.kind].severity > STATE_STYLES[a.kind].severity ? b : a, + ); + const item = STATE_STYLES[worst.kind].item; + this.#item.text = item?.text ?? IDLE_ITEM_TEXT; + this.#item.backgroundColor = item?.background + ? new vscode.ThemeColor(item.background) + : undefined; const tooltip = new vscode.MarkdownString(undefined, true); // Command links are only rendered in trusted markdown. @@ -202,8 +342,20 @@ export class StatusBar implements vscode.Disposable { // inside it, so the cells use ``/`` rather than `**`/`[]()` — the // sanitizer keeps `command:` hrefs as long as the string stays trusted. tooltip.supportHtml = true; - const rows = STACK_IDS.map((stack) => { - const state = this.stateOf(stack); + // Messages worth reading, collected while the rows are built and rendered + // under their own section break at the bottom — the whole point of showing + // them at all, rather than in a nested `title` nobody hovers twice to read. + // The state itself is carried, not its markup: everything the card renders + // is built in one place, below. + const notices: { + style: (typeof STATE_STYLES)[StackState['kind']]; + label: string; + detail: string; + }[] = []; + // Cells first, rows second: the column count is a property of the whole + // render, so every row has to be known before any of them can be written. + const cells = STACK_IDS.map((stack, index) => { + const state = states[index] ?? { kind: 'not-detected' as const }; const style = STATE_STYLES[state.kind]; const label = STACK_LABELS[stack]; // The per-stack actions repeat once per row, so they are icon-only: the @@ -226,47 +378,85 @@ export class StatusBar implements vscode.Disposable { ), ); } - // The state text is the icon's title rather than row text: spelling out - // "running — 2 folders" on every row is noise once the icon says it, and - // the details worth reading (a crash message, a version mismatch) are - // exactly the long ones. `state.detail` is arbitrary text a stack - // produced, hence the escaping. - const status = - `${style.icon}`; + // The state text stays on the icon's title so every row, including the + // ones whose detail is not spelled out below, can still be read in full. + // `stateText` embeds arbitrary text a stack produced, hence the escaping. + const status = stateIcon(style, stateText(state)); + const detail = style.spellsOutDetail + ? stateDetail(state)?.trim() + : undefined; + if (detail) { + notices.push({ style, label, detail }); + } + return { status, label, actions }; + }); + const slots = Math.max(...cells.map(({ actions }) => actions.length)); + const columns = tableColumns(slots); + const rows = cells.map(({ status, label, actions }) => { + // `width="100%"` on the label cell makes it absorb the table's slack, so + // the actions stay pinned to the card's right edge instead of drifting + // inwards once the notice below widens the card. + // + // One column per action, rather than both icons in one cell. That is + // structural: claiming the slack squeezes every other column to its + // min-content width, and the hover's `overflow-wrap` makes that one icon + // wide — so two icons sharing a cell wrapped onto two lines and doubled + // the row's height, which ` ` does not prevent and which sanitized + // html cannot fix from the outside (`nowrap` is not on the attribute + // allow-list, and `style` survives only on a ``, colours only). A + // cell holding a single inline element has no break opportunity at all, + // so the question stops being how narrow the column may get. + const actionCells = Array.from( + { length: slots }, + (_, index) => `${actions[index] ?? ''}`, + ); return ( - `${status} ${label}  ` + - `${actions.join(' ')}` + `${status}` + + ` ${label}  ` + + `${actionCells.join('')}` ); }); // In the same table as the stacks so all six icons share one column; a // second table would size its columns independently and the two halves - // would drift apart. One row per action rather than three across, for the - // same reason the actions above are icon-only — the hover sizes to its - // content, so the widest line sets the card's width. + // would drift apart. One row per action rather than three across, because + // three labelled actions do not fit across a card this narrow — they would + // wrap, and a wrapped row of links reads as one ragged paragraph. // // Unlike the per-stack restarts, "Relaunch" is unconditional: it is the // action for "nothing is active", which is precisely when no per-stack // restart is offered. const shellActions = [ - actionRow('rstack.restart', '$(debug-restart)', 'Relaunch'), - actionRow('rstack.showOutput', '$(selection)', 'Extension log'), - actionRow('rstack.migrateSettings', '$(arrow-right)', 'Migrate settings'), + actionRow(columns, 'rstack.restart', '$(debug-restart)', 'Relaunch'), + actionRow(columns, 'rstack.showOutput', '$(selection)', 'Extension log'), + actionRow( + columns, + 'rstack.migrateSettings', + '$(arrow-right)', + 'Migrate settings', + ), ]; - // The gap under the divider is an empty spacer row with an explicit - // `height`, the one pixel-precise spacing lever sanitized html has left: - // cell padding is unreachable (`style` survives only on a span, colours - // only) and everything line-based is quantized to a whole row. An empty - // cell has no line box, so its `height` is what it says. Above the rule the - // hover's own `hr { margin-top: 4px }` is enough — and its - // `margin-bottom: -4px` is why the underside needs the spacer at all. - const body = [ - ...rows, - '
', - '', - ...shellActions, - ].join(''); - tooltip.appendMarkdown(`${body}
`); + const body = [...rows, ...sectionBreak(columns), ...shellActions]; + // The notices sit under their own divider, one full-width cell each. Named + // by their stack, because down here a message has lost the row it belonged + // to. No hand-wrapping: the fixed table width is what the text wraps to, + // and the renderer places the breaks. Only the line breaks the message + // itself wrote — a stack trace, a numbered remedy — are kept, as `
`. + if (notices.length > 0) { + body.push( + ...sectionBreak(columns), + ...notices.map(({ style, label, detail }) => { + const prose = escapeHtml(detail.replace(/\r\n/g, '\n')) + .split('\n') + .join('
'); + return ( + `` + + `${stateIcon(style)} ${label}
${prose}` + ); + }), + ); + } + const width = notices.length > 0 ? CARD_WIDTH_WITH_NOTICES : CARD_WIDTH; + tooltip.appendMarkdown(`${body.join('')}
`); this.#item.tooltip = tooltip; } diff --git a/packages/vscode/tests/statusBar.test.ts b/packages/vscode/tests/statusBar.test.ts new file mode 100644 index 0000000..831c535 --- /dev/null +++ b/packages/vscode/tests/statusBar.test.ts @@ -0,0 +1,347 @@ +/** + * The hover markup is the unit under test. It is html assembled by hand into a + * *trusted* `MarkdownString`, so the assertions here are deliberately about the + * literal string: which rows exist, and what a stack's free-text detail looks + * like once it has been through the escaper. `vscode` is stubbed down to the + * four things the status bar touches — unit tests run in plain Node, and E2E + * stays the ground truth for how the card actually renders. + */ +import { beforeEach, describe, expect, it, rs } from '@rstest/core'; +import type vscode from 'vscode'; + +interface FakeItem { + name: string; + text: string; + tooltip: { value: string; isTrusted: boolean; supportHtml: boolean } | null; + command: string; + backgroundColor: { id: string } | undefined; + show(): void; + hide(): void; + dispose(): void; +} + +const harness = rs.hoisted(() => ({ items: [] as FakeItem[] })); + +rs.mock('vscode', () => { + const vscode = { + StatusBarAlignment: { Left: 1, Right: 2 }, + ThemeColor: class { + constructor(readonly id: string) {} + }, + MarkdownString: class { + value: string; + isTrusted = false; + supportHtml = false; + constructor(value?: string) { + this.value = value ?? ''; + } + appendMarkdown(text: string): this { + this.value += text; + return this; + } + }, + window: { + createStatusBarItem: (): FakeItem => { + const item: FakeItem = { + name: '', + text: '', + tooltip: null, + command: '', + backgroundColor: undefined, + show: () => undefined, + hide: () => undefined, + dispose: () => undefined, + }; + harness.items.push(item); + return item; + }, + }, + }; + return { ...vscode, default: vscode }; +}); + +import { StatusBar } from '../src/statusBar'; + +/** The status bar only ever logs transitions, so a sink is enough. */ +const output = { + info: () => undefined, +} as unknown as vscode.LogOutputChannel; + +const rowsOf = (html: string): string[] => + html + .split('') + .slice(1) + .map((row) => row.split('')[0] ?? ''); + +/** + * The notice rows: everything under the second divider, spacer skipped. An + * empty array while every stack is quiet — the divider itself only appears + * with something to put under it. + */ +const noticesOf = (html: string): string[] => { + const rows = rowsOf(html); + const dividers = rows.flatMap((row, i) => (row.includes('
') ? [i] : [])); + const second = dividers[1]; + return second === undefined ? [] : rows.slice(second + 2); +}; + +/** The rows of the stack half — everything above the divider. */ +const stackRows = (html: string): string[] => { + const rows = rowsOf(html); + const divider = rows.findIndex((row) => row.includes('
')); + return rows.slice(0, divider); +}; + +const build = (): { bar: StatusBar; html: () => string } => { + const bar = new StatusBar(output); + const item = harness.items.at(-1); + if (!item) { + throw new Error('no status bar item was created'); + } + return { bar, html: () => item.tooltip?.value ?? '' }; +}; + +beforeEach(() => { + harness.items.length = 0; +}); + +describe('StatusBar hover', () => { + it('shows one row per stack and the shell actions when nothing is detected', () => { + const { html } = build(); + expect(stackRows(html())).toHaveLength(3); + // Divider, spacer, and the three shell actions still follow the stacks. + expect(rowsOf(html())).toHaveLength(8); + expect(html()).toContain('command:rstack.restart'); + expect(html()).toContain('command:rstack.showOutput'); + expect(html()).toContain('command:rstack.migrateSettings'); + }); + + it('spells a failure message out below the table, named by its stack', () => { + const { bar, html } = build(); + bar.setState('rstest', { + kind: 'version-mismatch', + detail: + 'No Node.js >=22.18.0 is available to run tests. Tests will not run.', + }); + // In the table, but under its own divider — and it cannot stretch the + // rows above it, because the table's width is pinned. One cell, left for + // the renderer to wrap at the card's edge; no hand-wrapping. + const rows = stackRows(html()); + expect(rows).toHaveLength(3); + // Three columns, not four: no stack is active here, so no row carries a + // restart icon and the table is one slot narrower. + expect(noticesOf(html())).toEqual([ + '' + + '' + + '$(warning) Rstest
' + + 'No Node.js >=22.18.0 is available to run tests. ' + + 'Tests will not run.', + ]); + // A notice widens the card; quiet again, it narrows back. + expect(html()).toContain(''); + bar.setState('rstest', { kind: 'running' }); + expect(html()).toContain('
'); + bar.setState('rstest', { + kind: 'version-mismatch', + detail: + 'No Node.js >=22.18.0 is available to run tests. Tests will not run.', + }); + // The icon and its warning colour are untouched. + expect(rows[1]).toContain('Rstest'); + expect(rows[1]).toContain('$(warning)'); + expect(rows[1]).toContain('color:var(--vscode-editorWarning-foreground);'); + }); + + it('gives every action its own cell, so no row can wrap', () => { + // The hover's `overflow-wrap` computes a squeezed column's minimum as one + // icon wide, so two icons sharing a cell broke onto two lines. One icon per + // cell leaves no break opportunity at all — the width of the column stops + // mattering. The label cell still claims the slack, to keep the actions + // pinned to the card's right edge. + const { bar, html } = build(); + bar.setActive('rstest', true); + const row = stackRows(html())[1] ?? ''; + expect(html()).toContain('
'); + expect(row).toContain('', + ); + }); + + it('escapes the message everywhere it lands in the markup', () => { + const { bar, html } = build(); + bar.setState('rslint', { + kind: 'crashed', + detail: ' failed at a>b && c', + }); + const escaped = + '<img src=x onerror="alert(1)"> failed at a>b ' + + '&& c'; + // Both the prose notice and the icon's title attribute, which the message + // also reaches through `stateText`. + expect(stackRows(html())[0]).toContain(`title="crashed — ${escaped}"`); + expect(noticesOf(html())[0]).toContain(`Rslint
${escaped}`); + expect(html()).not.toContain(' { + // The whole card is one html block, and markdown-it does not run inline + // markdown inside an html block — so underscores and asterisks need no + // backslashes, and adding them would render them. + const { bar, html } = build(); + bar.setState('rslint', { + kind: 'crashed', + detail: 'cannot read __tests__/a_b.ts or *.config.*', + }); + expect(noticesOf(html())[0]).toContain( + 'cannot read __tests__/a_b.ts or *.config.*', + ); + }); + + it('keeps a multi-line message on multiple lines', () => { + const { bar, html } = build(); + bar.setState('fmt', { + kind: 'crashed', + detail: ' rs fmt exited with 1\nstderr: broken config\n', + }); + // Trimmed, so a message ending in a newline does not draw a blank line; + // inside a cell the message's own breaks become `
`. + expect(noticesOf(html())[0]).toContain( + 'rs fmt
rs fmt exited with 1
stderr: broken config', + ); + }); + + it('gives a stack with no message no extra row', () => { + const { bar, html } = build(); + // `running — 2 folders` is bookkeeping: it stays on the icon's title. + bar.setState('rslint', { kind: 'running', detail: '2 folders' }); + bar.setState('rstest', { kind: 'starting' }); + bar.setState('fmt', { kind: 'disabled' }); + const rows = stackRows(html()); + expect(rows).toHaveLength(3); + expect(rows[0]).toContain('title="running — 2 folders"'); + // `disabled` does spell its reason out — when it has one. Absent, it must + // not leave a stray notice (or its divider) behind. + expect(noticesOf(html())).toEqual([]); + expect(rowsOf(html())).toHaveLength(8); + }); + + it('spells out why a stack was deliberately turned off', () => { + const { bar, html } = build(); + bar.setState('fmt', { + kind: 'disabled', + reason: 'Restricted Mode: no processes are spawned', + }); + expect(noticesOf(html())[0]).toContain( + 'rs fmt
Restricted Mode: no processes are spawned', + ); + }); + + it('leaves a long message unbroken for the cell to wrap', () => { + // The fixed table width is what the text wraps to, and the renderer + // places the breaks; inserting them here would put them somewhere the + // renderer has not measured. + const path = + '/Users/somebody/very/deeply/nested/workspace/packages/app/rstest.config.ts'; + const { bar, html } = build(); + bar.setState('rstest', { kind: 'crashed', detail: `cannot read ${path}` }); + expect(noticesOf(html())[0]).toContain(`
cannot read ${path}`); + }); + + it('keeps the hover trusted html, which is what makes escaping load-bearing', () => { + build(); + const tooltip = harness.items.at(-1)?.tooltip; + expect(tooltip?.isTrusted).toBe(true); + expect(tooltip?.supportHtml).toBe(true); + }); +}); + +/** + * The item itself, not the hover. Its look is the worst state across the + * stacks, which is the one thing a user sees without hovering at all. + */ +describe('StatusBar item', () => { + const itemOf = () => { + const item = harness.items.at(-1); + if (!item) { + throw new Error('no status bar item was created'); + } + return item; + }; + + it('is idle when nothing is detected', () => { + build(); + expect(itemOf().text).toBe('$(zap) Rstack'); + expect(itemOf().backgroundColor).toBeUndefined(); + }); + + it('colours for a version mismatch but keeps the idle glyph', () => { + // The mismatch is advisory — the run goes ahead — so the amber background + // carries it alone. An `$(warning)` glyph here would read as "stopped". + const { bar } = build(); + bar.setState('rstest', { kind: 'version-mismatch', detail: 'old node' }); + expect(itemOf().text).toBe('$(zap) Rstack'); + expect(itemOf().backgroundColor?.id).toBe( + 'statusBarItem.warningBackground', + ); + }); + + it('lets a crash outrank a mismatch', () => { + const { bar } = build(); + bar.setState('rstest', { kind: 'version-mismatch', detail: 'old node' }); + bar.setState('rslint', { kind: 'crashed', detail: 'server died' }); + expect(itemOf().text).toBe('$(error) Rstack'); + expect(itemOf().backgroundColor?.id).toBe('statusBarItem.errorBackground'); + }); + + it('lets a mismatch outrank a healthy sibling', () => { + // Same glyph as healthy, so the background is what tells them apart. + const { bar } = build(); + bar.setState('rslint', { kind: 'running', detail: '2 folders' }); + bar.setState('fmt', { kind: 'running' }); + bar.setState('rstest', { kind: 'version-mismatch', detail: 'old node' }); + expect(itemOf().backgroundColor?.id).toBe( + 'statusBarItem.warningBackground', + ); + }); + + it('stays idle-looking while healthy', () => { + const { bar } = build(); + bar.setState('rslint', { kind: 'running', detail: '2 folders' }); + expect(itemOf().text).toBe('$(zap) Rstack'); + expect(itemOf().backgroundColor).toBeUndefined(); + }); +}); From 9705005a04271e58006a521c1462c9e4875f6047 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 10 Aug 2026 20:12:41 +0800 Subject: [PATCH 3/5] refactor(vscode): declare settings-triggered restarts on the controller 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. --- packages/vscode/src/extension.ts | 41 +++++++-- packages/vscode/src/stacks/lint/index.ts | 20 ++--- packages/vscode/src/types.ts | 23 ++--- packages/vscode/tests/extension.test.ts | 109 ++++++++++++++++++++++- 4 files changed, 158 insertions(+), 35 deletions(-) diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 739688f..2a9a0ea 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -68,14 +68,38 @@ class ExtensionShell { this.scheduleReconcile(); }), vscode.workspace.onDidChangeConfiguration((event) => { - const affectsGate = - event.affectsConfiguration('rstack.enable') || - STACK_IDS.some((stack) => - event.affectsConfiguration(`rstack.${stack}.enable`), - ); - if (affectsGate) { + // One event covers a whole batch of edits — saving settings.json moves + // everything at once — so the two paths are decided per stack rather + // than one short-circuiting the other. A stack whose gate moved is the + // reconcile's to deal with, and rebuilding it here would fight that: + // it may be on its way out. + const gated = new Set( + STACK_IDS.filter( + (stack) => + event.affectsConfiguration('rstack.enable') || + event.affectsConfiguration(`rstack.${stack}.enable`), + ), + ); + if (gated.size > 0) { this.scheduleReconcile(); } + // A reconcile deliberately leaves a live stack alone, so a setting a + // controller consumed at registration needs the restart path instead. + // Only live controllers are iterated: a stack behind a closed gate has + // nothing to rebuild, and one already being retired is gone from the + // map, which is what keeps a change landing mid-rebuild from queuing a + // second one. + for (const [stack, controller] of this.#controllers) { + if (gated.has(stack)) { + continue; + } + const moved = controller.restartOnSettings?.find((setting) => + event.affectsConfiguration(`rstack.${stack}.${setting}`), + ); + if (moved) { + void this.restart(stack, `rstack.${stack}.${moved} changed`); + } + } }), // Restricted Mode shows the status bar only; trust unlocks the stacks // without a window reload. @@ -122,7 +146,7 @@ class ExtensionShell { // Owned by the shell, not the stack: a stack cannot rebuild itself, and // the shallower alternative (bouncing just the tool's own process) leaves // the controller's package resolution and version check stale. Stacks - // reach the same operation through `StackContext.requestRestart`. + // reach the same operation by declaring `restartOnSettings`. register(stackCommand(stack, 'restart'), () => this.restart(stack)); } } @@ -212,7 +236,7 @@ class ExtensionShell { * still passes the gate, from scratch. * * `reason` is for the callers that are not a user picking the command — - * `StackContext.requestRestart` passes what moved. + * `restartOnSettings` passes what moved. */ restart(stack?: StackId, reason?: string): Promise { return this.enqueue(() => this.runRestart(stack, reason)); @@ -348,7 +372,6 @@ class ExtensionShell { status: this.#statusBar.reporterFor(stack), detection: snapshot, onDidChangeDetection: this.#detectionEmitter.event, - requestRestart: (reason) => this.restart(stack, reason), }); // Retiring it here rather than leaving it to the queued teardown skips // publishing exports and flipping `active` on for a stack the extension diff --git a/packages/vscode/src/stacks/lint/index.ts b/packages/vscode/src/stacks/lint/index.ts index c4c0599..e29c921 100644 --- a/packages/vscode/src/stacks/lint/index.ts +++ b/packages/vscode/src/stacks/lint/index.ts @@ -109,6 +109,12 @@ export const aggregateFolderStates = ( class RslintController implements StackController { readonly id = 'rslint' as const; + // A `binPath`/`customBinPath` change must re-resolve the binary, which only + // happens on a fresh start (upstream documents `customBinPath` as requiring a + // reload; a restart is strictly better). A shallower local restart would + // replace the coordinator but keep this controller's already-resolved binary + // and version check. + readonly restartOnSettings = ['binPath', 'customBinPath', 'trace.server']; #context: StackContext | undefined; #logger: Logger | undefined; @@ -135,20 +141,6 @@ class RslintController implements StackController { vscode.workspace.onDidChangeWorkspaceFolders((event) => { this.reconcileFolders(event); }), - // A `binPath`/`customBinPath` change must re-resolve the binary, which - // only happens on a fresh start (upstream documents `customBinPath` as - // requiring a reload; a restart is strictly better). It asks the shell - // for a full rebuild rather than restarting locally — a local restart - // would replace the coordinator but keep this controller's - // already-resolved binary and version check. - vscode.workspace.onDidChangeConfiguration((event) => { - for (const setting of ['binPath', 'customBinPath', 'trace.server']) { - if (event.affectsConfiguration(`rstack.rslint.${setting}`)) { - void context.requestRestart(`rstack.rslint.${setting} changed`); - return; - } - } - }), ); this.startCoordinator(); diff --git a/packages/vscode/src/types.ts b/packages/vscode/src/types.ts index 96851a3..f095d18 100644 --- a/packages/vscode/src/types.ts +++ b/packages/vscode/src/types.ts @@ -105,17 +105,6 @@ export interface StackContext { * only has to reconcile its own per-folder runtimes. */ readonly onDidChangeDetection: vscode.Event; - /** - * Asks the shell to rebuild this stack from scratch — the same thing - * `rstack..restart` does. A stack cannot rebuild itself (its own - * controller is what gets replaced), and settling for a shallower local - * restart would keep the controller's already-resolved binary and version - * check, which is the staleness the rebuild exists to clear. - * - * `reason` goes to the shell log, so a restart nobody asked for out loud - * still says where it came from. - */ - readonly requestRestart: (reason: string) => Promise; } /** @@ -134,6 +123,18 @@ export interface StackContext { */ export interface StackController { readonly id: StackId; + /** + * Setting names under `rstack..` whose change must rebuild this stack, + * because their value is consumed once at registration (a resolved binary, a + * probed Node) and a live controller would keep answering with the stale one. + * + * Declared as data because restart is the shell's concern: the shell owns the + * listener and the rebuild, so a stack states *what* moves it, never *how* to + * move itself. A stack that watches this itself would also have to get the + * teardown ordering right — a change landing mid-rebuild must not queue a + * second one — which the shell already handles by iterating live controllers. + */ + readonly restartOnSettings?: readonly string[]; register(context: StackContext): Promise | void>; /** Teardown may be asynchronous (stopping a language server, workers). */ dispose(): void | Promise; diff --git a/packages/vscode/tests/extension.test.ts b/packages/vscode/tests/extension.test.ts index 35d682b..70cab1e 100644 --- a/packages/vscode/tests/extension.test.ts +++ b/packages/vscode/tests/extension.test.ts @@ -10,6 +10,7 @@ import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; import type vscode from 'vscode'; interface FakeController { + readonly restartOnSettings?: readonly string[]; register(): Promise>; dispose(): Promise; } @@ -53,6 +54,12 @@ const harness = rs.hoisted(() => { shellLog: [] as string[], commands: new Map unknown>(), contextKeys: new Map(), + /** What each stack's controller declares as restart-triggering settings. */ + restartOnSettings: new Map(), + /** Every configuration listener the shell installed. */ + configListeners: [] as ((event: { + affectsConfiguration(section: string): boolean; + }) => void)[], }); const state = { ...defaults(), @@ -61,6 +68,7 @@ const harness = rs.hoisted(() => { }, controller(stack: string): FakeController { return { + restartOnSettings: state.restartOnSettings.get(stack), register: async () => { state.events.push(`register:${stack}`); const block = state.blockRegister.get(stack); @@ -181,7 +189,14 @@ rs.mock('vscode', () => { getConfiguration: () => ({ get: (_key: string, fallback?: unknown) => fallback, }), - onDidChangeConfiguration: () => disposable, + onDidChangeConfiguration: ( + listener: (event: { + affectsConfiguration(section: string): boolean; + }) => void, + ) => { + harness.configListeners.push(listener); + return disposable; + }, onDidChangeWorkspaceFolders: () => disposable, onDidGrantWorkspaceTrust: () => disposable, }, @@ -250,6 +265,17 @@ const run = async (command: string): Promise => { const restart = (): Promise => run('rstack.restart'); +/** + * Fires a configuration change naming exactly one section, the way VS Code + * reports one setting moving. Narrow on purpose: a fake that answered `true` + * for every section could not tell a gate change from a restart-triggering one. + */ +const changeSetting = (...sections: string[]): void => { + for (const listener of harness.configListeners) { + listener({ affectsConfiguration: (asked) => sections.includes(asked) }); + } +}; + /** * Lets every already-scheduled continuation run. A macrotask turn drains the * whole microtask queue behind it, so this is "whatever was going to happen @@ -258,6 +284,87 @@ const restart = (): Promise => run('rstack.restart'); const settle = (): Promise => new Promise((resolve) => setTimeout(resolve, 0)); +/** + * Restart is a shell concern, so *which settings trigger one* is too: a stack + * declares `restartOnSettings` as data and the shell owns the listener and the + + * rebuild. These cases pin that seam — a stack watching configuration itself + * would have to re-derive the gate exclusion and the mid-rebuild ordering the + * shell already gets right. + */ +describe('restart-triggering settings', () => { + beforeEach(async () => { + harness.reset(); + harness.detected = new Set(['rslint', 'rstest', 'fmt']); + harness.restartOnSettings = new Map([ + ['rstest', ['nodeExecutable']], + ['rslint', ['binPath', 'customBinPath']], + ]); + await activate(context); + harness.events.length = 0; + }); + + afterEach(async () => { + await deactivate(); + }); + + it("still rebuilds a stack when another stack's gate moved too", async () => { + // One event covers a whole batch — saving settings.json moves everything + // at once. A gate change used to short-circuit the whole listener, so the + // restart was dropped on the floor with no trace. + changeSetting('rstack.rslint.enable', 'rstack.rstest.nodeExecutable'); + await settle(); + expect(stacksOf('register')).toContain('rstest'); + }); + + it('leaves a stack whose own gate moved to the reconcile', async () => { + // Both moved for the same stack: rebuilding it here would fight a + // reconcile that may be retiring it. + changeSetting('rstack.rstest.enable', 'rstack.rstest.nodeExecutable'); + await settle(); + expect(stacksOf('register')).not.toContain('rstest'); + }); + + it('rebuilds only the stack that declared the setting', async () => { + changeSetting('rstack.rstest.nodeExecutable'); + await settle(); + expect(stacksOf('dispose')).toEqual(['rstest']); + expect(stacksOf('register')).toEqual(['rstest']); + }); + + it('honours every setting a stack declares, not just the first', async () => { + changeSetting('rstack.rslint.customBinPath'); + await settle(); + expect(stacksOf('register')).toEqual(['rslint']); + }); + + it('ignores a setting no stack declared', async () => { + changeSetting('rstack.rstest.nodeExecArgs'); + await settle(); + expect(harness.events).toEqual([]); + }); + + it('ignores a declared name under another stack namespace', async () => { + // The section is built as `rstack..`, so rslint's binPath + // must not move rstest even though both are declared somewhere. + changeSetting('rstack.rstest.binPath'); + await settle(); + expect(harness.events).toEqual([]); + }); + + it('leaves a stack behind a closed gate alone', async () => { + // Only live controllers are iterated: there is nothing to rebuild for a + // stack that never registered, and a restart would fight the gate. + harness.detected = new Set(['rslint', 'fmt']); + await run('rstack.restart'); + harness.events.length = 0; + + changeSetting('rstack.rstest.nodeExecutable'); + await settle(); + expect(harness.events).toEqual([]); + }); +}); + describe('the shell restart command', () => { beforeEach(async () => { harness.reset(); From 0f4c4128fdb3334ba44bdc8f92f6a6253e605744 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 10 Aug 2026 20:12:55 +0800 Subject: [PATCH 4/5] fix(vscode): probe the configured node and stand the shell probe in the project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CONTEXT.md | 8 + docs/adr/0001-node-runtime-selection.md | 52 +++++++ packages/vscode/AGENTS.md | 8 +- .../e2e/rstest/fixtures/workspace-1/.nvmrc | 1 + .../e2e/rstest/fixtures/workspace-2/.nvmrc | 1 + packages/vscode/e2e/rstest/runTest.ts | 7 + packages/vscode/src/shared/versionCheck.ts | 20 ++- packages/vscode/src/stacks/test/index.ts | 32 ++-- packages/vscode/src/stacks/test/master.ts | 69 +++++++-- .../vscode/src/stacks/test/nodeResolution.ts | 145 ++++++++++++++---- packages/vscode/src/stacks/test/status.ts | 26 +++- .../vscode/tests/stacks/test/master.test.ts | 79 ++++++++++ .../tests/stacks/test/nodeResolution.test.ts | 139 ++++++++++++++++- 13 files changed, 519 insertions(+), 68 deletions(-) create mode 100644 docs/adr/0001-node-runtime-selection.md create mode 100644 packages/vscode/e2e/rstest/fixtures/workspace-1/.nvmrc create mode 100644 packages/vscode/e2e/rstest/fixtures/workspace-2/.nvmrc diff --git a/CONTEXT.md b/CONTEXT.md index 2d0a372..df07973 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -9,6 +9,14 @@ Glossary of terms used across rstack-editor. Code, docs, commit messages and rev - **Detection** — the per-workspace-folder scan deciding which stacks a folder lights up. Detection signals are config files and installed tool binaries, never user settings. - **Gate** — the per-stack activation condition: detected, workspace trusted, and the enable settings on. +## Runtimes + +- **VS Code Node runtime** — the Node.js shipped inside VS Code, which the extension host itself runs on. Its version follows VS Code's release cadence, and it is Electron's Node, on a different ABI line from plain Node. _Avoid_: host runtime, extension host runtime. +- **User Node runtime** — the Node.js the user's own environment provides, discovered by the extension rather than shipped with it. _Avoid_: worker runtime, project-side Node. +- **Load surface** — the set of modules a piece of work can end up loading. A closed load surface holds only what the extension ships plus ABI-stable bindings; an open one can reach arbitrary project dependencies. Open load surfaces belong on the User Node runtime. +- **Preflight** — the check that picks a User Node runtime, run once per extension host before any worker is spawned. Its failure is a status, never a crash. +- **Runtime floor** — the minimum Node.js version supported for a User Node runtime. A declared support contract, not a probed capability. + ## fmt - **Cold format** — a format request served by spawning a fresh `rs fmt` process at request time; the request pays the full process start-up cost. diff --git a/docs/adr/0001-node-runtime-selection.md b/docs/adr/0001-node-runtime-selection.md new file mode 100644 index 0000000..d36f6ef --- /dev/null +++ b/docs/adr/0001-node-runtime-selection.md @@ -0,0 +1,52 @@ +# Node runtime selection + +The Node.js a test worker runs on is a **User Node runtime** — chosen by the extension from the user's own environment, the PATH `node` first and then the `node` the user's interactive shell would give them — and it must satisfy a uniform floor of `>=22.18.0`. The **VS Code Node runtime** is never a candidate. When nothing satisfies the floor, the test stack reports a status and runs nothing. + +## Why the floor is 22.18.0 + +The strictest thing a worker does is load an `rstack.config.*`, which rstack's shipped shim loads through `@rstackjs/load-config` with `loader: 'native'`. That path rethrows with no jiti fallback (`rstack-cli` `packages/rstack/src/config.ts:174`, bundled verbatim into `rstack@0.4.0`'s `dist/687.js`), and `native` never consults `process.features.typescript` — it calls `import()` directly. So the worker needs Node's native TypeScript stripping, on by default from 22.18.0. + +Native type stripping is the _only_ thing on the worker's path that needs more than the declared `engines` of the packages involved: `@rstest/core` 0.11.6 and `@rsbuild/core` declare `^20.19.0 || >=22.12.0`, `rstack` 0.4.0 declares `>=22.12.0`, and `Module.registerHooks` (used by rstack's `freshImport`, added in 22.15) has a three-level fallback. + +## Considered options + +**A per-project floor** — 22.18.0 only for projects driven by an `rstack.config.*`, 22.12.0 for a native `rstest.config.*`. Rejected: it buys back Node 20.19–22.17 at the cost of a second code path through every call site. Node 20 left support on 2026-04-30, so the users it genuinely serves are those on Node 22.12–22.17 — a supported LTS line, needing only a patch-level update within 22.x. That is a low-friction ask, and a single floor is a support contract the README and the status bar can each state in one sentence. + +**A capability probe instead of a version check** — asking each candidate for `process.features.typescript` rather than comparing semver. Rejected: `process.features.typescript` is itself Stability 1.2 (release candidate) and its value set has moved (`"transform"` existed on 22.18–25.1, removed in 26.0.0). More decisively, `loader: 'native'` never reads it, so the probe would not be testing the condition that actually fails. A floor is a contract; contracts are declared, not sniffed. + +**Falling back to the VS Code Node runtime** — using it when no User Node runtime satisfies the floor. Rejected, and this is the load-bearing "no". It is not a version argument; the VS Code Node runtime is new enough. It is that a green run in the editor must mean the same thing as a green run in the terminal. That runtime is Electron's Node, on its own ABI line (measured: `NODE_MODULE_VERSION` 146, against 137 for plain Node 24.18), so a non-N-API addon that loads in the terminal fails in the editor and the reverse — and its version tracks VS Code's release cadence rather than anything the project controls. A degraded success here produces a false signal, which is worse than not running. + +**Bun as a User Node runtime** — rejected for now: `bun run` on `@rstest/core` segfaults (verified, bun 1.3.2 × `@rstest/core` 0.11.5), though bun loads `rstack.config.ts` fine. + +## Where the shell probe stands + +The interactive-shell probe runs with its cwd set to the first detected workspace folder that does not pin `nodeExecutable`. The probe is cwd-sensitive: version managers resolve version files (`.nvmrc`, `.node-version`) against the shell's working directory, and fnm's default `version-file-strategy = local` never walks upward — a shell spawned from the extension host's own cwd (typically `/`) cannot see any project's version file and answers with the manager's global default (measured: a repository pinning 26 in `.nvmrc`, the probe answering with the 20.x global default). Standing in the workspace folder is what makes the probe answer the question it exists to answer: what a terminal opened on this project would say. + +The probe stays one-per-host — one PATH, one shell, one interactive start-up cost, and the fallback notice must fire once, not once per project — so one directory has to stand for the whole window. Two entry points share the memo, first caller wins: the activation warm-up, almost always first, derives its standpoint and its own reason to exist from one query — the first detected folder without a pinned `nodeExecutable` both proves the memo has a reader (pinned folders never read it) and is where the probe stands; the worker spawn path, first only when the warm-up found every folder pinned, stands in that project's cwd, the directory it is about to run the worker in. The folder root rather than a project directory is the deliberate default: version files overwhelmingly sit at the repository root, which in a monorepo is _above_ the package that owns the config. + +**Per-project probes** — rejected: N interactive shells for what is in practice a repository-level convention, and a window that genuinely needs a different Node per folder is `nodeExecutable`'s case — that setting is read per folder already. + +**Walking upward for `.git` or a version file** — rejected: it re-implements the version manager's own lookup policy. The extension stands where the user's terminal would stand; how the version manager answers from there is the manager's business. A user who opened a subdirectory of their repository probes from that subdirectory — the terminal they would open there answers the same way. + +## Where the VS Code Node runtime _is_ allowed + +The rule is not "never use it". The line is the **load surface**: work whose full transitive load set the extension controls (what it ships, plus N-API bindings, ABI-stable by construction) may run on the VS Code Node runtime; work that can reach arbitrary project dependencies must run on a User Node runtime. Loading a config is on the wrong side of that line — configs in this ecosystem import native bindings routinely — so config loading stays inside the worker, where the upstream machinery already puts it. + +Note that _worker_ names a process, not a runtime. The worker is our own code; the runtime it runs on is the user's. + +### The line is drawn for the test worker only + +This decision is implemented for one path: the rstest worker. Two others sit on the wrong side of the line today, and this ADR does not move them. Naming them, so the rule is not read as an invariant the extension already holds: + +- **fmt** spawns the project's `rs` bin on `process.execPath` with `ELECTRON_RUN_AS_NODE=1` (`stacks/fmt/run.ts`) — the VS Code Node runtime — and `rs fmt` loads the project's config in that process (`stacks/fmt/index.ts`). Open load surface, no floor, no preflight. +- **lint** imports the project's `@rslint/core/config-loader` into the extension host and loads the user's `rslint.config.ts` there (`stacks/lint/configLoader.ts`), and runs user plugin rules on the same runtime (`stacks/lint/PluginLintPool.ts`). `stacks/lint/jitiPreflight.ts` already records the resulting divergence in so many words: that loader "runs on the extension host's Node — whose version is fixed by VS Code, not by the user — so the jiti branch can trigger in the editor even when the CLI works fine". Its answer is a diagnostic, not a runtime choice. + +Neither is cheap to move — each needs its own spawn-and-protocol work — and neither has a reported bug behind it yet. Known debt, deliberately: the next stack to load project code should follow the rule, and nobody should describe the rule as already universal. + +## Consequences + +- An explicit `rstack.rstest.nodeExecutable` is always honoured, but it is probed too: falling short of the floor produces a status, not a refusal. The escape hatch stays an escape hatch; it stops being silent. +- A below-floor configured executable is reported through the same status as "no runtime found at all", so the two messages must state their _consequence_ explicitly — one says tests will not run, the other says the extension is running with it anyway. +- The interactive-shell probe is the recovery path and does not exist on Windows (no `-i -c` equivalent reliably evaluates a user's profile across cmd and PowerShell). A Windows user whose PATH `node` is below the floor gets the failure status with no second candidate. +- `NODE_OPTIONS` can carry `--no-strip-types`, which defeats the floor on any version. Deliberately not detected: the same setting breaks `rs test` in the terminal, so the editor failing identically is correct, and special-casing one flag would be permanent trivia bought for one diagnostic. +- An unreadable or unparseable `node --version` is treated as _not_ satisfying the floor, unlike the package checks in `shared/versionCheck.ts`, which soft-pass an unknown version. The difference is real: runtime candidates are an ordered list, so a soft pass lets a suspect PATH `node` beat a healthy one from the shell; a package check has no next candidate to fall through to. diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 9216f64..8060310 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -14,7 +14,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten 3. **Resolve-from-project** — no tool binaries or tool packages in the VSIX; everything resolves from the user's project so the editor runs the CLI's exact versions. Version floors surface as a status, never a crash. All cooperating lint pieces (binary, config loader, plugin host) must come from one resolution root. 4. **Status aggregation** — stacks own no UI chrome; they report to the shell's single status bar item, which always exists. 5. **Worker-cwd decoupling** (test) — a project's cwd is explicit, not derived from the config file path; for native configs behavior stays byte-identical to upstream. -6. **Worker runtime** (test) — the worker's Node is chosen by the extension against one uniform floor, never assumed from PATH; the recovery path is the user's own shell, and the extension host's own runtime is never a candidate. +6. **Node runtime selection** (test) — the worker's Node is a **User Node runtime** chosen by the extension against one uniform floor, never assumed from PATH; the recovery path is the user's own shell, and the dividing line is the **load surface** (terms in CONTEXT.md; the full rule and rationale in `docs/adr/0001-node-runtime-selection.md`). Implemented for the rstest worker only — fmt and lint still load project code on the VS Code Node runtime, known debt recorded in the ADR, not an invariant the extension already holds. ## Rules @@ -35,9 +35,9 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The fmt stack is a spawn-per-request `rs fmt --stdin-filepath` MVP. Its cwd is the governing config directory because rs fmt resolves config from cwd only, and formatting errors are log-only by design. A single pre-spawned standby that tracks the active editor (see CONTEXT.md) is the accepted, bounded exception to "no warm tier". Do not grow it into a daemon: no long-lived protocol, no process pool, no cross-request state. The endgame is an upstream LSP; the standby retires with it. - `projectModules.ts` has no cache-invalidation hook and restart must not grow one. Node's ESM registry is keyed by resolved URL and process-lifetime, so clearing the local memo hands back the identical module object (verified); a `?epoch=` query does reload the entry but relative specifiers inside it do not inherit the query, yielding a fresh entry over stale dependencies. In-place reinstalls under an unchanged path need a window reload — say so, don't fake it. - The VSIX is platform-targeted for exactly one reason: the test stack's AST collection loads a native parser binding. Do not add another native dependency — it multiplies the release matrix. -- `stacks/test/nodeResolution.ts` takes its shell and its notify callback as options instead of importing `vscode` and the stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveWorkerNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. Move it to `shared/` when a second stack has to run user code on a project-side Node — but not for a caller that only runs _our_ code on the extension host runtime (fmt, the lint plugin host), which has no candidate to choose between and only needs `nativeTypeStrippingAvailable()`. -- The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`): the strictest thing a worker does is load an `rstack.config.*` through rstack's shim, which hardcodes `loader: 'native'` with no jiti fallback and therefore needs native type stripping (22.18+). Specialising the floor per project was considered and rejected — it buys back only Node 20, whose support window ended 2026-04-30, at the cost of a second code path. -- Bun is not a supported worker runtime: `bun run` on `@rstest/core` segfaults (verified, bun 1.3.2 × @rstest/core 0.11.5), even though bun loads `rstack.config.ts` fine. If that is ever revisited, gate it on an explicit setting — never on `bun.lock`, since bun-as-package-manager still runs the `rs` bin through its `#!/usr/bin/env node` shebang. +- `stacks/test/nodeResolution.ts` takes its shell and its notify callback as options instead of importing `vscode` and the stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveWorkerNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. Move it to `shared/` when a second stack has to run user code on a User Node runtime — but not for a caller that only runs _our_ code on the VS Code Node runtime (fmt, the lint plugin host), which has no candidate to choose between and only needs `nativeTypeStrippingAvailable()`. +- The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`), because the strictest thing a worker does is load an `rstack.config.*` through rstack's shim, which hardcodes `loader: 'native'` with no jiti fallback and so needs native type stripping (22.18+). Do not specialise the floor per project — that was considered and rejected. Why, and what else was rejected: `docs/adr/0001-node-runtime-selection.md`. +- Bun is not a supported worker runtime (it segfaults running `@rstest/core`). If that is ever revisited, gate it on an explicit setting — never on `bun.lock`, since bun-as-package-manager still runs the `rs` bin through its `#!/usr/bin/env node` shebang. ## Testing diff --git a/packages/vscode/e2e/rstest/fixtures/workspace-1/.nvmrc b/packages/vscode/e2e/rstest/fixtures/workspace-1/.nvmrc new file mode 100644 index 0000000..6f4247a --- /dev/null +++ b/packages/vscode/e2e/rstest/fixtures/workspace-1/.nvmrc @@ -0,0 +1 @@ +26 diff --git a/packages/vscode/e2e/rstest/fixtures/workspace-2/.nvmrc b/packages/vscode/e2e/rstest/fixtures/workspace-2/.nvmrc new file mode 100644 index 0000000..6f4247a --- /dev/null +++ b/packages/vscode/e2e/rstest/fixtures/workspace-2/.nvmrc @@ -0,0 +1 @@ +26 diff --git a/packages/vscode/e2e/rstest/runTest.ts b/packages/vscode/e2e/rstest/runTest.ts index aa7ff9b..0bc2e2d 100644 --- a/packages/vscode/e2e/rstest/runTest.ts +++ b/packages/vscode/e2e/rstest/runTest.ts @@ -9,6 +9,13 @@ * the file it opened. The workspace file is therefore generated per run in the * scratch dir, so every run starts from the identical single-folder state and * a failure between the add and the remove never reaches the repository. + * + * Each fixture carries a `.nvmrc`: the extension's shell probe stands in the + * opened fixture folder and deliberately never walks upward (ADR 0001), so + * without a local pin a developer machine's version-manager *default* — not + * anything this repo controls — would decide whether the Node preflight + * clears the floor. Inert in CI, which puts a new-enough `node` on PATH so + * the shell probe never runs. */ import { createHash } from 'node:crypto'; import { existsSync, mkdtempSync, writeFileSync } from 'node:fs'; diff --git a/packages/vscode/src/shared/versionCheck.ts b/packages/vscode/src/shared/versionCheck.ts index 3c654dc..ae8679e 100644 --- a/packages/vscode/src/shared/versionCheck.ts +++ b/packages/vscode/src/shared/versionCheck.ts @@ -59,10 +59,22 @@ export const readPackageVersion = ( export const NODE_RUNTIME_RANGE = '>=22.18.0'; /** - * The whole version policy in one place: an unreadable or unparseable version - * is `unknown` (a soft pass — it must never cost a feature), and prereleases of - * a supported range (e.g. `1.0.0-beta.1`) are accepted, because the ecosystem - * ships them and refusing them would strand early adopters. + * Classifies a version against a range; it does not decide what the classes + * mean. Prereleases of a supported range (e.g. `1.0.0-beta.1`) count as `ok`, + * because the ecosystem ships them and refusing them would strand early + * adopters — that part *is* policy and is uniform. + * + * What `unknown` means is the caller's to choose, and the two callers choose + * opposite things on purpose: + * - `reportVersionCheck` below soft-passes it. A package whose version cannot + * be read is still installed, and there is no second candidate to fall back + * to, so refusing would cost the feature for nothing. + * - `satisfiesFloor` in `stacks/test/nodeResolution.ts` rejects it. Runtime + * candidates are an *ordered list*, so soft-passing lets a suspect PATH + * `node` win over a healthy one from the user's shell. + * + * A third caller must make this choice deliberately rather than copy whichever + * neighbour it read first. */ export const checkVersion = ( version: string | undefined, diff --git a/packages/vscode/src/stacks/test/index.ts b/packages/vscode/src/stacks/test/index.ts index b1ff072..2c9ac2a 100644 --- a/packages/vscode/src/stacks/test/index.ts +++ b/packages/vscode/src/stacks/test/index.ts @@ -7,8 +7,8 @@ import type { import { RstestDiagnostics } from './diagnostics'; import { TestErrorStore, testMessageText } from './errorStore'; import { logger } from './logger'; -import { runningWorkers, workerNodeOptions } from './master'; -import { resetWorkerNodeCache, resolveWorkerNodeOnce } from './nodeResolution'; +import { runningWorkers, warmWorkerNodePreflight } from './master'; +import { resetWorkerNodeCaches } from './nodeResolution'; import { Project, WorkspaceManager } from './project'; import { status } from './status'; import { disposeTerminal } from './terminal'; @@ -506,6 +506,15 @@ class Rstest implements vscode.Disposable { class RstestController implements StackController { readonly id = 'rstest' as const; + // The worker runtime is resolved once per registration and the memo in + // `nodeResolution.ts` caches the *rejection* as well as the success, so + // without a rebuild a user who reads the "no usable Node" status and then + // sets `nodeExecutable` gets no reaction at all — and one who removes it + // again keeps a populated tree whose every run now fails. Only the setting + // qualifies: installing a newer Node on the machine is not observable by the + // extension and stays a manual restart, the deliberate half of the deal (see + // `docs/adr/0001-node-runtime-selection.md`). + readonly restartOnSettings = ['nodeExecutable']; #rstest: Rstest | undefined; @@ -526,12 +535,14 @@ class RstestController implements StackController { status.unbind(); throw error; } - // Warm the host-level node preflight while detection and the config-glob - // scan are still running, so the first worker spawn awaits a settled - // promise instead of paying the probes on the critical path. Deliberately - // not awaited — `register()` must return fast (adaptation #1) — and the - // rejection is handled by whoever actually needs the resolution. - void resolveWorkerNodeOnce(workerNodeOptions()).catch(() => {}); + // Not awaited — `register()` must return fast (adaptation #1) — and the + // rejection is handled by whoever actually needs the resolution. Only the + // folders detection picked are considered: they are the ones that will + // spawn workers, and they are file-scheme by construction, so the probe + // never stands in a virtual folder's meaningless `fsPath`. + warmWorkerNodePreflight( + context.detection.foldersFor('rstest').map(({ folder }) => folder), + ); return this.#rstest.buildExports(); } @@ -540,8 +551,9 @@ class RstestController implements StackController { this.#rstest = undefined; // The third module singleton with this exact lifetime, alongside the two // binds above: a re-registered stack re-probes, which is what makes the - // restart command pick up a toolchain change. - resetWorkerNodeCache(); + // restart command pick up a toolchain change — and what + // `restartOnSettings` relies on, rather than resetting the memo itself. + resetWorkerNodeCaches(); status.unbind(); logger.unbind(); } diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index d896f3d..f97edb0 100644 --- a/packages/vscode/src/stacks/test/master.ts +++ b/packages/vscode/src/stacks/test/master.ts @@ -23,13 +23,13 @@ import type { TestErrorStore } from './errorStore'; import { logger } from './logger'; import { nodeRequire } from './nodeRequire'; import { - NODE_RUNTIME_STATUS_SOURCE, + configuredNodeBelowFloor, NodePreflightError, type ResolveWorkerNodeOptions, resolveWorkerNodeOnce, } from './nodeResolution'; import type { Project } from './project'; -import { status } from './status'; +import { NODE_RUNTIME_STATUS_SOURCE, status } from './status'; import { runInTerminal as sendToTerminal, shellQuote } from './terminal'; import { TestRunReporter } from './testRunReporter'; import { toErrorMessage } from './utils'; @@ -41,12 +41,47 @@ export const runningWorkers = new Set>(); * The host-level inputs to the worker-node preflight. `notify` must not close * over `this`: the resolution is memoized for the extension host's lifetime, so * a callback capturing a `Project` would pin it and its whole test tree. + * `cwd` is the caller's standpoint for the shell probe — see + * `probeShellNodePath`. */ -export const workerNodeOptions = (): ResolveWorkerNodeOptions => ({ +export const workerNodeOptions = ( + cwd: string | undefined, +): ResolveWorkerNodeOptions => ({ shell: vscode.env.shell || undefined, + cwd, notify: (message) => logger.info(message), }); +/** + * Fire-and-forget warm of the host-level preflight, so the first worker spawn + * awaits a settled promise instead of paying the probes on its critical path. + * + * One `find` answers both questions the warm-up has: *whether* to warm — a + * folder without a pinned `nodeExecutable` proves the memo has a reader, while + * pinned folders take the configured branch and never read it — and *where the + * shell probe stands* — that same folder, whose version file is the one its + * workers should honour. Deriving both from one query is what keeps them from + * disagreeing in a multi-root window where only some folders pin; the memo is + * first-caller-wins (ADR 0001, "Where the shell probe stands"). + * + * Skipping when every folder pins is not a rounding error: whoever sets the + * escape hatch is usually the one whose PATH `node` is broken, the very input + * that drives the preflight down its slowest path (five retried spawns, then + * a full interactive shell). + */ +export const warmWorkerNodePreflight = ( + folders: readonly vscode.WorkspaceFolder[], +): void => { + const target = folders.find( + (folder) => !getConfigValue('nodeExecutable', folder), + ); + if (target !== undefined) { + void resolveWorkerNodeOnce(workerNodeOptions(target.uri.fsPath)).catch( + () => {}, + ); + } +}; + // Default host for a fixed debug port. The spawn (`--inspect-wait`), the port // preflight, and the attach config must all use the same host: on a dual-stack // machine `localhost` can resolve to `::1` while the worker listens on IPv4, so @@ -147,11 +182,11 @@ export class RstestApi { /** * The node command for worker spawns — the node-preflight adaptation. Why the * PATH `node` cannot be trusted, and why the floor is uniform rather than - * per-project, lives in `nodeResolution.ts`. What is decided here is only - * that an explicitly configured `nodeExecutable` skips the preflight - * entirely: an explicit choice is the escape hatch for everything the - * preflight can get wrong, and a wrong one already surfaces through the - * spawn-error notification. + * per-project, lives in `nodeResolution.ts`. + * + * An explicitly configured `nodeExecutable` is honoured but probed all the + * same — the whys, and the by-path memo that keeps this every-spawn call at + * a lookup, live on `configuredNodeBelowFloor`. * * `vscode.env.shell` is read here so `nodeResolution.ts` needs no VS Code * import. @@ -163,18 +198,28 @@ export class RstestApi { const { nodeExecutable, nodeExecArgs, configured } = this.resolveNodeCommand(); if (configured) { + // Advisory, never gating — the message says the run is going ahead, and + // it does — so it is deliberately not awaited: the verdict must not sit + // on the spawn path. Latched under the host key for the same reason as + // the preflight failure below; re-reporting on a later spawn is a no-op. + void configuredNodeBelowFloor(nodeExecutable).then((message) => { + if (message) { + status.versionMismatch(message, NODE_RUNTIME_STATUS_SOURCE); + } + }); return { nodeExecutable, nodeExecArgs }; } try { - const resolution = await resolveWorkerNodeOnce(workerNodeOptions()); + const resolution = await resolveWorkerNodeOnce( + workerNodeOptions(this.cwd), + ); return { nodeExecutable: resolution.executable, nodeExecArgs }; } catch (error) { if (error instanceof NodePreflightError) { // The status-aggregation adaptation: no usable runtime anywhere is the // same "fix your toolchain" state as an unsupported package version. - // Latched under the host key, not `this.statusSource`: the failure - // belongs to the extension host, so N projects must not file N copies, - // and this project's own recovery must not clear it. + // Latched under the host key, not `this.statusSource` — see + // `NODE_RUNTIME_STATUS_SOURCE`. status.versionMismatch(error.message, NODE_RUNTIME_STATUS_SOURCE); } throw error; diff --git a/packages/vscode/src/stacks/test/nodeResolution.ts b/packages/vscode/src/stacks/test/nodeResolution.ts index d66d77e..fdfe291 100644 --- a/packages/vscode/src/stacks/test/nodeResolution.ts +++ b/packages/vscode/src/stacks/test/nodeResolution.ts @@ -15,10 +15,12 @@ import { checkVersion, NODE_RUNTIME_RANGE } from '../../shared/versionCheck'; * than per-project. It is set by the strictest thing a worker does — load an * `rstack.config.*` — and applying it to every project, including a native * `rstest.config.*` that Rsbuild's bundled jiti would load on older engines, is - * a deliberate simplification: it drops only Node 20, whose support window - * ended 2026-04-30 (`nodejs/Release`), and keeps one code path instead of two. + * a deliberate simplification. It costs Node 20.19–22.17 — but Node 20 left + * support on 2026-04-30 (`nodejs/Release`), so the users it actually turns away + * sit on 22.12–22.17, a supported LTS line where the remedy is a patch-level + * update within 22.x. See `docs/adr/0001-node-runtime-selection.md`. * - * There is deliberately NO fallback to the extension host's own runtime + * There is deliberately NO fallback to the VS Code Node runtime * (`process.execPath` + `ELECTRON_RUN_AS_NODE`). It would silently move the * test run onto Electron's Node — a different ABI line (measured: Electron * reports NODE_MODULE_VERSION 146 where plain Node 24.18 reports 137, so @@ -27,16 +29,6 @@ import { checkVersion, NODE_RUNTIME_RANGE } from '../../shared/versionCheck'; * the editor as in the terminal. */ -/** - * The status-latch key for a failed preflight. The reporting site's identity is - * the *host*, not a project: there is one PATH and one shell, so filing this - * under a project's source URI would write N entries for one fact and let any - * one project's unrelated recovery (`status.versionOk` after the `@rstest/core` - * package check) or disposal clear it. The `host:` prefix cannot collide with - * the URI and filesystem-path namespaces `status.ts` documents. - */ -export const NODE_RUNTIME_STATUS_SOURCE = 'host:node-runtime'; - /** `node --version` is instant when it works; a slow answer is a broken one. */ const VERSION_PROBE_TIMEOUT_MS = 3_000; const SHELL_PROBE_TIMEOUT_MS = 5_000; @@ -56,9 +48,9 @@ const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); /** - * What became of running ` --version`. `ok` with no version is the - * soft pass: an unparseable version must never cost the feature (the rule lives - * in `checkVersion`). The three cases are a union rather than a boolean plus an + * What became of running ` --version`. `ok` carries no version when + * the output could not be parsed — which does *not* satisfy the floor here (see + * `satisfiesFloor`). The three cases are a union rather than a boolean plus an * optional field so that "not found, but here is a version" is unrepresentable. */ export type NodeProbe = @@ -72,7 +64,10 @@ export type WorkerNodeResolution = { readonly executable: string; /** Which candidate won. Drives the fallback notice, nothing else. */ readonly source: 'path' | 'shell'; - /** Absent when the version could not be parsed (the soft pass). */ + /** + * Optional only because `NodeProbe` is: a winning candidate always has a + * version, since an unreadable one cannot clear the floor. + */ readonly version?: string; }; @@ -96,7 +91,12 @@ const formatNodePreflightFailure = (attempts: NodeAttempts): string => { ? 'interactive shell: not probed' : describeCandidate('interactive shell', attempts.shell), ].join(', '); - return `No Node.js ${NODE_RUNTIME_RANGE} is available to run tests (${candidates}). Rstest needs it to load TypeScript config files. Install a newer Node.js, or set "rstack.rstest.nodeExecutable" to one.`; + // The trailing consequence is load-bearing, not padding: this message and + // `formatConfiguredNodeBelowFloor` reach the user through the same + // `version-mismatch` status, and the outcomes are opposite — nothing runs + // here, the run goes ahead there. The sentence is the only thing that tells + // them apart. + return `No Node.js ${NODE_RUNTIME_RANGE} is available to run tests (${candidates}). Rstest needs it to load TypeScript config files. Install a newer Node.js, or set "rstack.rstest.nodeExecutable" to one; until then tests will not run.`; }; /** No candidate satisfied `NODE_RUNTIME_RANGE`. */ @@ -126,8 +126,9 @@ export const probeNodeVersion = (executable: string): Promise => ); }); -const START_TOKEN = '__RSTACK_NODE_START__'; -const END_TOKEN = '__RSTACK_NODE_END__'; +/** Bracket the one meaningful line in an interactive shell's noisy stdout. */ +export const START_TOKEN = '__RSTACK_NODE_START__'; +export const END_TOKEN = '__RSTACK_NODE_END__'; /** * Asks the user's own shell where its `node` is, the way their terminal would @@ -143,17 +144,28 @@ const END_TOKEN = '__RSTACK_NODE_END__'; * an outer `sh -c`, so no quoting is involved and the timeout kills the shell * itself rather than a wrapper. * + * The probe is cwd-sensitive: version managers resolve version files + * (`.nvmrc`, `.node-version`) against the shell's working directory, and fnm's + * default strategy never walks upward — so `cwd` must be the directory a + * terminal on this project would open in. Left unset, the shell inherits the + * extension host's cwd (typically `/`), where no project's version file is + * visible and a version manager answers with its global default. Which + * directory the callers stand in, and why: "Where the shell probe stands" in + * `docs/adr/0001-node-runtime-selection.md`. + * * Returns `undefined` on any failure; a shell that hangs on a slow rc file must * cost a bounded wait, not the Test Explorer. */ export const probeShellNodePath = ( shell: string, + cwd?: string, ): Promise => new Promise((resolve) => { const script = `node --version >/dev/null 2>&1; echo ${START_TOKEN}; command -v node; echo ${END_TOKEN}`; const child = spawn(shell, ['-i', '-c', script], { stdio: ['ignore', 'pipe', 'ignore'], timeout: SHELL_PROBE_TIMEOUT_MS, + cwd, }); let output = ''; child.stdout.on('data', (chunk: Buffer) => { @@ -178,13 +190,26 @@ export const probeShellNodePath = ( const versionIfUsable = (probe: NodeProbe): string | undefined => probe.kind === 'ok' ? probe.version : undefined; +/** + * Only a version that demonstrably clears the floor counts — an unknown one + * does not, deliberately diverging from the package checks' soft pass. The + * whole trade-off is documented at `checkVersion` in `shared/versionCheck`. + */ const satisfiesFloor = (probe: NodeProbe): boolean => probe.kind === 'ok' && - checkVersion(probe.version, NODE_RUNTIME_RANGE).kind !== 'mismatch'; + checkVersion(probe.version, NODE_RUNTIME_RANGE).kind === 'ok'; export type ResolveWorkerNodeOptions = { /** The user's shell, for the interactive probe. Omit to skip that step. */ readonly shell?: string; + /** + * Where the shell probe stands — the directory a terminal on this project + * would open in (see `probeShellNodePath` for why the standpoint decides + * what a version manager answers). Consumed only by the default + * `probeShellPath`, so an injected probe replaces the standpoint together + * with the probing itself and cannot silently drop it. + */ + readonly cwd?: string; readonly probe?: (executable: string) => Promise; readonly probeShellPath?: (shell: string) => Promise; readonly platform?: NodeJS.Platform; @@ -195,15 +220,18 @@ export type ResolveWorkerNodeOptions = { /** * Picks the executable for a test worker. Callers holding an explicit * `nodeExecutable` setting must not call this at all — an explicit choice is - * honored verbatim, unchecked, because it is the escape hatch for everything - * this function can get wrong. + * always honored, because it is the escape hatch for everything this function + * can get wrong. It is still probed, by `configuredNodeBelowFloor`, so that + * falling short of the floor produces a status rather than silence. * * Throws `NodePreflightError` when no candidate satisfies the floor. */ export async function resolveWorkerNode({ shell, + cwd, probe = probeNodeVersion, - probeShellPath = probeShellNodePath, + probeShellPath = (probedShell: string) => + probeShellNodePath(probedShell, cwd), platform = process.platform, }: ResolveWorkerNodeOptions = {}): Promise { let onPath = await probe('node'); @@ -253,7 +281,10 @@ export async function resolveWorkerNode({ * is one PATH and one shell, so a monorepo with N projects must not run N * identical probes — nor announce the outcome N times, which is why `notify` is * called from inside the memo and only on the pass that actually probes. The - * shell probe in particular costs a whole interactive shell startup. + * shell probe in particular costs a whole interactive shell startup. The one + * per-caller input, `cwd`, is first-caller-wins by the same token: a Node + * version is in practice a repository-level convention, not a per-project one + * (ADR 0001, "Where the shell probe stands"). * * The rejection is memoized too, so a pathological host pays the probe timeouts * once rather than once per project. @@ -279,6 +310,68 @@ export const resolveWorkerNodeOnce = ( return resolution; })); -export const resetWorkerNodeCache = (): void => { +/** + * An explicitly configured `nodeExecutable` is never refused — it is the escape + * hatch — but it is not trusted blindly either: a setting written against the + * Node of two years ago is exactly the failure this module exists to catch, and + * unprobed it produces a broken worker behind a green status bar. + * + * Memoized by resolved executable path because the caller + * (`RstestApi.resolveWorkerNodeCommand`) runs on every worker spawn — config + * init, `listTests`, every single run — and a process spawn per spawn is a cost + * nobody asked for. Keyed by path rather than a single slot so a multi-root + * window whose folders configure different executables probes each one. The + * memo holds the verdict rather than the probe, so a cache hit costs nothing + * beyond the lookup. + */ +const configuredVerdicts = new Map>(); + +type ConfiguredNodeOptions = { + readonly probe?: (executable: string) => Promise; +}; + +/** + * Says the run is going ahead — the consequence is what tells the two + * `version-mismatch` messages apart (see `formatNodePreflightFailure`) — and + * names the version and the setting because editing it is the user's next + * move. + */ +const formatConfiguredNodeBelowFloor = ( + executable: string, + version: string | undefined, +): string => + `The Node.js set in "rstack.rstest.nodeExecutable" (${executable}, version ${version ?? 'unknown'}) does not satisfy ${NODE_RUNTIME_RANGE}, which Rstest needs to load TypeScript config files. Rstack is running tests with it anyway because the setting is your explicit choice; point it at a newer Node.js, or clear it to let the extension pick one.`; + +/** + * The message for a configured executable that falls short of the floor, or + * `undefined` when it clears it. + * + * Unlike `resolveWorkerNodeOnce`, this does *not* raise its own notice from + * inside the memo. It would have to be the first caller's `notify` that wins, + * since later callers only see the settled promise — and a warmer that passed + * none would silence the complaint for the whole session. Reporting stays with + * the caller, where repetition is already absorbed: the status is a latch keyed + * by reporting site, so re-reporting the same verdict is a `Map.set` with the + * same key and value. + */ +export const configuredNodeBelowFloor = ( + executable: string, + { probe = probeNodeVersion }: ConfiguredNodeOptions = {}, +): Promise => { + let verdict = configuredVerdicts.get(executable); + if (verdict === undefined) { + verdict = probe(executable).then((probed) => + satisfiesFloor(probed) + ? undefined + : formatConfiguredNodeBelowFloor(executable, versionIfUsable(probed)), + ); + configuredVerdicts.set(executable, verdict); + } + return verdict; +}; + +/** Clears both memos this module holds — a restart exists to clear stale resolution. */ +export const resetWorkerNodeCaches = (): void => { cached = undefined; + configuredVerdicts.clear(); }; diff --git a/packages/vscode/src/stacks/test/status.ts b/packages/vscode/src/stacks/test/status.ts index 58c9fb2..5a6d250 100644 --- a/packages/vscode/src/stacks/test/status.ts +++ b/packages/vscode/src/stacks/test/status.ts @@ -1,5 +1,15 @@ import type { StackState, StatusReporter } from '../../types'; +/** + * The latch key for facts about the extension host rather than about one + * resolution root — today, which Node.js the workers can run on. There is one + * PATH and one shell, so filing this under a project's source URI would write N + * entries for one fact and let any one project's unrelated recovery clear it. + * Declared here, beside the latch contract it participates in, rather than at + * the call site that happens to raise it. + */ +export const NODE_RUNTIME_STATUS_SOURCE = 'host:node-runtime'; + /** * The status-aggregation adaptation: the stack owns no status UI. Everything that * used to be a one-shot `showWarningMessage` (the `@rstest/core` version check) @@ -18,12 +28,16 @@ class StatusHolder implements StatusReporter { // that was neither recovered nor retried. Each latch is keyed by its // reporting site's identity — a master reports under its project's source // URI (unique per project, so sibling configs in one directory stay - // independent), a bridge shim resolution under its config directory; the - // two namespaces (URI string vs filesystem path) never collide. A recovery - // observed under one key must not clear another key's live failure. An - // entry is cleared by the code path that observes the corresponding - // recovery (a worker that actually spawned, a version check that passed) or - // by `forget` when its reporter goes away. + // independent), a bridge shim resolution under its config directory, and a + // fact about the extension host itself under `NODE_RUNTIME_STATUS_SOURCE` + // below; the three namespaces (URI string, filesystem path, `host:`) never + // collide. A recovery observed under one key must not clear another key's + // live failure. An entry is cleared by the code path that observes the + // corresponding recovery (a worker that actually spawned, a version check + // that passed) or by `forget` when its reporter goes away — neither of which + // reaches a host-scoped entry, since both are per-resolution-root. A + // host-scoped latch is cleared only by `bind`, i.e. by re-registering the + // stack, which is exactly the lifetime of the resolution it reports on. #crashes = new Map(); #mismatches = new Map(); #lastRunningDetail: string | undefined; diff --git a/packages/vscode/tests/stacks/test/master.test.ts b/packages/vscode/tests/stacks/test/master.test.ts index 6c9fcd7..c9c9f5c 100644 --- a/packages/vscode/tests/stacks/test/master.test.ts +++ b/packages/vscode/tests/stacks/test/master.test.ts @@ -5,6 +5,13 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; import { logger } from '../../../src/stacks/test/logger'; import { RstestApi } from '../../../src/stacks/test/master'; +import { + type NodeProbe, + configuredNodeBelowFloor, + resetWorkerNodeCaches, +} from '../../../src/stacks/test/nodeResolution'; +import { status } from '../../../src/stacks/test/status'; +import type { StatusReporter } from '../../../src/types'; // The Rstest runner injects its own `@rstest/core` into every resolution path so // that test files can import it, which makes "the project has no @rstest/core" @@ -215,3 +222,75 @@ describe('RstestApi with an unresolvable rstestPackagePath', () => { expect(createdTerminals).toEqual([]); }); }); + +// An explicit `nodeExecutable` is the escape hatch and is always honoured — but +// a setting pointed at a Node that has since fallen below the floor is the very +// failure the worker-runtime adaptation exists to catch, and the spawn succeeds, +// so nothing else would ever say so. +describe('RstestApi with a configured nodeExecutable', () => { + const configuredNode = '/opt/node/bin/node'; + const mismatches: string[] = []; + + // Adaptation #4 again: the stack reports through a singleton that no-ops + // while unbound, which is what every other suite in this file sees. + const reporter: StatusReporter = { + stack: 'rstest', + report: () => {}, + starting: () => {}, + running: () => {}, + crashed: () => {}, + versionMismatch: (detail) => mismatches.push(detail), + }; + + // Seeding the memo is how the probe is injected: `resolveWorkerNodeCommand` + // takes no probe option (it is called from deep inside a spawn path), and the + // memo is keyed by executable path, so a seeded entry is the answer it gets. + const seedProbe = (probe: NodeProbe) => + configuredNodeBelowFloor(configuredNode, { + probe: () => Promise.resolve(probe), + }); + + // Reaching the private method keeps these cases on the decision under test + // instead of spawning a real worker process for each one. + const resolveWorkerNodeCommand = (api: RstestApi) => + (api as any).resolveWorkerNodeCommand() as Promise<{ + nodeExecutable: string; + }>; + + beforeEach(() => { + mismatches.length = 0; + resetWorkerNodeCaches(); + status.bind(reporter); + settings.nodeExecutable = configuredNode; + }); + + afterEach(() => { + status.unbind(); + resetWorkerNodeCaches(); + delete settings.nodeExecutable; + }); + + // The verdict is reported off the spawn path, so a spawn resolves before the + // report lands; awaiting the (memoized) verdict is what settles it. + const settleVerdict = () => configuredNodeBelowFloor(configuredNode); + + it('should stay silent when the configured executable clears the floor', async () => { + await seedProbe({ kind: 'ok', version: '24.0.0' }); + const { nodeExecutable } = await resolveWorkerNodeCommand(createApi()); + await settleVerdict(); + expect(nodeExecutable).toBe(configuredNode); + expect(mismatches).toEqual([]); + }); + + // The wording itself is `nodeResolution.test.ts`'s to pin; what this level + // owns is the wiring — a verdict reaches the status, and the executable is + // handed back regardless. + it('should report a below-floor executable and still run with it', async () => { + await seedProbe({ kind: 'ok', version: '20.19.4' }); + const { nodeExecutable } = await resolveWorkerNodeCommand(createApi()); + await settleVerdict(); + // Never refused: the setting is the escape hatch. + expect(nodeExecutable).toBe(configuredNode); + expect(mismatches).toHaveLength(1); + }); +}); diff --git a/packages/vscode/tests/stacks/test/nodeResolution.test.ts b/packages/vscode/tests/stacks/test/nodeResolution.test.ts index c25903d..4515d0a 100644 --- a/packages/vscode/tests/stacks/test/nodeResolution.test.ts +++ b/packages/vscode/tests/stacks/test/nodeResolution.test.ts @@ -1,14 +1,20 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import { afterEach, describe, expect, it } from '@rstest/core'; import { NODE_RUNTIME_RANGE } from '../../../src/shared/versionCheck'; import { + configuredNodeBelowFloor, + END_TOKEN, type NodeProbe, NodePreflightError, probeNodeVersion, probeShellNodePath, type ResolveWorkerNodeOptions, - resetWorkerNodeCache, + resetWorkerNodeCaches, resolveWorkerNode, resolveWorkerNodeOnce, + START_TOKEN, } from '../../../src/stacks/test/nodeResolution'; // Every case injects both probes: the point is the decision table, not the @@ -55,13 +61,30 @@ describe('resolveWorkerNode', () => { }); }); - it('soft-passes a PATH node whose version cannot be parsed', async () => { + it('falls through a PATH node whose version cannot be parsed', async () => { + // Runtime policy deliberately diverges from the package checks in + // `shared/versionCheck`, which soft-pass an unknown version: candidates + // here are an ordered list, so a soft pass would let a suspect PATH node + // beat a healthy one from the user's shell. A package check has no next + // candidate to fall through to. const resolution = await resolveWorkerNode({ + ...base, + probe: versionsOf({ + node: { kind: 'ok' }, + '/versions/24/bin/node': ok('24.0.0'), + }), + probeShellPath: shellFinds('/versions/24/bin/node'), + }); + expect(resolution.source).toBe('shell'); + }); + + it('fails rather than run on a node nobody could version', async () => { + const error = await preflightError({ ...base, probe: versionsOf({ node: { kind: 'ok' } }), - probeShellPath: never, + probeShellPath: shellFinds(undefined), }); - expect(resolution.source).toBe('path'); + expect(error.attempts.path).toBeUndefined(); }); it('accepts a prerelease of a satisfying version', async () => { @@ -203,6 +226,18 @@ describe('NodePreflightError', () => { expect(message).toContain('rstack.rstest.nodeExecutable'); }); + it('states the consequence, which is what a below-floor setting does not', () => { + // Both failures reach the user through the same `version-mismatch` status, + // so the consequence clause is the only thing distinguishing "nothing will + // run" from "we are running with it anyway". + const { message } = new NodePreflightError({ + path: '20.19.4', + shell: undefined, + shellSkipped: false, + }); + expect(message).toContain('tests will not run'); + }); + it('does not invent versions for candidates that found nothing', () => { const { message } = new NodePreflightError({ path: undefined, @@ -228,7 +263,7 @@ describe('NodePreflightError', () => { describe('resolveWorkerNodeOnce', () => { afterEach(() => { - resetWorkerNodeCache(); + resetWorkerNodeCaches(); }); const options = { @@ -242,7 +277,7 @@ describe('resolveWorkerNodeOnce', () => { const second = await resolveWorkerNodeOnce(options); expect(second).toBe(first); - resetWorkerNodeCache(); + resetWorkerNodeCaches(); const third = await resolveWorkerNodeOnce(options); expect(third).not.toBe(first); }); @@ -314,6 +349,74 @@ describe('resolveWorkerNodeOnce', () => { }); }); +describe('configuredNodeBelowFloor', () => { + afterEach(() => { + resetWorkerNodeCaches(); + }); + + const configured = '/opt/node/bin/node'; + const answers = (probe: NodeProbe) => ({ + probe: () => Promise.resolve(probe), + }); + + it('passes an explicit executable that clears the floor', async () => { + expect( + await configuredNodeBelowFloor(configured, answers(ok('24.0.0'))), + ).toBeUndefined(); + }); + + it('names the version, the floor and the setting, and says the run goes ahead', async () => { + const message = await configuredNodeBelowFloor( + configured, + answers(ok('20.19.4')), + ); + expect(message).toContain(configured); + expect(message).toContain('20.19.4'); + expect(message).toContain(NODE_RUNTIME_RANGE); + expect(message).toContain('rstack.rstest.nodeExecutable'); + // The opposite consequence from `NodePreflightError`: this one runs. + expect(message).toContain('running tests with it anyway'); + }); + + it('reports an executable that cannot answer at all', async () => { + // Same policy as the ordered candidate list: an unreadable version is not + // a pass. Nothing is claimed about a version that was never read. + const message = await configuredNodeBelowFloor( + configured, + answers({ kind: 'not-found' }), + ); + expect(message).toContain('version unknown'); + }); + + it('probes one executable once, until the cache is reset', async () => { + // `resolveWorkerNodeCommand` runs on every worker spawn, so an unmemoized + // probe would be a process spawn per config init, per list, per run. + let calls = 0; + const counting = { + probe: () => { + calls++; + return Promise.resolve(ok('24.0.0')); + }, + }; + await configuredNodeBelowFloor(configured, counting); + await configuredNodeBelowFloor(configured, counting); + expect(calls).toBe(1); + + resetWorkerNodeCaches(); + await configuredNodeBelowFloor(configured, counting); + expect(calls).toBe(2); + }); + + it('keeps one verdict per executable, for a multi-root window', async () => { + expect( + await configuredNodeBelowFloor('/opt/old/node', answers(ok('20.19.4'))), + ).toBeDefined(); + expect( + await configuredNodeBelowFloor('/opt/new/node', answers(ok('24.0.0'))), + ).toBeUndefined(); + }); +}); + describe('probeNodeVersion', () => { it('reads the version of a real node executable', async () => { // The test run's own node is the one binary guaranteed to exist. @@ -348,4 +451,28 @@ describe('probeShellNodePath', () => { expect(found).toBeDefined(); expect(found?.startsWith('/')).toBe(true); }); + + it('stands the spawned shell in the given cwd', async () => { + if (process.platform === 'win32') return; + // A stand-in shell that answers with its own working directory — the one + // thing the real script's `command -v` resolves against that the caller + // controls. A wrong or dropped cwd makes the paths disagree. The tmpdir + // is realpath'd up front so `$PWD` (from getcwd(), symlinks resolved) + // compares against the same form. + const dir = fs.mkdtempSync( + path.join(fs.realpathSync(os.tmpdir()), 'rstack-shell-probe-'), + ); + try { + const fakeShell = path.join(dir, 'pwd-shell'); + fs.writeFileSync( + fakeShell, + `#!/bin/sh\necho ${START_TOKEN}\necho "$PWD/node"\necho ${END_TOKEN}\n`, + { mode: 0o755 }, + ); + const found = await probeShellNodePath(fakeShell, dir); + expect(found).toBe(path.join(dir, 'node')); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); }); From 1f916da941c65e3fd6ed54e5d521833f2033a27f Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 10 Aug 2026 20:31:58 +0800 Subject: [PATCH 5/5] docs: rename "load surface" to "load bound" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CONTEXT.md | 2 +- docs/adr/0001-node-runtime-selection.md | 4 ++-- packages/vscode/AGENTS.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index df07973..bf09b34 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -13,7 +13,7 @@ Glossary of terms used across rstack-editor. Code, docs, commit messages and rev - **VS Code Node runtime** — the Node.js shipped inside VS Code, which the extension host itself runs on. Its version follows VS Code's release cadence, and it is Electron's Node, on a different ABI line from plain Node. _Avoid_: host runtime, extension host runtime. - **User Node runtime** — the Node.js the user's own environment provides, discovered by the extension rather than shipped with it. _Avoid_: worker runtime, project-side Node. -- **Load surface** — the set of modules a piece of work can end up loading. A closed load surface holds only what the extension ships plus ABI-stable bindings; an open one can reach arbitrary project dependencies. Open load surfaces belong on the User Node runtime. +- **Load bound** — the limit on what a piece of work can end up loading: what the extension ships, plus ABI-stable N-API bindings. Work that stays inside the bound may run on the VS Code Node runtime; work that can load project code has no load bound and belongs on a User Node runtime. _Avoid_: load surface. - **Preflight** — the check that picks a User Node runtime, run once per extension host before any worker is spawned. Its failure is a status, never a crash. - **Runtime floor** — the minimum Node.js version supported for a User Node runtime. A declared support contract, not a probed capability. diff --git a/docs/adr/0001-node-runtime-selection.md b/docs/adr/0001-node-runtime-selection.md index d36f6ef..ca39c0a 100644 --- a/docs/adr/0001-node-runtime-selection.md +++ b/docs/adr/0001-node-runtime-selection.md @@ -30,7 +30,7 @@ The probe stays one-per-host — one PATH, one shell, one interactive start-up c ## Where the VS Code Node runtime _is_ allowed -The rule is not "never use it". The line is the **load surface**: work whose full transitive load set the extension controls (what it ships, plus N-API bindings, ABI-stable by construction) may run on the VS Code Node runtime; work that can reach arbitrary project dependencies must run on a User Node runtime. Loading a config is on the wrong side of that line — configs in this ecosystem import native bindings routinely — so config loading stays inside the worker, where the upstream machinery already puts it. +The rule is not "never use it". The line is the **load bound**: work whose loads stay inside what the extension controls (what it ships, plus N-API bindings, ABI-stable by construction) may run on the VS Code Node runtime; work that can load arbitrary project dependencies is unbounded and must run on a User Node runtime. Loading a config is on the wrong side of that line — configs in this ecosystem import native bindings routinely — so config loading stays inside the worker, where the upstream machinery already puts it. Note that _worker_ names a process, not a runtime. The worker is our own code; the runtime it runs on is the user's. @@ -38,7 +38,7 @@ Note that _worker_ names a process, not a runtime. The worker is our own code; t This decision is implemented for one path: the rstest worker. Two others sit on the wrong side of the line today, and this ADR does not move them. Naming them, so the rule is not read as an invariant the extension already holds: -- **fmt** spawns the project's `rs` bin on `process.execPath` with `ELECTRON_RUN_AS_NODE=1` (`stacks/fmt/run.ts`) — the VS Code Node runtime — and `rs fmt` loads the project's config in that process (`stacks/fmt/index.ts`). Open load surface, no floor, no preflight. +- **fmt** spawns the project's `rs` bin on `process.execPath` with `ELECTRON_RUN_AS_NODE=1` (`stacks/fmt/run.ts`) — the VS Code Node runtime — and `rs fmt` loads the project's config in that process (`stacks/fmt/index.ts`). Unbounded load, no floor, no preflight. - **lint** imports the project's `@rslint/core/config-loader` into the extension host and loads the user's `rslint.config.ts` there (`stacks/lint/configLoader.ts`), and runs user plugin rules on the same runtime (`stacks/lint/PluginLintPool.ts`). `stacks/lint/jitiPreflight.ts` already records the resulting divergence in so many words: that loader "runs on the extension host's Node — whose version is fixed by VS Code, not by the user — so the jiti branch can trigger in the editor even when the CLI works fine". Its answer is a diagnostic, not a runtime choice. Neither is cheap to move — each needs its own spawn-and-protocol work — and neither has a reported bug behind it yet. Known debt, deliberately: the next stack to load project code should follow the rule, and nobody should describe the rule as already universal. diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 8060310..6f1a9bb 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -14,7 +14,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten 3. **Resolve-from-project** — no tool binaries or tool packages in the VSIX; everything resolves from the user's project so the editor runs the CLI's exact versions. Version floors surface as a status, never a crash. All cooperating lint pieces (binary, config loader, plugin host) must come from one resolution root. 4. **Status aggregation** — stacks own no UI chrome; they report to the shell's single status bar item, which always exists. 5. **Worker-cwd decoupling** (test) — a project's cwd is explicit, not derived from the config file path; for native configs behavior stays byte-identical to upstream. -6. **Node runtime selection** (test) — the worker's Node is a **User Node runtime** chosen by the extension against one uniform floor, never assumed from PATH; the recovery path is the user's own shell, and the dividing line is the **load surface** (terms in CONTEXT.md; the full rule and rationale in `docs/adr/0001-node-runtime-selection.md`). Implemented for the rstest worker only — fmt and lint still load project code on the VS Code Node runtime, known debt recorded in the ADR, not an invariant the extension already holds. +6. **Node runtime selection** (test) — the worker's Node is a **User Node runtime** chosen by the extension against one uniform floor, never assumed from PATH; the recovery path is the user's own shell, and the dividing line is the **load bound** (terms in CONTEXT.md; the full rule and rationale in `docs/adr/0001-node-runtime-selection.md`). Implemented for the rstest worker only — fmt and lint still load project code on the VS Code Node runtime, known debt recorded in the ADR, not an invariant the extension already holds. ## Rules
'); + // Two action cells, and neither holds more than a single anchor. + const cells = row.split('').slice(1); + expect(cells).toHaveLength(2); + for (const cell of cells) { + expect(cell.split('').length - 1).toBeLessThanOrEqual(1); + } + // Rows keep their column count when a stack has only one action. + const idle = stackRows(html())[0] ?? ''; + expect(idle.split('')).toHaveLength(3); + }); + + it('sizes the table to the widest row, not to a fixed slot count', () => { + // The column count is derived so an action added to the row builder cannot + // be silently dropped off the end of a fixed-width table. + const { bar, html } = build(); + const slotsOf = (row: string) => row.split('').length - 1; + expect(stackRows(html()).map(slotsOf)).toEqual([1, 1, 1]); + expect(html()).toContain('colspan="3"'); + // One active stack widens every row, so the columns stay aligned. + bar.setActive('fmt', true); + expect(stackRows(html()).map(slotsOf)).toEqual([2, 2, 2]); + expect(html()).toContain('colspan="4"'); + }); + + it('keeps the per-stack actions in the table, not in the notice', () => { + const { bar, html } = build(); + bar.setActive('rstest', true); + bar.setState('rstest', { kind: 'crashed', detail: 'worker exited' }); + expect(stackRows(html())[1]).toContain( + 'command:rstack.rstest.output.focus', + ); + expect(stackRows(html())[1]).toContain('command:rstack.rstest.restart'); + expect(noticesOf(html())[0]).toContain( + '$(error) Rstest
worker exited