Skip to content

Commit 4e568f9

Browse files
authored
fix(vscode): treat missing dependencies as a state, not an errorfix(vscode): 将缺失的依赖项视为一种状态,而非错误 (#29)
* fix(vscode): treat missing dependencies as a state, not an error A nested rstack.config.* whose own dependencies are never installed (create-rstack's template-* beside their generator) made the Rstest stack log '[error] Failed to initialize project config' with a full stack trace per template, on every detection pass. 'Not installed' is now reported uniformly across the three stacks (new AGENTS.md rule): a disabled status whose reason names the restart command as the way out, plus one warn line in the output channel — never a crashed status, a stack trace, or a notification. - shared/notInstalled.ts owns the wording for all three stacks (the formatVersionMismatch precedent); the restart hint derives from the new stackCommandTitle, checked against the manifest in tests. - The Rstest worker classifies a config import failure on Node's own error code (the IPC channel drops it) and returns the verdict as data (NormalizedConfigResult); Project branches on it and latches a per-project disabled status that installs clear and dispose forgets. - StatusHolder gains a notInstalled latch ranked below crash and version mismatch, idempotent across refresh repaints. - Lint's report moves wholly into the onDocumentFailure hook, so the upstream-tracked RuntimeManager only defers to it; missing rstack logs one warn line instead of an error with a stack. - Missing @rstest/core now reports through the same path (warn + disabled status) at all three master resolution sites. * fix(vscode): keep the not-installed classification honest across stacks Review follow-ups on the uniform not-installed policy: - lint: a missing native @rslint/core is the not-installed state, not a crash; the code-to-package mapping (missingPackageOf) is shared by the status and the warn line, and the warn names the runtime a document keeps. A misconfigured rslint corePath now throws invalid-package so a wrong setting is never reported as "install your dependencies". - rstest worker: @rstest/core is loaded before the classified config load, so a broken core install reports its real error instead of "a config dependency is missing". - rstest bridge: the not-installed latch clears the moment the rstack package resolves, and an install that ships no Rstest shim latches a version-mismatch instead of painting the folder healthy. * fix(vscode): classify only bare package imports as not installed Second review round on the uniform not-installed policy: - worker: missingDependencyCauseOf replaces isMissingDependencyError — Node's code alone also covers a typo'd relative import or a missing generated file, which installing dependencies cannot fix, so only a bare (package-name) specifier counts and anything else keeps the full error report. The returned cause is the message's first line, keeping the warn to one line without the CJS require stack. - shared: the config-dependency log line moves into shared/notInstalled (formatConfigDependencyMissingLog), deriving its consequence from STACK_LABELS, so no stack owns its own wording. - rstest status: StatusHolder latches now supersede each other per source (one source, one verdict) — a stale higher-ranked crash or mismatch can no longer paint over a newer not-installed observation, and raise sites need no manual cross-latch cleanup. * fix(vscode): keep independent failure facts from masking each other Third review round on the uniform not-installed policy: - rstest bridge: the missing-rstack warning goes through the shared formatNotInstalledLog instead of its own sentence. - rstest status: a package-state observation (mismatch or not-installed) restates its root — it retires the other kind AND a stale crash, whose only other exit (workerSpawned) cannot fire while the package is unusable. The config-dependency verdict moves to its own config-deps: latch key (the nodeRuntimeStatusSource precedent), so it coexists with the core version check instead of erasing it. - worker classifier: a bare-looking subpath of an installed package (require('pkg/missing')) is a source error, not the not-installed state — confirmed against the physical node_modules with the same uncached walk-up the rest of the stack resolves packages with. * docs(vscode): scope the config-import case to Rstest, tracked in #30
1 parent 0be5644 commit 4e568f9

24 files changed

Lines changed: 927 additions & 128 deletions

packages/vscode/AGENTS.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten
2323

2424
- **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: whenever a change touches a floor in `SUPPORT_MATRIX`, set it to the latest release at that time — do not reason about which older release would still work — and raise it without a transition story (the floor status names the required version). No settings migration exists either — not for earlier states of this extension, and not for the two retired standalone extensions (removed in #15; users re-enter their settings under `rstack.*`).
2525
- **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.
26+
- **Not installed is a state, not an error — uniformly.** A folder or project whose dependencies are not installed (no `rstack`, no `@rstest/core`, no `@rslint/core`, a config importing a package that is not there) is the normal state of a fresh clone and of scaffolded templates beside their generator (`create-rstack`'s `template-*`, which declare their own dependencies and are never installed). Every stack reports it the same way: a `disabled` status whose reason names the restart command as the way out (ADR 0002: an install that changes no lockfile fires no detection pass), one `warn` line in the output channel without a stack trace, never a `crashed` status and never a notification. The words come from one place, `shared/notInstalled.ts` (the `formatVersionMismatch` precedent) — each stack keeps its own status machinery, none its own wording; the restart hint is derived from `stackCommandTitle`, which `tests/extension.test.ts` checks against the manifest. Lint's report lives in the `onDocumentFailure` hook (`stacks/lint/index.ts`), which owns the log line too, so the upstream-tracked `RuntimeManager` only defers to it. Rstest classifies the config-import case in the worker (`missingDependencyCauseOf`: Node's `code`, a bare — package-name — specifier, and for a subpath a walk-up proving the package really is absent, so a typo'd relative import or a missing subpath of an installed package stays a real error) because the IPC channel drops the `code` — `NormalizedConfigResult` carries the verdict as data end to end, and `Project` branches on it. The config-import case is implemented for Rstest only today — lint and fmt load configs inside their own servers and cannot classify there yet (#30).
2627
- One stack failing to register or crashing must never take another stack (or the shell) down.
2728
- 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.
2829
- 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.
@@ -38,7 +39,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten
3839

3940
- The lint × `rstack.config.*` bridge stays thin on purpose: only a root Rstack config can claim a bridged folder, any native config anywhere in the folder wins ownership, and the worker evaluates rstack's published shim from the folder root. Never generate a shim, load the Rstack config in the extension host, or interpret `define.lint()` ourselves.
4041
- **Yarn Plug'n'Play is unsupported by decision, extension-wide.** Every stack resolves through physical `node_modules` (`shared/packageResolve.ts`, `resolution.ts`'s rstack → `@rslint/core` chain, the fmt bin probe, the rstest package lookup) and the lint worker's own `createRequire` from the core directory does too. Lint once carried a `.pnp.cjs` branch for the find-`@rslint/core` hop only; nothing after that hop (config evaluation, plugin resolution, the other stacks) had PnP hooks, so it never produced a working folder, and upstream removed its own PnP path in the same refactor that introduced `corePath`. Real support would be a PnP editor-SDK-shaped project across all three stacks, not a resolver branch — do not reintroduce one.
41-
- **A Lint runtime lives as long as a document needs it, and a folder with none is `running: idle`.** Since the #1617 sync, `RuntimeManager` refcounts each runtime by open document: the first document to resolve a core starts one, the last to release it closes it, so a detected folder with nothing open holds zero workers and zero Go processes. That folder still reports `running` — with the detail `idle` — because it is live and will start a runtime on the next `didOpen`; do **not** add a `StackState` kind for it (the shell's status bar and `when` clauses read the kinds, and idle is not a kind of health). A folder's state is the **worst of** its runtimes plus any document whose core resolution currently fails (last-good: that document keeps the runtime it already had), so one failing core is never masked by a healthy sibling — the same invariant fmt pins across folders, applied inside one and across them alike (lint's rank table matches fmt's: `disabled` there means "no `rstack`", not the kill switch). Triggers: the shell's detection pass (which already covers lockfiles) plus one lint-owned watcher on `node_modules/@rslint/core/package.json` — upstream's glob minus the lockfiles detection owns. Failures report through the status only: upstream's `window.showWarningMessage` is dropped, since stacks own no UI chrome. Consequently `whenStackActive('rslint')` means "the controller registered its folders", not "a server is up" — E2E suites open a document and await diagnostics.
42+
- **A Lint runtime lives as long as a document needs it, and a folder with none is `running: idle`.** Since the #1617 sync, `RuntimeManager` refcounts each runtime by open document: the first document to resolve a core starts one, the last to release it closes it, so a detected folder with nothing open holds zero workers and zero Go processes. That folder still reports `running` — with the detail `idle` — because it is live and will start a runtime on the next `didOpen`; do **not** add a `StackState` kind for it (the shell's status bar and `when` clauses read the kinds, and idle is not a kind of health). A folder's state is the **worst of** its runtimes plus any document whose core resolution currently fails (last-good: that document keeps the runtime it already had), so one failing core is never masked by a healthy sibling — the same invariant fmt pins across folders, applied inside one and across them alike (lint's rank table matches fmt's: `disabled` there means "a package is not installed" — no `rstack`, or no `@rslint/core` — not the kill switch). Triggers: the shell's detection pass (which already covers lockfiles) plus one lint-owned watcher on `node_modules/@rslint/core/package.json` — upstream's glob minus the lockfiles detection owns. Failures report through the status only: upstream's `window.showWarningMessage` is dropped, since stacks own no UI chrome. Consequently `whenStackActive('rslint')` means "the controller registered its folders", not "a server is up" — E2E suites open a document and await diagnostics.
4243
- The lint worker is deliberately vscode-free so it can move upstream whole. It takes explicit `--core` / `--config` native paths, writes logs only to stderr because stdout is LSP, and owns the Go child plus config/plugin lifecycles. Config edits use `rslint/configRefresh` with the same pinned path; a native ↔ bridged ownership change replaces the whole folder runtime because protocol 2 locks that choice for the process lifetime.
4344
- 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. Bridged projects resolve `@rstest/core` from the resolved rstack package directory, mirroring lint, so rstack's dependency remains visible under isolated installs. Never re-implement rstack config semantics in the extension.
4445
- 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 below `SUPPORT_MATRIX.rstack`; that is a version gate, 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`.
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import {
2+
COMMAND_CATEGORY,
3+
STACK_LABELS,
4+
type StackId,
5+
stackCommandTitle,
6+
} from '../types';
7+
8+
/**
9+
* The not-installed policy's words, once for all three stacks (AGENTS.md
10+
* rules): a project whose dependencies are not installed is a `disabled`
11+
* status whose reason names the way out, plus one `warn` line in the output
12+
* channel. The stacks share the wording the way they share
13+
* `formatVersionMismatch` — each keeps its own status machinery, but what
14+
* the user reads is one sentence, not three near-copies.
15+
*
16+
* The trailing hint covers the recovery no watcher sees: an install that
17+
* changes no lockfile (a fresh clone whose lockfile is already current) fires
18+
* no detection pass, so the restart command is the way out and the status is
19+
* where it has to be named (ADR 0002).
20+
*/
21+
const restartHint = (stack: StackId): string =>
22+
`then run "${COMMAND_CATEGORY}: ${stackCommandTitle(stack, 'restart')}" if this status stays`;
23+
24+
/** The `disabled` reason for a package the stack needs and cannot find. */
25+
export const formatNotInstalledStatus = (
26+
stack: StackId,
27+
packageName: string,
28+
): string =>
29+
`${packageName} is not installed (node_modules missing) — install it, ${restartHint(stack)}`;
30+
31+
/**
32+
* The `disabled` reason for a config that evaluates but imports a package
33+
* that is not there. `configPath` is workspace-relative: the status has no
34+
* room for more.
35+
*/
36+
export const formatConfigDependencyMissingStatus = (
37+
stack: StackId,
38+
configPath: string,
39+
): string =>
40+
`${configPath} imports a package that is not installed — install the project dependencies, ${restartHint(stack)}`;
41+
42+
/**
43+
* The output-channel line for a config that imports a package that is not
44+
* installed. `cause` is the loader's own first line, which names the
45+
* specifier and the importer.
46+
*/
47+
export const formatConfigDependencyMissingLog = (
48+
stack: StackId,
49+
configPath: string,
50+
cause: string,
51+
): string =>
52+
`Cannot load ${configPath}: ${cause}. Install the project dependencies to enable ${STACK_LABELS[stack]} for this config.`;
53+
54+
/**
55+
* The output-channel line: where the stack looked, plus the stack's own
56+
* consequence — the same shape as the shared Node preflight message
57+
* (adaptation 6), where each caller appends what the state means for it.
58+
*/
59+
export const formatNotInstalledLog = (
60+
packageName: string,
61+
folderName: string,
62+
searchedFrom: string,
63+
consequence?: string,
64+
): string =>
65+
`${packageName} is not installed in ${folderName} (node_modules missing); searched from ${searchedFrom}${
66+
consequence ? `; ${consequence}` : ''
67+
}`;

packages/vscode/src/stacks/fmt/index.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ import {
1111
} from 'vscode-languageclient/node';
1212
import { RSTACK_CONFIG_GLOB } from '../../detection';
1313
import { getConfiguredNodeExecutable } from '../../shared/nodeExecutableSetting';
14+
import {
15+
formatNotInstalledLog,
16+
formatNotInstalledStatus,
17+
} from '../../shared/notInstalled';
1418
import {
1519
configuredNodeBelowFloor,
1620
NODE_EXECUTABLE_SETTING,
@@ -299,12 +303,9 @@ class FmtFolderRuntime {
299303
// install that changes no lockfile (a fresh clone whose lockfile is
300304
// already current) fires no file event, so nothing rebuilds this
301305
// runtime — the status message is where the way out has to live.
302-
this.setState(
303-
'disabled',
304-
'rstack is not installed (node_modules missing) — install it, then run "Rstack: Restart rs fmt" if this status stays',
305-
);
306+
this.setState('disabled', formatNotInstalledStatus('fmt', 'rstack'));
306307
context.output.warn(
307-
`rstack is not installed in ${this.folder.name} (node_modules missing); searched from ${folderRoot}`,
308+
formatNotInstalledLog('rstack', this.folder.name, folderRoot),
308309
);
309310
return;
310311
}

packages/vscode/src/stacks/lint/RuntimeManager.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ export interface DocumentResolutionFailure {
5959
readonly error: unknown;
6060
/** The core whose runtime failed to start; absent when resolution itself failed. */
6161
readonly resolved?: ResolvedCoreRuntime;
62+
/** The package directory of the runtime the document keeps (last-good), if any. */
63+
readonly keeping?: string;
6264
}
6365

6466
export interface RuntimeManagerOptions {
@@ -394,7 +396,9 @@ export class RuntimeManager {
394396
* category. Here the same event becomes a folder status entry (the stack
395397
* owns no UI chrome), so no deduplication is needed: a status is a value,
396398
* not a notification, and the controller replaces the document's previous
397-
* one. The Output-channel line stays.
399+
* one. The hook owns the whole report, the Output-channel line included, so
400+
* its level and wording live in lint-owned code; upstream's line is kept
401+
* only for a manager without a hook.
398402
*/
399403
private reportFailure(
400404
document: TextDocument,
@@ -403,19 +407,22 @@ export class RuntimeManager {
403407
existing: RuntimeEntry | undefined,
404408
resolved?: ResolvedCoreRuntime,
405409
): void {
406-
const suffix = existing
407-
? ` (keeping ${existing.resolved.installation.packageDirectory} active)`
408-
: '';
410+
const keeping = existing?.resolved.installation.packageDirectory;
411+
if (this.options.onDocumentFailure) {
412+
this.options.onDocumentFailure({
413+
document,
414+
workspaceFolder,
415+
error,
416+
resolved,
417+
keeping,
418+
});
419+
return;
420+
}
421+
const suffix = keeping ? ` (keeping ${keeping} active)` : '';
409422
this.logger.error(
410423
`Could not select an Rslint core for ${document.uri}${suffix}`,
411424
error,
412425
);
413-
this.options.onDocumentFailure?.({
414-
document,
415-
workspaceFolder,
416-
error,
417-
resolved,
418-
});
419426
}
420427

421428
private isCurrentDocument(document: TextDocument, epoch: number): boolean {

packages/vscode/src/stacks/lint/index.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type {
66
StackState,
77
} from '../../types';
88
import { NODE_EXECUTABLE_SETTING } from '../../shared/nodeResolution';
9+
import { formatNotInstalledLog } from '../../shared/notInstalled';
910
import { CoreResolver, type ResolvedCoreRuntime } from './CoreResolver';
1011
import { Logger } from './logger';
1112
import { Rslint } from './Rslint';
@@ -17,6 +18,7 @@ import {
1718
attributeToCore,
1819
foldRslintFolderState,
1920
statusForRslintStartFailure,
21+
missingPackageOf,
2022
} from './status';
2123
import { WorkspaceDocumentRouter } from './WorkspaceDocumentRouter';
2224

@@ -198,7 +200,35 @@ class RslintController implements StackController {
198200
logger,
199201
{
200202
folderMode: (folder) => this.folderMode(folder),
201-
onDocumentFailure: ({ document, workspaceFolder, error, resolved }) => {
203+
onDocumentFailure: ({
204+
document,
205+
workspaceFolder,
206+
error,
207+
resolved,
208+
keeping,
209+
}) => {
210+
// The hook owns the report. The Output-channel line: a folder whose
211+
// `rstack` or `@rslint/core` is not installed is the not-installed
212+
// state (AGENTS.md) — one warn line, no stack; anything else is
213+
// upstream's error. A document with a last-good runtime still
214+
// lints, so its consequence says what it keeps, not "will not".
215+
const missing = missingPackageOf(error);
216+
if (missing !== undefined) {
217+
logger.warn(
218+
formatNotInstalledLog(
219+
missing,
220+
workspaceFolder.name,
221+
workspaceFolder.uri.fsPath,
222+
`${document.uri} ${keeping ? `keeps ${keeping}` : 'will not lint'} until it is installed`,
223+
),
224+
);
225+
} else {
226+
const suffix = keeping ? ` (keeping ${keeping} active)` : '';
227+
logger.error(
228+
`Could not select an Rslint core for ${document.uri}${suffix}`,
229+
error,
230+
);
231+
}
202232
// Last-good semantics: the document keeps whatever runtime it had.
203233
// The failure is still the folder's worst news, so it is folded in
204234
// beside the runtimes rather than shown as a toast. A start failure

packages/vscode/src/stacks/lint/resolution.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,11 @@ function resolveConfiguredCore(
7979
try {
8080
if (!fs.statSync(packageJsonPath).isFile()) throw new Error('not a file');
8181
} catch (error) {
82+
// Not `missing-core`: the user pointed `corePath` at this directory, so
83+
// the fix is correcting the setting, not installing dependencies — it must
84+
// not take the not-installed state (`missingPackageOf`).
8285
throw new RslintResolutionError(
83-
'missing-core',
86+
'invalid-package',
8487
`Could not access @rslint/core at ${directory}`,
8588
{ cause: error },
8689
);

0 commit comments

Comments
 (0)