From b4279293013061e5cd2bec940fb2191e8dc4d0ce Mon Sep 17 00:00:00 2001 From: fi3ework Date: Wed, 12 Aug 2026 21:01:26 +0800 Subject: [PATCH 01/12] feat(vscode): format through rs fmt --lsp on the User Node runtime rstack-cli 0.5.2 ships an LSP mode for rs fmt; the fmt stack becomes a vscode-languageclient client of one rs fmt --lsp server per detected workspace folder, replacing the spawn-per-request --stdin-filepath path and its pre-spawned standby machinery. - One server per workspace folder, anchored at the folder root so the editor formats exactly like rs fmt run from that root; deepest-config anchoring is removed (docs/adr/0002-fmt-lsp-on-user-node-runtime.md). - The server runs on the User Node runtime picked by the shared preflight; nodeResolution moves from stacks/test/ to shared/, and the escape hatch becomes the resource-scoped rstack.nodeExecutable (rstest.nodeExecutable and rstack.rstest.nodeExecutable migrate to it; two pins set in one layer plan one write plus a superseded skip). - Version gate SUPPORT_MATRIX.rstack >= 0.5.2, no stdin fallback. - Config create/change/delete restarts the owning folder's server (debounced); rstack.restart also resets the host-wide Node caches so a restart behaves like a window reload for runtime selection. - E2E: run locally as VSCODE_CLI=1 pnpm test:e2e so the extension host keeps the caller's PATH (documented in AGENTS.md). --- CONTEXT.md | 13 +- README.md | 2 +- docs/adr/0001-node-runtime-selection.md | 12 +- docs/adr/0002-fmt-lsp-on-user-node-runtime.md | 41 + packages/vscode/AGENTS.md | 12 +- packages/vscode/README.md | 10 +- .../vscode/e2e/fixtures/rstack/package.json | 2 +- packages/vscode/e2e/suite/fmt.test.ts | 206 ++-- packages/vscode/package.json | 12 +- packages/vscode/src/extension.ts | 56 +- packages/vscode/src/migration.ts | 71 +- .../src/shared/nodeExecutableSetting.ts | 36 + .../{stacks/test => shared}/nodeResolution.ts | 96 +- packages/vscode/src/shared/versionCheck.ts | 19 +- packages/vscode/src/stacks/fmt/binEntry.ts | 28 + packages/vscode/src/stacks/fmt/index.ts | 883 ++++++++++-------- packages/vscode/src/stacks/fmt/run.ts | 336 ------- packages/vscode/src/stacks/fmt/standby.ts | 172 ---- packages/vscode/src/stacks/test/config.ts | 1 - packages/vscode/src/stacks/test/index.ts | 23 +- packages/vscode/src/stacks/test/master.ts | 60 +- packages/vscode/src/types.ts | 9 +- packages/vscode/tests/extension.test.ts | 21 + packages/vscode/tests/migration.test.ts | 49 +- .../test => shared}/nodeResolution.test.ts | 89 +- .../vscode/tests/stacks/fmt/binEntry.test.ts | 28 + packages/vscode/tests/stacks/fmt/run.test.ts | 252 ----- .../vscode/tests/stacks/fmt/standby.test.ts | 284 ------ .../vscode/tests/stacks/fmt/stubProcess.ts | 45 - .../vscode/tests/stacks/test/bridge.test.ts | 8 +- .../vscode/tests/stacks/test/master.test.ts | 12 +- packages/vscode/tests/versionCheck.test.ts | 4 +- 32 files changed, 1127 insertions(+), 1765 deletions(-) create mode 100644 docs/adr/0002-fmt-lsp-on-user-node-runtime.md create mode 100644 packages/vscode/src/shared/nodeExecutableSetting.ts rename packages/vscode/src/{stacks/test => shared}/nodeResolution.ts (79%) create mode 100644 packages/vscode/src/stacks/fmt/binEntry.ts delete mode 100644 packages/vscode/src/stacks/fmt/run.ts delete mode 100644 packages/vscode/src/stacks/fmt/standby.ts rename packages/vscode/tests/{stacks/test => shared}/nodeResolution.test.ts (87%) create mode 100644 packages/vscode/tests/stacks/fmt/binEntry.test.ts delete mode 100644 packages/vscode/tests/stacks/fmt/run.test.ts delete mode 100644 packages/vscode/tests/stacks/fmt/standby.test.ts delete mode 100644 packages/vscode/tests/stacks/fmt/stubProcess.ts diff --git a/CONTEXT.md b/CONTEXT.md index 091ce87..7f78930 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -14,7 +14,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 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. +- **Preflight** — the check that picks a User Node runtime, run once per extension host and shared by every process that loads project code (the test worker, the fmt server). Its failure is a status, never a crash. - **Runtime floor** — the version range a User Node runtime must satisfy (`NODE_RUNTIME_RANGE` in `shared/versionCheck.ts`). A declared support contract, not a probed capability. ## Tools and configs @@ -24,13 +24,12 @@ Glossary of terms used across rstack-editor. Code, docs, commit messages and rev - **Rstack config** — the unified `rstack.config.*` file consumed by rstack-cli (`rs`), holding per-tool sections. Tools never read it themselves; `rs` hands each tool its section through a shim. - **Shim** — the module rstack-cli ships per tool that loads the Rstack config and exposes that tool's section through the tool's ordinary explicit-config channel. The extension points upstream machinery at the shim rather than re-implementing Rstack config semantics. - **Bridged project** — a test project the extension synthesizes for a directory whose test signal is a Rstack config, wired to the shim. _Avoid_: virtual project, rstack project. +- **Generated shim** — a shim the extension writes itself for a bridged folder, baking in the absolute Rstack config path via the loader rstack publishes (`rstack/config`). Used where the tool's channel evaluates modules away from the project directory, so rstack-cli's shipped shim (which probes the current directory) cannot apply. +- **Bridged folder** — a workspace folder whose lint runs against a Rstack config: no native Rslint config exists anywhere in the folder, a Rstack config sits at the folder root, and the language server is pinned to a generated shim for its whole lifetime. _Avoid_: bridged workspace. +- **Config root** — the directory a tool's config is loaded from, which is also the directory the tool's process stands in. The editor always uses the workspace folder root, so it loads the config a terminal opened on that folder would; a subproject that needs its own config becomes its own workspace folder. _Avoid_: config directory, project root. - **Ownership** — the editor-side rule assigning a directory to one tool when both a native config and a Rstack config are present there: the atomic tool's native config wins and the bridge yields. This rule exists only in the editor; upstream CLIs never face the choice, since each reads only its own config. ## 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. -- **Standby** — the single pre-spawned `rs fmt` process held ready for one specific file, so the next format of that file skips the start-up cost. There is at most one standby, and it is only ever armed for the active editor's file ("the standby tracks the active editor"). An editor change that cannot be armed kills it; an editor holding nothing this stack formats leaves it to expire. -- **Arm** — create the standby for a file. Arming happens when the active editor lands on an eligible file and again right after a format consumed the previous standby. -- **Consume** — serve a format request with the armed standby. A standby serves exactly one request; a request the standby cannot serve falls back to a cold format. -- **Hot format** — a format request served by consuming the standby. -- **Expire** — kill an idle standby to reclaim its memory. An expired standby is not an error; the next eligible event simply arms a new one. +- **Fmt server** — the `rs fmt` language server the extension runs for one workspace folder, and the only thing that formats documents in it. It loads that folder's config root once and holds it for its lifetime, so a config change is a **restart** of the server, never a message to it. _Avoid_: formatter daemon, fmt worker. +- **Fmt folder set** — the workspace folders that currently have a fmt server, kept in step with detection: a newly detected folder gains one, a folder that loses detection loses its own, and a folder in both sets keeps the server it already has. diff --git a/README.md b/README.md index 3c2a829..63826de 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ The extension takes its configuration from five sources. The tool-native configs | `rslint.config.*` | **Supported.** Diagnostics, quick fixes and the language server, all resolved from the `@rslint/core` installed in your project. | | `rstest.config.*` | **Supported.** Test discovery, run and debug, watch mode, coverage and snapshot updates in the Test Explorer. | | `define.test()` in `rstack.config.*` | **Supported.** Tests run through the same config shim `rs test` uses, so the editor and the CLI resolve the config identically. | -| `define.fmt()` in `rstack.config.*` | **Supported.** Document formatting through the project-local `rs fmt --stdin-filepath`, resolving the config the same way the CLI does; an `rs fmt` language server is the longer-term path. | +| `define.fmt()` in `rstack.config.*` | **Supported.** Document formatting through the project-local `rs fmt` language server (`rs fmt --lsp`), one per workspace folder, loading the config from the folder root — the same config `rs fmt` run there would use. Needs `rstack` 0.5.2 or newer. | | `define.lint()` in `rstack.config.*` | **Planned.** Linting a project configured only through `rstack.config.*` needs upstream changes in Rslint and rstack-cli before the editor can evaluate it correctly. `rs lint` on the command line is unaffected. | ## License diff --git a/docs/adr/0001-node-runtime-selection.md b/docs/adr/0001-node-runtime-selection.md index 6c5c65d..dc4ac56 100644 --- a/docs/adr/0001-node-runtime-selection.md +++ b/docs/adr/0001-node-runtime-selection.md @@ -22,7 +22,7 @@ Native type stripping is the _only_ thing on the worker's path that needs more t 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. +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. The 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; a fmt server start (ADR 0002) stands in its own workspace folder root, and gets there first whenever no rstest warm-up preceded it. 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. @@ -34,18 +34,18 @@ The rule is not "never use it". The line is the **load bound**: work whose loads 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 +### Where the line is drawn today -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: +This decision was written for one path, the rstest worker, and named two others that sat on the wrong side of the line. One of them has since moved: -- **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. +- **fmt** used to spawn the project's `rs` bin on `process.execPath` with `ELECTRON_RUN_AS_NODE=1` (`stacks/fmt/run.ts`) — the VS Code Node runtime — and let `rs fmt` load the project's config in that process: unbounded load, no floor, no preflight. It now runs `rs fmt --lsp` as a language server on a User Node runtime chosen by this decision's own logic, against the same floor, with the shared `rstack.nodeExecutable` as its escape hatch. Why the server, and why one per workspace folder: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. - **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. +Lint is what moving costs when it is not cheap: fmt's move needed a whole upstream language server to exist first, and lint needs its own spawn-and-protocol work for the config loader and the plugin host, with no reported bug behind it yet. It stays known debt, deliberately — the rule is not universal until that entry is gone, and nobody should describe it as if it were. ## 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. +- An explicit `rstack.nodeExecutable` (shipped as `rstack.rstest.nodeExecutable` when this was written, shared with the fmt server since ADR 0002 and migrated for existing users) 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. diff --git a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md new file mode 100644 index 0000000..7305180 --- /dev/null +++ b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md @@ -0,0 +1,41 @@ +# Formatting through the `rs fmt` language server + +Document formatting is served by **`rs fmt --lsp`** — one language server per detected workspace folder, spawned with its cwd at the folder root, running on a **User Node runtime** that satisfies the floor ADR 0001 sets (`^22.18.0 || >=23.6.0`). It replaces a spawn-per-request `rs fmt --stdin-filepath` MVP that ran on the VS Code Node runtime and kept one pre-spawned process warm for the active editor. The floor for the project's `rstack` rises to `>=0.5.2`, the first release that ships `--lsp`; older releases surface as `version mismatch` and format nothing. + +## Why a server rather than a process per request + +`rs fmt --lsp` is a real LSP server over stdio. It advertises exactly two things — `documentFormattingProvider` and full-document text sync — so the client registers the formatting provider from the capability and the stack registers no provider of its own. Every format then costs one request on a live process instead of a Node start-up, which is what the standby existed to hide: a single pre-spawned `rs fmt` armed for the active editor, with its own arm/consume/expire lifecycle, a bounded exception to "no warm tier" that is now unnecessary. The stdin path goes with it: the server formats the in-memory buffer through ordinary `didChange` notifications and carries its own staleness guard. + +The server also owns the caching that the old path could not do at all. It loads one config — on the first formatting request, from the workspace root the client reports — and holds it for its process lifetime. That is why a config change is a **restart** of that folder's server: `rs fmt --lsp` in 0.5.2 has no config-change notification and no watcher of its own, and upstream's own guidance is to launch one server per config root and restart it after editing the config. + +## Why the folder root, not the deepest config + +The old path spawned in the deepest directory holding an `rstack.config.*` above the file being formatted. `rs fmt` loads exactly one config, from its cwd, with no upward walk and no merging — and the config a project documents is the one `define.fmt()` sits in at the repository root, which is where a user runs `rs fmt`. Deepest-config-wins therefore let the editor format a file against a config the terminal would never have chosen: the same class of divergence ADR 0001 rejects a runtime fallback for, arrived at through cwd instead of through Node. It also disagreed with the lint stack, which anchors on the folder root because there is one server per workspace folder and its cwd is that root. + +One rule across both stacks now: the workspace folder root is the config root. A project that genuinely needs a different fmt config per subproject adds that subproject as its own workspace folder — the same remedy the lint bridge already gives, and the only one that keeps the editor and a terminal opened in that directory agreeing. + +## Why the User Node runtime + +The server loads the project's `rstack.config.*` through `@rstackjs/load-config` with `loader: 'native'` — the exact path ADR 0001 analysed to set the worker floor, with no jiti fallback and no `process.features.typescript` consultation. So fmt is not a new case: it is the second caller of the same decision, and it takes the floor, the candidate order (PATH `node`, then the user's interactive shell) and the failure reporting out of the one shared module, `shared/nodeResolution.ts`. The escape hatch is shared too — `rstack.nodeExecutable`, resource-scoped, honoured whenever it is set and probed anyway, advisory-only. A user pinning a Node for one tool means it for the toolchain, so there is one setting rather than one per stack (the retired `rstack.rstest.nodeExecutable` migrates to it). + +Falling back to the VS Code Node runtime stays rejected — ADR 0001's load-bearing "no", unchanged. It is worth naming that the old fmt path did exactly that: `process.execPath` with `ELECTRON_RUN_AS_NODE=1`, loading the user's config on Electron's Node, with no floor and no preflight. Moving the server onto a User Node runtime is what takes fmt off that ADR's debt list. + +## Considered options + +**Keeping `--stdin-filepath` as a fallback for `rstack < 0.5.2`** — rejected: two formatting code paths, of which the fallback is the one nobody exercises, and the two do not fail alike (the stdin path anchored on a directory, the server on a workspace root). A version gate states the requirement once, in a place the status bar can read out, and leaves one path to test. + +**A server per config root** — the shape the old `pickConfigDir` implied, now with processes: discover every `rstack.config.*` under the folder and run a server for each. Rejected: it reproduces exactly the editor/terminal divergence above, at a higher price (N long-lived processes and a routing rule per document), and it would be a rule the fmt stack alone holds. + +**One server for the whole window** — rejected: the server binds one config root, so a multi-root window would have to elect a folder and silently format the others against a foreign config. + +**The VS Code Node runtime** — rejected in ADR 0001 ("Falling back to the VS Code Node runtime"), and the fmt server is squarely on the wrong side of the load bound: it loads arbitrary project code through the config. + +**Watching the config and telling the live server** — rejected because there is nothing to tell: 0.5.2 handles `initialize`, the sync notifications, `textDocument/formatting` and `shutdown`/`exit`, and nothing else. Restarting the folder's runtime is the only way to drop its cached config. + +## Consequences + +- There is no cold path any more, so formatting is briefly unavailable after activation, after a restart and after a config change, while that folder's server starts. A format requested before the client has registered the server's capability finds no formatter for the document; nothing falls back to a fresh process. +- The server advertises document formatting only: no range or selection formatting, no format-on-type, no diagnostics. It also ignores the editor's `FormattingOptions` (tab size, spaces) and the client's language id — the file path picks the parser and the project's config decides the style. An editor setting that disagrees with the project config loses, which is the same answer `rs fmt` gives in a terminal. +- Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server. +- An `rstack.config.*` create, change or delete restarts the server of the folder that contains it, and only that one; a detection change reconciles the folder set and leaves already-running servers alone. Both are deliberate: a healthy server's cached config is worth keeping. +- The extension now holds one long-lived Node process per detected folder for fmt. Each is owned by the same process owner the lint client uses, so a stop is bounded (SIGTERM, then SIGKILL) and the automatic restart vscode-languageclient performs cannot leave an orphan behind. diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 80a10da..f9506c7 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 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. +6. **Node runtime selection** (test, fmt) — the Node a project-loading child process runs on 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`). Both callers — the rstest worker and the `rs fmt --lsp` server — take the decision from the one shared module (`shared/nodeResolution.ts`) and share one escape hatch, the resource-scoped `rstack.nodeExecutable` (`shared/nodeExecutableSetting.ts`); each appends its own consequence to the shared preflight message. Lint still loads project code on the VS Code Node runtime — known debt recorded in the ADR, not an invariant the extension already holds. ## Rules @@ -23,7 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - Reconciles and restarts share one serialized queue (`enqueue`); a reconcile leaves a live stack alone, so the restart commands are the only path that rebuilds one. Do not add a second queue. - Restart is a shell concern, not a stack one: `rstack.restart` rebuilds every controller, `rstack..restart` rebuilds one. A stack must never register its own restart command — a shallower "bounce the tool's process" restart keeps that controller's stale package resolution and version check, which is the bug the command exists to clear. - Deprecated `rslint.json` / `rslint.jsonc` are unsupported by decision, not omission — never make them detection signals. -- Never share a child process across stacks: the tools have incompatible cwd semantics (lint LSP anchors on spawn cwd; test worker pins to project root; `rs fmt` resolves config from spawn cwd with no upward walk). +- Never share a child process across stacks: the tools have incompatible cwd semantics (lint LSP anchors on spawn cwd; test worker pins to project root; the `rs fmt` server takes its config root from the workspace folder the client reports, falling back to spawn cwd, with no upward walk either way). The lint and fmt servers now happen to stand in the same directory — the folder root — which changes nothing: they are different CLIs, different protocols and different version gates. - In Restricted Mode (workspace trust), only the status bar runs — no process spawns, no project code loaded. - The activation exports exist only for the E2E suites; they are not a stable API and carry no compatibility guarantees. - Watch-pattern globs must not contain nested brace groups — VS Code's glob parser silently fails on them (regression-tested). @@ -32,11 +32,12 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The lint × `rstack.config.*` bridge was built and deliberately removed: a partial editor-side bridge gave wrong results, and a correct one needs upstream work first. `TODO(rstack-bridge)` markers carry the plan. Do not reintroduce a partial bridge. - The test × `rstack.config.*` bridge stays thin on purpose: it points the upstream machinery at rstack's shipped shim and lets the shim interpret the config inside the worker, same as the CLI. Never re-implement rstack config semantics in the extension. -- 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. +- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. +- fmt importing `stacks/lint/LanguageServerProcessOwner.ts` is not a refactor across the copies: that file has no lint imports and no lint behaviour, it only owns the native children of one language client — including the ones vscode-languageclient's automatic restart creates, which is exactly the leak an ad-hoc copy would reintroduce. Lint's `ManagedLanguageClient` is _restated_ in `stacks/fmt/index.ts` instead, because importing it from `Rslint.ts` would drag the lint runtime graph (plugin pool, config loader, rstack bridge) into the fmt stack. Keep that line where it is: shared process ownership yes, shared stack runtime no. - `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 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+; on the 23.x line only from 23.6). 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`. +- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. `stacks/fmt/binEntry.ts` is a one-function module for the same testability reason: `stacks/fmt/index.ts` evaluates `vscode` and `vscode-languageclient`, so a pure helper left in it is not unit-testable. +- The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`), because the strictest thing any of these processes does — the test worker through rstack's shim, the `rs fmt` server through `@rstackjs/load-config` — is load an `rstack.config.*` with `loader: 'native'`, no jiti fallback, and so needs native type stripping (22.18+; on the 23.x line only from 23.6). 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 @@ -44,3 +45,4 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - E2E suites ported from upstream keep upstream's assertion semantics; every intentional deviation is documented in a comment in the test itself. A failing ported test is a regression, not a test to adjust. - E2E fixtures install published npm packages (not workspace links): the extension must work against what users actually install. Fixture `node_modules` are disposable and never committed. - Prefer running the E2E slice that covers the change over the full chain: `pnpm test:e2e ` (or the `test:e2e:` aliases). Slices are declared in the `SLICES` table in `e2e/run.mjs` (name, fixtures, entry) — the package.json scripts are thin forwards and carry no slice knowledge. `RSTACK_LINT_E2E_SUITES=` filters lint suites. +- Run E2E locally as `VSCODE_CLI=1 pnpm test:e2e `. Without it, the launched VS Code overwrites the extension host's `PATH` with a login-shell snapshot; on a machine whose login-shell `node` is below the runtime floor, the User Node preflight (correctly) refuses and every fmt test times out waiting for a server. CI is unaffected — its PATH `node` is new enough either way. The heavier alternative, `--force-disable-user-env` in `e2e/runTest.ts`, was deliberately not taken: it would change env fidelity for every slice. diff --git a/packages/vscode/README.md b/packages/vscode/README.md index e7b5c56..11617ee 100644 --- a/packages/vscode/README.md +++ b/packages/vscode/README.md @@ -13,7 +13,7 @@ The extension ships no tool binaries: `@rslint/core`, `@rstest/core` and `rstack - **Linting (Rslint)** — diagnostics, quick fixes and auto-fix on save via Rslint's language server. - **Testing (Rstest)** — a Test Explorer tree built from your test files: run or debug individual tests, suites or files; the tree stays in sync as files change; failed tests show up as editor diagnostics. -- **rstack-cli** — document formatting through the project-local `rs fmt` CLI. +- **rstack-cli** — document formatting through the project-local `rs fmt` language server, one per workspace folder. - **One status bar item** — a single `Rstack` entry shows which tools are active in the current workspace and why. ## Detection @@ -38,7 +38,9 @@ The project-resolved packages are checked against a support matrix at runtime; a | -------------- | --------- | | `@rslint/core` | `>=0.7.2` | | `@rstest/core` | `>=0.6.0` | -| `rstack` | `>=0.3.5` | +| `rstack` | `>=0.5.2` | + +`rstack` 0.5.2 is the first release with `rs fmt --lsp`, the language server the formatter is a client of; on older releases the formatter reports `version mismatch` instead of formatting. ## Auto-fix on save (Rslint) @@ -75,6 +77,7 @@ All settings live under the unified `rstack.*` namespace. There are no `rslint.* | Setting | Default | Description | | --- | --- | --- | | `rstack.enable` | `true` | Master switch for the whole extension. | +| `rstack.nodeExecutable` | — | Node binary used for the processes that load your project: the test worker and the `rs fmt` language server. Empty means the extension picks one (`PATH` first, then the `node` your interactive shell resolves). | | `rstack.rslint.enable` | `true` | Enable/disable the Rslint integration. | | `rstack.rslint.binPath` | `local` | `local` (project `node_modules`, incl. Yarn PnP) or `custom`. | | `rstack.rslint.customBinPath` | — | Binary path used when `binPath` is `custom`. | @@ -84,7 +87,6 @@ All settings live under the unified `rstack.*` namespace. There are no `rslint.* | `rstack.rstest.testCaseCollectMethod` | `ast` | `ast` (fast) or `runtime` (supports dynamic test generation). | | `rstack.rstest.applyDiagnostic` | `true` | Show diagnostics in the editor and Problems panel for failures. | | `rstack.rstest.rstestPackagePath` | — | Explicit `@rstest/core` `package.json`, last-resort override. | -| `rstack.rstest.nodeExecutable` | — | Node binary used for the test worker. | | `rstack.rstest.nodeExecArgs` | `[]` | Extra Node args for the test worker. | | `rstack.rstest.nodeEnv` | `null` | Extra env for the test worker. | | `rstack.rstest.debugNodeEnv` | `null` | Extra env when debugging tests. | @@ -98,6 +100,8 @@ All settings live under the unified `rstack.*` namespace. There are no `rslint.* To use `rs fmt` as the formatter for supported documents, opt in through your VS Code settings: `"editor.defaultFormatter": "rstack.rstack"`. The extension never changes `editor.defaultFormatter` itself. +Formatting runs one `rs fmt` language server per workspace folder, which loads `define.fmt()` from the `rstack.config.*` at the **folder root** — the same config `rs fmt` in a terminal there would use, so a config in a subdirectory is not picked up (open that subdirectory as its own workspace folder if it needs different settings). Editing the config restarts the server for you. Your editor's own formatting options (tab size, spaces) are not consulted: the project config decides, exactly as on the command line. + ## Migrating from the standalone extensions Run **Rstack: Migrate Rslint/Rstest Settings** from the Command Palette (it is also offered once, dismissibly, when legacy keys are found). diff --git a/packages/vscode/e2e/fixtures/rstack/package.json b/packages/vscode/e2e/fixtures/rstack/package.json index 6f1070e..8bdf70c 100644 --- a/packages/vscode/e2e/fixtures/rstack/package.json +++ b/packages/vscode/e2e/fixtures/rstack/package.json @@ -5,7 +5,7 @@ "type": "module", "description": "E2E fixture: an rstack-cli project whose only config is `rstack.config.ts`, which lights the Rstest and rs fmt stacks.", "dependencies": { - "rstack": "0.5.0-alpha.1" + "rstack": "0.5.2" }, "devDependencies": { "jiti": "^2.0.0" diff --git a/packages/vscode/e2e/suite/fmt.test.ts b/packages/vscode/e2e/suite/fmt.test.ts index 6f5d2d7..fe0b006 100644 --- a/packages/vscode/e2e/suite/fmt.test.ts +++ b/packages/vscode/e2e/suite/fmt.test.ts @@ -4,23 +4,15 @@ import type { RstackExtensionExports } from '../../src/types'; import { eventually } from './helpers'; const EXTENSION_ID = 'rstack.rstack'; -let provider: vscode.DocumentFormattingEditProvider; -let armedFilePath: () => string | undefined; -let lastServe: () => 'hot' | 'cold' | undefined; - /** - * Shows a document and waits for it to own the standby. Showing it is what arms - * one — the invariant is that the standby tracks the active editor — but arming - * is debounced and an earlier test may have left a standby on another file, so - * the wait polls until the armed file is this one. + * The fmt stack's E2E-only exports. They describe the folder set, not a + * provider: the formatting provider is registered by vscode-languageclient from + * each server's `documentFormattingProvider` capability, so the only thing the + * stack itself can be asked about is which folders have a live server. */ -const armStandbyFor = async (uri: vscode.Uri): Promise => { - const editor = await vscode.window.showTextDocument(uri); - await eventually(() => { - assert.equal(armedFilePath(), uri.fsPath); - }, `the standby to be armed for ${uri.fsPath}`); - return editor; -}; +let languages: readonly string[]; +let formats: (fsPath: string) => boolean; +let folderStates: () => Record; const folderNamed = (name: string): vscode.WorkspaceFolder => { const folder = (vscode.workspace.workspaceFolders ?? []).find( @@ -30,6 +22,18 @@ const folderNamed = (name: string): vscode.WorkspaceFolder => { return folder; }; +const fixtureFile = (folder: string, ...segments: string[]): vscode.Uri => + vscode.Uri.joinPath(folderNamed(folder).uri, ...segments); + +/** Waits until the rstack fixture's `rs fmt` server covers the given file. */ +const waitForCoverage = (uri: vscode.Uri): Promise => + eventually(() => { + assert.ok( + formats(uri.fsPath), + `${uri.fsPath} is not covered by a running rs fmt server`, + ); + }, 'the rstack folder to have a running rs fmt server'); + suite('fmt', () => { suiteSetup(async () => { const extension = @@ -37,23 +41,37 @@ suite('fmt', () => { assert.ok(extension, `${EXTENSION_ID} is not installed in the test host`); const api = await extension.activate(); const exports = await api.whenStackActive('fmt'); - assert.ok(exports.provider, 'the fmt stack did not export its provider'); - provider = exports.provider as vscode.DocumentFormattingEditProvider; assert.ok( - typeof exports.armedFilePath === 'function', - 'the fmt stack did not export its standby hook', + Array.isArray(exports.languages), + 'the fmt stack did not export its language list', ); - armedFilePath = exports.armedFilePath as () => string | undefined; - lastServe = exports.lastServe as () => 'hot' | 'cold' | undefined; + languages = exports.languages as readonly string[]; + assert.ok( + typeof exports.formats === 'function', + 'the fmt stack did not export its coverage hook', + ); + formats = exports.formats as (fsPath: string) => boolean; + assert.ok( + typeof exports.folderStates === 'function', + 'the fmt stack did not export its folder-state hook', + ); + folderStates = exports.folderStates as () => Record; }); test('formats through the provider without touching the workspace', async () => { - const uri = vscode.Uri.joinPath( - folderNamed('rstack').uri, - 'src', - 'needs-format.ts', - ); - const { document, edits } = await eventually(async () => { + const uri = fixtureFile('rstack', 'src', 'needs-format.ts'); + // Deviation from the pre-LSP suite, in two parts. The stack no longer + // registers a provider at activation: one appears only once the folder's + // `rs fmt --lsp` server has started and its client has registered the + // server's `documentFormattingProvider` capability. Until then VS Code's + // built-in TypeScript formatter is the only candidate, and it answers + // *successfully* — with single-quoted, non-Prettier text. So waiting for + // "some edits" is not enough (that is what the built-in returns): the + // suite waits for the folder's server first, and the retry loop asserts + // the formatted text, not merely a non-empty edit list. + await waitForCoverage(uri); + + const document = await eventually(async () => { const document = await vscode.workspace.openTextDocument(uri); const edits = await vscode.commands.executeCommand( 'vscode.executeFormatDocumentProvider', @@ -61,97 +79,69 @@ suite('fmt', () => { { tabSize: 2, insertSpaces: true }, ); assert.ok(edits && edits.length > 0, 'the formatter returned no edits'); - return { document, edits }; - }, 'the rs fmt provider to return an edit'); - const text = document.getText(); - let applied = text; - // The command post-processes our single minimal edit through VS Code's - // `computeMoreMinimalEdits`, so apply its result from the end backwards. - for (const edit of [...edits].sort( - (left, right) => - document.offsetAt(right.range.start) - - document.offsetAt(left.range.start), - )) { - const start = document.offsetAt(edit.range.start); - const end = document.offsetAt(edit.range.end); - applied = applied.slice(0, start) + edit.newText + applied.slice(end); - } - // The quote normalization is Prettier-specific, so VS Code's built-in - // TypeScript formatter cannot mask a failed Rstack provider via fallback. - assert.equal(applied, 'const answer = { value: "42" };\n'); - }); + let applied = document.getText(); + // The command post-processes our single minimal edit through VS Code's + // `computeMoreMinimalEdits`, so apply its result from the end backwards. + for (const edit of [...edits].sort( + (left, right) => + document.offsetAt(right.range.start) - + document.offsetAt(left.range.start), + )) { + const start = document.offsetAt(edit.range.start); + const end = document.offsetAt(edit.range.end); + applied = applied.slice(0, start) + edit.newText + applied.slice(end); + } + // The quote normalization is Prettier-specific, so VS Code's built-in + // TypeScript formatter cannot mask a failed Rstack provider via fallback. + assert.equal(applied, 'const answer = { value: "42" };\n'); + return document; + }, 'the rs fmt language server to return a Prettier-formatted edit'); - test('formats from the standby armed for the active editor', async () => { - const uri = vscode.Uri.joinPath( - folderNamed('rstack').uri, - 'src', - 'needs-format.ts', + // The server formats the buffer it was sent and returns edits; nothing on + // disk may move, which is what "without touching the workspace" means. + assert.equal( + (await vscode.workspace.fs.readFile(uri)).toString(), + "const answer={value:'42'};\n", ); - const editor = await armStandbyFor(uri); + // The document is only ever opened, never edited by the format request. + assert.equal(document.isDirty, false); + }); - const edits = await vscode.commands.executeCommand( - 'vscode.executeFormatDocumentProvider', - uri, - { tabSize: 2, insertSpaces: true }, - ); - assert.ok(edits && edits.length > 0, 'the formatter returned no edits'); + test('covers only the folders where fmt is detected', async () => { + // Replaces the pre-LSP "returns no edits for a folder where fmt is not + // detected": there is no stack-owned provider left to call with a + // foreign document. The equivalent question — is this path covered by a + // running `rs fmt` server — is now the `formats` export, and it answers + // per folder because a server's reach is its workspace folder. + await waitForCoverage(fixtureFile('rstack', 'src', 'needs-format.ts')); - const text = editor.document.getText(); - let applied = text; - // The command post-processes our single minimal edit through VS Code's - // `computeMoreMinimalEdits`, so apply its result from the end backwards. - for (const edit of [...edits].sort( - (left, right) => - editor.document.offsetAt(right.range.start) - - editor.document.offsetAt(left.range.start), - )) { - const start = editor.document.offsetAt(edit.range.start); - const end = editor.document.offsetAt(edit.range.end); - applied = applied.slice(0, start) + edit.newText + applied.slice(end); - } - assert.equal(applied, 'const answer = { value: "42" };\n'); - // The cold path produces the same text, so only this proves the request - // actually consumed the standby. - assert.equal(lastServe(), 'hot'); + // TypeScript, and one of the languages the stack claims, but fmt is not + // detected for the rslint fixture — no server covers it. + const uncovered = fixtureFile('rslint', 'src', 'index.ts'); + assert.ok(languages.includes('typescript')); + assert.equal(formats(uncovered.fsPath), false); }); - test('clears the standby when the active editor cannot be armed', async () => { - await armStandbyFor( - vscode.Uri.joinPath(folderNamed('rstack').uri, 'src', 'needs-format.ts'), - ); - - // TypeScript, so the provider matches it, but fmt is not detected for this - // folder — nothing can be armed, and the previous file's standby must not - // outlive the editor that owned it. - const unarmable = vscode.Uri.joinPath( - folderNamed('rslint').uri, - 'src', - 'index.ts', - ); - await vscode.window.showTextDocument(unarmable); + test('runs one server for the detected folder and none for the others', async () => { await eventually(() => { - assert.equal(armedFilePath(), undefined); - }, 'the standby to be cleared'); - }); + assert.equal( + folderStates()[folderNamed('rstack').uri.fsPath], + 'running', + `folder states: ${JSON.stringify(folderStates())}`, + ); + }, 'the rstack folder runtime to report running'); - test('returns no edits for a folder where fmt is not detected', async () => { - const uri = vscode.Uri.joinPath( - folderNamed('rslint').uri, - 'src', - 'index.ts', - ); - const document = await vscode.workspace.openTextDocument(uri); - const cancellation = new vscode.CancellationTokenSource(); - try { - const edits = await provider.provideDocumentFormattingEdits( - document, - { tabSize: 2, insertSpaces: true }, - cancellation.token, + const states = folderStates(); + // A runtime exists only for a folder detection lit, so the two fixtures + // without an Rstack config must not appear at all — a stopped or crashed + // entry for them would mean a server was spawned where none belongs. + for (const name of ['rslint', 'rstest']) { + assert.equal( + folderNamed(name).uri.fsPath in states, + false, + `${name} has an rs fmt runtime: ${JSON.stringify(states)}`, ); - assert.ok(!edits || edits.length === 0); - } finally { - cancellation.dispose(); } }); }); diff --git a/packages/vscode/package.json b/packages/vscode/package.json index cf1f756..3c67597 100644 --- a/packages/vscode/package.json +++ b/packages/vscode/package.json @@ -134,6 +134,12 @@ "default": true, "scope": "window", "markdownDescription": "Master switch for the Rstack extension. When disabled, no stack is registered — the status bar item stays visible so the extension can still be told apart from a broken install." + }, + "rstack.nodeExecutable": { + "order": 1, + "type": "string", + "scope": "resource", + "markdownDescription": "Overrides the `node` binary used to spawn processes that load your project — the Rstest test worker and the `rs fmt` language server. Provide an absolute path to a Node.js executable (for example, a version-manager or custom build). An explicit choice is always honoured — the escape hatch when no suitable Node.js can be found automatically — but the version check still runs as an advisory: pointing at a Node.js below the supported floor shows a status-bar warning while runs proceed with it. 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." } } }, @@ -234,12 +240,6 @@ "default": true, "description": "Show diagnostics in the editor and Problems panel for failed tests." }, - "rstack.rstest.nodeExecutable": { - "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). An explicit choice is always honoured — the escape hatch when no suitable Node.js can be found automatically — but the version check still runs as an advisory: pointing at a Node.js below the supported floor shows a status-bar warning while runs proceed with it. 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, "type": "array", diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 2a9a0ea..f5fcbf9 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -2,6 +2,7 @@ import vscode from 'vscode'; import { Channels } from './channels'; import { DetectionService } from './detection'; import { maybePromptForMigration, runSettingsMigration } from './migration'; +import { resetUserNodeCaches } from './shared/nodeResolution'; import { StatusBar } from './statusBar'; import { type DetectionSnapshot, @@ -89,17 +90,36 @@ class ExtensionShell { // 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. + // One save can move a setting shared by several stacks (the runtime + // pin), so the moved stacks are collected and restarted as one pass — + // one detection sweep, one retire/reconcile wave — instead of one full + // restart each. + const moved: StackId[] = []; + const reasons: string[] = []; for (const [stack, controller] of this.#controllers) { if (gated.has(stack)) { continue; } - const moved = controller.restartOnSettings?.find((setting) => - event.affectsConfiguration(`rstack.${stack}.${setting}`), + // Entries are relative to the stack's namespace unless they name a + // fully qualified `rstack.*` key — the form shared settings use. + const settingKey = (setting: string): string => + setting.startsWith('rstack.') + ? setting + : `rstack.${stack}.${setting}`; + const setting = controller.restartOnSettings?.find((candidate) => + event.affectsConfiguration(settingKey(candidate)), ); - if (moved) { - void this.restart(stack, `rstack.${stack}.${moved} changed`); + if (setting) { + moved.push(stack); + const key = settingKey(setting); + if (!reasons.includes(key)) { + reasons.push(key); + } } } + if (moved.length > 0) { + void this.restart(moved, `${reasons.join(', ')} changed`); + } }), // Restricted Mode shows the status bar only; trust unlocks the stacks // without a window reload. @@ -238,16 +258,26 @@ class ExtensionShell { * `reason` is for the callers that are not a user picking the command — * `restartOnSettings` passes what moved. */ - restart(stack?: StackId, reason?: string): Promise { - return this.enqueue(() => this.runRestart(stack, reason)); + restart( + stacks?: StackId | readonly StackId[], + reason?: string, + ): Promise { + return this.enqueue(() => this.runRestart(stacks, reason)); } - private async runRestart(only?: StackId, reason?: string): Promise { + private async runRestart( + only?: StackId | readonly StackId[], + reason?: string, + ): Promise { if (this.#disposed) { return; } - const stacks = only ? [only] : STACK_IDS; - const what = only ? STACK_LABELS[only] : 'Rstack'; + const stacks = + only === undefined ? STACK_IDS : typeof only === 'string' ? [only] : only; + const what = + stacks.length === STACK_IDS.length + ? 'Rstack' + : stacks.map((stack) => STACK_LABELS[stack]).join(', '); this.#channels.shell.info( `Restarting ${what}${reason ? ` (${reason})` : ''}`, ); @@ -259,6 +289,14 @@ class ExtensionShell { if (this.#disposed) { return; } + // Restart exists to clear stale resolution, and the User Node preflight + // memo is host-scoped state shared by every stack — so the shell clears it + // once per pass, after the retire wave and before anything re-registers. + // Owned here rather than in the stacks' `dispose()`: a stack-owned reset + // fires on every teardown (deactivate, detection loss) and clears the + // resolution a live sibling stack is relying on, twice per shared-setting + // change. + resetUserNodeCaches(); try { // A plain pass: `refresh` updates the snapshot whether or not the // signature moved, and the reconcile below rebuilds every stack from it. diff --git a/packages/vscode/src/migration.ts b/packages/vscode/src/migration.ts index b89aad8..0414890 100644 --- a/packages/vscode/src/migration.ts +++ b/packages/vscode/src/migration.ts @@ -1,8 +1,10 @@ import vscode from 'vscode'; /** - * Settings migration from the two retired standalone extensions (`rstack.rslint` - * and `rstack.rstest`) into the unified `rstack.*` namespace. + * Settings migration from retired names into current ones: the two standalone + * extensions (`rstack.rslint` and `rstack.rstest`) into the unified `rstack.*` + * namespace, plus keys this extension itself has since renamed + * (`rstack.rstest.nodeExecutable` -> `rstack.nodeExecutable`). * * Shape of the feature: * @@ -44,7 +46,13 @@ export type SkipReason = /** The new key already has an explicit value in the same layer. */ | 'target-already-set' /** A folder-layer value for a window-scoped target setting. */ - | 'not-folder-scoped'; + | 'not-folder-scoped' + /** + * A later legacy key in the same layer maps to the same target (the two + * retired runtime pins both map to `rstack.nodeExecutable`); the later one — + * the key more recently in effect — carries the value. + */ + | 'superseded'; type ValueMapping = | { readonly kind: 'value'; readonly value: unknown } @@ -89,7 +97,9 @@ const mapBinPath = (value: unknown): ValueMapping => { /** * Legacy Rstest keys, in manifest order. Every one of them is a mechanical * `rstest.` -> `rstack.rstest.` rename; only the scope of the *new* - * key differs, and it is the new manifest that decides it. + * key differs, and it is the new manifest that decides it. The one exception, + * `rstest.nodeExecutable`, is mapped explicitly below: its target is the + * shared `rstack.nodeExecutable`, not a `rstack.rstest.*` key. * * Derived from `rstest/packages/vscode/package.json` (14 settings) and this * repository's `contributes.configuration`. Rstest contributes no `enable` @@ -97,7 +107,6 @@ const mapBinPath = (value: unknown): ValueMapping => { */ const RSTEST_KEYS: readonly (readonly [string, 'resource' | 'window'])[] = [ ['rstestPackagePath', 'resource'], - ['nodeExecutable', 'resource'], ['nodeExecArgs', 'resource'], ['nodeEnv', 'resource'], ['debugNodeEnv', 'resource'], @@ -113,8 +122,9 @@ const RSTEST_KEYS: readonly (readonly [string, 'resource' | 'window'])[] = [ ]; /** - * The complete legacy inventory: 4 Rslint keys + 14 Rstest keys. Kept in one - * table so the preview, the writer and the tests cannot disagree. + * The complete legacy inventory: 4 Rslint keys + 14 Rstest keys + 1 key from + * this extension's own earlier releases. Kept in one table so the preview, the + * writer and the tests cannot disagree. */ export const LEGACY_MAPPINGS: readonly LegacyMapping[] = [ { @@ -141,6 +151,21 @@ export const LEGACY_MAPPINGS: readonly LegacyMapping[] = [ to: 'rstack.rslint.trace.server', targetScope: 'resource', }, + { + // The runtime pin became a shared setting when the fmt LSP server joined + // the test worker on the User Node runtime, so the standalone extension's + // key maps to `rstack.nodeExecutable`, not to a `rstack.rstest.*` one. + from: 'rstest.nodeExecutable', + to: 'rstack.nodeExecutable', + targetScope: 'resource', + }, + { + // Same target, different source: earlier releases of *this* extension + // shipped the pin as `rstack.rstest.nodeExecutable`. + from: 'rstack.rstest.nodeExecutable', + to: 'rstack.nodeExecutable', + targetScope: 'resource', + }, ...RSTEST_KEYS.map(([key, targetScope]) => ({ from: `rstest.${key}`, to: `rstack.rstest.${key}`, @@ -310,7 +335,27 @@ export const planMigration = ( continue; } - scopeFor(reading).push({ + const writes = scopeFor(reading); + // Two legacy keys can map to one target (the retired runtime pins). Where + // both are set in one layer, the later mapping wins — and the earlier one + // is surfaced as a skip, so the preview never shows two writes both + // claiming the same key with no hint which value survives. + const claimed = writes.findIndex((write) => write.to === mapping.to); + if (claimed !== -1) { + const [previous] = writes.splice(claimed, 1); + if (previous) { + skips.push({ + scopeId: previous.scopeId, + layer: previous.layer, + folderLabel: previous.folderLabel, + from: previous.from, + to: previous.to, + value: previous.fromValue, + reason: 'superseded', + }); + } + } + writes.push({ scopeId: reading.scopeId, layer: reading.layer, folderLabel: reading.folderLabel, @@ -357,6 +402,8 @@ const SKIP_EXPLANATIONS: Readonly< 'target-already-set': (skip) => `${skip.to} is already set here`, 'not-folder-scoped': (skip) => `${skip.to} is a window-scoped setting and cannot be set per folder`, + superseded: (skip) => + `another legacy key here also maps to ${skip.to} and carries the newer value`, }; /** @@ -563,7 +610,7 @@ export const runSettingsMigration = async ( void vscode.window.showInformationMessage( plan.skips.length > 0 ? 'Rstack: nothing left to migrate. See the Rstack output channel for the settings that were left untouched.' - : 'Rstack: no legacy rslint.* / rstest.* settings were found.', + : 'Rstack: no settings under legacy names were found.', ); } return false; @@ -638,14 +685,12 @@ export const maybePromptForMigration = async ( if (plan.writeCount === 0) { return; } - output.info( - `Found ${plan.writeCount} legacy setting(s) from the standalone Rslint/Rstest extensions.`, - ); + output.info(`Found ${plan.writeCount} setting(s) under legacy names.`); const migrate = 'Migrate…'; const dismiss = "Don't ask again"; const choice = await vscode.window.showInformationMessage( - 'Rstack found settings from the standalone Rslint/Rstest extensions. Migrate them to the rstack.* namespace?', + 'Rstack found settings under legacy names (from the standalone Rslint/Rstest extensions, or an earlier Rstack release). Migrate them to their current names?', migrate, 'Not now', dismiss, diff --git a/packages/vscode/src/shared/nodeExecutableSetting.ts b/packages/vscode/src/shared/nodeExecutableSetting.ts new file mode 100644 index 0000000..bfb52d4 --- /dev/null +++ b/packages/vscode/src/shared/nodeExecutableSetting.ts @@ -0,0 +1,36 @@ +import vscode from 'vscode'; +import { NODE_EXECUTABLE_SETTING } from './nodeResolution'; + +/** + * The `${workspaceFolder}` substitution the manifest promises for the runtime + * pin and the test stack's exec-args. One definition: any change to the rule + * (a second placeholder, trimming) must hold for every setting that documents + * it. + */ +export const expandWorkspaceFolder = ( + value: string, + folder: vscode.WorkspaceFolder, +): string => value.replaceAll('${workspaceFolder}', folder.uri.fsPath); + +/** + * The shared User Node runtime pin (`rstack.nodeExecutable`), read by every + * stack that spawns a project-loading child process (the rstest worker, the + * `rs fmt --lsp` server). One setting rather than one per stack on purpose: + * the runtime selection logic is uniform across stacks, so the escape hatch + * must be too — a user pinning Node for one tool means it for the toolchain. + * + * Returns the configured executable for a folder, `${workspaceFolder}` + * expanded, or `undefined` when the setting is unset or blank (both mean "let + * the extension pick one"). + */ +export const getConfiguredNodeExecutable = ( + folder: vscode.WorkspaceFolder, +): string | undefined => { + const value = vscode.workspace + .getConfiguration(undefined, folder.uri) + .get(NODE_EXECUTABLE_SETTING); + if (typeof value !== 'string' || value === '') { + return undefined; + } + return expandWorkspaceFolder(value, folder); +}; diff --git a/packages/vscode/src/stacks/test/nodeResolution.ts b/packages/vscode/src/shared/nodeResolution.ts similarity index 79% rename from packages/vscode/src/stacks/test/nodeResolution.ts rename to packages/vscode/src/shared/nodeResolution.ts index a97271d..a4fa746 100644 --- a/packages/vscode/src/stacks/test/nodeResolution.ts +++ b/packages/vscode/src/shared/nodeResolution.ts @@ -4,10 +4,11 @@ import { checkVersion, NODE_RUNTIME_LABEL, NODE_RUNTIME_RANGE, -} from '../../shared/versionCheck'; +} from './versionCheck'; /** - * Choosing the Node.js a test worker runs on. + * Choosing the User Node runtime a stack's project-loading child process runs + * on (the rstest worker, the `rs fmt --lsp` server). * * 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 @@ -17,23 +18,32 @@ import { * 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 costs Node 20.19–22.17 — but Node 20 left + * than per-project or per-stack. It is set by the strictest thing any of these + * children do — load an `rstack.config.*` — and applying it uniformly, + * including to projects whose config an older engine could load, is 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 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 + * 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 one spelling of the shared runtime pin's key. The manifest, the two + * user-facing messages below and every `restartOnSettings` declaration refer + * to the same setting; this constant is what keeps them from drifting. + * (Defined here rather than in `nodeExecutableSetting.ts` so the messages can + * use it without this module gaining a `vscode` import.) + */ +export const NODE_EXECUTABLE_SETTING = 'rstack.nodeExecutable'; + /** `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; @@ -65,7 +75,7 @@ export type NodeProbe = /** It exists but did not answer: non-zero exit, or hung past the timeout. */ | { readonly kind: 'unusable' }; -export type WorkerNodeResolution = { +export type UserNodeResolution = { readonly executable: string; /** Which candidate won. Drives the fallback notice, nothing else. */ readonly source: 'path' | 'shell'; @@ -96,20 +106,36 @@ const formatNodePreflightFailure = (attempts: NodeAttempts): string => { ? 'interactive shell: not probed' : describeCandidate('interactive shell', attempts.shell), ].join(', '); - // 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_LABEL} 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.`; + return `No Node.js ${NODE_RUNTIME_LABEL} is available (${candidates}). Rstack needs it to load TypeScript config files. Install a newer Node.js, or set "${NODE_EXECUTABLE_SETTING}" to one`; }; -/** No candidate satisfied `NODE_RUNTIME_RANGE`. */ +/** + * No candidate satisfied `NODE_RUNTIME_RANGE`. + * + * `message` states the facts and the remedy but deliberately no consequence: + * one preflight serves stacks whose consequences differ ("tests will not run" + * vs "rs fmt will not format"), and the resolution is memoized host-wide, so + * the first caller's phrasing must not speak for the others. The 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). Reporting sites therefore go through + * `messageWith`, which owns the joining rule, rather than each hand-building + * the suffix. + */ export class NodePreflightError extends Error { constructor(readonly attempts: NodeAttempts) { super(formatNodePreflightFailure(attempts)); this.name = 'NodePreflightError'; } + + /** + * The user-facing message with the reporting stack's consequence attached, + * e.g. `messageWith('tests will not run')`. + */ + messageWith(consequence: string): string { + return `${this.message}; until then ${consequence}.`; + } } export const probeNodeVersion = (executable: string): Promise => @@ -212,7 +238,7 @@ const satisfiesFloor = (probe: NodeProbe): boolean => probe.kind === 'ok' && checkVersion(probe.version, NODE_RUNTIME_RANGE).kind === 'ok'; -export type ResolveWorkerNodeOptions = { +export type ResolveUserNodeOptions = { /** The user's shell, for the interactive probe. Omit to skip that step. */ readonly shell?: string; /** @@ -231,22 +257,23 @@ 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 - * 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. + * Picks the executable for a stack's project-loading child process. Callers + * holding an explicit `rstack.nodeExecutable` setting must not call this at + * all — an explicit choice is 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({ +export async function resolveUserNode({ shell, cwd, probe = probeNodeVersion, probeShellPath = (probedShell: string) => probeShellNodePath(probedShell, cwd), platform = process.platform, -}: ResolveWorkerNodeOptions = {}): Promise { +}: ResolveUserNodeOptions = {}): Promise { let onPath = await probe('node'); for ( let attempt = 0; @@ -304,18 +331,17 @@ export async function resolveWorkerNode({ * * `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/`. + * also what lets the tests drive it as a pure decision table. */ -let cached: Promise | undefined; +let cached: Promise | undefined; -export const resolveWorkerNodeOnce = ( - options: ResolveWorkerNodeOptions = {}, -): Promise => - (cached ??= resolveWorkerNode(options).then((resolution) => { +export const resolveUserNodeOnce = ( + options: ResolveUserNodeOptions = {}, +): Promise => + (cached ??= resolveUserNode(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}${ + `Node.js on the extension host PATH does not satisfy ${NODE_RUNTIME_RANGE}; using ${resolution.executable}${ resolution.version ? ` (${resolution.version})` : '' } from your shell instead`, ); @@ -329,7 +355,7 @@ export const resolveWorkerNodeOnce = ( * 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 + * Memoized by resolved executable path because the busiest 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 @@ -353,13 +379,13 @@ 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_LABEL}, 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 Node.js set in "${NODE_EXECUTABLE_SETTING}" (${executable}, version ${version ?? 'unknown'}) does not satisfy ${NODE_RUNTIME_LABEL}, which Rstack needs to load TypeScript config files. Rstack is using 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 + * Unlike `resolveUserNodeOnce`, 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 @@ -384,7 +410,7 @@ export const configuredNodeBelowFloor = ( }; /** Clears both memos this module holds — a restart exists to clear stale resolution. */ -export const resetWorkerNodeCaches = (): void => { +export const resetUserNodeCaches = (): void => { cached = undefined; configuredVerdicts.clear(); }; diff --git a/packages/vscode/src/shared/versionCheck.ts b/packages/vscode/src/shared/versionCheck.ts index 56ad19b..112d021 100644 --- a/packages/vscode/src/shared/versionCheck.ts +++ b/packages/vscode/src/shared/versionCheck.ts @@ -11,13 +11,15 @@ import { readPackageJson } from './packageResolve'; * - `@rslint/core >= 0.7.2` — first version whose package exports * `./config-loader` and `./eslint-plugin`. * - `@rstest/core >= 0.6.0` — the existing `MIN_CORE_VERSION` upstream. - * - `rstack >= 0.3.5` — first release with the full supported config and - * formatter surface. + * - `rstack >= 0.5.2` — first release with `rs fmt --lsp`, the language server + * the fmt stack is a client of. There is no stdin fallback for older + * releases: the editor would then format through a different code path than + * the one it is tested against, so the floor is a version gate instead. */ export const SUPPORT_MATRIX = { '@rslint/core': '>=0.7.2', '@rstest/core': '>=0.6.0', - rstack: '>=0.3.5', + rstack: '>=0.5.2', } as const; export type SupportedPackage = keyof typeof SUPPORT_MATRIX; @@ -46,7 +48,8 @@ export const readPackageVersion = ( }; /** - * The floor for the Node.js a test worker runs on. Not part of + * The floor for the Node.js a project-loading child process runs on — the test + * worker and the `rs fmt --lsp` server alike. 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. @@ -56,14 +59,14 @@ export const readPackageVersion = ( * unflagged in 23.6.0 and backported to the LTS line in 22.18.0, so 23.0–23.5 * compare above 22.18.0 yet predate it — hence a disjunction, not a plain * floor. Verified: 22.17.1 reports `process.features.typescript` false, - * 22.18.0 reports `strip`. See `stacks/test/nodeResolution.ts` for how - * candidates are probed. + * 22.18.0 reports `strip`. See `shared/nodeResolution.ts` for how candidates + * are probed. */ export const NODE_RUNTIME_RANGE = '^22.18.0 || >=23.6.0'; /** * `NODE_RUNTIME_RANGE` for human eyes, interpolated into the user-facing - * messages in `stacks/test/nodeResolution.ts`. The raw range reads as npm + * messages in `shared/nodeResolution.ts`. The raw range reads as npm * jargon in a status-bar sentence — a user on 23.2 would have to parse a `^` * to learn why they were refused. Logs keep the raw range, the precise * register there. Keep the two in lockstep. @@ -81,7 +84,7 @@ export const NODE_RUNTIME_LABEL = '22.18+ (excluding 23.0–23.5)'; * - `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 + * - `satisfiesFloor` in `shared/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. * diff --git a/packages/vscode/src/stacks/fmt/binEntry.ts b/packages/vscode/src/stacks/fmt/binEntry.ts new file mode 100644 index 0000000..6d1949b --- /dev/null +++ b/packages/vscode/src/stacks/fmt/binEntry.ts @@ -0,0 +1,28 @@ +/** + * Where the `rs` CLI entry sits inside a resolved `rstack` package, taken from + * its package.json `bin` field — npm allows both the string and the object + * form, and rstack has shipped both. + * + * Its own module rather than a function in `index.ts` for the same reason + * `shared/nodeResolution.ts` keeps its inputs explicit: this is a pure decision + * over one JSON value, and a unit test for it must not have to stand up a VS + * Code API stub (importing the stack module evaluates `vscode` and + * vscode-languageclient). + */ +export const pickBinEntry = (bin: unknown): string => { + if (typeof bin === 'string') { + return bin; + } + if ( + bin !== null && + typeof bin === 'object' && + 'rs' in bin && + typeof bin.rs === 'string' + ) { + return bin.rs; + } + // What every published rstack has used; a package.json without a usable + // `bin` is broken either way, and spawning this path produces the clearer + // failure than resolving to nothing. + return 'bin/rs.js'; +}; diff --git a/packages/vscode/src/stacks/fmt/index.ts b/packages/vscode/src/stacks/fmt/index.ts index ba3f0b5..7bf455a 100644 --- a/packages/vscode/src/stacks/fmt/index.ts +++ b/packages/vscode/src/stacks/fmt/index.ts @@ -1,27 +1,39 @@ import path from 'node:path'; import vscode from 'vscode'; +import { + CloseAction, + ErrorAction, + LanguageClient, + State, + type ErrorHandler, + type LanguageClientOptions, + type ServerOptions, +} from 'vscode-languageclient/node'; import { RSTACK_CONFIG_GLOB } from '../../detection'; +import { getConfiguredNodeExecutable } from '../../shared/nodeExecutableSetting'; +import { + configuredNodeBelowFloor, + NODE_EXECUTABLE_SETTING, + NodePreflightError, + resolveUserNodeOnce, +} from '../../shared/nodeResolution'; import { findPackageJsonUncached, readPackageJson, } from '../../shared/packageResolve'; -import { - checkPackageVersion, - reportVersionCheck, -} from '../../shared/versionCheck'; +import { reportVersionCheck } from '../../shared/versionCheck'; import type { DetectionSnapshot, StackContext, StackController, } from '../../types'; -import { - isRsFmtLaunchError, - minimalEdit, - pickConfigDir, - runRsFmt, - stderrTail, -} from './run'; -import { FmtStandby, type StandbyKey } from './standby'; +// `LanguageServerProcessOwner` lives under `stacks/lint` because that is where +// it was first needed, but nothing in it is lint-specific: it owns the native +// children of one language client, including the ones vscode-languageclient's +// automatic restart creates. Importing it is not a refactor across the stacks — +// no lint behaviour is shared, and the file has no lint imports. +import { LanguageServerProcessOwner } from '../lint/LanguageServerProcessOwner'; +import { pickBinEntry } from './binEntry'; // prettier 3.9.6 getSupportInfo() vscodeLanguageIds snapshot (rs fmt's pinned // prettier). Revisit when the pinned prettier changes. @@ -52,244 +64,524 @@ const LANGUAGE_IDS = [ 'yaml', ] as const; -const SELECTOR: vscode.DocumentSelector = LANGUAGE_IDS.map((language) => ({ - language, - scheme: 'file', -})); +/** + * The document selector of one folder's server: the 24 languages `rs fmt` can + * parse, each confined to that folder. The per-folder pattern is what keeps two + * servers in a multi-root window from both claiming a document — the same rule + * (and the same `RelativePattern`) the lint stack applies in + * `createWorkspaceDocumentSelector`. + */ +const createFolderDocumentSelector = ( + folder: vscode.WorkspaceFolder, +): vscode.DocumentFilter[] => { + const pattern = new vscode.RelativePattern(folder, '**/*'); + return LANGUAGE_IDS.map((language) => ({ + scheme: 'file', + language, + pattern, + })); +}; /** - * How long the active editor must hold still before it gets a standby. Arming - * spawns a process, so scrolling through a dozen tabs must not spawn a dozen - * children; the first arm at registration and the re-arm right after a consume - * skip the wait because both target an editor that is already settled. + * How long a folder's config events settle before its server restarts. The + * same window detection uses to coalesce a redetect burst. */ -const ARM_DEBOUNCE_MS = 2_000; +const CONFIG_RESTART_DEBOUNCE_MS = 300; -/** Everything a format request needs once the folder has been resolved. */ -interface FmtTarget { - readonly cwd: string; - readonly rsBinJs: string; -} +/** True when `filePath` is inside `dir` (or is `dir` itself). */ +const contains = (dir: string, filePath: string): boolean => { + const relative = path.relative(path.resolve(dir), path.resolve(filePath)); + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); +}; -const standbyKey = (uri: vscode.Uri, target: FmtTarget): StandbyKey => ({ - cwd: target.cwd, - filePath: uri.fsPath, - rsBinJs: target.rsBinJs, -}); +/** + * A folder runtime's state, as the E2E exports report it. + * + * `disabled`, `version-mismatch` and `crashed` mirror the status the runtime + * pushed to the shell when it stopped short; `stopped` is a runtime that was + * never started or was shut down deliberately. + */ +type FmtRuntimeState = + | 'stopped' + | 'starting' + | 'running' + | 'disabled' + | 'version-mismatch' + | 'crashed'; /** - * Resolution outcome, kept free of side effects so the arming path (which must - * stay silent) and the format path (which reports and logs) can share it. + * vscode-languageclient calls `stop()` without observing its promise when an + * initialize request fails, and its base `stop` rejects for non-Running states. + * The process owner handles those states, so only that inactive case is + * normalized. (The lint stack carries the same class as `ManagedLanguageClient`; + * it is restated here rather than imported so the two stacks share no runtime.) */ -type FmtResolution = - | { readonly kind: 'ok'; readonly target: FmtTarget } - | { readonly kind: 'no-folder' } - | { readonly kind: 'undetected'; readonly folder: vscode.WorkspaceFolder } - | { - readonly kind: 'missing-package'; - readonly folder: vscode.WorkspaceFolder; - readonly cwd: string; +class FmtLanguageClient extends LanguageClient { + public override async stop(timeout?: number): Promise { + const stateBeforeStop = this.state; + try { + await super.stop(timeout); + } catch (error) { + if (stateBeforeStop === State.Running) { + throw error; + } } - | { readonly kind: 'version-mismatch'; readonly version: string | undefined }; + } +} /** - * Formatter backed by the project-resolved rstack CLI. The process cwd selects - * the nearest governing rstack config because `rs fmt` intentionally performs - * cwd-only config resolution. + * One `rs fmt --lsp` language server, spawned with cwd = the workspace folder + * root and told that folder is its workspace. * - * A request is served either by consuming the standby (hot) or by spawning a - * process for it (cold); the two paths differ only in where the process came - * from. + * The folder root — not the deepest directory holding an `rstack.config.*` — is + * the anchor because `rs fmt` loads exactly one config, from its cwd, with no + * upward walk and no merging. Anchoring deeper made the editor format a file + * differently from `rs fmt` run at the repo root, which is where the config the + * project actually documents lives. The server caches that config for its + * process lifetime, so a config edit is a restart of this runtime, never a + * message to a live server. + */ +class FmtFolderRuntime { + #state: FmtRuntimeState = 'stopped'; + #owner: LanguageServerProcessOwner | undefined; + #client: LanguageClient | undefined; + #defaultErrorHandler: ErrorHandler | undefined; + #stateWatcher: vscode.Disposable | undefined; + #closing = false; + #disposed = false; + /** + * Every lifecycle transition of one folder runs here, so a config event + * arriving mid-start cannot interleave a second start with the first. + */ + #queue: Promise = Promise.resolve(); + + constructor( + private readonly folder: vscode.WorkspaceFolder, + private readonly context: StackContext, + /** Called after a successful start so the shell's status says `running`. */ + private readonly onRunning: () => void, + ) {} + + get state(): FmtRuntimeState { + return this.#state; + } + + get folderPath(): string { + return this.folder.uri.fsPath; + } + + start(): Promise { + return this.enqueue(async () => { + if (this.#disposed) { + return; + } + await this.startImpl(); + }); + } + + /** A config change invalidates the server's cached config; only a new process clears it. */ + restart(reason: string): Promise { + return this.enqueue(async () => { + if (this.#disposed) { + return; + } + this.context.output.info( + `Restarting the rs fmt server for ${this.folder.name}: ${reason}`, + ); + await this.stopImpl(); + if (this.#disposed) { + return; + } + await this.startImpl(); + }); + } + + stop(): Promise { + this.#disposed = true; + return this.enqueue(async () => { + await this.stopImpl(); + }); + } + + private enqueue(operation: () => Promise): Promise { + const next = this.#queue.then(operation, operation); + this.#queue = next.catch((error: unknown) => { + this.context.output.error( + `rs fmt lifecycle step failed in ${this.folder.name}`, + error, + ); + }); + return this.#queue; + } + + /** + * Package resolution, version check, Node selection and client start, in that + * order. Every failure short of a genuine launch failure is a status: the + * stack owns no UI chrome, and a project without `rstack` installed is not a + * crash. + */ + private async startImpl(): Promise { + const context = this.context; + const folderRoot = this.folder.uri.fsPath; + this.#closing = false; + this.#state = 'starting'; + + const pkgJsonPath = findPackageJsonUncached('rstack', folderRoot); + if (!pkgJsonPath) { + const reason = `rstack is not installed in ${this.folder.name} (node_modules missing)`; + this.#state = 'disabled'; + context.status.report({ kind: 'disabled', reason }); + context.output.warn(`${reason}; searched from ${folderRoot}`); + return; + } + + // One read for the version and the bin entry; `readPackageJson` re-reads + // from disk by design, so a reinstall is picked up on the next start. + const pkg = readPackageJson(pkgJsonPath); + const version = typeof pkg?.version === 'string' ? pkg.version : undefined; + if (!reportVersionCheck(context.status, 'rstack', version)) { + this.#state = 'version-mismatch'; + return; + } + const rsBinJs = path.resolve( + path.dirname(pkgJsonPath), + pickBinEntry(pkg?.bin), + ); + + const nodeExecutable = await this.resolveNodeExecutable(); + if (nodeExecutable === undefined || this.#disposed) { + return; + } + + const owner = new LanguageServerProcessOwner( + nodeExecutable, + [rsBinJs, 'fmt', '--lsp'], + folderRoot, + ); + this.#owner = owner; + const serverOptions: ServerOptions = async () => owner.start(); + const client = new FmtLanguageClient( + 'rstack-fmt', + `rs fmt Language Server (${this.folder.name})`, + serverOptions, + this.createClientOptions(), + ); + // Created once per client, not per callback: the default handler carries + // the restart budget (N crashes in K minutes), and it can only be created + // from the client the options were built for. + this.#defaultErrorHandler = client.createDefaultErrorHandler(); + this.#client = client; + this.#stateWatcher = client.onDidChangeState((event) => { + if (this.#closing || this.#disposed || client !== this.#client) { + return; + } + if (event.newState === State.Stopped) { + // Whether a restart follows is the error handler's and the process + // owner's call; either way this folder is currently not formatting. + this.#state = 'crashed'; + context.status.crashed('the rs fmt language server stopped'); + } else if (event.newState === State.Running) { + // The one writer for `running`. It fires on the first start + // (synchronously, before `client.start()` resolves) and again when + // vscode-languageclient's error handler restarts a crashed server — + // the way back out of `crashed`, the same transition the lint stack's + // state watcher makes. + this.#state = 'running'; + this.onRunning(); + } + }); + + context.output.debug( + `Starting rs fmt --lsp in ${folderRoot}: ${nodeExecutable} ${rsBinJs}`, + ); + try { + // The client registers the formatting provider from the server's + // `documentFormattingProvider` capability — the stack registers none of + // its own. The state watcher above is what records the successful start: + // the client reaches `State.Running` before this await resolves. + await client.start(); + } catch (error) { + // Teardown first: `stopImpl` ends in `#state = 'stopped'`, so the + // `crashed` verdict has to be written after it, not raced against it. + await this.stopImpl(); + this.#state = 'crashed'; + context.status.crashed( + `the rs fmt language server for ${this.folder.name} failed to start: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + context.output.error('Failed to start the rs fmt language server', error); + return; + } + context.output.info(`rs fmt language server started for ${folderRoot}`); + } + + /** + * The User Node runtime this folder's server runs on, or `undefined` when no + * candidate cleared the floor (already reported as `version mismatch`). + * + * The server loads the project's `rstack.config.*` through + * `@rstackjs/load-config` with `loader: 'native'`, which is the exact path the + * uniform floor exists for — so fmt takes the same runtime decision as the + * rstest worker, out of the same shared module. + */ + private async resolveNodeExecutable(): Promise { + const context = this.context; + const configured = getConfiguredNodeExecutable(this.folder); + if (configured !== undefined) { + // An explicit pin is always honoured — it is the escape hatch — but it is + // still probed, advisory-only, off the start path. + void configuredNodeBelowFloor(configured).then((message) => { + if (message !== undefined && !this.#disposed) { + context.status.versionMismatch(message); + } + }); + return configured; + } + try { + const resolution = await resolveUserNodeOnce({ + shell: vscode.env.shell || undefined, + cwd: this.folderPath, + notify: (message) => { + context.output.info(message); + }, + }); + return resolution.executable; + } catch (error) { + if (error instanceof NodePreflightError) { + this.#state = 'version-mismatch'; + context.status.versionMismatch( + error.messageWith('rs fmt will not format'), + ); + return undefined; + } + throw error; + } + } + + /** + * The error handler delegates through `#defaultErrorHandler` because the + * default handler can only be created from the client that will use it, + * which does not exist yet when the options are built. + */ + private createClientOptions(): LanguageClientOptions { + const documentSelector = createFolderDocumentSelector(this.folder); + const errorHandler: ErrorHandler = { + error: async (error, message, count) => + Promise.resolve( + this.#defaultErrorHandler?.error(error, message, count) ?? { + action: ErrorAction.Shutdown, + }, + ), + closed: async () => { + if (this.#closing || this.#disposed) { + return { action: CloseAction.DoNotRestart, handled: true }; + } + return Promise.resolve( + this.#defaultErrorHandler?.closed() ?? { + action: CloseAction.DoNotRestart, + }, + ); + }, + }; + return { + workspaceFolder: this.folder, + // languageclient v9 types this client-only selector as the LSP shape, + // whose pattern is string-only. Its converter forwards the pattern to + // VS Code's DocumentFilter, which supports RelativePattern and preserves + // an unambiguous workspace base even when the path contains glob syntax. + documentSelector: + // rslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + documentSelector as unknown as LanguageClientOptions['documentSelector'], + outputChannel: this.context.output, + errorHandler, + }; + } + + private async stopImpl(): Promise { + const client = this.#client; + const owner = this.#owner; + this.#client = undefined; + this.#owner = undefined; + this.#closing = true; + this.#stateWatcher?.dispose(); + this.#stateWatcher = undefined; + // Block vscode-languageclient's automatic restart callback before the + // graceful shutdown begins; the owner force-terminates whatever survives. + owner?.beginClose(); + if (client) { + try { + await client.dispose(); + } catch (error) { + this.context.output.debug( + `Disposing the rs fmt language client for ${this.folder.name} failed: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + } + try { + await owner?.close(); + } catch (error) { + this.context.output.error( + `Failed to stop the rs fmt language server for ${this.folder.name}`, + error, + ); + } + this.#state = 'stopped'; + } +} + +/** + * The fmt stack: one `rs fmt --lsp` language server per detected workspace + * folder, each rooted at its folder. + * + * The controller owns no formatting provider of its own — every server declares + * `documentFormattingProvider`, and its client registers the provider for that + * folder's documents. What the controller owns is the folder set: detection + * decides which folders have a server, and an `rstack.config.*` event restarts + * the one whose folder holds the file. */ class FmtController implements StackController { readonly id = 'fmt' as const; + /** + * The Node runtime is chosen once per server start, so a changed pin can only + * reach a live server through a rebuild of this controller. + */ + readonly restartOnSettings = [NODE_EXECUTABLE_SETTING]; #context: StackContext | undefined; + /** The newest detection result, which `running` reports the reason from. */ #snapshot: DetectionSnapshot | undefined; + readonly #runtimes = new Map(); + /** Per-folder config-event debounce (see `register`'s `onConfigEvent`). */ + readonly #restartTimers = new Map(); readonly #subscriptions: vscode.Disposable[] = []; - // One-shot log lines, keyed by `:`. Cleared when detection - // changes so a fixed setup gets a fresh explanation. - readonly #loggedOnce = new Set(); - readonly #abortController = new AbortController(); #disposed = false; - #standby: FmtStandby | undefined; - #armTimer: NodeJS.Timeout | undefined; - /** E2E-only: how the most recent format request was served. */ - #lastServe: 'hot' | 'cold' | undefined; - async register(context: StackContext): Promise> { + register(context: StackContext): Promise> { this.#context = context; this.#snapshot = context.detection; - this.#standby = new FmtStandby({ - log: (message) => context.output.debug(message), - }); - const provider: vscode.DocumentFormattingEditProvider = { - provideDocumentFormattingEdits: (document, _options, token) => - this.provideDocumentFormattingEdits(document, token), - }; - // `rs fmt` loads the project config while it drains stdin, so a parked - // process already carries the old config and every config event has to - // invalidate it. Detection cannot stand in for this watcher: its signature - // records only which config files exist, so it misses a content edit, and - // it misses the delete/create pair an atomic save produces for one - // unchanged path just the same. + // Detection cannot stand in for this watcher: its signature records only + // which config files exist, so it misses a content edit, and it misses the + // delete/create pair an atomic save produces for one unchanged path. const configWatcher = vscode.workspace.createFileSystemWatcher(RSTACK_CONFIG_GLOB); - const onConfigEvent = (uri: vscode.Uri): void => - this.invalidateStandby(`${uri.fsPath} changed`); + const onConfigEvent = (uri: vscode.Uri): void => { + const folder = vscode.workspace.getWorkspaceFolder(uri); + // Only the folder root's config can be the one the server read — it + // loads exactly one config, from its cwd, with no upward walk — so an + // `rstack.config.*` event deeper in the tree must not cost a restart. + if (!folder || path.dirname(uri.fsPath) !== folder.uri.fsPath) { + return; + } + const folderPath = folder.uri.fsPath; + if (!this.#runtimes.has(folderPath)) { + return; + } + // An atomic save arrives as a delete/create pair and an editor can save + // in bursts; a short debounce folds them into one restart (same window + // as detection's redetect debounce). + clearTimeout(this.#restartTimers.get(folderPath)); + this.#restartTimers.set( + folderPath, + setTimeout(() => { + this.#restartTimers.delete(folderPath); + void this.#runtimes.get(folderPath)?.restart(`${uri.fsPath} changed`); + }, CONFIG_RESTART_DEBOUNCE_MS), + ); + }; this.#subscriptions.push( context.onDidChangeDetection((snapshot) => { this.#snapshot = snapshot; - this.#loggedOnce.clear(); - // The cwd, the resolved bin and the config set can all have moved. - this.invalidateStandby('detection changed'); - // The reconcile leaves a still-detected controller alone, so the - // running reason must follow the new snapshot here rather than wait - // for the next successful format. - this.reportRunning(context, snapshot); + this.reconcile(); + // The shell's reconcile leaves a still-detected controller alone, so + // the running reason must follow the new snapshot here. + this.reportRunning(); }), - vscode.languages.registerDocumentFormattingEditProvider( - SELECTOR, - provider, - ), configWatcher, configWatcher.onDidCreate(onConfigEvent), configWatcher.onDidChange(onConfigEvent), configWatcher.onDidDelete(onConfigEvent), - vscode.window.onDidChangeActiveTextEditor(() => this.scheduleArm()), ); - this.reportRunning(context, context.detection); - // The editor the user is already looking at needs no settling wait, but - // arming spawns a process and `register()` must return fast. - this.scheduleArm(0); - return { + this.reportRunning(); + // Starting a server spawns a process; `register()` must return fast, so the + // folder set is reconciled without awaiting any start. + this.reconcile(); + return Promise.resolve({ languages: LANGUAGE_IDS, - provider, - armedFilePath: (): string | undefined => this.#standby?.armedFilePath, - lastServe: (): 'hot' | 'cold' | undefined => this.#lastServe, - }; - } - - /** Kills the standby, then re-arms the active editor through the debounce. */ - private invalidateStandby(reason: string): void { - this.#standby?.kill(reason); - this.scheduleArm(); - } - - private scheduleArm(delayMs = ARM_DEBOUNCE_MS): void { - clearTimeout(this.#armTimer); - this.#armTimer = setTimeout(() => { - this.#armTimer = undefined; - this.armActiveEditor(); - }, delayMs); + /** + * E2E only: whether a *running* server covers this path — the folder's + * server has started and the path lies inside that folder. It answers by + * folder, not by language: what it exists to distinguish is a folder with + * a live server from one without. + */ + formats: (fsPath: string): boolean => + [...this.#runtimes.values()].some( + (runtime) => + runtime.state === 'running' && contains(runtime.folderPath, fsPath), + ), + /** E2E only: every folder runtime's lifecycle state, keyed by folder path. */ + folderStates: (): Record => + Object.fromEntries( + [...this.#runtimes].map(([folderPath, runtime]) => [ + folderPath, + runtime.state, + ]), + ), + }); } /** - * Arms a standby for whatever the active editor is *now* — the invariant is - * "the standby tracks the active editor", so the editor is never captured - * when the arm was scheduled. - * - * Nothing formattable being active leaves the standby alone, whether the - * active editor holds no text document at all (a settings tab, an image - * preview) or holds one this stack does not format. Neither is worth a kill: - * an idle standby expires on its own, and both are a keystroke away - * from the file that owns it. A formattable document that cannot be armed is - * the other case — the editor really did move on, so the standby goes too. + * Brings the folder set in line with detection: a newly detected folder gets + * a server, an undetected one loses it, and a folder that is in both sets is + * left alone — restarting a healthy server on an unrelated folder's detection + * change would drop its cached config for nothing. */ - private armActiveEditor(): void { + private reconcile(): void { const context = this.#context; - const standby = this.#standby; - if (!context || !standby) { - return; - } - const document = vscode.window.activeTextEditor?.document; - // The provider's own selector is the eligibility rule, so the two cannot - // drift apart. - if (!document || vscode.languages.match(SELECTOR, document) === 0) { - return; - } - // Every invalidation kills the standby before asking for a re-arm, so an - // armed standby on this file is still valid — and resolving is synchronous - // filesystem work that runs on the UI thread. - if (standby.armedFilePath === document.uri.fsPath) { - return; - } - const resolution = this.resolve(document.uri); - if (resolution.kind !== 'ok') { - // Arming is silent: an unresolvable editor only means the next format - // there is cold, which is exactly what happened before the standby. - context.output.debug( - `Standby not armed for ${document.uri.fsPath}: ${resolution.kind}`, - ); - // The editor still moved to another file, so the previous file's standby - // no longer tracks it. `arm` would have killed it; there is nothing to - // arm here, so this path has to. - standby.kill('the active editor moved to a file that cannot be armed'); - return; - } - standby.arm(standbyKey(document.uri, resolution.target)); - } - - /** - * Where a document's `rs fmt` would run. Pure: it reports no status and logs - * nothing, because the arming path must not move the status bar. - */ - private resolve(uri: vscode.Uri): FmtResolution { const snapshot = this.#snapshot; - const folder = vscode.workspace.getWorkspaceFolder(uri); - if (!snapshot || !folder) { - return { kind: 'no-folder' }; - } - const fmtDetection = snapshot.forFolder(folder)?.stacks.fmt; - if (!fmtDetection?.detected) { - return { kind: 'undetected', folder }; + if (!context || !snapshot || this.#disposed) { + return; } - - const cwd = pickConfigDir( - uri.fsPath, - fmtDetection.rstackConfigFiles.map((configUri) => configUri.fsPath), - folder.uri.fsPath, + const detected = new Map( + snapshot + .foldersFor('fmt') + .map((entry) => [entry.folder.uri.fsPath, entry.folder] as const), ); - const pkgJsonPath = findPackageJsonUncached('rstack', cwd); - if (!pkgJsonPath) { - return { kind: 'missing-package', folder, cwd }; - } - - // One read for both the version and the bin entry: `resolve` now runs on - // the arming path too, and `readPackageJson` re-reads from disk by design. - const pkg = readPackageJson(pkgJsonPath); - const version = typeof pkg?.version === 'string' ? pkg.version : undefined; - if (checkPackageVersion('rstack', version).kind === 'mismatch') { - // Reporting stays with the caller: the arming path must not move the - // status bar, and the format path goes through `reportVersionCheck` so - // the shared contract has one implementation. - return { kind: 'version-mismatch', version }; + for (const [folderPath, runtime] of [...this.#runtimes]) { + if (!detected.has(folderPath)) { + this.#runtimes.delete(folderPath); + void runtime.stop(); + } } - - const bin = pkg?.bin; - let binEntry = 'bin/rs.js'; - if (typeof bin === 'string') { - binEntry = bin; - } else if (bin && typeof bin === 'object') { - const rs = (bin as Record).rs; - if (typeof rs === 'string') { - binEntry = rs; + for (const [folderPath, folder] of detected) { + if (this.#runtimes.has(folderPath)) { + continue; } + // The callback re-reads `#snapshot`, so a server that finishes starting + // after a detection change reports from the freshest snapshot — and the + // closure captures nothing beyond `this`. + const runtime = new FmtFolderRuntime(folder, context, () => + this.reportRunning(), + ); + this.#runtimes.set(folderPath, runtime); + void runtime.start(); } - return { - kind: 'ok', - target: { - cwd, - rsBinJs: path.resolve(path.dirname(pkgJsonPath), binEntry), - }, - }; } /** `running` always carries the reason the stack is on: where it was detected. */ - private reportRunning( - context: StackContext, - snapshot: DetectionSnapshot, - ): void { + private reportRunning(): void { + const context = this.#context; + const snapshot = this.#snapshot; + if (!context || !snapshot || this.#disposed) { + return; + } const names = snapshot.foldersFor('fmt').map((entry) => entry.folder.name); if (names.length === 0) { // Nothing detected means the shell is about to retire this controller; @@ -303,183 +595,26 @@ class FmtController implements StackController { ); } - private async provideDocumentFormattingEdits( - document: vscode.TextDocument, - token: vscode.CancellationToken, - ): Promise { - const context = this.#context; - const snapshot = this.#snapshot; - if ( - this.#disposed || - !context || - !snapshot || - document.uri.scheme !== 'file' - ) { - return []; - } - - const resolution = this.resolve(document.uri); - if (resolution.kind === 'no-folder') { - return []; - } - if (resolution.kind === 'undetected') { - // The formatter is offered per language, so a request can land in a - // folder without an rstack setup. That is routine, not a fault — one - // info line per folder says why nothing happened. - const folder = resolution.folder; - if (!this.#loggedOnce.has(`undetected:${folder.uri.toString()}`)) { - this.#loggedOnce.add(`undetected:${folder.uri.toString()}`); - context.output.info( - `A format request in ${folder.name} was skipped: fmt is not detected there (no rstack.config.* and no rstack CLI at the folder root)`, - ); - } - return []; - } - - // Per-request logging follows prettier-vscode's shape (same in-host, - // work-per-request architecture): a fixed entry and outcome line at info, - // resolution detail at debug — the channel is a LogOutputChannel, so the - // user raises the level from its context menu when needed. - const startedAt = Date.now(); - context.output.info(`Formatting ${document.uri.fsPath}`); - if (resolution.kind === 'missing-package') { - const reason = `rstack is not installed in ${resolution.folder.name} (node_modules missing)`; - context.status.report({ kind: 'disabled', reason }); - if (!this.#loggedOnce.has(`missing:${resolution.cwd}`)) { - this.#loggedOnce.add(`missing:${resolution.cwd}`); - context.output.warn(`${reason}; searched from ${resolution.cwd}`); - } - return []; - } - if (resolution.kind === 'version-mismatch') { - reportVersionCheck(context.status, 'rstack', resolution.version); - return []; - } - - const { cwd, rsBinJs } = resolution.target; - context.output.debug(`cwd: ${cwd}; bin: ${rsBinJs}`); - - const text = document.getText(); - const version = document.version; - const requestController = new AbortController(); - const abortRequest = (): void => requestController.abort(); - const cancellation = token.onCancellationRequested(abortRequest); - this.#abortController.signal.addEventListener('abort', abortRequest, { - once: true, - }); - if (token.isCancellationRequested || this.#abortController.signal.aborted) { - requestController.abort(); - } - - const key = standbyKey(document.uri, resolution.target); - // An already-cancelled request must not burn the standby. - const hot = requestController.signal.aborted - ? undefined - : this.#standby?.consume(key, { - text, - signal: requestController.signal, - }); - const serve = hot ? 'hot' : 'cold'; - this.#lastServe = serve; - - let result; - try { - result = await (hot ?? - runRsFmt({ - text, - filePath: document.uri.fsPath, - cwd, - rsBinJs, - signal: requestController.signal, - })); - } finally { - cancellation.dispose(); - this.#abortController.signal.removeEventListener('abort', abortRequest); - // A standby serves exactly one request, and a real format request is - // itself proof the active file is worth one — re-arm after cold serves - // too, so an expired or crashed standby comes back on the next use - // instead of leaving the file cold until the editor changes. Scheduled - // rather than immediate: spawning here would delay the edits the caller - // is waiting for. - this.scheduleArm(0); - } - - if ( - token.isCancellationRequested || - document.version !== version || - this.#disposed - ) { - context.output.debug( - `Formatting result for ${document.uri.fsPath} discarded (document changed or request cancelled)`, - ); - return []; - } - - const elapsed = Date.now() - startedAt; - if (result.kind === 'ok' || result.kind === 'skipped') { - // Same tailing as the error path: a chatty warning stream must not land - // in the log unbounded. - const stderr = stderrTail(result.stderr); - if (stderr !== '') { - context.output.debug(`rs fmt stderr: ${stderr}`); - } - } - switch (result.kind) { - case 'ok': { - // The freshest snapshot, not the request's capture: detection may - // have changed while the format was in flight. - this.reportRunning(context, this.#snapshot ?? snapshot); - const edit = minimalEdit(text, result.formatted); - // The hot/cold marker is the only way to tell from a log whether the - // measured time included a process start-up. - context.output.info( - `Formatting completed in ${elapsed}ms (${serve}${ - edit ? '' : ', already formatted' - })`, - ); - if (!edit) { - return []; - } - return [ - vscode.TextEdit.replace( - new vscode.Range( - document.positionAt(edit.start), - document.positionAt(edit.end), - ), - edit.newText, - ), - ]; - } - case 'skipped': - context.output.info( - `Skipped ${document.uri.fsPath}: rs fmt returned no output (the file is ignored or has no parser)`, - ); - return []; - case 'cancelled': - context.output.debug(`Formatting cancelled for ${document.uri.fsPath}`); - return []; - case 'error': - context.output.error(`rs fmt failed in ${cwd}: ${result.message}`); - if (isRsFmtLaunchError(result)) { - context.status.crashed(result.message); - } - return []; - } - } - - dispose(): void { + /** + * Covers `rstack.fmt.restart` (the shell rebuilds the controller), a folder + * losing detection and a workspace losing its trust: none of them may leave a + * server process behind, so the teardown is awaited. + */ + async dispose(): Promise { this.#disposed = true; - this.#abortController.abort(); - // Covers `rstack.fmt.restart` (the shell rebuilds the controller) and a - // workspace losing its trust: neither may leave a process behind. - clearTimeout(this.#armTimer); - this.#armTimer = undefined; - this.#standby?.dispose(); - this.#standby = undefined; + for (const timer of this.#restartTimers.values()) { + clearTimeout(timer); + } + this.#restartTimers.clear(); for (const subscription of this.#subscriptions.splice(0)) { subscription.dispose(); } - this.#loggedOnce.clear(); + const runtimes = [...this.#runtimes.values()]; + this.#runtimes.clear(); + await Promise.allSettled(runtimes.map(async (runtime) => runtime.stop())); + // The User Node preflight memo is deliberately not reset here: it is + // host-scoped state shared with the rstest stack, and the shell's restart + // pass owns the reset (see `runRestart`). this.#context = undefined; this.#snapshot = undefined; } diff --git a/packages/vscode/src/stacks/fmt/run.ts b/packages/vscode/src/stacks/fmt/run.ts deleted file mode 100644 index d9effff..0000000 --- a/packages/vscode/src/stacks/fmt/run.ts +++ /dev/null @@ -1,336 +0,0 @@ -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; -import path from 'node:path'; - -const TIMEOUT_MS = 60_000; -const LAUNCH_ERROR_PREFIX = 'Unable to run rs fmt at '; - -/** Deepest config directory that contains the document, or the workspace root. */ -export const pickConfigDir = ( - documentPath: string, - configFilePaths: readonly string[], - fallbackDir: string, -): string => { - const documentDir = path.dirname(path.resolve(documentPath)); - let selected = path.resolve(fallbackDir); - let selectedDepth = -1; - - for (const configFilePath of configFilePaths) { - const configDir = path.dirname(path.resolve(configFilePath)); - const relative = path.relative(configDir, documentDir); - const containsDocument = - relative === '' || - (relative !== '..' && - !relative.startsWith(`..${path.sep}`) && - !path.isAbsolute(relative)); - if (!containsDocument) { - continue; - } - - const depth = configDir.split(path.sep).filter(Boolean).length; - if (depth > selectedDepth) { - selected = configDir; - selectedDepth = depth; - } - } - - return selected; -}; - -export interface RsFmtRun { - readonly text: string; - readonly filePath: string; - readonly cwd: string; - readonly rsBinJs: string; - readonly signal: AbortSignal; -} - -export type RsFmtResult = - // `stderr` carries the CLI's own voice (warnings on otherwise-successful - // runs) so the caller can surface it, the way an LSP-based tool would push - // `window/logMessage`. - | { readonly kind: 'ok'; readonly formatted: string; readonly stderr: string } - | { readonly kind: 'skipped'; readonly stderr: string } - | { readonly kind: 'cancelled' } - | { readonly kind: 'error'; readonly message: string }; - -export const errorMessage = (error: unknown): string => - error instanceof Error ? error.message : String(error); - -/** Bounds any CLI stderr headed for a log line to its meaningful tail. */ -export const stderrTail = (stderr: string): string => - stderr.trim().split(/\r?\n/).slice(-10).join('\n'); - -const launchError = (rsBinJs: string, detail: string): RsFmtResult => ({ - kind: 'error', - message: `${LAUNCH_ERROR_PREFIX}${rsBinJs}: ${detail}`, -}); - -/** True only when the CLI itself could not be launched or loaded. */ -export const isRsFmtLaunchError = (result: RsFmtResult): boolean => - result.kind === 'error' && result.message.startsWith(LAUNCH_ERROR_PREFIX); - -/** How an `rs fmt` child ended. */ -type RsFmtExit = - | { readonly kind: 'error'; readonly error: unknown } - | { readonly kind: 'close'; readonly code: number | null }; - -/** - * Only the tail of stderr is ever reported, but a parked standby can hold a - * child for minutes — a chatty one must not grow the buffer without bound. - */ -const MAX_STDERR_CHARS = 64 * 1024; - -/** Built once: `process.env` does not change over an extension host's life. */ -const CHILD_ENV = { ...process.env, ELECTRON_RUN_AS_NODE: '1' }; - -/** - * A spawned `rs fmt --stdin-filepath` child whose streams drain from the moment - * it starts. Both fmt paths share it: a cold format writes stdin immediately, - * while a standby parks the child on stdin — `rs fmt` loads the project config - * concurrently with draining stdin, so a parked child has already paid that - * cost — and writes only when a request consumes it. - */ -export interface RsFmtProcess { - readonly rsBinJs: string; - /** - * The terminal event. It is a value rather than a callback slot because the - * child has two independent observers over its life: the standby watches it - * while parked, and `serveRsFmt` watches it for the duration of a request. - */ - readonly exited: Promise; - /** Sends the document and closes stdin. */ - write(text: string): void; - kill(): void; - /** Chunks collected so far; joined once the child closes. */ - readonly stdout: readonly string[]; - readonly stderr: readonly string[]; -} - -export interface RsFmtSpawn { - readonly filePath: string; - readonly cwd: string; - readonly rsBinJs: string; -} - -export type RsFmtSpawnResult = - | { readonly kind: 'spawned'; readonly process: RsFmtProcess } - | { readonly kind: 'error'; readonly error: unknown }; - -/** Starts an `rs fmt` child and begins draining its streams. */ -export const spawnRsFmt = (options: RsFmtSpawn): RsFmtSpawnResult => { - let child: ChildProcessWithoutNullStreams; - try { - child = spawn( - process.execPath, - [ - options.rsBinJs, - 'fmt', - '--stdin-filepath', - options.filePath, - '--ignore-unknown', - ], - { - cwd: options.cwd, - env: CHILD_ENV, - stdio: 'pipe', - }, - ); - } catch (error) { - return { kind: 'error', error }; - } - - const stdout: string[] = []; - const stderr: string[] = []; - let stderrChars = 0; - child.stdout.setEncoding('utf8'); - child.stderr.setEncoding('utf8'); - child.stdout.on('data', (chunk: string) => stdout.push(chunk)); - child.stderr.on('data', (chunk: string) => { - stderr.push(chunk); - stderrChars += chunk.length; - while (stderrChars > MAX_STDERR_CHARS && stderr.length > 1) { - const dropped = stderr.shift(); - stderrChars -= dropped?.length ?? 0; - } - }); - // A child may reject the request before consuming stdin. Its exit status is - // authoritative; EPIPE and write-after-end must not become unhandled errors. - child.stdin.on('error', () => {}); - - const exited = new Promise((resolve) => { - child.on('error', (error) => resolve({ kind: 'error', error })); - child.on('close', (code) => resolve({ kind: 'close', code })); - }); - - return { - kind: 'spawned', - process: { - rsBinJs: options.rsBinJs, - stdout, - stderr, - exited, - write(text) { - try { - child.stdin.end(text); - } catch { - // Wait for the child's close/error event. - } - }, - kill() { - child.kill(); - }, - }, - }; -}; - -const interpretExit = ( - rsFmt: RsFmtProcess, - text: string, - code: number | null, -): RsFmtResult => { - const formatted = rsFmt.stdout.join(''); - const stderrText = rsFmt.stderr.join(''); - if (code === 0) { - if (formatted.length > 0 || text.trim() === '') { - return { kind: 'ok', formatted, stderr: stderrText }; - } - return { kind: 'skipped', stderr: stderrText }; - } - const tail = stderrTail(stderrText); - const missingEntry = stderrText - .split(/\r?\n/) - .some( - (line) => - line.includes('Cannot find module') && line.includes(rsFmt.rsBinJs), - ); - if (missingEntry) { - return launchError(rsFmt.rsBinJs, tail); - } - return { - kind: 'error', - message: - tail || - `rs fmt exited with code ${code === null ? 'unknown' : String(code)}`, - }; -}; - -export interface RsFmtServe { - readonly process: RsFmtProcess; - readonly text: string; - readonly signal: AbortSignal; - /** - * Guard timeout. It measures the request, so for a standby it starts at - * consume time rather than at spawn time. - */ - readonly timeoutMs?: number; -} - -/** Writes the document to a child's stdin and resolves with its verdict. */ -export const serveRsFmt = (serve: RsFmtServe): Promise => - new Promise((resolve) => { - const rsFmt = serve.process; - const signal = serve.signal; - const timeoutMs = serve.timeoutMs ?? TIMEOUT_MS; - let settled = false; - // Boxed rather than a plain `let`: the timeout is assigned below the - // closure that clears it, and `prefer-const` mis-reads that order as - // "never reassigned". - const guard: { timeout?: NodeJS.Timeout } = {}; - - const onAbort = (): void => { - rsFmt.kill(); - settle({ kind: 'cancelled' }); - }; - const settle = (result: RsFmtResult): void => { - if (settled) { - return; - } - settled = true; - clearTimeout(guard.timeout); - signal.removeEventListener('abort', onAbort); - resolve(result); - }; - - void rsFmt.exited.then((exit) => { - if (signal.aborted) { - settle({ kind: 'cancelled' }); - return; - } - settle( - exit.kind === 'error' - ? launchError(rsFmt.rsBinJs, errorMessage(exit.error)) - : interpretExit(rsFmt, serve.text, exit.code), - ); - }); - if (signal.aborted) { - // `addEventListener` never fires on an already-aborted signal, and a - // standby's child is not bound to the request signal at spawn time. - onAbort(); - return; - } - - signal.addEventListener('abort', onAbort, { once: true }); - guard.timeout = setTimeout(() => { - rsFmt.kill(); - settle({ - kind: 'error', - message: `rs fmt at ${rsFmt.rsBinJs} timed out after ${timeoutMs / 1000} seconds`, - }); - }, timeoutMs); - - rsFmt.write(serve.text); - }); - -export const runRsFmt = async (run: RsFmtRun): Promise => { - if (run.signal.aborted) { - return { kind: 'cancelled' }; - } - - const spawned = spawnRsFmt({ - filePath: run.filePath, - cwd: run.cwd, - rsBinJs: run.rsBinJs, - }); - if (spawned.kind === 'error') { - return launchError(run.rsBinJs, errorMessage(spawned.error)); - } - - return serveRsFmt({ - process: spawned.process, - text: run.text, - signal: run.signal, - }); -}; - -/** A single minimal replacement, expressed as offsets to stay vscode-free. */ -export const minimalEdit = ( - original: string, - formatted: string, -): { start: number; end: number; newText: string } | undefined => { - if (original === formatted) { - return undefined; - } - - let start = 0; - const sharedLength = Math.min(original.length, formatted.length); - while (start < sharedLength && original[start] === formatted[start]) { - start += 1; - } - - let originalEnd = original.length; - let formattedEnd = formatted.length; - while ( - originalEnd > start && - formattedEnd > start && - original[originalEnd - 1] === formatted[formattedEnd - 1] - ) { - originalEnd -= 1; - formattedEnd -= 1; - } - - return { - start, - end: originalEnd, - newText: formatted.slice(start, formattedEnd), - }; -}; diff --git a/packages/vscode/src/stacks/fmt/standby.ts b/packages/vscode/src/stacks/fmt/standby.ts deleted file mode 100644 index 8a56ce8..0000000 --- a/packages/vscode/src/stacks/fmt/standby.ts +++ /dev/null @@ -1,172 +0,0 @@ -import { - errorMessage, - type RsFmtProcess, - type RsFmtResult, - serveRsFmt, - spawnRsFmt, -} from './run'; - -/** - * What a standby is bound to. `rs fmt` reads `--stdin-filepath` from argv and - * resolves its config from the spawn cwd, so none of the three can change - * after the process starts: a request that does not match all three has to be - * formatted cold. - */ -export interface StandbyKey { - readonly cwd: string; - readonly filePath: string; - readonly rsBinJs: string; -} - -export const sameStandbyKey = (left: StandbyKey, right: StandbyKey): boolean => - left.cwd === right.cwd && - left.filePath === right.filePath && - left.rsBinJs === right.rsBinJs; - -/** - * How long an armed-but-unconsumed standby is kept. Expiry is not an error — - * it reclaims the memory of a process nobody is going to use, and the next - * eligible event arms a new one. - */ -const IDLE_TIMEOUT_MS = 5 * 60_000; - -export interface FmtStandbyOptions { - /** - * Debug-level sink. The standby is invisible to the user: arming never - * reports status and never logs above debug. - */ - readonly log: (message: string) => void; - /** Idle lifetime of an armed standby; injected short by the unit tests. */ - readonly idleTimeoutMs?: number; - /** Guard timeout for a hot format; starts at consume, not at spawn. */ - readonly requestTimeoutMs?: number; -} - -interface ArmedStandby { - readonly key: StandbyKey; - readonly process: RsFmtProcess; - readonly idleTimer: NodeJS.Timeout; -} - -/** - * The single pre-spawned `rs fmt` process that tracks the active editor. - * - * It is a bounded exception to the extension's "no warm tier" rule: at most one - * process, bound to one file, serving exactly one request. There is no - * protocol, no pool and no state carried between requests — everything the - * standby saves is the process start-up plus config load a cold format pays on - * the critical path. - * - * Deliberately vscode-free (like `run.ts`) so the whole lifecycle is unit - * testable; the controller owns every VS Code event that drives it. - */ -export class FmtStandby { - #armed: ArmedStandby | undefined; - #disposed = false; - - constructor(private readonly options: FmtStandbyOptions) {} - - /** The file the standby currently targets, if any. */ - get armedFilePath(): string | undefined { - return this.#armed?.key.filePath; - } - - /** - * Spawns the standby for `key`, replacing any standby on another key. - * Returns whether a standby is now armed for `key`. Failures are reported - * through `log` only: a standby that cannot be armed costs a cold format, - * never a user-visible error. - */ - arm(key: StandbyKey): boolean { - if (this.#disposed) { - return false; - } - if (this.#armed) { - if (sameStandbyKey(this.#armed.key, key)) { - return true; - } - // Replacing, not invalidating: the caller is already arming the - // replacement, so this kill must not ask for another one. - this.kill('the active editor moved to another file'); - } - - const spawned = spawnRsFmt({ - cwd: key.cwd, - filePath: key.filePath, - rsBinJs: key.rsBinJs, - }); - if (spawned.kind === 'error') { - this.options.log( - `Standby not armed for ${key.filePath}: ${errorMessage(spawned.error)}`, - ); - return false; - } - - const armed: ArmedStandby = { - key, - process: spawned.process, - idleTimer: setTimeout(() => { - this.kill('expired after idling'); - }, this.options.idleTimeoutMs ?? IDLE_TIMEOUT_MS), - }; - this.#armed = armed; - // A parked child that dies on its own (a crash, a config load throwing) is - // not an error either — the next request simply formats cold. The identity - // check is what makes this a no-op once the child has been consumed or - // replaced, so nothing has to unsubscribe. - void spawned.process.exited.then(() => { - if (this.#armed === armed) { - this.kill('the parked process exited'); - } - }); - this.options.log(`Standby armed for ${key.filePath}`); - return true; - } - - /** - * Serves one format request from the standby, or returns `undefined` when it - * cannot — nothing armed, a different key, or a standby already taken by a - * concurrent request. The caller then formats cold. - */ - consume( - key: StandbyKey, - request: { readonly text: string; readonly signal: AbortSignal }, - ): Promise | undefined { - const armed = this.#armed; - if (!armed || !sameStandbyKey(armed.key, key)) { - return undefined; - } - // A standby serves exactly one request. Taking it here is also what makes a - // second, concurrent request miss and go cold. - this.#armed = undefined; - clearTimeout(armed.idleTimer); - this.options.log(`Standby consumed for ${key.filePath}`); - return serveRsFmt({ - process: armed.process, - text: request.text, - signal: request.signal, - timeoutMs: this.options.requestTimeoutMs, - }); - } - - /** - * Invalidates the standby. The caller re-arms afterwards if the active editor - * still qualifies — a parked process has already loaded the project config, - * so a config edit (or any detection change) makes it wrong, not stale. - */ - kill(reason: string): void { - const armed = this.#armed; - if (!armed) { - return; - } - this.#armed = undefined; - clearTimeout(armed.idleTimer); - armed.process.kill(); - this.options.log(`Standby killed (${reason})`); - } - - dispose(): void { - this.#disposed = true; - this.kill('the fmt stack was disposed'); - } -} diff --git a/packages/vscode/src/stacks/test/config.ts b/packages/vscode/src/stacks/test/config.ts index f4ee53d..6d040b4 100644 --- a/packages/vscode/src/stacks/test/config.ts +++ b/packages/vscode/src/stacks/test/config.ts @@ -28,7 +28,6 @@ const configSchema = object({ // The path to a package.json file of a Rstest executable. // Used as a last resort if the extension cannot auto-detect @rstest/core. rstestPackagePath: fallback(optional(string()), undefined), - nodeExecutable: fallback(optional(string()), undefined), nodeExecArgs: fallback(array(string()), []), nodeEnv: fallback(optional(record(string(), string())), undefined), debugNodeEnv: fallback(optional(record(string(), string())), undefined), diff --git a/packages/vscode/src/stacks/test/index.ts b/packages/vscode/src/stacks/test/index.ts index 2c9ac2a..a55bdb3 100644 --- a/packages/vscode/src/stacks/test/index.ts +++ b/packages/vscode/src/stacks/test/index.ts @@ -8,7 +8,7 @@ import { RstestDiagnostics } from './diagnostics'; import { TestErrorStore, testMessageText } from './errorStore'; import { logger } from './logger'; import { runningWorkers, warmWorkerNodePreflight } from './master'; -import { resetWorkerNodeCaches } from './nodeResolution'; +import { NODE_EXECUTABLE_SETTING } from '../../shared/nodeResolution'; import { Project, WorkspaceManager } from './project'; import { status } from './status'; import { disposeTerminal } from './terminal'; @@ -507,14 +507,14 @@ 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 + // `shared/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 `rstack.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']; + readonly restartOnSettings = [NODE_EXECUTABLE_SETTING]; #rstest: Rstest | undefined; @@ -549,11 +549,12 @@ class RstestController implements StackController { 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 — and what - // `restartOnSettings` relies on, rather than resetting the memo itself. - resetWorkerNodeCaches(); + // The User Node preflight memo is deliberately NOT reset here: it is + // host-scoped state shared with the fmt stack, and a stack-scoped dispose + // (deactivate, detection loss) clearing it would force a live sibling to + // re-probe for nothing. The shell's restart pass owns the reset — which is + // what makes the restart command and `restartOnSettings` pick up a + // toolchain change. status.unbind(); logger.unbind(); } diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index 145fd6e..efadfc9 100644 --- a/packages/vscode/src/stacks/test/master.ts +++ b/packages/vscode/src/stacks/test/master.ts @@ -12,6 +12,10 @@ import { readPackageVersion, reportVersionCheck, } from '../../shared/versionCheck'; +import { + expandWorkspaceFolder, + getConfiguredNodeExecutable, +} from '../../shared/nodeExecutableSetting'; import { CONFIG_SECTION, getConfigValue } from './config'; import { formatConfiguredCoreNotFoundMessage, @@ -25,9 +29,9 @@ import { nodeRequire } from './nodeRequire'; import { configuredNodeBelowFloor, NodePreflightError, - type ResolveWorkerNodeOptions, - resolveWorkerNodeOnce, -} from './nodeResolution'; + type ResolveUserNodeOptions, + resolveUserNodeOnce, +} from '../../shared/nodeResolution'; import type { Project } from './project'; import { NODE_RUNTIME_STATUS_SOURCE, status } from './status'; import { runInTerminal as sendToTerminal, shellQuote } from './terminal'; @@ -48,7 +52,7 @@ export const runningWorkers = new Set>(); */ export const workerNodeOptions = ( cwd: string | undefined, -): ResolveWorkerNodeOptions => ({ +): ResolveUserNodeOptions => ({ shell: vscode.env.shell || undefined, cwd, notify: (message) => logger.info(message), @@ -74,11 +78,9 @@ export const workerNodeOptions = ( export const warmWorkerNodePreflight = ( folders: readonly vscode.WorkspaceFolder[], ): void => { - const target = folders.find( - (folder) => !getConfigValue('nodeExecutable', folder), - ); + const target = folders.find((folder) => !getConfiguredNodeExecutable(folder)); if (target !== undefined) { - void resolveWorkerNodeOnce(workerNodeOptions(target.uri.fsPath)).catch( + void resolveUserNodeOnce(workerNodeOptions(target.uri.fsPath)).catch( () => {}, ); } @@ -145,7 +147,7 @@ export class RstestApi { } private expandWorkspaceFolder(value: string): string { - return value.replaceAll('${workspaceFolder}', this.workspace.uri.fsPath); + return expandWorkspaceFolder(value, this.workspace); } // Regex source that selects a single reported case by its name path. Shared by @@ -158,27 +160,22 @@ export class RstestApi { return `^${regexpEscape(testCaseNamePath.join(' '))}${isSuite ? ' ' : '$'}`; } - // 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. + // The node executable + exec args honoring the shared `rstack.nodeExecutable` + // and the stack's `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', - this.workspace, - ); + const configuredExecutable = getConfiguredNodeExecutable(this.workspace); return { - nodeExecutable: configuredExecutable - ? this.expandWorkspaceFolder(configuredExecutable) - : 'node', - configured: Boolean(configuredExecutable), + nodeExecutable: configuredExecutable ?? 'node', + configured: configuredExecutable !== undefined, nodeExecArgs: getConfigValue('nodeExecArgs', this.workspace).map((arg) => this.expandWorkspaceFolder(arg), ), @@ -213,14 +210,14 @@ 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`. + * per-project, lives in `shared/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. + * `vscode.env.shell` is read here so `shared/nodeResolution.ts` needs no VS + * Code import. */ private async resolveWorkerNodeCommand(): Promise<{ nodeExecutable: string; @@ -240,15 +237,16 @@ export class RstestApi { return { nodeExecutable, nodeExecArgs }; } try { - const resolution = await resolveWorkerNodeOnce( - workerNodeOptions(this.cwd), - ); + const resolution = await resolveUserNodeOnce(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. - this.reportNodeRuntimeIssue(error.message, NODE_RUNTIME_STATUS_SOURCE); + this.reportNodeRuntimeIssue( + error.messageWith('tests will not run'), + NODE_RUNTIME_STATUS_SOURCE, + ); } throw error; } diff --git a/packages/vscode/src/types.ts b/packages/vscode/src/types.ts index f095d18..c964c9a 100644 --- a/packages/vscode/src/types.ts +++ b/packages/vscode/src/types.ts @@ -124,9 +124,12 @@ 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. + * Setting names 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. A bare name is + * relative to the stack's namespace (`rstack..`); a name starting + * with `rstack.` is taken as-is, for shared settings like + * `rstack.nodeExecutable`. * * 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 diff --git a/packages/vscode/tests/extension.test.ts b/packages/vscode/tests/extension.test.ts index 70cab1e..5d303ba 100644 --- a/packages/vscode/tests/extension.test.ts +++ b/packages/vscode/tests/extension.test.ts @@ -338,6 +338,27 @@ describe('restart-triggering settings', () => { expect(stacksOf('register')).toEqual(['rslint']); }); + it('treats a declared rstack.* name as fully qualified, shared across stacks', async () => { + // The shared runtime pin (`rstack.nodeExecutable`) lives outside any stack + // namespace, so its declaration must not be re-prefixed into + // `rstack..rstack.nodeExecutable` — and one change event rebuilds + // every stack that declared it. + await deactivate(); + harness.reset(); + harness.detected = new Set(['rslint', 'rstest', 'fmt']); + harness.restartOnSettings = new Map([ + ['rstest', ['rstack.nodeExecutable']], + ['fmt', ['rstack.nodeExecutable']], + ]); + await activate(context); + harness.events.length = 0; + + changeSetting('rstack.nodeExecutable'); + await settle(); + expect(stacksOf('dispose').sort()).toEqual(['fmt', 'rstest']); + expect(stacksOf('register').sort()).toEqual(['fmt', 'rstest']); + }); + it('ignores a setting no stack declared', async () => { changeSetting('rstack.rstest.nodeExecArgs'); await settle(); diff --git a/packages/vscode/tests/migration.test.ts b/packages/vscode/tests/migration.test.ts index 85035b6..9be44b5 100644 --- a/packages/vscode/tests/migration.test.ts +++ b/packages/vscode/tests/migration.test.ts @@ -44,14 +44,16 @@ const folderReading = ( describe('LEGACY_MAPPINGS', () => { it('covers the full legacy inventory of both retired extensions', () => { // 4 settings from rslint/packages/vscode-extension + 14 from - // rstest/packages/vscode, verified against their manifests. + // rstest/packages/vscode, verified against their manifests, plus the + // runtime pin this extension itself renamed to `rstack.nodeExecutable`. expect(LEGACY_MAPPINGS.map((mapping) => mapping.from)).toEqual([ 'rslint.enable', 'rslint.binPath', 'rslint.customBinPath', 'rslint.trace.server', - 'rstest.rstestPackagePath', 'rstest.nodeExecutable', + 'rstack.rstest.nodeExecutable', + 'rstest.rstestPackagePath', 'rstest.nodeExecArgs', 'rstest.nodeEnv', 'rstest.debugNodeEnv', @@ -67,20 +69,36 @@ describe('LEGACY_MAPPINGS', () => { ]); }); - it('renames every key into the rstack..* namespace', () => { + it('renames every key into the rstack..* namespace, except the shared runtime pin', () => { for (const mapping of LEGACY_MAPPINGS) { + if (mapping.to === 'rstack.nodeExecutable') { + continue; + } const [stack, ...rest] = mapping.from.split('.'); expect(mapping.to).toBe(`rstack.${stack}.${rest.join('.')}`); } }); - it('has no duplicate source or target keys', () => { + it('has no duplicate source keys, and shares a target only for the runtime pin', () => { expect(new Set(LEGACY_MAPPINGS.map((m) => m.from)).size).toBe( LEGACY_MAPPINGS.length, ); - expect(new Set(LEGACY_MAPPINGS.map((m) => m.to)).size).toBe( - LEGACY_MAPPINGS.length, + // Both retired pins converge on `rstack.nodeExecutable`; every other + // target is unique. When both sources are set in one layer, the mapping + // order decides: the later mapping — this extension's own retired key, the + // one more recently in effect — wins, and the earlier one is planned as a + // 'superseded' skip. + const shared = LEGACY_MAPPINGS.filter( + (m) => m.to === 'rstack.nodeExecutable', + ); + expect(shared.map((m) => m.from)).toEqual([ + 'rstest.nodeExecutable', + 'rstack.rstest.nodeExecutable', + ]); + const rest = LEGACY_MAPPINGS.filter( + (m) => m.to !== 'rstack.nodeExecutable', ); + expect(new Set(rest.map((m) => m.to)).size).toBe(rest.length); }); it('marks the window-scoped settings as such', () => { @@ -311,6 +329,25 @@ describe('planMigration — conflicts', () => { }); }); + it('plans one write and a superseded skip when both retired pins are set in one layer', () => { + const plan = planMigration([ + reading({ key: 'rstest.nodeExecutable', value: '/old/node' }), + reading({ key: 'rstack.rstest.nodeExecutable', value: '/new/node' }), + ]); + expect(plan.writeCount).toBe(1); + expect(plan.scopes[0]?.writes[0]).toMatchObject({ + from: 'rstack.rstest.nodeExecutable', + to: 'rstack.nodeExecutable', + value: '/new/node', + }); + expect(plan.skips[0]).toMatchObject({ + from: 'rstest.nodeExecutable', + to: 'rstack.nodeExecutable', + value: '/old/node', + reason: 'superseded', + }); + }); + it('treats a conflict as layer-local', () => { const plan = planMigration([ reading({ key: 'rslint.enable', value: true, targetValue: false }), diff --git a/packages/vscode/tests/stacks/test/nodeResolution.test.ts b/packages/vscode/tests/shared/nodeResolution.test.ts similarity index 87% rename from packages/vscode/tests/stacks/test/nodeResolution.test.ts rename to packages/vscode/tests/shared/nodeResolution.test.ts index 486e101..556a002 100644 --- a/packages/vscode/tests/stacks/test/nodeResolution.test.ts +++ b/packages/vscode/tests/shared/nodeResolution.test.ts @@ -2,7 +2,7 @@ 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_LABEL } from '../../../src/shared/versionCheck'; +import { NODE_RUNTIME_LABEL } from '../../src/shared/versionCheck'; import { configuredNodeBelowFloor, END_TOKEN, @@ -10,12 +10,12 @@ import { NodePreflightError, probeNodeVersion, probeShellNodePath, - type ResolveWorkerNodeOptions, - resetWorkerNodeCaches, - resolveWorkerNode, - resolveWorkerNodeOnce, + type ResolveUserNodeOptions, + resetUserNodeCaches, + resolveUserNode, + resolveUserNodeOnce, START_TOKEN, -} from '../../../src/stacks/test/nodeResolution'; +} from '../../src/shared/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. @@ -38,18 +38,18 @@ const base = { shell: '/bin/zsh' } as const; /** Asserts the call rejects, and hands back the error already narrowed. */ const preflightError = ( - options: ResolveWorkerNodeOptions, + options: ResolveUserNodeOptions, ): Promise => - resolveWorkerNode(options).then( + resolveUserNode(options).then( () => { throw new Error('expected a NodePreflightError'); }, (error: unknown) => error as NodePreflightError, ); -describe('resolveWorkerNode', () => { +describe('resolveUserNode', () => { it('uses the PATH node when it satisfies the floor', async () => { - const resolution = await resolveWorkerNode({ + const resolution = await resolveUserNode({ ...base, probe: versionsOf({ node: ok('22.18.0') }), probeShellPath: never, @@ -67,7 +67,7 @@ describe('resolveWorkerNode', () => { // 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({ + const resolution = await resolveUserNode({ ...base, probe: versionsOf({ node: { kind: 'ok' }, @@ -90,7 +90,7 @@ describe('resolveWorkerNode', () => { 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({ + const resolution = await resolveUserNode({ ...base, probe: versionsOf({ node: ok('24.0.0-nightly20260101') }), probeShellPath: never, @@ -99,7 +99,7 @@ describe('resolveWorkerNode', () => { }); it('falls back to the shell node when the PATH node is too old', async () => { - const resolution = await resolveWorkerNode({ + const resolution = await resolveUserNode({ ...base, probe: versionsOf({ node: ok('20.19.4'), @@ -117,7 +117,7 @@ describe('resolveWorkerNode', () => { it('falls through a pre-23.6 Node 23, which does not strip types', async () => { // 23.0–23.5 sit above 22.18.0 yet predate default type stripping — the // hole `NODE_RUNTIME_RANGE`'s disjunction encodes. - const resolution = await resolveWorkerNode({ + const resolution = await resolveUserNode({ ...base, probe: versionsOf({ node: ok('23.5.0'), @@ -129,7 +129,7 @@ describe('resolveWorkerNode', () => { }); it('falls back to the shell node when no PATH node runs at all', async () => { - const resolution = await resolveWorkerNode({ + const resolution = await resolveUserNode({ ...base, probe: versionsOf({ '/versions/24/bin/node': ok('24.0.0') }), probeShellPath: shellFinds('/versions/24/bin/node'), @@ -141,7 +141,7 @@ describe('resolveWorkerNode', () => { // 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({ + const resolution = await resolveUserNode({ ...base, probe: (executable) => { if (executable !== 'node') @@ -237,19 +237,34 @@ describe('NodePreflightError', () => { expect(message).toContain(NODE_RUNTIME_LABEL); expect(message).toContain('PATH: 20.19.4'); expect(message).toContain('interactive shell: 22.14.0'); - expect(message).toContain('rstack.rstest.nodeExecutable'); + expect(message).toContain('rstack.nodeExecutable'); }); - it('states the consequence, which is what a below-floor setting does not', () => { + it('carries no consequence of its own — each reporting site appends one', () => { // 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". + // and the consequence clause is the only thing distinguishing "nothing + // will run" from "we are running with it anyway". The preflight serves + // stacks with different consequences ("tests will not run" vs "rs fmt will + // not format") and is memoized host-wide, so the message must stay neutral + // and composable: no trailing period, consequence appended by the caller. const { message } = new NodePreflightError({ path: '20.19.4', shell: undefined, shellSkipped: false, }); - expect(message).toContain('tests will not run'); + expect(message).not.toContain('will not run'); + expect(message.endsWith('to one')).toBe(true); + }); + + it('joins a caller consequence through messageWith', () => { + const error = new NodePreflightError({ + path: '20.19.4', + shell: undefined, + shellSkipped: false, + }); + expect(error.messageWith('tests will not run')).toBe( + `${error.message}; until then tests will not run.`, + ); }); it('does not invent versions for candidates that found nothing', () => { @@ -275,9 +290,9 @@ describe('NodePreflightError', () => { }); }); -describe('resolveWorkerNodeOnce', () => { +describe('resolveUserNodeOnce', () => { afterEach(() => { - resetWorkerNodeCaches(); + resetUserNodeCaches(); }); const options = { @@ -287,12 +302,12 @@ describe('resolveWorkerNodeOnce', () => { }; 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); + const first = await resolveUserNodeOnce(options); + const second = await resolveUserNodeOnce(options); expect(second).toBe(first); - resetWorkerNodeCaches(); - const third = await resolveWorkerNodeOnce(options); + resetUserNodeCaches(); + const third = await resolveUserNodeOnce(options); expect(third).not.toBe(first); }); @@ -306,8 +321,8 @@ describe('resolveWorkerNodeOnce', () => { }, probeShellPath: shellFinds(undefined), }; - await resolveWorkerNodeOnce(failing).catch(() => {}); - await resolveWorkerNodeOnce(failing).catch(() => {}); + await resolveUserNodeOnce(failing).catch(() => {}); + await resolveUserNodeOnce(failing).catch(() => {}); expect(calls).toBe(1); }); @@ -326,15 +341,15 @@ describe('resolveWorkerNodeOnce', () => { notify: (message: string) => notices.push(message), }; - await resolveWorkerNodeOnce(shellOptions); - await resolveWorkerNodeOnce(shellOptions); + await resolveUserNodeOnce(shellOptions); + await resolveUserNodeOnce(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({ + await resolveUserNodeOnce({ ...options, notify: (message) => notices.push(message), }); @@ -356,7 +371,7 @@ describe('resolveWorkerNodeOnce', () => { probeShellPath: never, }; const all = await Promise.all( - Array.from({ length: 20 }, () => resolveWorkerNodeOnce(slow)), + Array.from({ length: 20 }, () => resolveUserNodeOnce(slow)), ); expect(calls).toBe(1); expect(new Set(all).size).toBe(1); @@ -365,7 +380,7 @@ describe('resolveWorkerNodeOnce', () => { describe('configuredNodeBelowFloor', () => { afterEach(() => { - resetWorkerNodeCaches(); + resetUserNodeCaches(); }); const configured = '/opt/node/bin/node'; @@ -387,9 +402,9 @@ describe('configuredNodeBelowFloor', () => { expect(message).toContain(configured); expect(message).toContain('20.19.4'); expect(message).toContain(NODE_RUNTIME_LABEL); - expect(message).toContain('rstack.rstest.nodeExecutable'); + expect(message).toContain('rstack.nodeExecutable'); // The opposite consequence from `NodePreflightError`: this one runs. - expect(message).toContain('running tests with it anyway'); + expect(message).toContain('using it anyway'); }); it('reports an executable that cannot answer at all', async () => { @@ -416,7 +431,7 @@ describe('configuredNodeBelowFloor', () => { await configuredNodeBelowFloor(configured, counting); expect(calls).toBe(1); - resetWorkerNodeCaches(); + resetUserNodeCaches(); await configuredNodeBelowFloor(configured, counting); expect(calls).toBe(2); }); diff --git a/packages/vscode/tests/stacks/fmt/binEntry.test.ts b/packages/vscode/tests/stacks/fmt/binEntry.test.ts new file mode 100644 index 0000000..92d9c69 --- /dev/null +++ b/packages/vscode/tests/stacks/fmt/binEntry.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from '@rstest/core'; +import { pickBinEntry } from '../../../src/stacks/fmt/binEntry'; + +/** + * The `rs` entry the fmt language server is spawned with comes out of the + * resolved `rstack` package.json, whose `bin` field npm allows in two shapes. A + * wrong pick spawns nothing at all, and the failure would reach the user as a + * language server that never starts. + */ +describe('pickBinEntry', () => { + it('takes the string form as the entry', () => { + expect(pickBinEntry('dist/cli.js')).toBe('dist/cli.js'); + }); + + it('takes the `rs` key out of the object form', () => { + expect(pickBinEntry({ rs: 'bin/rs.mjs', rstack: 'bin/other.js' })).toBe( + 'bin/rs.mjs', + ); + }); + + it('falls back to the documented default when the field is unusable', () => { + expect(pickBinEntry(undefined)).toBe('bin/rs.js'); + expect(pickBinEntry(null)).toBe('bin/rs.js'); + expect(pickBinEntry({})).toBe('bin/rs.js'); + expect(pickBinEntry({ rstack: 'bin/rs.js' })).toBe('bin/rs.js'); + expect(pickBinEntry({ rs: 42 })).toBe('bin/rs.js'); + }); +}); diff --git a/packages/vscode/tests/stacks/fmt/run.test.ts b/packages/vscode/tests/stacks/fmt/run.test.ts deleted file mode 100644 index bef30fa..0000000 --- a/packages/vscode/tests/stacks/fmt/run.test.ts +++ /dev/null @@ -1,252 +0,0 @@ -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from '@rstest/core'; -import { - minimalEdit, - pickConfigDir, - runRsFmt, - type RsFmtRun, -} from '../../../src/stacks/fmt/run'; -import { createStubRoot, type StubRoot } from './stubProcess'; - -describe('pickConfigDir', () => { - let root: string; - - beforeEach(() => { - root = fs.realpathSync( - fs.mkdtempSync(path.join(os.tmpdir(), 'rstack-fmt-path-')), - ); - }); - - afterEach(() => { - fs.rmSync(root, { recursive: true, force: true }); - }); - - const config = (dir: string): string => path.join(dir, 'rstack.config.ts'); - - it('uses the directory of an ancestor config', () => { - const app = path.join(root, 'app'); - expect( - pickConfigDir(path.join(app, 'src', 'index.ts'), [config(app)], root), - ).toBe(app); - }); - - it('uses the deepest ancestor config', () => { - const app = path.join(root, 'app'); - const nested = path.join(app, 'packages', 'nested'); - expect( - pickConfigDir( - path.join(nested, 'src', 'index.ts'), - [config(app), config(nested)], - root, - ), - ).toBe(nested); - }); - - it('falls back when every config is outside the document tree', () => { - expect( - pickConfigDir( - path.join(root, 'app', 'index.ts'), - [config(path.join(root, 'other'))], - root, - ), - ).toBe(root); - }); - - it('does not confuse a sibling path prefix for an ancestor', () => { - expect( - pickConfigDir( - path.join(root, 'abc', 'index.ts'), - [config(path.join(root, 'ab'))], - root, - ), - ).toBe(root); - }); - - it('uses a config next to the document', () => { - const app = path.join(root, 'app'); - expect(pickConfigDir(path.join(app, 'index.ts'), [config(app)], root)).toBe( - app, - ); - }); -}); - -describe('minimalEdit', () => { - it('returns no edit for identical text', () => { - expect(minimalEdit('same', 'same')).toBeUndefined(); - }); - - it('handles a pure insertion', () => { - expect(minimalEdit('ac', 'abc')).toEqual({ - start: 1, - end: 1, - newText: 'b', - }); - }); - - it('handles a pure deletion', () => { - expect(minimalEdit('abc', 'ac')).toEqual({ - start: 1, - end: 2, - newText: '', - }); - }); - - it('handles a change at the start', () => { - expect(minimalEdit('old tail', 'new tail')).toEqual({ - start: 0, - end: 3, - newText: 'new', - }); - }); - - it('handles a change at the end', () => { - expect(minimalEdit('head old', 'head new')).toEqual({ - start: 5, - end: 8, - newText: 'new', - }); - }); - - it('handles a complete rewrite', () => { - expect(minimalEdit('abc', 'xyz')).toEqual({ - start: 0, - end: 3, - newText: 'xyz', - }); - }); - - it('inserts into an empty original', () => { - expect(minimalEdit('', 'text')).toEqual({ - start: 0, - end: 0, - newText: 'text', - }); - }); - - it('deletes the full original for an empty result', () => { - expect(minimalEdit(' ', '')).toEqual({ - start: 0, - end: 3, - newText: '', - }); - }); - - it('does not overlap the prefix and suffix scans', () => { - expect(minimalEdit('aa', 'aba')).toEqual({ - start: 1, - end: 1, - newText: 'b', - }); - }); - - it('normalizes a CRLF document in one replacement', () => { - expect(minimalEdit('a\r\nb\r\n', 'a\nb\n')).toEqual({ - start: 1, - end: 5, - newText: '\nb', - }); - }); -}); - -describe('runRsFmt', () => { - let stubs: StubRoot; - - beforeEach(() => { - stubs = createStubRoot('run'); - }); - - afterEach(() => { - stubs.remove(); - }); - - const writeStub = (source: string): string => stubs.write(source); - - const run = ( - text: string, - rsBinJs: string, - signal: AbortSignal = new AbortController().signal, - ): Promise< - ReturnType extends Promise ? T : never - > => { - const options: RsFmtRun = { - text, - filePath: path.join(stubs.path, 'input.ts'), - cwd: stubs.path, - rsBinJs, - signal, - }; - return runRsFmt(options); - }; - - it('captures the complete formatted stdout', async () => { - const stub = writeStub('process.stdin.pipe(process.stdout);\n'); - await expect(run('const value = 1;\n', stub)).resolves.toEqual({ - kind: 'ok', - formatted: 'const value = 1;\n', - stderr: '', - }); - }); - - it('drains stdout while writing a large document', async () => { - const stub = writeStub('process.stdin.pipe(process.stdout);\n'); - const text = 'x'.repeat(1024 * 1024 + 17); - await expect(run(text, stub)).resolves.toEqual({ - kind: 'ok', - formatted: text, - stderr: '', - }); - }); - - it('returns the stderr tail for a formatter failure', async () => { - const stub = writeStub( - "for (let i = 0; i < 12; i++) console.error('line-' + i); process.exit(2);\n", - ); - const result = await run('broken', stub); - expect(result.kind).toBe('error'); - if (result.kind === 'error') { - expect(result.message).toContain('line-11'); - expect(result.message).not.toContain('line-0'); - } - }); - - it('handles a child exiting without reading stdin', async () => { - const stub = writeStub('process.exit(2);\n'); - const result = await run('x'.repeat(1024 * 1024), stub); - expect(result).toEqual({ - kind: 'error', - message: 'rs fmt exited with code 2', - }); - }); - - it('returns cancelled when aborted mid-run', async () => { - const stub = writeStub('setTimeout(() => {}, 30_000);\n'); - const controller = new AbortController(); - const resultPromise = run('const value = 1;', stub, controller.signal); - setTimeout(() => controller.abort(), 50); - await expect(resultPromise).resolves.toEqual({ kind: 'cancelled' }); - }); - - it('does not spawn for an already-aborted signal', async () => { - const controller = new AbortController(); - controller.abort(); - await expect( - run( - 'const value = 1;', - path.join(stubs.path, 'missing.js'), - controller.signal, - ), - ).resolves.toEqual({ kind: 'cancelled' }); - }); - - it('names an unloadable rs entry path', async () => { - const missing = path.join(stubs.path, 'missing.js'); - const result = await run('const value = 1;', missing); - expect(result.kind).toBe('error'); - if (result.kind === 'error') { - expect(result.message).toContain(missing); - expect(result.message).toContain('Unable to run rs fmt'); - } - }); -}); diff --git a/packages/vscode/tests/stacks/fmt/standby.test.ts b/packages/vscode/tests/stacks/fmt/standby.test.ts deleted file mode 100644 index 82eadeb..0000000 --- a/packages/vscode/tests/stacks/fmt/standby.test.ts +++ /dev/null @@ -1,284 +0,0 @@ -import path from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from '@rstest/core'; -import { FmtStandby, type StandbyKey } from '../../../src/stacks/fmt/standby'; -import { createStubRoot, type StubRoot } from './stubProcess'; - -/** - * The stub scripts stand in for `rs fmt`: they are spawned exactly the way the - * CLI is, so the tests exercise the real parking, stdin write and exit - * interpretation rather than a mocked child process (same technique as - * `run.test.ts`). - */ -describe('FmtStandby', () => { - let stubs: StubRoot; - let standbys: FmtStandby[]; - let logs: string[]; - - beforeEach(() => { - stubs = createStubRoot('standby'); - standbys = []; - logs = []; - }); - - afterEach(() => { - for (const standby of standbys) { - standby.dispose(); - } - stubs.remove(); - }); - - const writeStub = (source: string): string => stubs.write(source); - const echoStub = (): string => stubs.echo(); - - const key = (rsBinJs: string, file = 'input.ts'): StandbyKey => ({ - cwd: stubs.path, - filePath: path.join(stubs.path, file), - rsBinJs, - }); - - const createStandby = ( - options: { idleTimeoutMs?: number; requestTimeoutMs?: number } = {}, - ): FmtStandby => { - const standby = new FmtStandby({ - log: (message) => logs.push(message), - ...options, - }); - standbys.push(standby); - return standby; - }; - - const consume = ( - standby: FmtStandby, - standbyKey: StandbyKey, - text: string, - signal: AbortSignal = new AbortController().signal, - ) => standby.consume(standbyKey, { text, signal }); - - const eventually = async ( - probe: () => boolean, - what: string, - ): Promise => { - const deadline = Date.now() + 5_000; - while (!probe()) { - if (Date.now() >= deadline) { - throw new Error(`timed out waiting for ${what}`); - } - await new Promise((resolve) => setTimeout(resolve, 10)); - } - }; - - it('arms a parked process that has not been written to', async () => { - // The stub only ever prints once it has read stdin, so seeing no output - // after arming is what "parked" means. - const stub = writeStub( - 'process.stdin.on("data", () => process.stdout.write("served"));\n', - ); - const standby = createStandby(); - const armed = key(stub); - expect(standby.arm(armed)).toBe(true); - expect(standby.armedFilePath).toBe(armed.filePath); - expect(logs).toContain(`Standby armed for ${armed.filePath}`); - - await new Promise((resolve) => setTimeout(resolve, 100)); - expect(standby.armedFilePath).toBe(armed.filePath); - }); - - it('serves the request it was armed for from the parked process', async () => { - const standby = createStandby(); - const armed = key(echoStub()); - expect(standby.arm(armed)).toBe(true); - const text = `const value = 1;\n${'x'.repeat(1024 * 64)}\n`; - await expect(consume(standby, armed, text)).resolves.toEqual({ - kind: 'ok', - formatted: text, - stderr: '', - }); - expect(logs).toContain(`Standby consumed for ${armed.filePath}`); - }); - - it('serves exactly one request', async () => { - const standby = createStandby(); - const armed = key(echoStub()); - standby.arm(armed); - await consume(standby, armed, 'const value = 1;\n'); - expect(consume(standby, armed, 'const value = 1;\n')).toBeUndefined(); - expect(standby.armedFilePath).toBeUndefined(); - }); - - it('misses when the request key does not match', async () => { - const standby = createStandby(); - const stub = echoStub(); - const armed = key(stub); - standby.arm(armed); - - expect(consume(standby, key(stub, 'other.ts'), 'x')).toBeUndefined(); - expect( - consume(standby, { ...armed, cwd: path.join(stubs.path, 'nested') }, 'x'), - ).toBeUndefined(); - expect( - consume(standby, { ...armed, rsBinJs: echoStub() }, 'x'), - ).toBeUndefined(); - // A miss leaves the standby armed for the file it does match. - expect(standby.armedFilePath).toBe(armed.filePath); - await expect(consume(standby, armed, 'x')).resolves.toEqual({ - kind: 'ok', - formatted: 'x', - stderr: '', - }); - }); - - it('re-arms on another file by replacing the parked process', () => { - const standby = createStandby(); - const stub = echoStub(); - standby.arm(key(stub)); - const next = key(stub, 'other.ts'); - expect(standby.arm(next)).toBe(true); - expect(standby.armedFilePath).toBe(next.filePath); - expect(logs).toContain( - 'Standby killed (the active editor moved to another file)', - ); - }); - - it('keeps the same process when re-armed for the same key', () => { - const standby = createStandby(); - const armed = key(echoStub()); - standby.arm(armed); - expect(standby.arm(armed)).toBe(true); - expect( - logs.filter((line) => line.startsWith('Standby armed')), - ).toHaveLength(1); - }); - - it('kills a parked process on invalidation', () => { - const standby = createStandby(); - standby.arm(key(echoStub())); - standby.kill('the config changed'); - expect(standby.armedFilePath).toBeUndefined(); - expect(logs).toContain('Standby killed (the config changed)'); - expect(consume(standby, key(echoStub()), 'x')).toBeUndefined(); - }); - - it('discards a parked process that exits on its own', async () => { - const standby = createStandby(); - const armed = key(writeStub('process.exit(3);\n')); - expect(standby.arm(armed)).toBe(true); - await eventually( - () => standby.armedFilePath === undefined, - 'the parked process to be discarded', - ); - expect(logs).toContain('Standby killed (the parked process exited)'); - // The next request misses and the caller formats cold. - expect(consume(standby, armed, 'x')).toBeUndefined(); - }); - - it('expires a standby nobody consumed', async () => { - const standby = createStandby({ idleTimeoutMs: 20 }); - const armed = key(echoStub()); - standby.arm(armed); - await eventually( - () => standby.armedFilePath === undefined, - 'the standby to expire', - ); - expect(logs).toContain('Standby killed (expired after idling)'); - expect(consume(standby, armed, 'x')).toBeUndefined(); - }); - - it('does not expire a standby that is serving a request', async () => { - const standby = createStandby({ idleTimeoutMs: 30 }); - const armed = key( - writeStub( - 'process.stdin.on("data", (chunk) => setTimeout(() => { process.stdout.write(chunk); process.exit(0); }, 150));\n', - ), - ); - standby.arm(armed); - await expect(consume(standby, armed, 'late')).resolves.toEqual({ - kind: 'ok', - formatted: 'late', - stderr: '', - }); - }); - - it('starts the guard timeout at consume, not at arm', async () => { - const standby = createStandby({ requestTimeoutMs: 100 }); - const stub = writeStub('setTimeout(() => {}, 30_000);\n'); - const armed = key(stub); - standby.arm(armed); - // The parked process outlives the guard window before the request starts. - await new Promise((resolve) => setTimeout(resolve, 200)); - const result = await consume(standby, armed, 'const value = 1;\n'); - expect(result).toEqual({ - kind: 'error', - message: `rs fmt at ${stub} timed out after 0.1 seconds`, - }); - }); - - it('cancels a hot request and kills its process', async () => { - const standby = createStandby(); - const armed = key(writeStub('setTimeout(() => {}, 30_000);\n')); - standby.arm(armed); - const controller = new AbortController(); - const result = consume( - standby, - armed, - 'const value = 1;\n', - controller.signal, - ); - setTimeout(() => controller.abort(), 50); - await expect(result).resolves.toEqual({ kind: 'cancelled' }); - }); - - it('reports the CLI verdict for a failing hot request', async () => { - const standby = createStandby(); - const armed = key( - writeStub( - 'process.stdin.on("data", () => { console.error("boom"); process.exit(2); });\n', - ), - ); - standby.arm(armed); - await expect(consume(standby, armed, 'broken')).resolves.toEqual({ - kind: 'error', - message: 'boom', - }); - }); - - it('distinguishes an ignored file from a whitespace-only document', async () => { - const standby = createStandby(); - const silent = writeStub('process.stdin.resume();\n'); - const ignored = key(silent); - standby.arm(ignored); - await expect( - consume(standby, ignored, 'const value = 1;\n'), - ).resolves.toEqual({ kind: 'skipped', stderr: '' }); - - const blank = key(silent, 'blank.ts'); - standby.arm(blank); - await expect(consume(standby, blank, ' \n')).resolves.toEqual({ - kind: 'ok', - formatted: '', - stderr: '', - }); - }); - - it('does not arm after dispose', () => { - const standby = createStandby(); - standby.arm(key(echoStub())); - standby.dispose(); - expect(standby.armedFilePath).toBeUndefined(); - expect(logs).toContain('Standby killed (the fmt stack was disposed)'); - expect(standby.arm(key(echoStub()))).toBe(false); - expect(standby.armedFilePath).toBeUndefined(); - }); - - it('discards a standby whose CLI entry cannot be loaded', async () => { - const standby = createStandby(); - // The spawn succeeds — node reports an unloadable entry by exiting — so an - // arm on a broken resolution costs nothing but a debug line. - const armed = key(path.join(stubs.path, 'missing.js')); - expect(standby.arm(armed)).toBe(true); - await eventually( - () => standby.armedFilePath === undefined, - 'the unloadable standby to be discarded', - ); - expect(consume(standby, armed, 'x')).toBeUndefined(); - }); -}); diff --git a/packages/vscode/tests/stacks/fmt/stubProcess.ts b/packages/vscode/tests/stacks/fmt/stubProcess.ts deleted file mode 100644 index 3d23c99..0000000 --- a/packages/vscode/tests/stacks/fmt/stubProcess.ts +++ /dev/null @@ -1,45 +0,0 @@ -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; - -/** - * Stub-script scaffolding shared by the fmt unit suites. - * - * The suites stand in for `rs fmt` with tiny node scripts spawned exactly the - * way the CLI is, so they exercise real process behaviour — parking on stdin, - * back-pressure, EPIPE, exit codes — instead of a mocked child. That technique - * is load-bearing for both `run.test.ts` and `standby.test.ts`, so it lives - * here rather than being copied into each. - * - * Not a `*.test.ts` file (the runner would collect it as a suite) and not a - * build entry, so it never ships in the VSIX. - */ -export interface StubRoot { - /** Temp directory the stubs live in; also used as the spawn cwd. */ - readonly path: string; - /** Writes a stub script and returns its path. */ - write(source: string): string; - /** Echoes stdin back, i.e. an `rs fmt` that finds nothing to change. */ - echo(): string; - remove(): void; -} - -export const createStubRoot = (prefix: string): StubRoot => { - const root = fs.realpathSync( - fs.mkdtempSync(path.join(os.tmpdir(), `rstack-fmt-${prefix}-`)), - ); - const write = (source: string): string => { - const filePath = path.join( - root, - `rs-${Math.random().toString(16).slice(2)}.js`, - ); - fs.writeFileSync(filePath, source); - return filePath; - }; - return { - path: root, - write, - echo: () => write('process.stdin.pipe(process.stdout);\n'), - remove: () => fs.rmSync(root, { recursive: true, force: true }), - }; -}; diff --git a/packages/vscode/tests/stacks/test/bridge.test.ts b/packages/vscode/tests/stacks/test/bridge.test.ts index eaddcef..f37ac26 100644 --- a/packages/vscode/tests/stacks/test/bridge.test.ts +++ b/packages/vscode/tests/stacks/test/bridge.test.ts @@ -53,7 +53,7 @@ const makeTmpDir = (): string => { * `dist/rstestConfig.js`. */ const createWorkspace = ({ - version = '0.3.5', + version = '0.5.2', shim = true, }: { version?: string | null; shim?: boolean } = {}): string => { const root = makeTmpDir(); @@ -103,7 +103,7 @@ describe('resolveRstackShim', () => { const shim = resolveRstackShim(configDir); expect(shim).toBeDefined(); - expect(shim?.version).toBe('0.3.5'); + expect(shim?.version).toBe('0.5.2'); // The same file `rs test` injects with `--config`. expect(shim?.configFilePath).toBe( path.join(configDir, 'node_modules', 'rstack', 'dist', 'rstestConfig.js'), @@ -134,14 +134,14 @@ describe('resolveRstackShim', () => { }); it('refuses an rstack older than the support matrix floor', () => { - const configDir = createWorkspace({ version: '0.3.4' }); + const configDir = createWorkspace({ version: '0.5.1' }); expect(resolveRstackShim(configDir)).toBeUndefined(); expect(reported).toEqual([ { kind: 'version-mismatch', detail: - 'rstack 0.3.4 is not supported, this extension requires >=0.3.5', + 'rstack 0.5.1 is not supported, this extension requires >=0.5.2', }, ]); }); diff --git a/packages/vscode/tests/stacks/test/master.test.ts b/packages/vscode/tests/stacks/test/master.test.ts index fcc27ef..145d9ea 100644 --- a/packages/vscode/tests/stacks/test/master.test.ts +++ b/packages/vscode/tests/stacks/test/master.test.ts @@ -8,8 +8,8 @@ import { RstestApi } from '../../../src/stacks/test/master'; import { type NodeProbe, configuredNodeBelowFloor, - resetWorkerNodeCaches, -} from '../../../src/stacks/test/nodeResolution'; + resetUserNodeCaches, +} from '../../../src/shared/nodeResolution'; import { status } from '../../../src/stacks/test/status'; import type { StatusReporter } from '../../../src/types'; @@ -274,14 +274,16 @@ describe('RstestApi with a configured nodeExecutable', () => { beforeEach(() => { mismatches.length = 0; repaints = 0; - resetWorkerNodeCaches(); + resetUserNodeCaches(); status.bind(reporter); - settings.nodeExecutable = configuredNode; + // The shared pin is read by its fully-qualified name through a section-less + // `getConfiguration(undefined, folder)`, so the stub key carries the dots. + settings['rstack.nodeExecutable'] = configuredNode; }); afterEach(() => { status.unbind(); - resetWorkerNodeCaches(); + resetUserNodeCaches(); }); // The verdict is reported off the spawn path, so a spawn resolves before the diff --git a/packages/vscode/tests/versionCheck.test.ts b/packages/vscode/tests/versionCheck.test.ts index 45778cb..a350134 100644 --- a/packages/vscode/tests/versionCheck.test.ts +++ b/packages/vscode/tests/versionCheck.test.ts @@ -13,7 +13,7 @@ describe('support matrix', () => { expect(SUPPORT_MATRIX).toEqual({ '@rslint/core': '>=0.7.2', '@rstest/core': '>=0.6.0', - rstack: '>=0.3.5', + rstack: '>=0.5.2', }); }); }); @@ -23,7 +23,7 @@ describe('checkPackageVersion', () => { expect(checkPackageVersion('@rslint/core', '0.7.2').kind).toBe('ok'); expect(checkPackageVersion('@rslint/core', '1.2.3').kind).toBe('ok'); expect(checkPackageVersion('@rstest/core', '0.11.5').kind).toBe('ok'); - expect(checkPackageVersion('rstack', '0.3.5').kind).toBe('ok'); + expect(checkPackageVersion('rstack', '0.5.2').kind).toBe('ok'); }); it('accepts prereleases of a supported range', () => { From 513fee559320591b76b9bdde88adf73752a3c045 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 13 Aug 2026 16:28:38 +0800 Subject: [PATCH 02/12] fix(vscode): fold per-folder fmt states into one severity-ranked status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a multi-root workspace every FmtFolderRuntime wrote to the stack's single status reporter directly, so a folder that started or recovered after a sibling failed replaced the failure with a global 'running' — the displayed state depended on event ordering. Runtimes now record their state and detail and notify the controller, which is the one writer to the shell: the folder set is folded by severity (crashed > version mismatch > disabled > pin advisory > running), so a healthy sibling never overwrites another folder's failure and a recovery clears only its own. Failure details name the folder they belong to. ADR 0002 records the fold. --- docs/adr/0002-fmt-lsp-on-user-node-runtime.md | 2 +- packages/vscode/src/stacks/fmt/index.ts | 131 ++++++++++++++---- 2 files changed, 105 insertions(+), 28 deletions(-) diff --git a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md index 7305180..f062fd3 100644 --- a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md +++ b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md @@ -36,6 +36,6 @@ Falling back to the VS Code Node runtime stays rejected — ADR 0001's load-bear - There is no cold path any more, so formatting is briefly unavailable after activation, after a restart and after a config change, while that folder's server starts. A format requested before the client has registered the server's capability finds no formatter for the document; nothing falls back to a fresh process. - The server advertises document formatting only: no range or selection formatting, no format-on-type, no diagnostics. It also ignores the editor's `FormattingOptions` (tab size, spaces) and the client's language id — the file path picks the parser and the project's config decides the style. An editor setting that disagrees with the project config loses, which is the same answer `rs fmt` gives in a terminal. -- Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server. +- Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server, and the stack's single status report is the folder set folded by severity (`crashed` > `version mismatch` > `disabled` > `running`), so a healthy sibling starting or recovering never overwrites another folder's failure. - An `rstack.config.*` create, change or delete restarts the server of the folder that contains it, and only that one; a detection change reconciles the folder set and leaves already-running servers alone. Both are deliberate: a healthy server's cached config is worth keeping. - The extension now holds one long-lived Node process per detected folder for fmt. Each is owned by the same process owner the lint client uses, so a stop is bounded (SIGTERM, then SIGKILL) and the automatic restart vscode-languageclient performs cannot leave an orphan behind. diff --git a/packages/vscode/src/stacks/fmt/index.ts b/packages/vscode/src/stacks/fmt/index.ts index 7bf455a..f7dd0aa 100644 --- a/packages/vscode/src/stacks/fmt/index.ts +++ b/packages/vscode/src/stacks/fmt/index.ts @@ -21,7 +21,10 @@ import { findPackageJsonUncached, readPackageJson, } from '../../shared/packageResolve'; -import { reportVersionCheck } from '../../shared/versionCheck'; +import { + checkPackageVersion, + formatVersionMismatch, +} from '../../shared/versionCheck'; import type { DetectionSnapshot, StackContext, @@ -148,6 +151,14 @@ class FmtLanguageClient extends LanguageClient { */ class FmtFolderRuntime { #state: FmtRuntimeState = 'stopped'; + /** The user-facing sentence behind a `disabled`/`version-mismatch`/`crashed` state. */ + #detail = ''; + /** + * A non-gating warning while the runtime keeps running — today only the + * configured-pin-below-floor verdict. Separate from `#detail` because it + * coexists with `running`. + */ + #advisory: string | undefined; #owner: LanguageServerProcessOwner | undefined; #client: LanguageClient | undefined; #defaultErrorHandler: ErrorHandler | undefined; @@ -163,18 +174,37 @@ class FmtFolderRuntime { constructor( private readonly folder: vscode.WorkspaceFolder, private readonly context: StackContext, - /** Called after a successful start so the shell's status says `running`. */ - private readonly onRunning: () => void, + /** + * Called on every state or advisory change. The runtime never reports to + * the shell itself: in a multi-root window one folder's `running` would + * overwrite a sibling folder's failure, so the controller aggregates all + * folder states into the stack's one status report. + */ + private readonly onDidChangeState: () => void, ) {} get state(): FmtRuntimeState { return this.#state; } + get statusDetail(): string { + return this.#detail; + } + + get advisory(): string | undefined { + return this.#advisory; + } + get folderPath(): string { return this.folder.uri.fsPath; } + private setState(state: FmtRuntimeState, detail = ''): void { + this.#state = state; + this.#detail = detail; + this.onDidChangeState(); + } + start(): Promise { return this.enqueue(async () => { if (this.#disposed) { @@ -221,21 +251,20 @@ class FmtFolderRuntime { /** * Package resolution, version check, Node selection and client start, in that - * order. Every failure short of a genuine launch failure is a status: the - * stack owns no UI chrome, and a project without `rstack` installed is not a - * crash. + * order. Every failure short of a genuine launch failure is a state the + * controller reports: the stack owns no UI chrome, and a project without + * `rstack` installed is not a crash. */ private async startImpl(): Promise { const context = this.context; const folderRoot = this.folder.uri.fsPath; this.#closing = false; - this.#state = 'starting'; + this.setState('starting'); const pkgJsonPath = findPackageJsonUncached('rstack', folderRoot); if (!pkgJsonPath) { const reason = `rstack is not installed in ${this.folder.name} (node_modules missing)`; - this.#state = 'disabled'; - context.status.report({ kind: 'disabled', reason }); + this.setState('disabled', reason); context.output.warn(`${reason}; searched from ${folderRoot}`); return; } @@ -244,8 +273,12 @@ class FmtFolderRuntime { // from disk by design, so a reinstall is picked up on the next start. const pkg = readPackageJson(pkgJsonPath); const version = typeof pkg?.version === 'string' ? pkg.version : undefined; - if (!reportVersionCheck(context.status, 'rstack', version)) { - this.#state = 'version-mismatch'; + const versionCheck = checkPackageVersion('rstack', version); + if (versionCheck.kind === 'mismatch') { + this.setState( + 'version-mismatch', + `${formatVersionMismatch('rstack', versionCheck)} (resolved in ${this.folder.name})`, + ); return; } const rsBinJs = path.resolve( @@ -283,16 +316,17 @@ class FmtFolderRuntime { if (event.newState === State.Stopped) { // Whether a restart follows is the error handler's and the process // owner's call; either way this folder is currently not formatting. - this.#state = 'crashed'; - context.status.crashed('the rs fmt language server stopped'); + this.setState( + 'crashed', + `the rs fmt language server for ${this.folder.name} stopped`, + ); } else if (event.newState === State.Running) { // The one writer for `running`. It fires on the first start // (synchronously, before `client.start()` resolves) and again when // vscode-languageclient's error handler restarts a crashed server — // the way back out of `crashed`, the same transition the lint stack's // state watcher makes. - this.#state = 'running'; - this.onRunning(); + this.setState('running'); } }); @@ -306,11 +340,11 @@ class FmtFolderRuntime { // the client reaches `State.Running` before this await resolves. await client.start(); } catch (error) { - // Teardown first: `stopImpl` ends in `#state = 'stopped'`, so the + // Teardown first: `stopImpl` ends in the `stopped` state, so the // `crashed` verdict has to be written after it, not raced against it. await this.stopImpl(); - this.#state = 'crashed'; - context.status.crashed( + this.setState( + 'crashed', `the rs fmt language server for ${this.folder.name} failed to start: ${ error instanceof Error ? error.message : String(error) }`, @@ -338,7 +372,8 @@ class FmtFolderRuntime { // still probed, advisory-only, off the start path. void configuredNodeBelowFloor(configured).then((message) => { if (message !== undefined && !this.#disposed) { - context.status.versionMismatch(message); + this.#advisory = message; + this.onDidChangeState(); } }); return configured; @@ -354,8 +389,8 @@ class FmtFolderRuntime { return resolution.executable; } catch (error) { if (error instanceof NodePreflightError) { - this.#state = 'version-mismatch'; - context.status.versionMismatch( + this.setState( + 'version-mismatch', error.messageWith('rs fmt will not format'), ); return undefined; @@ -433,7 +468,10 @@ class FmtFolderRuntime { error, ); } - this.#state = 'stopped'; + // A stop retires this start's advisory with it: a restart re-probes the + // (memoized) pin verdict, and a changed pin rebuilds the controller anyway. + this.#advisory = undefined; + this.setState('stopped'); } } @@ -502,14 +540,14 @@ class FmtController implements StackController { this.reconcile(); // The shell's reconcile leaves a still-detected controller alone, so // the running reason must follow the new snapshot here. - this.reportRunning(); + this.reportStatus(); }), configWatcher, configWatcher.onDidCreate(onConfigEvent), configWatcher.onDidChange(onConfigEvent), configWatcher.onDidDelete(onConfigEvent), ); - this.reportRunning(); + this.reportStatus(); // Starting a server spawns a process; `register()` must return fast, so the // folder set is reconciled without awaiting any start. this.reconcile(); @@ -568,20 +606,59 @@ class FmtController implements StackController { // after a detection change reports from the freshest snapshot — and the // closure captures nothing beyond `this`. const runtime = new FmtFolderRuntime(folder, context, () => - this.reportRunning(), + this.reportStatus(), ); this.#runtimes.set(folderPath, runtime); void runtime.start(); } } - /** `running` always carries the reason the stack is on: where it was detected. */ - private reportRunning(): void { + /** + * The one writer to the shell's status: the whole folder set folded into a + * single report, most severe folder first. Folding is what keeps a healthy + * sibling from overwriting another folder's failure — with per-runtime + * reporting the displayed state was whichever folder spoke last. + * + * `running` always carries the reason the stack is on: where it was detected. + */ + private reportStatus(): void { const context = this.#context; const snapshot = this.#snapshot; if (!context || !snapshot || this.#disposed) { return; } + const runtimes = [...this.#runtimes.values()]; + const firstIn = (state: FmtRuntimeState): FmtFolderRuntime | undefined => + runtimes.find((runtime) => runtime.state === state); + + const crashed = firstIn('crashed'); + if (crashed) { + context.status.crashed(crashed.statusDetail); + return; + } + const mismatch = firstIn('version-mismatch'); + if (mismatch) { + context.status.versionMismatch(mismatch.statusDetail); + return; + } + const disabled = firstIn('disabled'); + if (disabled) { + context.status.report({ + kind: 'disabled', + reason: disabled.statusDetail, + }); + return; + } + // Below the failures because it is not one: the pinned Node is below the + // floor but the server runs with it anyway (same verdict, same rank as the + // test stack's configured-pin advisory). + const advisory = runtimes + .map((runtime) => runtime.advisory) + .find((message) => message !== undefined); + if (advisory !== undefined) { + context.status.versionMismatch(advisory); + return; + } const names = snapshot.foldersFor('fmt').map((entry) => entry.folder.name); if (names.length === 0) { // Nothing detected means the shell is about to retire this controller; From fcd4ad388e5ab22b06dfe0d057222784ffc3bc4e Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 13 Aug 2026 16:47:41 +0800 Subject: [PATCH 03/12] refactor(vscode): extract the fmt status fold into a testable pure module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /simplify pass over the aggregation fix, converging with review feedback: - stacks/fmt/status.ts: foldFolderStatus() owns severity (an exhaustive rank table, so a new state cannot silently fall through to running), multi-root folder-name prefixes with tie joining, and the healthy-sibling-never-masks-a-failure invariant — now unit-tested, which the single-folder E2E fixture structurally cannot cover. - The status reports starting, not running, until a folder's server is actually up (during Node preflight and server initialization no formatting provider exists yet); stopped ranks with starting, so a config-change restart no longer flashes running. - A running folder's pin advisory folds at version-mismatch rank, above disabled — matching the test stack's configured-pin advisory instead of contradicting it. - Runtime details are folder-agnostic; the fold owns the prefixing, so the Node-preflight message is attributable in a multi-root window too. - setAdvisory() joins setState() as the only notify paths. --- docs/adr/0002-fmt-lsp-on-user-node-runtime.md | 2 +- packages/vscode/AGENTS.md | 2 +- packages/vscode/src/stacks/fmt/index.ts | 133 ++++++----------- packages/vscode/src/stacks/fmt/status.ts | 127 +++++++++++++++++ .../vscode/tests/stacks/fmt/status.test.ts | 134 ++++++++++++++++++ 5 files changed, 310 insertions(+), 88 deletions(-) create mode 100644 packages/vscode/src/stacks/fmt/status.ts create mode 100644 packages/vscode/tests/stacks/fmt/status.test.ts diff --git a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md index f062fd3..d8e6e7d 100644 --- a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md +++ b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md @@ -36,6 +36,6 @@ Falling back to the VS Code Node runtime stays rejected — ADR 0001's load-bear - There is no cold path any more, so formatting is briefly unavailable after activation, after a restart and after a config change, while that folder's server starts. A format requested before the client has registered the server's capability finds no formatter for the document; nothing falls back to a fresh process. - The server advertises document formatting only: no range or selection formatting, no format-on-type, no diagnostics. It also ignores the editor's `FormattingOptions` (tab size, spaces) and the client's language id — the file path picks the parser and the project's config decides the style. An editor setting that disagrees with the project config loses, which is the same answer `rs fmt` gives in a terminal. -- Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server, and the stack's single status report is the folder set folded by severity (`crashed` > `version mismatch` > `disabled` > `running`), so a healthy sibling starting or recovering never overwrites another folder's failure. +- Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server, and the stack's single status report is the folder set folded by severity (`crashed` > `version mismatch`, which is also where a running folder's pin advisory ranks > `disabled` > `starting` > `running`; the pure fold lives in `stacks/fmt/status.ts`), so a healthy sibling starting or recovering never overwrites another folder's failure — and the status says `starting`, not `running`, until a server actually formats. - An `rstack.config.*` create, change or delete restarts the server of the folder that contains it, and only that one; a detection change reconciles the folder set and leaves already-running servers alone. Both are deliberate: a healthy server's cached config is worth keeping. - The extension now holds one long-lived Node process per detected folder for fmt. Each is owned by the same process owner the lint client uses, so a stop is bounded (SIGTERM, then SIGKILL) and the automatic restart vscode-languageclient performs cannot leave an orphan behind. diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index f9506c7..61246b0 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -36,7 +36,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - fmt importing `stacks/lint/LanguageServerProcessOwner.ts` is not a refactor across the copies: that file has no lint imports and no lint behaviour, it only owns the native children of one language client — including the ones vscode-languageclient's automatic restart creates, which is exactly the leak an ad-hoc copy would reintroduce. Lint's `ManagedLanguageClient` is _restated_ in `stacks/fmt/index.ts` instead, because importing it from `Rslint.ts` would drag the lint runtime graph (plugin pool, config loader, rstack bridge) into the fmt stack. Keep that line where it is: shared process ownership yes, shared stack runtime no. - `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. -- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. `stacks/fmt/binEntry.ts` is a one-function module for the same testability reason: `stacks/fmt/index.ts` evaluates `vscode` and `vscode-languageclient`, so a pure helper left in it is not unit-testable. +- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. `stacks/fmt/binEntry.ts` and `stacks/fmt/status.ts` are separate pure modules for the same testability reason: `stacks/fmt/index.ts` evaluates `vscode` and `vscode-languageclient`, so a pure helper left in it is not unit-testable — and `status.ts` carries the invariant the fold exists for (a healthy sibling folder never masks another folder's failure), which only a unit test can pin, since the E2E fixtures run a single fmt folder. - The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`), because the strictest thing any of these processes does — the test worker through rstack's shim, the `rs fmt` server through `@rstackjs/load-config` — is load an `rstack.config.*` with `loader: 'native'`, no jiti fallback, and so needs native type stripping (22.18+; on the 23.x line only from 23.6). 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. diff --git a/packages/vscode/src/stacks/fmt/index.ts b/packages/vscode/src/stacks/fmt/index.ts index f7dd0aa..ce29319 100644 --- a/packages/vscode/src/stacks/fmt/index.ts +++ b/packages/vscode/src/stacks/fmt/index.ts @@ -37,6 +37,11 @@ import type { // no lint behaviour is shared, and the file has no lint imports. import { LanguageServerProcessOwner } from '../lint/LanguageServerProcessOwner'; import { pickBinEntry } from './binEntry'; +import { + foldFolderStatus, + type FmtFolderStatus, + type FmtRuntimeState, +} from './status'; // prettier 3.9.6 getSupportInfo() vscodeLanguageIds snapshot (rs fmt's pinned // prettier). Revisit when the pinned prettier changes. @@ -102,21 +107,6 @@ const contains = (dir: string, filePath: string): boolean => { ); }; -/** - * A folder runtime's state, as the E2E exports report it. - * - * `disabled`, `version-mismatch` and `crashed` mirror the status the runtime - * pushed to the shell when it stopped short; `stopped` is a runtime that was - * never started or was shut down deliberately. - */ -type FmtRuntimeState = - | 'stopped' - | 'starting' - | 'running' - | 'disabled' - | 'version-mismatch' - | 'crashed'; - /** * vscode-languageclient calls `stop()` without observing its promise when an * initialize request fails, and its base `stop` rejects for non-Running states. @@ -151,13 +141,9 @@ class FmtLanguageClient extends LanguageClient { */ class FmtFolderRuntime { #state: FmtRuntimeState = 'stopped'; - /** The user-facing sentence behind a `disabled`/`version-mismatch`/`crashed` state. */ + /** See {@link FmtFolderStatus.detail} — folder-agnostic, the fold prefixes. */ #detail = ''; - /** - * A non-gating warning while the runtime keeps running — today only the - * configured-pin-below-floor verdict. Separate from `#detail` because it - * coexists with `running`. - */ + /** See {@link FmtFolderStatus.advisory}. */ #advisory: string | undefined; #owner: LanguageServerProcessOwner | undefined; #client: LanguageClient | undefined; @@ -175,24 +161,25 @@ class FmtFolderRuntime { private readonly folder: vscode.WorkspaceFolder, private readonly context: StackContext, /** - * Called on every state or advisory change. The runtime never reports to - * the shell itself: in a multi-root window one folder's `running` would - * overwrite a sibling folder's failure, so the controller aggregates all - * folder states into the stack's one status report. + * Called on every {@link folderStatus} change. The runtime never reports + * to the shell itself — the controller folds all folder statuses into the + * stack's one report (`foldFolderStatus`). */ - private readonly onDidChangeState: () => void, + private readonly onDidChangeStatus: () => void, ) {} get state(): FmtRuntimeState { return this.#state; } - get statusDetail(): string { - return this.#detail; - } - - get advisory(): string | undefined { - return this.#advisory; + /** This folder's contribution to the stack status (see `status.ts`). */ + get folderStatus(): FmtFolderStatus { + return { + name: this.folder.name, + state: this.#state, + detail: this.#detail, + advisory: this.#advisory, + }; } get folderPath(): string { @@ -202,7 +189,12 @@ class FmtFolderRuntime { private setState(state: FmtRuntimeState, detail = ''): void { this.#state = state; this.#detail = detail; - this.onDidChangeState(); + this.onDidChangeStatus(); + } + + private setAdvisory(message: string): void { + this.#advisory = message; + this.onDidChangeStatus(); } start(): Promise { @@ -263,9 +255,13 @@ class FmtFolderRuntime { const pkgJsonPath = findPackageJsonUncached('rstack', folderRoot); if (!pkgJsonPath) { - const reason = `rstack is not installed in ${this.folder.name} (node_modules missing)`; - this.setState('disabled', reason); - context.output.warn(`${reason}; searched from ${folderRoot}`); + this.setState( + 'disabled', + 'rstack is not installed (node_modules missing)', + ); + context.output.warn( + `rstack is not installed in ${this.folder.name} (node_modules missing); searched from ${folderRoot}`, + ); return; } @@ -277,7 +273,7 @@ class FmtFolderRuntime { if (versionCheck.kind === 'mismatch') { this.setState( 'version-mismatch', - `${formatVersionMismatch('rstack', versionCheck)} (resolved in ${this.folder.name})`, + formatVersionMismatch('rstack', versionCheck), ); return; } @@ -316,10 +312,7 @@ class FmtFolderRuntime { if (event.newState === State.Stopped) { // Whether a restart follows is the error handler's and the process // owner's call; either way this folder is currently not formatting. - this.setState( - 'crashed', - `the rs fmt language server for ${this.folder.name} stopped`, - ); + this.setState('crashed', 'the rs fmt language server stopped'); } else if (event.newState === State.Running) { // The one writer for `running`. It fires on the first start // (synchronously, before `client.start()` resolves) and again when @@ -345,7 +338,7 @@ class FmtFolderRuntime { await this.stopImpl(); this.setState( 'crashed', - `the rs fmt language server for ${this.folder.name} failed to start: ${ + `the rs fmt language server failed to start: ${ error instanceof Error ? error.message : String(error) }`, ); @@ -372,8 +365,7 @@ class FmtFolderRuntime { // still probed, advisory-only, off the start path. void configuredNodeBelowFloor(configured).then((message) => { if (message !== undefined && !this.#disposed) { - this.#advisory = message; - this.onDidChangeState(); + this.setAdvisory(message); } }); return configured; @@ -615,11 +607,9 @@ class FmtController implements StackController { /** * The one writer to the shell's status: the whole folder set folded into a - * single report, most severe folder first. Folding is what keeps a healthy - * sibling from overwriting another folder's failure — with per-runtime - * reporting the displayed state was whichever folder spoke last. - * - * `running` always carries the reason the stack is on: where it was detected. + * single report (`foldFolderStatus` — severity, multi-root prefixes and the + * healthy-sibling-never-masks-a-failure invariant all live there, where they + * are unit-testable). */ private reportStatus(): void { const context = this.#context; @@ -627,48 +617,19 @@ class FmtController implements StackController { if (!context || !snapshot || this.#disposed) { return; } - const runtimes = [...this.#runtimes.values()]; - const firstIn = (state: FmtRuntimeState): FmtFolderRuntime | undefined => - runtimes.find((runtime) => runtime.state === state); - - const crashed = firstIn('crashed'); - if (crashed) { - context.status.crashed(crashed.statusDetail); - return; - } - const mismatch = firstIn('version-mismatch'); - if (mismatch) { - context.status.versionMismatch(mismatch.statusDetail); - return; - } - const disabled = firstIn('disabled'); - if (disabled) { - context.status.report({ - kind: 'disabled', - reason: disabled.statusDetail, - }); - return; - } - // Below the failures because it is not one: the pinned Node is below the - // floor but the server runs with it anyway (same verdict, same rank as the - // test stack's configured-pin advisory). - const advisory = runtimes - .map((runtime) => runtime.advisory) - .find((message) => message !== undefined); - if (advisory !== undefined) { - context.status.versionMismatch(advisory); - return; - } const names = snapshot.foldersFor('fmt').map((entry) => entry.folder.name); if (names.length === 0) { // Nothing detected means the shell is about to retire this controller; - // its gate state, not `running`, is the truthful report. + // its gate state, not a fold over runtimes, is the truthful report. return; } - context.status.running( - names.length <= 3 - ? `detected in ${names.join(', ')}` - : `detected in ${names.length} folders`, + context.status.report( + foldFolderStatus( + [...this.#runtimes.values()].map((runtime) => runtime.folderStatus), + names.length <= 3 + ? `detected in ${names.join(', ')}` + : `detected in ${names.length} folders`, + ), ); } diff --git a/packages/vscode/src/stacks/fmt/status.ts b/packages/vscode/src/stacks/fmt/status.ts new file mode 100644 index 0000000..7751a59 --- /dev/null +++ b/packages/vscode/src/stacks/fmt/status.ts @@ -0,0 +1,127 @@ +import type { StackState } from '../../types'; + +/** + * A folder runtime's lifecycle state, as the E2E exports report it. + * + * `disabled`, `version-mismatch` and `crashed` are the states the fold below + * surfaces to the shell; `stopped` is a runtime that was never started or was + * shut down deliberately (mid-restart, mid-teardown). + */ +export type FmtRuntimeState = + | 'stopped' + | 'starting' + | 'running' + | 'disabled' + | 'version-mismatch' + | 'crashed'; + +/** One folder runtime's contribution to the stack's single status report. */ +export interface FmtFolderStatus { + readonly name: string; + readonly state: FmtRuntimeState; + /** + * The user-facing sentence behind a `disabled`/`version-mismatch`/`crashed` + * state. Folder-agnostic on purpose: the fold prefixes the folder name when + * the window has more than one folder, so no message has to guess where it + * will be displayed. + */ + readonly detail: string; + /** + * A non-gating warning while the folder keeps formatting — today only the + * configured-pin-below-floor verdict. Folded at `version-mismatch` rank, + * like the test stack's configured-pin advisory, because the fix it asks + * for is the same. + */ + readonly advisory: string | undefined; +} + +/** + * Severity of each runtime state, worst first. A `Record` over the full union + * on purpose: a new state cannot be added without ranking itself, so nothing + * silently falls through to `running` — which is the exact bug class the fold + * exists to prevent (`statusBar.ts` keeps its severity table for the same + * reason). + * + * `disabled` outranks `running` — deliberately the opposite of the shell's + * and lint's tables, where `disabled` is a kill switch the user flipped. Here + * it means "this folder has no `rstack` installed and will never format", a + * fact worth showing over a healthy sibling. `stopped` ranks with `starting`: + * both are "no server right now, none of it an error" (a restart passes + * through `stopped` on its way back up). + */ +const STATE_RANK: Readonly> = { + crashed: 5, + 'version-mismatch': 4, + disabled: 3, + starting: 2, + stopped: 2, + running: 1, +}; + +/** + * Folds every folder runtime's state into the one report the shell shows for + * the fmt stack. The worst folder wins, and with multiple folders the detail + * names the folders it came from — "crashed" without a folder name is not + * actionable. A healthy sibling starting or recovering can therefore never + * overwrite another folder's failure: it only changes its own contribution. + * + * No folder `running` yet — including the moment before any runtime exists — + * reports `starting`, not `running`: during Node preflight and server + * initialization no formatting provider is registered, and the status must + * not claim otherwise. + * + * `runningDetail` is the reason the stack is on (where it was detected), + * reported when every folder runs clean. + */ +export const foldFolderStatus = ( + statuses: readonly FmtFolderStatus[], + runningDetail: string, +): StackState => { + // A running folder carrying an advisory competes as a `version-mismatch`: + // the server formats, but the status has a warning to show. + const effective = statuses.map((status) => + status.state === 'running' && status.advisory !== undefined + ? { + name: status.name, + state: 'version-mismatch' as const, + detail: status.advisory, + } + : status, + ); + // No runtimes yet (the moment between registration and the first + // reconcile) is the same truth as every runtime still starting. + let worst: FmtRuntimeState = effective[0]?.state ?? 'starting'; + for (const { state } of effective) { + if (STATE_RANK[state] > STATE_RANK[worst]) { + worst = state; + } + } + + const multiRoot = statuses.length > 1; + const details = effective + .filter((entry) => entry.state === worst && entry.detail !== '') + .map((entry) => + multiRoot ? `${entry.name}: ${entry.detail}` : entry.detail, + ); + const detail = details.length > 0 ? details.join(' | ') : undefined; + + switch (worst) { + case 'crashed': + return { + kind: 'crashed', + detail: detail ?? 'the rs fmt language server stopped', + }; + case 'version-mismatch': + return { + kind: 'version-mismatch', + detail: detail ?? 'unsupported rstack version', + }; + case 'disabled': + return { kind: 'disabled', reason: detail }; + case 'starting': + case 'stopped': + return { kind: 'starting', detail: runningDetail }; + case 'running': + return { kind: 'running', detail: runningDetail }; + } +}; diff --git a/packages/vscode/tests/stacks/fmt/status.test.ts b/packages/vscode/tests/stacks/fmt/status.test.ts new file mode 100644 index 0000000..e2b750a --- /dev/null +++ b/packages/vscode/tests/stacks/fmt/status.test.ts @@ -0,0 +1,134 @@ +import { describe, expect, it } from '@rstest/core'; +import { + foldFolderStatus, + type FmtFolderStatus, + type FmtRuntimeState, +} from '../../../src/stacks/fmt/status'; + +const folder = ( + name: string, + state: FmtRuntimeState, + overrides: Partial> = {}, +): FmtFolderStatus => ({ + name, + state, + detail: overrides.detail ?? '', + advisory: overrides.advisory, +}); + +const DETECTED = 'detected in app'; + +describe('foldFolderStatus', () => { + it('reports starting before any runtime exists', () => { + // The moment between registration and the first reconcile: folders are + // detected, no server has been scheduled yet — the formatter is not + // available and the status must not claim it is. + expect(foldFolderStatus([], DETECTED)).toEqual({ + kind: 'starting', + detail: DETECTED, + }); + }); + + it('reports starting until every folder is running', () => { + expect( + foldFolderStatus( + [folder('a', 'running'), folder('b', 'starting')], + DETECTED, + ).kind, + ).toBe('starting'); + // A restart passes through `stopped` on its way back up — same truth. + expect( + foldFolderStatus( + [folder('a', 'running'), folder('b', 'stopped')], + DETECTED, + ).kind, + ).toBe('starting'); + }); + + it('reports running with the detection reason when every folder runs clean', () => { + expect( + foldFolderStatus( + [folder('a', 'running'), folder('b', 'running')], + DETECTED, + ), + ).toEqual({ kind: 'running', detail: DETECTED }); + }); + + it("never lets a healthy sibling mask another folder's failure", () => { + // The invariant this module exists for: the fold's answer must not depend + // on which folder reported last, so a failure wins from either side. + const crashed = folder('bad', 'crashed', { detail: 'server stopped' }); + const running = folder('good', 'running'); + for (const statuses of [ + [crashed, running], + [running, crashed], + ]) { + expect(foldFolderStatus(statuses, DETECTED)).toEqual({ + kind: 'crashed', + detail: 'bad: server stopped', + }); + } + }); + + it('ranks crashed over version-mismatch over disabled', () => { + const statuses = [ + folder('a', 'disabled', { detail: 'rstack is not installed' }), + folder('b', 'crashed', { detail: 'server stopped' }), + folder('c', 'version-mismatch', { detail: 'rstack 0.4.0 unsupported' }), + ]; + expect(foldFolderStatus(statuses, DETECTED).kind).toBe('crashed'); + expect(foldFolderStatus(statuses.slice(0, 1), DETECTED)).toEqual({ + kind: 'disabled', + reason: 'rstack is not installed', + }); + }); + + it('shows a disabled folder over a running sibling', () => { + // Unlike the shell's kill-switch `disabled`, this one means "this folder + // will never format" — worth showing over a healthy sibling. + expect( + foldFolderStatus( + [ + folder('good', 'running'), + folder('bare', 'disabled', { detail: 'rstack is not installed' }), + ], + DETECTED, + ), + ).toEqual({ kind: 'disabled', reason: 'bare: rstack is not installed' }); + }); + + it('prefixes folder names only in a multi-root window, joining ties', () => { + const detail = 'server stopped'; + expect( + foldFolderStatus([folder('only', 'crashed', { detail })], DETECTED), + ).toEqual({ kind: 'crashed', detail }); + expect( + foldFolderStatus( + [ + folder('a', 'crashed', { detail }), + folder('b', 'crashed', { detail }), + ], + DETECTED, + ), + ).toEqual({ + kind: 'crashed', + detail: 'a: server stopped | b: server stopped', + }); + }); + + it('folds a running folder with a pin advisory as a version-mismatch', () => { + // The server formats — the state stays `running` for the E2E exports — + // but the status carries the warning, at the same rank as the test + // stack's configured-pin advisory: above `disabled`, below `crashed`. + const advisory = 'node 20.19.4 is below the floor; using it anyway'; + expect( + foldFolderStatus( + [ + folder('pinned', 'running', { advisory }), + folder('bare', 'disabled', { detail: 'rstack is not installed' }), + ], + DETECTED, + ), + ).toEqual({ kind: 'version-mismatch', detail: `pinned: ${advisory}` }); + }); +}); From c15ec6522558c9a1fb6eff64cb34bfde1273be4b Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 13 Aug 2026 17:05:49 +0800 Subject: [PATCH 04/12] fix(vscode): make fmt teardown immune to a hung initialize and detection flaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two lifecycle holes in the per-folder fmt runtime, from review: - A server that spawned but never answers the LSP initialize request held the runtime's serialized queue inside client.start(), so a queued stop or restart never reached the process teardown — a hung shell restart and a live orphan. stop()/restart() now interrupt an in-flight start by closing the process owner outside the queue, which fails the pending initialize and lets the queue drain. The interrupt window is exact (#startInFlight spans only the client.start() await): a healthy server keeps its graceful LSP shutdown, and a manufactured failure ends in 'stopped', not a spurious 'crashed'. - A folder that lost and regained detection across two passes could run two servers at once: reconcile deleted the map entry and stopped the runtime asynchronously, and the replacement spawned immediately. The controller now reserves the folder (#retiring, identity-guarded) and the replacement start awaits the predecessor's retirement inside its own queue, so a config event during the wait lines up behind it instead of spawning early. dispose() drains retiring runtimes too. --- packages/vscode/src/stacks/fmt/index.ts | 81 +++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 4 deletions(-) diff --git a/packages/vscode/src/stacks/fmt/index.ts b/packages/vscode/src/stacks/fmt/index.ts index ce29319..f661cc8 100644 --- a/packages/vscode/src/stacks/fmt/index.ts +++ b/packages/vscode/src/stacks/fmt/index.ts @@ -151,6 +151,8 @@ class FmtFolderRuntime { #stateWatcher: vscode.Disposable | undefined; #closing = false; #disposed = false; + /** True only across `startImpl`'s `client.start()` await — the window `interruptInFlightStart` exists for. */ + #startInFlight = false; /** * Every lifecycle transition of one folder runs here, so a config event * arriving mid-start cannot interleave a second start with the first. @@ -197,8 +199,16 @@ class FmtFolderRuntime { this.onDidChangeStatus(); } - start(): Promise { + /** + * `waitFor` is the previous runtime's retirement (see the controller's + * `#retiring`): awaited *inside* the queue, so a config-event `restart()` + * arriving during the wait lines up behind it instead of spawning early. + */ + start(waitFor?: Promise): Promise { return this.enqueue(async () => { + if (waitFor) { + await waitFor; + } if (this.#disposed) { return; } @@ -208,6 +218,7 @@ class FmtFolderRuntime { /** A config change invalidates the server's cached config; only a new process clears it. */ restart(reason: string): Promise { + this.interruptInFlightStart(); return this.enqueue(async () => { if (this.#disposed) { return; @@ -225,11 +236,39 @@ class FmtFolderRuntime { stop(): Promise { this.#disposed = true; + this.interruptInFlightStart(); return this.enqueue(async () => { await this.stopImpl(); }); } + /** + * `startImpl` holds the queue while awaiting `client.start()`, and a server + * that spawned but never answers the LSP `initialize` request would hold it + * forever — the queued stop or restart behind it could then never reach the + * process teardown. Closing the owner outside the queue kills the child, + * which fails the pending initialize and lets the queue drain into whatever + * was queued. + * + * A genuine no-op outside that window (`#startInFlight` is true only across + * the `client.start()` await), so an ordinary restart or stop of a healthy + * server keeps its graceful LSP shutdown instead of a kill. Clearing the + * flag is also what tells `startImpl`'s catch that the failure was + * manufactured. (The pre-owner phase — package resolution, Node preflight — + * is not interruptible, but its probes are themselves timeout-bounded.) + */ + private interruptInFlightStart(): void { + const owner = this.#owner; + if (!owner || !this.#startInFlight) { + return; + } + this.#startInFlight = false; + this.#closing = true; + void owner.close().catch(() => { + // `stopImpl` reports close failures; this early kick only unblocks. + }); + } + private enqueue(operation: () => Promise): Promise { const next = this.#queue.then(operation, operation); this.#queue = next.catch((error: unknown) => { @@ -331,11 +370,22 @@ class FmtFolderRuntime { // `documentFormattingProvider` capability — the stack registers none of // its own. The state watcher above is what records the successful start: // the client reaches `State.Running` before this await resolves. + this.#startInFlight = true; await client.start(); + this.#startInFlight = false; } catch (error) { + // A cleared flag means the failure was manufactured by + // `interruptInFlightStart` — a stop or restart arrived mid-initialize — + // and the teardown below ends in the truthful state, so no `crashed` + // verdict and no error log for it. + const interrupted = !this.#startInFlight; + this.#startInFlight = false; // Teardown first: `stopImpl` ends in the `stopped` state, so the // `crashed` verdict has to be written after it, not raced against it. await this.stopImpl(); + if (interrupted || this.#disposed) { + return; + } this.setState( 'crashed', `the rs fmt language server failed to start: ${ @@ -489,6 +539,11 @@ class FmtController implements StackController { /** The newest detection result, which `running` reports the reason from. */ #snapshot: DetectionSnapshot | undefined; readonly #runtimes = new Map(); + /** + * Folders whose previous runtime is still shutting down (see `reconcile`); + * a replacement runtime waits for this promise before spawning its server. + */ + readonly #retiring = new Map>(); /** Per-folder config-event debounce (see `register`'s `onConfigEvent`). */ readonly #restartTimers = new Map(); readonly #subscriptions: vscode.Disposable[] = []; @@ -587,7 +642,20 @@ class FmtController implements StackController { for (const [folderPath, runtime] of [...this.#runtimes]) { if (!detected.has(folderPath)) { this.#runtimes.delete(folderPath); - void runtime.stop(); + // Reserve the folder until the asynchronous stop settles: a folder + // that loses and regains detection across two passes must not get a + // second server while the retiring one still owns its process (and + // its formatting provider). The stop is bounded: a hung initialize is + // interrupted, the pre-owner probes carry their own timeouts, and the + // owner escalates SIGTERM to SIGKILL. The delete is identity-guarded + // so an earlier retirement settling late cannot drop a successor's + // reservation. + const retirement = runtime.stop().finally(() => { + if (this.#retiring.get(folderPath) === retirement) { + this.#retiring.delete(folderPath); + } + }); + this.#retiring.set(folderPath, retirement); } } for (const [folderPath, folder] of detected) { @@ -601,7 +669,7 @@ class FmtController implements StackController { this.reportStatus(), ); this.#runtimes.set(folderPath, runtime); - void runtime.start(); + void runtime.start(this.#retiring.get(folderPath)); } } @@ -649,7 +717,12 @@ class FmtController implements StackController { } const runtimes = [...this.#runtimes.values()]; this.#runtimes.clear(); - await Promise.allSettled(runtimes.map(async (runtime) => runtime.stop())); + await Promise.allSettled([ + ...runtimes.map(async (runtime) => runtime.stop()), + // Runtimes already retiring out of `reconcile` are stopping too — the + // no-orphan guarantee covers them as much as the live set. + ...this.#retiring.values(), + ]); // The User Node preflight memo is deliberately not reset here: it is // host-scoped state shared with the rstest stack, and the shell's restart // pass owns the reset (see `runRestart`). From 75b72e68625fe11553dacff4897415237c4c3cee Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 13 Aug 2026 17:39:43 +0800 Subject: [PATCH 05/12] fix(vscode): scope the Node-memo reset to full restarts and log e2e failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round three, plus the CI diagnosis groundwork: - runRestart resets the host-scoped User Node preflight memo only when no consumer stack (rstest, fmt) survives the retire wave. A single-stack rstack.fmt.restart next to a live Rstest controller kept clearing the decision its existing workers were built on, so the next worker spawn could silently re-probe onto a different runtime. The full rstack.restart — the 'like a window reload' gesture — still always clears it, as does the batched restart a rstack.nodeExecutable change triggers. Two unit tests pin both sides. - Nested workspace folders (parent and subdirectory both detected for fmt) are recorded as a documented limitation in AGENTS.md and ADR 0002: the supported shape is sibling folders, and per-document routing was considered and deferred. - The rstest e2e suite names each failing test via console.error: the extension host's stdout (mocha's reporter) is not forwarded to CI logs, so a CI-only failure was previously unidentifiable. --- docs/adr/0002-fmt-lsp-on-user-node-runtime.md | 1 + packages/vscode/AGENTS.md | 4 +- packages/vscode/e2e/rstest/suite/index.ts | 15 +++++++- packages/vscode/src/extension.ts | 37 +++++++++++++++---- packages/vscode/tests/extension.test.ts | 34 +++++++++++++++++ 5 files changed, 81 insertions(+), 10 deletions(-) diff --git a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md index d8e6e7d..a8842d5 100644 --- a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md +++ b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md @@ -38,4 +38,5 @@ Falling back to the VS Code Node runtime stays rejected — ADR 0001's load-bear - The server advertises document formatting only: no range or selection formatting, no format-on-type, no diagnostics. It also ignores the editor's `FormattingOptions` (tab size, spaces) and the client's language id — the file path picks the parser and the project's config decides the style. An editor setting that disagrees with the project config loses, which is the same answer `rs fmt` gives in a terminal. - Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server, and the stack's single status report is the folder set folded by severity (`crashed` > `version mismatch`, which is also where a running folder's pin advisory ranks > `disabled` > `starting` > `running`; the pure fold lives in `stacks/fmt/status.ts`), so a healthy sibling starting or recovering never overwrites another folder's failure — and the status says `starting`, not `running`, until a server actually formats. - An `rstack.config.*` create, change or delete restarts the server of the folder that contains it, and only that one; a detection change reconciles the folder set and leaves already-running servers alone. Both are deliberate: a healthy server's cached config is worth keeping. +- "A subproject becomes its own workspace folder" means a _sibling_ folder (or opening only the subproject). Keeping the parent **and** the nested subdirectory as workspace folders with fmt detected in both is a documented limitation: the parent's selector also matches the nested files, and which of the two servers VS Code asks is not defined. Per-document routing to the deepest folder was considered (lint carries a `WorkspaceDocumentRouter` for exactly this) and deferred — complexity the scenario does not yet justify. - The extension now holds one long-lived Node process per detected folder for fmt. Each is owned by the same process owner the lint client uses, so a stop is bounded (SIGTERM, then SIGKILL) and the automatic restart vscode-languageclient performs cannot leave an orphan behind. diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 61246b0..6f2cbf2 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -32,11 +32,11 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The lint × `rstack.config.*` bridge was built and deliberately removed: a partial editor-side bridge gave wrong results, and a correct one needs upstream work first. `TODO(rstack-bridge)` markers carry the plan. Do not reintroduce a partial bridge. - The test × `rstack.config.*` bridge stays thin on purpose: it points the upstream machinery at rstack's shipped shim and lets the shim interpret the config inside the worker, same as the CLI. Never re-implement rstack config semantics in the extension. -- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. +- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. **Nested workspace folders are a documented limitation, by decision**: when a folder and its subdirectory are both workspace folders and both detect fmt, the parent's per-folder selector also matches the nested folder's files, and which server VS Code hands the request to is not defined — the supported shape is subprojects as _sibling_ workspace folders (or only the subproject opened), not parent-plus-child. Routing (lint's `WorkspaceDocumentRouter` shape, or a formatting middleware) was considered and deferred as complexity the scenario doesn't yet justify. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. - fmt importing `stacks/lint/LanguageServerProcessOwner.ts` is not a refactor across the copies: that file has no lint imports and no lint behaviour, it only owns the native children of one language client — including the ones vscode-languageclient's automatic restart creates, which is exactly the leak an ad-hoc copy would reintroduce. Lint's `ManagedLanguageClient` is _restated_ in `stacks/fmt/index.ts` instead, because importing it from `Rslint.ts` would drag the lint runtime graph (plugin pool, config loader, rstack bridge) into the fmt stack. Keep that line where it is: shared process ownership yes, shared stack runtime no. - `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. -- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. `stacks/fmt/binEntry.ts` and `stacks/fmt/status.ts` are separate pure modules for the same testability reason: `stacks/fmt/index.ts` evaluates `vscode` and `vscode-languageclient`, so a pure helper left in it is not unit-testable — and `status.ts` carries the invariant the fold exists for (a healthy sibling folder never masks another folder's failure), which only a unit test can pin, since the E2E fixtures run a single fmt folder. +- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. Its host-scoped preflight memo is reset by the shell's restart pass only when **no** consumer stack (`USER_NODE_STACKS`) survives the pass — a single-stack `rstack.fmt.restart` next to a live Rstest controller deliberately keeps the memo, or the surviving stack's next worker spawn would silently re-take the runtime decision its existing workers were built on; the full `rstack.restart` always clears it. `stacks/fmt/binEntry.ts` and `stacks/fmt/status.ts` are separate pure modules for the same testability reason: `stacks/fmt/index.ts` evaluates `vscode` and `vscode-languageclient`, so a pure helper left in it is not unit-testable — and `status.ts` carries the invariant the fold exists for (a healthy sibling folder never masks another folder's failure), which only a unit test can pin, since the E2E fixtures run a single fmt folder. - The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`), because the strictest thing any of these processes does — the test worker through rstack's shim, the `rs fmt` server through `@rstackjs/load-config` — is load an `rstack.config.*` with `loader: 'native'`, no jiti fallback, and so needs native type stripping (22.18+; on the 23.x line only from 23.6). 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. diff --git a/packages/vscode/e2e/rstest/suite/index.ts b/packages/vscode/e2e/rstest/suite/index.ts index b9c3a2e..830ced7 100644 --- a/packages/vscode/e2e/rstest/suite/index.ts +++ b/packages/vscode/e2e/rstest/suite/index.ts @@ -33,13 +33,26 @@ export function run(): Promise { return new Promise((resolve, reject) => { try { - mocha.run((failures) => { + const runner = mocha.run((failures) => { if (failures > 0) { reject(new Error(`${failures} Rstest E2E test(s) failed.`)); } else { resolve(); } }); + // Mocha's reporter writes to the extension host's stdout, which the CI + // test runner does not forward — a failing run's log shows only the + // final count. `console.error` does reach it, so name each failure + // there; without this, a CI-only failure cannot even be identified. + runner.on('fail', (test, error: unknown) => { + console.error( + `[e2e-fail] ${test.fullTitle()}: ${ + error instanceof Error + ? (error.stack ?? error.message) + : String(error) + }`, + ); + }); } catch (error) { reject(error instanceof Error ? error : new Error(String(error))); } diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index f5fcbf9..ed2f22d 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -25,6 +25,14 @@ const STACK_FACTORIES: Readonly> = { fmt: createFmtController, }; +/** + * The stacks that run project code on a User Node runtime and therefore read + * the host-scoped preflight memo (`shared/nodeResolution.ts`) — the set + * `runRestart` checks before resetting it. Lint is deliberately absent: it + * still runs on the VS Code Node runtime (ADR 0001's recorded debt). + */ +const USER_NODE_STACKS: readonly StackId[] = ['rstest', 'fmt']; + const errorMessage = (error: unknown): string => error instanceof Error ? (error.stack ?? error.message) : String(error); @@ -290,13 +298,28 @@ class ExtensionShell { return; } // Restart exists to clear stale resolution, and the User Node preflight - // memo is host-scoped state shared by every stack — so the shell clears it - // once per pass, after the retire wave and before anything re-registers. - // Owned here rather than in the stacks' `dispose()`: a stack-owned reset - // fires on every teardown (deactivate, detection loss) and clears the - // resolution a live sibling stack is relying on, twice per shared-setting - // change. - resetUserNodeCaches(); + // memo is host-scoped state shared by every stack that runs project code + // on a User Node runtime — so the shell clears it once per pass, after the + // retire wave and before anything re-registers. Owned here rather than in + // the stacks' `dispose()`: a stack-owned reset fires on every teardown + // (deactivate, detection loss) and clears the resolution a live sibling + // stack is relying on, twice per shared-setting change. + // + // Cleared only when no consumer of the memo survives the retire wave: a + // single-stack `rstack.fmt.restart` must not yank the decision a live + // Rstest controller's next worker spawn would silently re-take — its + // controller was never rebuilt, so it could end up on a different runtime + // than the workers it already has. The full `rstack.restart` (the "like a + // window reload" gesture) always qualifies, as does the batched restart a + // `rstack.nodeExecutable` change triggers, since every declaring stack is + // in that batch. `#controllers` holds only the survivors at this point — + // `retireAll` above removed everything being restarted. + const memoConsumerSurvives = USER_NODE_STACKS.some((stack) => + this.#controllers.has(stack), + ); + if (!memoConsumerSurvives) { + resetUserNodeCaches(); + } try { // A plain pass: `refresh` updates the snapshot whether or not the // signature moved, and the reconcile below rebuilds every stack from it. diff --git a/packages/vscode/tests/extension.test.ts b/packages/vscode/tests/extension.test.ts index 5d303ba..30c917f 100644 --- a/packages/vscode/tests/extension.test.ts +++ b/packages/vscode/tests/extension.test.ts @@ -56,6 +56,8 @@ const harness = rs.hoisted(() => { contextKeys: new Map(), /** What each stack's controller declares as restart-triggering settings. */ restartOnSettings: new Map(), + /** How often `runRestart` reset the host-scoped User Node memo. */ + nodeResets: 0, /** Every configuration listener the shell installed. */ configListeners: [] as ((event: { affectsConfiguration(section: string): boolean; @@ -241,6 +243,12 @@ rs.mock('../src/migration', () => ({ maybePromptForMigration: async () => undefined, runSettingsMigration: async () => undefined, })); +// The real reset is inert in tests; the harness counts the calls. +rs.mock('../src/shared/nodeResolution', () => ({ + resetUserNodeCaches: () => { + harness.nodeResets += 1; + }, +})); import { activate, deactivate } from '../src/extension'; @@ -447,6 +455,32 @@ describe('the shell restart command', () => { expect(harness.contextKeys.get('rstack.rstest.active')).toBe(true); }); + it('resets the shared Node memo only when no consumer stack survives the pass', async () => { + // A single-stack fmt restart leaves the live Rstest controller standing + // on the memoized runtime decision — the pass must not clear it under + // the workers that already took it. + await run('rstack.fmt.restart'); + expect(harness.nodeResets).toBe(0); + + // The full restart is the "like a window reload" gesture: every consumer + // is rebuilt, so the memo goes with them. + await restart(); + expect(harness.nodeResets).toBe(1); + }); + + it('resets the memo on a single-stack restart once it is the only consumer', async () => { + harness.detected.delete('rstest'); + // Rstest retires through the gate; fmt is still live, so no reset yet. + await run('rstack.rstest.restart'); + expect(harness.nodeResets).toBe(0); + + // fmt is now the only User-Node consumer and it is in the pass. The + // surviving lint controller does not hold the memo alive: it runs on the + // VS Code Node runtime and never reads it. + await run('rstack.fmt.restart'); + expect(harness.nodeResets).toBe(1); + }); + it('leaves a single-stack restart to the gate, same as a full one', async () => { harness.detected.delete('rstest'); From 564f76fdd15013db4001b945683b7e4e2099e9ab Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 13 Aug 2026 18:03:03 +0800 Subject: [PATCH 06/12] test(vscode): mirror the rstest output channel to stderr in CI A CI-only E2E failure on windows-latest (every rstest suite discovering zero tests since @rstest/core 0.11.7 / @rspack/core 2.1.10 floated in) cannot be diagnosed from the CI log: worker stdout/stderr land only in the output channel, which CI cannot open. Gate a console.error mirror of every log entry behind RSTACK_E2E_MIRROR_LOGS=1 and set it from the rstest E2E harness in CI, so the next failing run names the actual error. --- packages/vscode/AGENTS.md | 2 +- packages/vscode/e2e/rstest/runTest.ts | 6 ++++++ packages/vscode/src/stacks/test/logger.ts | 12 ++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 6f2cbf2..5fa3de0 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -12,7 +12,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten 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. +4. **Status aggregation** — stacks own no UI chrome; they report to the shell's single status bar item, which always exists. The test stack's `MasterLogger` additionally mirrors every entry to the extension host's console when `RSTACK_E2E_MIRROR_LOGS=1` (set by `e2e/rstest/runTest.ts` in CI only) — CI cannot open an output channel, and worker stdout/stderr land nowhere else, so this is the only way to diagnose a CI-only worker failure. 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, fmt) — the Node a project-loading child process runs on 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`). Both callers — the rstest worker and the `rs fmt --lsp` server — take the decision from the one shared module (`shared/nodeResolution.ts`) and share one escape hatch, the resource-scoped `rstack.nodeExecutable` (`shared/nodeExecutableSetting.ts`); each appends its own consequence to the shared preflight message. Lint still loads project code on the VS Code Node runtime — known debt recorded in the ADR, not an invariant the extension already holds. diff --git a/packages/vscode/e2e/rstest/runTest.ts b/packages/vscode/e2e/rstest/runTest.ts index e3740dc..9c19941 100644 --- a/packages/vscode/e2e/rstest/runTest.ts +++ b/packages/vscode/e2e/rstest/runTest.ts @@ -91,6 +91,12 @@ async function main() { vscodeExecutablePath: process.env.VSCODE_TEST_EXECUTABLE || undefined, extensionDevelopmentPath, extensionTestsPath, + // In CI the rstest output channel is unreadable, so the extension mirrors + // it to the console (see `src/stacks/test/logger.ts`) — the only way to + // see why a worker failed on a platform we cannot reproduce locally. + extensionTestsEnv: process.env.CI + ? { RSTACK_E2E_MIRROR_LOGS: '1' } + : undefined, launchArgs: [ workspaceFile, // Keep VS Code's CI-only extension inventory and AgentHost info logs out diff --git a/packages/vscode/src/stacks/test/logger.ts b/packages/vscode/src/stacks/test/logger.ts index ec65899..ce24f25 100644 --- a/packages/vscode/src/stacks/test/logger.ts +++ b/packages/vscode/src/stacks/test/logger.ts @@ -13,11 +13,23 @@ import { BaseLogger, type LogLevel } from './shared/logger'; * flip, a trust grant) logs into the fresh channel and a disposed stack logs * nowhere instead of throwing on a disposed channel. */ +// A CI-only worker failure is otherwise undiagnosable: worker stdout and +// stderr land only in the output channel, which CI cannot open. The E2E +// harness sets this flag in CI (via `extensionTestsEnv`, so it is fixed at +// host launch) and the logger mirrors every entry to the extension host's +// stderr. It must be `console.error` for every level: CI forwards only the +// extension host's stderr, not its stdout (see `e2e/rstest/suite/index.ts`), +// so `console[level]` would silently drop everything below `error`. +const mirrorToConsole = process.env.RSTACK_E2E_MIRROR_LOGS === '1'; + class MasterLogger extends BaseLogger { #channel: vscode.LogOutputChannel | undefined; protected override log(level: LogLevel, message: string) { this.#channel?.[level](message); + if (mirrorToConsole) { + console.error(`[rstest ${level}] ${message}`); + } } public bind(channel: vscode.LogOutputChannel) { From b6b9b0b2b0fd0c52dd5bcb29131e4f81e4e02088 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 13 Aug 2026 22:31:04 +0800 Subject: [PATCH 07/12] docs(vscode): record the uniform rstack floor as a decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raising SUPPORT_MATRIX.rstack to >=0.5.2 for rs fmt --lsp also gates the Rstest bridge, which checks the same entry — a project on rstack 0.3.5-0.5.1 reports version mismatch for tests too. Review flagged the side effect; keeping one toolchain-wide floor is the deliberate answer (per-stack rstack floors considered and rejected), so state it in the matrix doc and ADR 0002 instead of splitting the entry. --- docs/adr/0002-fmt-lsp-on-user-node-runtime.md | 2 +- packages/vscode/src/shared/versionCheck.ts | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md index a8842d5..453fcce 100644 --- a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md +++ b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md @@ -1,6 +1,6 @@ # Formatting through the `rs fmt` language server -Document formatting is served by **`rs fmt --lsp`** — one language server per detected workspace folder, spawned with its cwd at the folder root, running on a **User Node runtime** that satisfies the floor ADR 0001 sets (`^22.18.0 || >=23.6.0`). It replaces a spawn-per-request `rs fmt --stdin-filepath` MVP that ran on the VS Code Node runtime and kept one pre-spawned process warm for the active editor. The floor for the project's `rstack` rises to `>=0.5.2`, the first release that ships `--lsp`; older releases surface as `version mismatch` and format nothing. +Document formatting is served by **`rs fmt --lsp`** — one language server per detected workspace folder, spawned with its cwd at the folder root, running on a **User Node runtime** that satisfies the floor ADR 0001 sets (`^22.18.0 || >=23.6.0`). It replaces a spawn-per-request `rs fmt --stdin-filepath` MVP that ran on the VS Code Node runtime and kept one pre-spawned process warm for the active editor. The floor for the project's `rstack` rises to `>=0.5.2`, the first release that ships `--lsp`; older releases surface as `version mismatch` and format nothing. The raise is **toolchain-wide by decision**, not fmt-scoped: `SUPPORT_MATRIX.rstack` is one entry and the Rstest bridge gates on the same one, so a project on `rstack` 0.3.5–0.5.1 also stops running tests until it upgrades. A per-stack floor was considered and rejected — "which rstack does the extension support?" should have one answer, and the mismatch status names the required version. ## Why a server rather than a process per request diff --git a/packages/vscode/src/shared/versionCheck.ts b/packages/vscode/src/shared/versionCheck.ts index 112d021..95229e4 100644 --- a/packages/vscode/src/shared/versionCheck.ts +++ b/packages/vscode/src/shared/versionCheck.ts @@ -15,6 +15,14 @@ import { readPackageJson } from './packageResolve'; * the fmt stack is a client of. There is no stdin fallback for older * releases: the editor would then format through a different code path than * the one it is tested against, so the floor is a version gate instead. + * The floor is **uniform across consumers by decision**: the Rstest bridge + * checks the same entry, so a project on rstack 0.3.5–0.5.1 reports + * `version mismatch` for tests too, even though only fmt strictly needs + * 0.5.2. One matrix entry means "which rstack does the extension support?" + * has one answer; per-stack rstack floors were considered and rejected — + * they make the answer depend on which status the user happens to look at, + * for the price of keeping tests alive on releases the toolchain has moved + * past. The status message names the required version either way. */ export const SUPPORT_MATRIX = { '@rslint/core': '>=0.7.2', From 4a4e2d52bf3b773da42a8f47c5f3852deb0e779f Mon Sep 17 00:00:00 2001 From: fi3ework Date: Fri, 14 Aug 2026 13:01:15 +0800 Subject: [PATCH 08/12] docs(vscode): tighten the new AGENTS.md entries --- packages/vscode/AGENTS.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 5fa3de0..ddcdccb 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -12,7 +12,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten 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. The test stack's `MasterLogger` additionally mirrors every entry to the extension host's console when `RSTACK_E2E_MIRROR_LOGS=1` (set by `e2e/rstest/runTest.ts` in CI only) — CI cannot open an output channel, and worker stdout/stderr land nowhere else, so this is the only way to diagnose a CI-only worker failure. +4. **Status aggregation** — stacks own no UI chrome; they report to the shell's single status bar item, which always exists. In CI the test stack's `MasterLogger` also mirrors every entry to stderr (`RSTACK_E2E_MIRROR_LOGS=1`, set by `e2e/rstest/runTest.ts`) — the output channel is unreadable there; rationale in `stacks/test/logger.ts`. 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, fmt) — the Node a project-loading child process runs on 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`). Both callers — the rstest worker and the `rs fmt --lsp` server — take the decision from the one shared module (`shared/nodeResolution.ts`) and share one escape hatch, the resource-scoped `rstack.nodeExecutable` (`shared/nodeExecutableSetting.ts`); each appends its own consequence to the shared preflight message. Lint still loads project code on the VS Code Node runtime — known debt recorded in the ADR, not an invariant the extension already holds. @@ -32,11 +32,11 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The lint × `rstack.config.*` bridge was built and deliberately removed: a partial editor-side bridge gave wrong results, and a correct one needs upstream work first. `TODO(rstack-bridge)` markers carry the plan. Do not reintroduce a partial bridge. - The test × `rstack.config.*` bridge stays thin on purpose: it points the upstream machinery at rstack's shipped shim and lets the shim interpret the config inside the worker, same as the CLI. Never re-implement rstack config semantics in the extension. -- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. **Nested workspace folders are a documented limitation, by decision**: when a folder and its subdirectory are both workspace folders and both detect fmt, the parent's per-folder selector also matches the nested folder's files, and which server VS Code hands the request to is not defined — the supported shape is subprojects as _sibling_ workspace folders (or only the subproject opened), not parent-plus-child. Routing (lint's `WorkspaceDocumentRouter` shape, or a formatting middleware) was considered and deferred as complexity the scenario doesn't yet justify. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. +- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. **Nested workspace folders are a documented limitation, by decision**: when a folder and its subdirectory are both workspace folders and both detect fmt, the parent's per-folder selector also matches the nested folder's files, and which server VS Code hands the request to is not defined — the supported shape is subprojects as _sibling_ workspace folders (or only the subproject opened), not parent-plus-child. Routing (lint's `WorkspaceDocumentRouter` shape) was considered and deferred. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. - fmt importing `stacks/lint/LanguageServerProcessOwner.ts` is not a refactor across the copies: that file has no lint imports and no lint behaviour, it only owns the native children of one language client — including the ones vscode-languageclient's automatic restart creates, which is exactly the leak an ad-hoc copy would reintroduce. Lint's `ManagedLanguageClient` is _restated_ in `stacks/fmt/index.ts` instead, because importing it from `Rslint.ts` would drag the lint runtime graph (plugin pool, config loader, rstack bridge) into the fmt stack. Keep that line where it is: shared process ownership yes, shared stack runtime no. - `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. -- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. Its host-scoped preflight memo is reset by the shell's restart pass only when **no** consumer stack (`USER_NODE_STACKS`) survives the pass — a single-stack `rstack.fmt.restart` next to a live Rstest controller deliberately keeps the memo, or the surviving stack's next worker spawn would silently re-take the runtime decision its existing workers were built on; the full `rstack.restart` always clears it. `stacks/fmt/binEntry.ts` and `stacks/fmt/status.ts` are separate pure modules for the same testability reason: `stacks/fmt/index.ts` evaluates `vscode` and `vscode-languageclient`, so a pure helper left in it is not unit-testable — and `status.ts` carries the invariant the fold exists for (a healthy sibling folder never masks another folder's failure), which only a unit test can pin, since the E2E fixtures run a single fmt folder. +- `shared/nodeResolution.ts` takes its shell, its cwd and its notify callback as options instead of importing `vscode` and a stack's `logger` singleton, unlike its neighbours. That is not stylistic: it keeps `resolveUserNode` a pure decision table over its inputs, which is what makes the case-by-case unit tests possible without a `vscode` stub. It sat in `stacks/test/` until fmt became the second stack running project code on a User Node runtime — the exact condition its old note named — and moved on that trigger, not before. Do not route the remaining callers through it: work that only runs _our_ code on the VS Code Node runtime (the lint plugin host) has no candidate to choose between and needs `nativeTypeStrippingAvailable()` alone. Its host-scoped preflight memo is reset by the shell's restart pass only when **no** consumer stack (`USER_NODE_STACKS`) survives the pass — a single-stack `rstack.fmt.restart` beside a live Rstest controller deliberately keeps the memo, since the survivor's existing workers were built on that decision; the full `rstack.restart` always clears it. `stacks/fmt/binEntry.ts` and `stacks/fmt/status.ts` are separate pure modules for the same testability reason — `stacks/fmt/index.ts` evaluates `vscode`, and `status.ts` pins the fold's invariant (a healthy sibling folder never masks another folder's failure) in a unit test the single-folder E2E fixtures cannot. - The uniform Node floor deliberately exceeds `@rstest/core`'s own `engines` (`^20.19.0 || >=22.12.0`), because the strictest thing any of these processes does — the test worker through rstack's shim, the `rs fmt` server through `@rstackjs/load-config` — is load an `rstack.config.*` with `loader: 'native'`, no jiti fallback, and so needs native type stripping (22.18+; on the 23.x line only from 23.6). 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. From 0b03b70b00ce00ae44cf5fc1952601d0265b8a78 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Fri, 14 Aug 2026 13:42:54 +0800 Subject: [PATCH 09/12] fix(vscode): retry failed fmt folder runtimes on detection passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A folder whose runtime had failed (disabled, version mismatch, crashed) was kept as-is by every reconcile, so the install or upgrade that fixed it was never picked up — formatting stayed dead until a manual restart, while the test and lint stacks both retry on the same detection pass. Restart such a runtime in place, on the path a config change already uses, which re-runs package resolution, the version check and the Node preflight. Healthy and starting runtimes stay untouched. The one recovery no watcher sees — an install that changes no lockfile — now has its way out written into the disabled status message, which names the restart command. --- docs/adr/0002-fmt-lsp-on-user-node-runtime.md | 4 ++-- packages/vscode/src/stacks/fmt/index.ts | 23 +++++++++++++++---- packages/vscode/src/stacks/fmt/status.ts | 12 ++++++++++ .../vscode/tests/stacks/fmt/status.test.ts | 21 +++++++++++++++++ 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md index 453fcce..3f361c3 100644 --- a/docs/adr/0002-fmt-lsp-on-user-node-runtime.md +++ b/docs/adr/0002-fmt-lsp-on-user-node-runtime.md @@ -16,7 +16,7 @@ One rule across both stacks now: the workspace folder root is the config root. A ## Why the User Node runtime -The server loads the project's `rstack.config.*` through `@rstackjs/load-config` with `loader: 'native'` — the exact path ADR 0001 analysed to set the worker floor, with no jiti fallback and no `process.features.typescript` consultation. So fmt is not a new case: it is the second caller of the same decision, and it takes the floor, the candidate order (PATH `node`, then the user's interactive shell) and the failure reporting out of the one shared module, `shared/nodeResolution.ts`. The escape hatch is shared too — `rstack.nodeExecutable`, resource-scoped, honoured whenever it is set and probed anyway, advisory-only. A user pinning a Node for one tool means it for the toolchain, so there is one setting rather than one per stack (the retired `rstack.rstest.nodeExecutable` migrates to it). +The server loads the project's `rstack.config.*` through `@rstackjs/load-config` with `loader: 'native'` — the exact path ADR 0001 analysed to set the worker floor, with no jiti fallback and no `process.features.typescript` consultation. So fmt is not a new case: it is the second caller of the same decision, and it takes the floor, the candidate order (PATH `node`, then the user's interactive shell) and the failure reporting out of the one shared module, `shared/nodeResolution.ts`. The escape hatch is shared too — `rstack.nodeExecutable`, resource-scoped, honoured whenever it is set and probed anyway, advisory-only. A user pinning a Node for one tool means it for the toolchain, so there is one setting rather than one per stack (the standalone Rstest extension's `rstest.nodeExecutable` migrates to it). Falling back to the VS Code Node runtime stays rejected — ADR 0001's load-bearing "no", unchanged. It is worth naming that the old fmt path did exactly that: `process.execPath` with `ELECTRON_RUN_AS_NODE=1`, loading the user's config on Electron's Node, with no floor and no preflight. Moving the server onto a User Node runtime is what takes fmt off that ADR's debt list. @@ -37,6 +37,6 @@ Falling back to the VS Code Node runtime stays rejected — ADR 0001's load-bear - There is no cold path any more, so formatting is briefly unavailable after activation, after a restart and after a config change, while that folder's server starts. A format requested before the client has registered the server's capability finds no formatter for the document; nothing falls back to a fresh process. - The server advertises document formatting only: no range or selection formatting, no format-on-type, no diagnostics. It also ignores the editor's `FormattingOptions` (tab size, spaces) and the client's language id — the file path picks the parser and the project's config decides the style. An editor setting that disagrees with the project config loses, which is the same answer `rs fmt` gives in a terminal. - Failures stay per folder and stay statuses: no `rstack` installed is `disabled`, an `rstack` below `0.5.2` is `version mismatch`, no Node clearing the floor is `version mismatch` with fmt's own consequence appended to the shared preflight message ("until then rs fmt will not format"), and only a server that fails to launch or stops on its own is `crashed`. One folder in any of those states does not affect another folder's server, and the stack's single status report is the folder set folded by severity (`crashed` > `version mismatch`, which is also where a running folder's pin advisory ranks > `disabled` > `starting` > `running`; the pure fold lives in `stacks/fmt/status.ts`), so a healthy sibling starting or recovering never overwrites another folder's failure — and the status says `starting`, not `running`, until a server actually formats. -- An `rstack.config.*` create, change or delete restarts the server of the folder that contains it, and only that one; a detection change reconciles the folder set and leaves already-running servers alone. Both are deliberate: a healthy server's cached config is worth keeping. +- An `rstack.config.*` create, change or delete restarts the server of the folder that contains it, and only that one; a detection change reconciles the folder set, leaves healthy servers alone — a healthy server's cached config is worth keeping — and restarts a folder whose runtime already failed (`disabled`, `version mismatch`, `crashed`) in place, on the same path a config change uses, which re-runs package resolution and the version check. Detection notifies on lockfile events even when the folder set is unchanged precisely so that the install or upgrade that fixes a failed resolution is picked up without a manual restart. The remaining blind spot is an install that changes no lockfile (a fresh clone whose lockfile is already current): no file event fires, so the `disabled` status names the restart command as the way out. Watching `node_modules` for that case was rejected (unreliable under pnpm's layout and excluded by VS Code's default watcher excludes), and a bundled fallback formatter — the usual way editor extensions mask this blind spot — is ruled out by resolve-from-project. - "A subproject becomes its own workspace folder" means a _sibling_ folder (or opening only the subproject). Keeping the parent **and** the nested subdirectory as workspace folders with fmt detected in both is a documented limitation: the parent's selector also matches the nested files, and which of the two servers VS Code asks is not defined. Per-document routing to the deepest folder was considered (lint carries a `WorkspaceDocumentRouter` for exactly this) and deferred — complexity the scenario does not yet justify. - The extension now holds one long-lived Node process per detected folder for fmt. Each is owned by the same process owner the lint client uses, so a stop is bounded (SIGTERM, then SIGKILL) and the automatic restart vscode-languageclient performs cannot leave an orphan behind. diff --git a/packages/vscode/src/stacks/fmt/index.ts b/packages/vscode/src/stacks/fmt/index.ts index f661cc8..333607d 100644 --- a/packages/vscode/src/stacks/fmt/index.ts +++ b/packages/vscode/src/stacks/fmt/index.ts @@ -41,6 +41,7 @@ import { foldFolderStatus, type FmtFolderStatus, type FmtRuntimeState, + isFailedFmtState, } from './status'; // prettier 3.9.6 getSupportInfo() vscodeLanguageIds snapshot (rs fmt's pinned @@ -294,9 +295,13 @@ class FmtFolderRuntime { const pkgJsonPath = findPackageJsonUncached('rstack', folderRoot); if (!pkgJsonPath) { + // The trailing hint covers the one recovery path no watcher sees: an + // install that changes no lockfile (a fresh clone whose lockfile is + // already current) fires no file event, so nothing rebuilds this + // runtime — the status message is where the way out has to live. this.setState( 'disabled', - 'rstack is not installed (node_modules missing)', + 'rstack is not installed (node_modules missing) — install it, then run "Rstack: Restart rs fmt" if this status stays', ); context.output.warn( `rstack is not installed in ${this.folder.name} (node_modules missing); searched from ${folderRoot}`, @@ -624,9 +629,10 @@ class FmtController implements StackController { /** * Brings the folder set in line with detection: a newly detected folder gets - * a server, an undetected one loses it, and a folder that is in both sets is - * left alone — restarting a healthy server on an unrelated folder's detection - * change would drop its cached config for nothing. + * a server, an undetected one loses it, and a folder that is in both sets + * keeps a healthy server — restarting it on an unrelated folder's detection + * change would drop its cached config for nothing — but has a failed one + * (`isFailedFmtState`) restarted in place. */ private reconcile(): void { const context = this.#context; @@ -659,7 +665,14 @@ class FmtController implements StackController { } } for (const [folderPath, folder] of detected) { - if (this.#runtimes.has(folderPath)) { + const existing = this.#runtimes.get(folderPath); + if (existing) { + if (isFailedFmtState(existing.state)) { + // A failed runtime is retried in place, on the same path a config + // change uses: restart re-runs package resolution, the version + // check and the Node preflight. Why: `isFailedFmtState`'s doc. + void existing.restart('a dependency change may have fixed it'); + } continue; } // The callback re-reads `#snapshot`, so a server that finishes starting diff --git a/packages/vscode/src/stacks/fmt/status.ts b/packages/vscode/src/stacks/fmt/status.ts index 7751a59..21b052d 100644 --- a/packages/vscode/src/stacks/fmt/status.ts +++ b/packages/vscode/src/stacks/fmt/status.ts @@ -58,6 +58,18 @@ const STATE_RANK: Readonly> = { running: 1, }; +/** + * The states a detection pass restarts instead of keeping: nothing about a + * failed runtime is worth preserving, and the pass may be the very install or + * upgrade that fixes it — detection notifies on lockfile events even when the + * detected folder set is unchanged, for exactly this retry. `starting` is + * deliberately not here: interrupting a server mid-start on every lockfile + * event would thrash a healthy launch, and a start that ends in failure is + * retried by the next pass. + */ +export const isFailedFmtState = (state: FmtRuntimeState): boolean => + state === 'disabled' || state === 'version-mismatch' || state === 'crashed'; + /** * Folds every folder runtime's state into the one report the shell shows for * the fmt stack. The worst folder wins, and with multiple folders the detail diff --git a/packages/vscode/tests/stacks/fmt/status.test.ts b/packages/vscode/tests/stacks/fmt/status.test.ts index e2b750a..f47bd92 100644 --- a/packages/vscode/tests/stacks/fmt/status.test.ts +++ b/packages/vscode/tests/stacks/fmt/status.test.ts @@ -3,6 +3,7 @@ import { foldFolderStatus, type FmtFolderStatus, type FmtRuntimeState, + isFailedFmtState, } from '../../../src/stacks/fmt/status'; const folder = ( @@ -132,3 +133,23 @@ describe('foldFolderStatus', () => { ).toEqual({ kind: 'version-mismatch', detail: `pinned: ${advisory}` }); }); }); + +describe('isFailedFmtState', () => { + it('restarts exactly the terminal failures and keeps every live state', () => { + // Exhaustive over the union: reconcile restarts a failed runtime because + // the detection pass may be the install or upgrade that fixes it, and + // keeps healthy or in-progress ones — `starting` must stay kept, or every + // lockfile event would interrupt a healthy launch. + const verdicts: Record = { + disabled: true, + 'version-mismatch': true, + crashed: true, + stopped: false, + starting: false, + running: false, + }; + for (const [state, failed] of Object.entries(verdicts)) { + expect(isFailedFmtState(state as FmtRuntimeState)).toBe(failed); + } + }); +}); From e403423ed8e073ebefe64f862afdd66f5655b80a Mon Sep 17 00:00:00 2001 From: fi3ework Date: Fri, 14 Aug 2026 13:43:03 +0800 Subject: [PATCH 10/12] chore(vscode): drop compat for this extension's unpublished states The rstack.rstest.nodeExecutable -> rstack.nodeExecutable mapping covered a rename that happened before the extension was ever published, so no settings file can hold the legacy key. Remove it together with the superseded-skip and collision machinery that existed only for the two-sources-one-target case, and the prompt clause naming the phantom source. Record the two policies this acts on in AGENTS.md: pre-1.0.0 the extension breaks freely (only the latest released tools need support), and the three tools are treated uniformly by default, diverging only when a tool forces it. --- docs/adr/0001-node-runtime-selection.md | 2 +- packages/vscode/AGENTS.md | 4 ++- packages/vscode/src/migration.ts | 46 +++---------------------- packages/vscode/tests/migration.test.ts | 44 ++++------------------- 4 files changed, 15 insertions(+), 81 deletions(-) diff --git a/docs/adr/0001-node-runtime-selection.md b/docs/adr/0001-node-runtime-selection.md index dc4ac56..e0a0732 100644 --- a/docs/adr/0001-node-runtime-selection.md +++ b/docs/adr/0001-node-runtime-selection.md @@ -45,7 +45,7 @@ Lint is what moving costs when it is not cheap: fmt's move needed a whole upstre ## Consequences -- An explicit `rstack.nodeExecutable` (shipped as `rstack.rstest.nodeExecutable` when this was written, shared with the fmt server since ADR 0002 and migrated for existing users) 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. +- An explicit `rstack.nodeExecutable` (shared with the fmt server since ADR 0002; the standalone Rstest extension's `rstest.nodeExecutable` migrates to it) 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. diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index ddcdccb..b94e09c 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -18,6 +18,8 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten ## Rules +- **Pre-1.0.0 the extension breaks freely.** No compatibility is owed with earlier unpublished states of this extension — settings, command ids and behavior may change without deprecation paths, and dead compat code for them is removed, not kept. Only the **latest released** `rstack`, `@rstest/core` and `@rslint/core` need support; version floors may be raised without a transition story (the floor status names the required version). The settings migration exists for users of the two retired standalone extensions, never for earlier states of this one. +- **The three tools are treated uniformly by default.** Detection, dependency-change retry, restart semantics, version gating and status reporting follow one shared pattern across the lint/test/fmt stacks; a stack diverges only when its tool forces it, and the divergence is recorded here as a gotcha. When adding behavior to one stack, first ask whether it belongs to all three. This is about behavior, not code — the upstream copies still must not be deduplicated. - One stack failing to register or crashing must never take another stack (or the shell) down. - The shell always activates; per-folder config detection decides which stacks start, and re-runs on config/lockfile changes without a window reload. Enable-settings are coarse kill switches only. - Reconciles and restarts share one serialized queue (`enqueue`); a reconcile leaves a live stack alone, so the restart commands are the only path that rebuilds one. Do not add a second queue. @@ -32,7 +34,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The lint × `rstack.config.*` bridge was built and deliberately removed: a partial editor-side bridge gave wrong results, and a correct one needs upstream work first. `TODO(rstack-bridge)` markers carry the plan. Do not reintroduce a partial bridge. - The test × `rstack.config.*` bridge stays thin on purpose: it points the upstream machinery at rstack's shipped shim and lets the shim interpret the config inside the worker, same as the CLI. Never re-implement rstack config semantics in the extension. -- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. **Nested workspace folders are a documented limitation, by decision**: when a folder and its subdirectory are both workspace folders and both detect fmt, the parent's per-folder selector also matches the nested folder's files, and which server VS Code hands the request to is not defined — the supported shape is subprojects as _sibling_ workspace folders (or only the subproject opened), not parent-plus-child. Routing (lint's `WorkspaceDocumentRouter` shape) was considered and deferred. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. +- The fmt stack is an LSP client: one `rs fmt --lsp` server per detected workspace folder, spawned at the **folder root** even when a deeper `rstack.config.*` exists. Deepest-config-wins was removed deliberately — `rs fmt` loads one config from its cwd with no upward walk, so anchoring deeper made the editor disagree with `rs fmt` in a terminal; a subproject that needs its own fmt config becomes its own workspace folder. The stack registers **no** `DocumentFormattingEditProvider`: the client registers the provider from the server's `documentFormattingProvider` capability, and adding one by hand would double-register. A config create/change/delete **restarts** the owning folder's server (the server caches its config for its process lifetime and has no config-change message), which is also why the stack watches `RSTACK_CONFIG_GLOB` itself instead of relying on detection — a detection signature records which config files exist, not their contents. A detection pass keeps healthy servers and restarts failed ones in place (`isFailedFmtState`) — lockfile events notify even when the folder set is unchanged, precisely so a completed install or upgrade is retried without a manual restart. There is no stdin fallback for `rstack < 0.5.2`; that is a version gate (`SUPPORT_MATRIX.rstack`), not an omission. **Nested workspace folders are a documented limitation, by decision**: when a folder and its subdirectory are both workspace folders and both detect fmt, the parent's per-folder selector also matches the nested folder's files, and which server VS Code hands the request to is not defined — the supported shape is subprojects as _sibling_ workspace folders (or only the subproject opened), not parent-plus-child. Routing (lint's `WorkspaceDocumentRouter` shape) was considered and deferred. Why all of it: `docs/adr/0002-fmt-lsp-on-user-node-runtime.md`. - fmt importing `stacks/lint/LanguageServerProcessOwner.ts` is not a refactor across the copies: that file has no lint imports and no lint behaviour, it only owns the native children of one language client — including the ones vscode-languageclient's automatic restart creates, which is exactly the leak an ad-hoc copy would reintroduce. Lint's `ManagedLanguageClient` is _restated_ in `stacks/fmt/index.ts` instead, because importing it from `Rslint.ts` would drag the lint runtime graph (plugin pool, config loader, rstack bridge) into the fmt stack. Keep that line where it is: shared process ownership yes, shared stack runtime no. - `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. diff --git a/packages/vscode/src/migration.ts b/packages/vscode/src/migration.ts index 0414890..a7eba33 100644 --- a/packages/vscode/src/migration.ts +++ b/packages/vscode/src/migration.ts @@ -3,8 +3,7 @@ import vscode from 'vscode'; /** * Settings migration from retired names into current ones: the two standalone * extensions (`rstack.rslint` and `rstack.rstest`) into the unified `rstack.*` - * namespace, plus keys this extension itself has since renamed - * (`rstack.rstest.nodeExecutable` -> `rstack.nodeExecutable`). + * namespace. * * Shape of the feature: * @@ -46,13 +45,7 @@ export type SkipReason = /** The new key already has an explicit value in the same layer. */ | 'target-already-set' /** A folder-layer value for a window-scoped target setting. */ - | 'not-folder-scoped' - /** - * A later legacy key in the same layer maps to the same target (the two - * retired runtime pins both map to `rstack.nodeExecutable`); the later one — - * the key more recently in effect — carries the value. - */ - | 'superseded'; + | 'not-folder-scoped'; type ValueMapping = | { readonly kind: 'value'; readonly value: unknown } @@ -122,9 +115,8 @@ const RSTEST_KEYS: readonly (readonly [string, 'resource' | 'window'])[] = [ ]; /** - * The complete legacy inventory: 4 Rslint keys + 14 Rstest keys + 1 key from - * this extension's own earlier releases. Kept in one table so the preview, the - * writer and the tests cannot disagree. + * The complete legacy inventory: 4 Rslint keys + 14 Rstest keys. Kept in one + * table so the preview, the writer and the tests cannot disagree. */ export const LEGACY_MAPPINGS: readonly LegacyMapping[] = [ { @@ -159,13 +151,6 @@ export const LEGACY_MAPPINGS: readonly LegacyMapping[] = [ to: 'rstack.nodeExecutable', targetScope: 'resource', }, - { - // Same target, different source: earlier releases of *this* extension - // shipped the pin as `rstack.rstest.nodeExecutable`. - from: 'rstack.rstest.nodeExecutable', - to: 'rstack.nodeExecutable', - targetScope: 'resource', - }, ...RSTEST_KEYS.map(([key, targetScope]) => ({ from: `rstest.${key}`, to: `rstack.rstest.${key}`, @@ -336,25 +321,6 @@ export const planMigration = ( } const writes = scopeFor(reading); - // Two legacy keys can map to one target (the retired runtime pins). Where - // both are set in one layer, the later mapping wins — and the earlier one - // is surfaced as a skip, so the preview never shows two writes both - // claiming the same key with no hint which value survives. - const claimed = writes.findIndex((write) => write.to === mapping.to); - if (claimed !== -1) { - const [previous] = writes.splice(claimed, 1); - if (previous) { - skips.push({ - scopeId: previous.scopeId, - layer: previous.layer, - folderLabel: previous.folderLabel, - from: previous.from, - to: previous.to, - value: previous.fromValue, - reason: 'superseded', - }); - } - } writes.push({ scopeId: reading.scopeId, layer: reading.layer, @@ -402,8 +368,6 @@ const SKIP_EXPLANATIONS: Readonly< 'target-already-set': (skip) => `${skip.to} is already set here`, 'not-folder-scoped': (skip) => `${skip.to} is a window-scoped setting and cannot be set per folder`, - superseded: (skip) => - `another legacy key here also maps to ${skip.to} and carries the newer value`, }; /** @@ -690,7 +654,7 @@ export const maybePromptForMigration = async ( const migrate = 'Migrate…'; const dismiss = "Don't ask again"; const choice = await vscode.window.showInformationMessage( - 'Rstack found settings under legacy names (from the standalone Rslint/Rstest extensions, or an earlier Rstack release). Migrate them to their current names?', + 'Rstack found settings under legacy names (from the standalone Rslint/Rstest extensions). Migrate them to their current names?', migrate, 'Not now', dismiss, diff --git a/packages/vscode/tests/migration.test.ts b/packages/vscode/tests/migration.test.ts index 9be44b5..ef44e0e 100644 --- a/packages/vscode/tests/migration.test.ts +++ b/packages/vscode/tests/migration.test.ts @@ -44,15 +44,13 @@ const folderReading = ( describe('LEGACY_MAPPINGS', () => { it('covers the full legacy inventory of both retired extensions', () => { // 4 settings from rslint/packages/vscode-extension + 14 from - // rstest/packages/vscode, verified against their manifests, plus the - // runtime pin this extension itself renamed to `rstack.nodeExecutable`. + // rstest/packages/vscode, verified against their manifests. expect(LEGACY_MAPPINGS.map((mapping) => mapping.from)).toEqual([ 'rslint.enable', 'rslint.binPath', 'rslint.customBinPath', 'rslint.trace.server', 'rstest.nodeExecutable', - 'rstack.rstest.nodeExecutable', 'rstest.rstestPackagePath', 'rstest.nodeExecArgs', 'rstest.nodeEnv', @@ -79,26 +77,15 @@ describe('LEGACY_MAPPINGS', () => { } }); - it('has no duplicate source keys, and shares a target only for the runtime pin', () => { + it('has no duplicate source keys and no shared targets', () => { + // One source per target: the planner writes each target at most once per + // layer, so no mapping order can silently decide which value survives. expect(new Set(LEGACY_MAPPINGS.map((m) => m.from)).size).toBe( LEGACY_MAPPINGS.length, ); - // Both retired pins converge on `rstack.nodeExecutable`; every other - // target is unique. When both sources are set in one layer, the mapping - // order decides: the later mapping — this extension's own retired key, the - // one more recently in effect — wins, and the earlier one is planned as a - // 'superseded' skip. - const shared = LEGACY_MAPPINGS.filter( - (m) => m.to === 'rstack.nodeExecutable', - ); - expect(shared.map((m) => m.from)).toEqual([ - 'rstest.nodeExecutable', - 'rstack.rstest.nodeExecutable', - ]); - const rest = LEGACY_MAPPINGS.filter( - (m) => m.to !== 'rstack.nodeExecutable', + expect(new Set(LEGACY_MAPPINGS.map((m) => m.to)).size).toBe( + LEGACY_MAPPINGS.length, ); - expect(new Set(rest.map((m) => m.to)).size).toBe(rest.length); }); it('marks the window-scoped settings as such', () => { @@ -329,25 +316,6 @@ describe('planMigration — conflicts', () => { }); }); - it('plans one write and a superseded skip when both retired pins are set in one layer', () => { - const plan = planMigration([ - reading({ key: 'rstest.nodeExecutable', value: '/old/node' }), - reading({ key: 'rstack.rstest.nodeExecutable', value: '/new/node' }), - ]); - expect(plan.writeCount).toBe(1); - expect(plan.scopes[0]?.writes[0]).toMatchObject({ - from: 'rstack.rstest.nodeExecutable', - to: 'rstack.nodeExecutable', - value: '/new/node', - }); - expect(plan.skips[0]).toMatchObject({ - from: 'rstest.nodeExecutable', - to: 'rstack.nodeExecutable', - value: '/old/node', - reason: 'superseded', - }); - }); - it('treats a conflict as layer-local', () => { const plan = planMigration([ reading({ key: 'rslint.enable', value: true, targetValue: false }), From 0720c78f868433a14d2cf37c43bbc1f7be7d2022 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Fri, 14 Aug 2026 14:38:05 +0800 Subject: [PATCH 11/12] fix(vscode): answer any relevant settings change with one full restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The listener decided per stack between two paths: a gate change went to the reconcile, a declared restartOnSettings change to a targeted restart, and a stack in both sets was left to the reconcile. That split swallowed the restart when one save wrote a gate key at its already-effective value alongside a shared setting — the reconcile saw a live controller behind a still-open gate and kept it, so the stack stayed on the previous Node executable. Settings edits are rare, so selectivity bought nothing but that hole: any relevant key now triggers one full restart pass, which re-evaluates every gate and rebuilds every controller — a gate flip in either direction, a moved shared setting, or both in one save are handled by construction. The dead array arm of restart()/runRestart() and the unused reconcile() parameter go with it. --- packages/vscode/AGENTS.md | 3 +- packages/vscode/src/extension.ts | 108 +++++++++--------------- packages/vscode/src/types.ts | 9 +- packages/vscode/tests/extension.test.ts | 80 ++++++++++++------ 4 files changed, 105 insertions(+), 95 deletions(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index b94e09c..58377c7 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -22,8 +22,9 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - **The three tools are treated uniformly by default.** Detection, dependency-change retry, restart semantics, version gating and status reporting follow one shared pattern across the lint/test/fmt stacks; a stack diverges only when its tool forces it, and the divergence is recorded here as a gotcha. When adding behavior to one stack, first ask whether it belongs to all three. This is about behavior, not code — the upstream copies still must not be deduplicated. - One stack failing to register or crashing must never take another stack (or the shell) down. - The shell always activates; per-folder config detection decides which stacks start, and re-runs on config/lockfile changes without a window reload. Enable-settings are coarse kill switches only. -- Reconciles and restarts share one serialized queue (`enqueue`); a reconcile leaves a live stack alone, so the restart commands are the only path that rebuilds one. Do not add a second queue. +- Reconciles and restarts share one serialized queue (`enqueue`); a reconcile leaves a live stack alone, so the restart path — the commands, and the full pass any relevant settings change triggers — is the only thing that rebuilds one. Do not add a second queue. - Restart is a shell concern, not a stack one: `rstack.restart` rebuilds every controller, `rstack..restart` rebuilds one. A stack must never register its own restart command — a shallower "bounce the tool's process" restart keeps that controller's stale package resolution and version check, which is the bug the command exists to clear. +- A relevant settings change (a gate key, or a key a live controller declares in `restartOnSettings`) triggers **one full restart pass**, never a targeted one. Per-stack selectivity was removed deliberately: settings edits are rare, and deciding per stack between "reconcile handles the gate" and "restart handles the setting" swallowed the restart when one save wrote a gate key at its already-effective value alongside a shared setting. The full pass re-evaluates every gate, so flips in either direction need no special casing. - Deprecated `rslint.json` / `rslint.jsonc` are unsupported by decision, not omission — never make them detection signals. - Never share a child process across stacks: the tools have incompatible cwd semantics (lint LSP anchors on spawn cwd; test worker pins to project root; the `rs fmt` server takes its config root from the workspace folder the client reports, falling back to spawn cwd, with no upward walk either way). The lint and fmt servers now happen to stand in the same directory — the folder root — which changes nothing: they are different CLIs, different protocols and different version gates. - In Restricted Mode (workspace trust), only the status bar runs — no process spawns, no project code loaded. diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index ed2f22d..f7c0f67 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -78,55 +78,41 @@ class ExtensionShell { }), vscode.workspace.onDidChangeConfiguration((event) => { // 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(); + // everything at once — and the answer to any relevant one is a single + // full restart pass: it re-evaluates every gate and rebuilds every + // controller, so a gate flip, a moved shared setting, or both in one + // save are all handled by construction. Per-stack selectivity (gate + // changes to the reconcile, declared settings to a targeted restart) + // was removed deliberately: settings edits are rare, and the split + // could swallow a restart when one save wrote a gate key at its + // already-effective value alongside a shared setting. + const reasons = new Set(); + const note = (key: string): void => { + if (event.affectsConfiguration(key)) { + reasons.add(key); + } + }; + note('rstack.enable'); + for (const stack of STACK_IDS) { + note(`rstack.${stack}.enable`); } - // 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. - // One save can move a setting shared by several stacks (the runtime - // pin), so the moved stacks are collected and restarted as one pass — - // one detection sweep, one retire/reconcile wave — instead of one full - // restart each. - const moved: StackId[] = []; - const reasons: string[] = []; + // Declared settings are known per live controller. A stack behind a + // closed gate has none — and needs none: its own settings cannot + // matter until an enable flip (caught above) lets it register, which + // reads everything fresh. for (const [stack, controller] of this.#controllers) { - if (gated.has(stack)) { - continue; - } - // Entries are relative to the stack's namespace unless they name a - // fully qualified `rstack.*` key — the form shared settings use. - const settingKey = (setting: string): string => - setting.startsWith('rstack.') - ? setting - : `rstack.${stack}.${setting}`; - const setting = controller.restartOnSettings?.find((candidate) => - event.affectsConfiguration(settingKey(candidate)), - ); - if (setting) { - moved.push(stack); - const key = settingKey(setting); - if (!reasons.includes(key)) { - reasons.push(key); - } + for (const setting of controller.restartOnSettings ?? []) { + // Entries are relative to the stack's namespace unless they name a + // fully qualified `rstack.*` key — the form shared settings use. + note( + setting.startsWith('rstack.') + ? setting + : `rstack.${stack}.${setting}`, + ); } } - if (moved.length > 0) { - void this.restart(moved, `${reasons.join(', ')} changed`); + if (reasons.size > 0) { + void this.restart(undefined, `${[...reasons].join(', ')} changed`); } }), // Restricted Mode shows the status bar only; trust unlocks the stacks @@ -241,8 +227,8 @@ class ExtensionShell { * assume they already own the queue — going through `enqueue` from inside * one would wait on itself. Everything else calls the wrappers. */ - private reconcile(stacks?: readonly StackId[]): Promise { - return this.enqueue(() => this.runReconcile(stacks)); + private reconcile(): Promise { + return this.enqueue(() => this.runReconcile()); } private scheduleReconcile(): void { @@ -266,26 +252,16 @@ class ExtensionShell { * `reason` is for the callers that are not a user picking the command — * `restartOnSettings` passes what moved. */ - restart( - stacks?: StackId | readonly StackId[], - reason?: string, - ): Promise { - return this.enqueue(() => this.runRestart(stacks, reason)); + restart(stack?: StackId, reason?: string): Promise { + return this.enqueue(() => this.runRestart(stack, reason)); } - private async runRestart( - only?: StackId | readonly StackId[], - reason?: string, - ): Promise { + private async runRestart(only?: StackId, reason?: string): Promise { if (this.#disposed) { return; } - const stacks = - only === undefined ? STACK_IDS : typeof only === 'string' ? [only] : only; - const what = - stacks.length === STACK_IDS.length - ? 'Rstack' - : stacks.map((stack) => STACK_LABELS[stack]).join(', '); + const stacks = only === undefined ? STACK_IDS : [only]; + const what = only === undefined ? 'Rstack' : STACK_LABELS[only]; this.#channels.shell.info( `Restarting ${what}${reason ? ` (${reason})` : ''}`, ); @@ -310,10 +286,10 @@ class ExtensionShell { // Rstest controller's next worker spawn would silently re-take — its // controller was never rebuilt, so it could end up on a different runtime // than the workers it already has. The full `rstack.restart` (the "like a - // window reload" gesture) always qualifies, as does the batched restart a - // `rstack.nodeExecutable` change triggers, since every declaring stack is - // in that batch. `#controllers` holds only the survivors at this point — - // `retireAll` above removed everything being restarted. + // window reload" gesture) always qualifies, as does the full pass any + // relevant settings change triggers. `#controllers` holds only the + // survivors at this point — `retireAll` above removed everything being + // restarted. const memoConsumerSurvives = USER_NODE_STACKS.some((stack) => this.#controllers.has(stack), ); diff --git a/packages/vscode/src/types.ts b/packages/vscode/src/types.ts index c964c9a..1701355 100644 --- a/packages/vscode/src/types.ts +++ b/packages/vscode/src/types.ts @@ -124,11 +124,12 @@ export interface StackContext { export interface StackController { readonly id: StackId; /** - * Setting names whose change must rebuild this stack, because their value is + * Setting names whose change must trigger a rebuild, 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. A bare name is - * relative to the stack's namespace (`rstack..`); a name starting - * with `rstack.` is taken as-is, for shared settings like + * live controller would keep answering with the stale one. The shell + * answers with one full restart pass — every stack, not just the declarer. + * A bare name is relative to the stack's namespace (`rstack..`); + * a name starting with `rstack.` is taken as-is, for shared settings like * `rstack.nodeExecutable`. * * Declared as data because restart is the shell's concern: the shell owns the diff --git a/packages/vscode/tests/extension.test.ts b/packages/vscode/tests/extension.test.ts index 30c917f..00b6efa 100644 --- a/packages/vscode/tests/extension.test.ts +++ b/packages/vscode/tests/extension.test.ts @@ -56,6 +56,12 @@ const harness = rs.hoisted(() => { contextKeys: new Map(), /** What each stack's controller declares as restart-triggering settings. */ restartOnSettings: new Map(), + /** + * Explicit setting values by fully qualified key (`rstack.fmt.enable`); + * anything absent resolves to the caller's fallback, like a defaults-only + * configuration. + */ + settings: new Map(), /** How often `runRestart` reset the host-scoped User Node memo. */ nodeResets: 0, /** Every configuration listener the shell installed. */ @@ -188,8 +194,13 @@ rs.mock('vscode', () => { workspace: { isTrusted: true, workspaceFolders: [], - getConfiguration: () => ({ - get: (_key: string, fallback?: unknown) => fallback, + getConfiguration: (section?: string) => ({ + get: (key: string, fallback?: unknown) => { + const qualified = section ? `${section}.${key}` : key; + return harness.settings.has(qualified) + ? harness.settings.get(qualified) + : fallback; + }, }), onDidChangeConfiguration: ( listener: (event: { @@ -316,41 +327,61 @@ describe('restart-triggering settings', () => { 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'); + it('runs one full restart pass for a declared setting', async () => { + // Selectivity was removed deliberately: settings edits are rare, and a + // full pass re-evaluates every gate, so there is no per-stack decision + // left to get wrong. One declared setting rebuilds everything. + changeSetting('rstack.rstest.nodeExecutable'); await settle(); - expect(stacksOf('register')).toContain('rstest'); + expect(stacksOf('dispose').sort()).toEqual(['fmt', 'rslint', 'rstest']); + expect(stacksOf('register').sort()).toEqual(['fmt', 'rslint', '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. + it('does not swallow a shared setting saved alongside a no-op gate write', async () => { + // The regression the full pass exists to prevent: one save writes a gate + // key at its already-effective value (`"rstack.rstest.enable": true` when + // the default is already true) and moves a declared setting. The old + // per-stack split classified the stack as "gate moved — reconcile's + // business", the reconcile saw a live controller behind a still-open gate + // and kept it, and the declared-setting restart was silently dropped. changeSetting('rstack.rstest.enable', 'rstack.rstest.nodeExecutable'); await settle(); - expect(stacksOf('register')).not.toContain('rstest'); + expect(stacksOf('register')).toContain('rstest'); }); - it('rebuilds only the stack that declared the setting', async () => { - changeSetting('rstack.rstest.nodeExecutable'); + it('runs a full pass for a bare gate write', async () => { + // The gate half of the trigger set, pinned in isolation: the enable keys + // must fire the pass on their own — the other gate tests here also move a + // declared setting, which would fire the pass regardless. + changeSetting('rstack.enable'); await settle(); - expect(stacksOf('dispose')).toEqual(['rstest']); - expect(stacksOf('register')).toEqual(['rstest']); + expect(stacksOf('register').sort()).toEqual(['fmt', 'rslint', 'rstest']); + }); + + it('drops a stack whose gate actually closed in the same pass', async () => { + // The full pass re-reads the gates: a stack whose enable flipped off is + // retired and not re-registered, with no reconcile hand-off needed. The + // per-stack enable key is the only changed setting, so this also pins + // that key as a trigger on its own. + harness.settings.set('rstack.rstest.enable', false); + changeSetting('rstack.rstest.enable'); + await settle(); + expect(stacksOf('dispose')).toContain('rstest'); + expect(stacksOf('register')).not.toContain('rstest'); + expect(stacksOf('register').sort()).toEqual(['fmt', 'rslint']); }); it('honours every setting a stack declares, not just the first', async () => { changeSetting('rstack.rslint.customBinPath'); await settle(); - expect(stacksOf('register')).toEqual(['rslint']); + expect(stacksOf('register').sort()).toEqual(['fmt', 'rslint', 'rstest']); }); it('treats a declared rstack.* name as fully qualified, shared across stacks', async () => { // The shared runtime pin (`rstack.nodeExecutable`) lives outside any stack // namespace, so its declaration must not be re-prefixed into - // `rstack..rstack.nodeExecutable` — and one change event rebuilds - // every stack that declared it. + // `rstack..rstack.nodeExecutable` — re-prefixed, the change would + // match nothing and no restart would fire at all. await deactivate(); harness.reset(); harness.detected = new Set(['rslint', 'rstest', 'fmt']); @@ -363,8 +394,7 @@ describe('restart-triggering settings', () => { changeSetting('rstack.nodeExecutable'); await settle(); - expect(stacksOf('dispose').sort()).toEqual(['fmt', 'rstest']); - expect(stacksOf('register').sort()).toEqual(['fmt', 'rstest']); + expect(stacksOf('register').sort()).toEqual(['fmt', 'rslint', 'rstest']); }); it('ignores a setting no stack declared', async () => { @@ -381,9 +411,11 @@ describe('restart-triggering settings', () => { 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. + it('ignores a setting declared by a stack that is not live', async () => { + // Declared settings are enumerated per live controller: a stack that never + // registered cannot have consumed the value, so its keys do not trigger a + // pass — an enable flip is what lets it register, reading everything + // fresh. harness.detected = new Set(['rslint', 'fmt']); await run('rstack.restart'); harness.events.length = 0; From 81326609f444df83ac501f4a188dc975c5a4c460 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Fri, 14 Aug 2026 14:44:57 +0800 Subject: [PATCH 12/12] docs: scope the glossary to what this branch ships MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Generated-shim and Bridged-folder entries describe the lint bridge, which is not on this branch — they move to the branch that introduces it. Config root claimed the workspace folder root for every tool, but the test stack keeps upstream's per-project cwd rule (adaptation 5); scope the anchor to the fmt server. --- CONTEXT.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 7f78930..951f397 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -24,9 +24,7 @@ Glossary of terms used across rstack-editor. Code, docs, commit messages and rev - **Rstack config** — the unified `rstack.config.*` file consumed by rstack-cli (`rs`), holding per-tool sections. Tools never read it themselves; `rs` hands each tool its section through a shim. - **Shim** — the module rstack-cli ships per tool that loads the Rstack config and exposes that tool's section through the tool's ordinary explicit-config channel. The extension points upstream machinery at the shim rather than re-implementing Rstack config semantics. - **Bridged project** — a test project the extension synthesizes for a directory whose test signal is a Rstack config, wired to the shim. _Avoid_: virtual project, rstack project. -- **Generated shim** — a shim the extension writes itself for a bridged folder, baking in the absolute Rstack config path via the loader rstack publishes (`rstack/config`). Used where the tool's channel evaluates modules away from the project directory, so rstack-cli's shipped shim (which probes the current directory) cannot apply. -- **Bridged folder** — a workspace folder whose lint runs against a Rstack config: no native Rslint config exists anywhere in the folder, a Rstack config sits at the folder root, and the language server is pinned to a generated shim for its whole lifetime. _Avoid_: bridged workspace. -- **Config root** — the directory a tool's config is loaded from, which is also the directory the tool's process stands in. The editor always uses the workspace folder root, so it loads the config a terminal opened on that folder would; a subproject that needs its own config becomes its own workspace folder. _Avoid_: config directory, project root. +- **Config root** — the directory a tool's config is loaded from, which is also the directory the tool's process stands in. For the fmt server the editor anchors it at the workspace folder root, so it loads the config a terminal opened on that folder would, and a subproject that needs its own config becomes its own workspace folder. The test stack does not share this anchor: a project's cwd is set per project (for native configs, upstream's config-file-directory rule). _Avoid_: config directory, project root. - **Ownership** — the editor-side rule assigning a directory to one tool when both a native config and a Rstack config are present there: the atomic tool's native config wins and the bridge yields. This rule exists only in the editor; upstream CLIs never face the choice, since each reads only its own config. ## fmt