From 6230de53f3ea3c5b0afd16d04adaca889d5c7adc Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 24 Aug 2026 15:33:55 +0800 Subject: [PATCH 1/5] fix(vscode): treat missing dependencies as a state, not an error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/vscode/AGENTS.md | 1 + packages/vscode/src/shared/notInstalled.ts | 50 +++++++++++++++ packages/vscode/src/stacks/fmt/index.ts | 11 ++-- .../vscode/src/stacks/lint/RuntimeManager.ts | 27 +++++--- packages/vscode/src/stacks/lint/index.ts | 29 ++++++++- packages/vscode/src/stacks/lint/status.ts | 18 ++++-- packages/vscode/src/stacks/test/bridge.ts | 8 +++ .../vscode/src/stacks/test/coreResolution.ts | 27 ++++++-- packages/vscode/src/stacks/test/master.ts | 43 ++++++++++--- packages/vscode/src/stacks/test/project.ts | 55 +++++++++++----- packages/vscode/src/stacks/test/status.ts | 47 ++++++++++++-- packages/vscode/src/stacks/test/types.ts | 17 +++++ .../vscode/src/stacks/test/worker/index.ts | 55 ++++++++++------ packages/vscode/src/types.ts | 13 ++++ packages/vscode/tests/extension.test.ts | 25 ++++++-- .../vscode/tests/shared/notInstalled.test.ts | 40 ++++++++++++ .../vscode/tests/stacks/lint/status.test.ts | 17 +++++ .../vscode/tests/stacks/test/bridge.test.ts | 43 ++++++++----- .../tests/stacks/test/coreResolution.test.ts | 56 ++++++++++++++++- .../vscode/tests/stacks/test/master.test.ts | 36 ++++++++--- .../vscode/tests/stacks/test/project.test.ts | 62 ++++++++++++++++++- .../vscode/tests/stacks/test/status.test.ts | 44 +++++++++++++ .../tests/stacks/test/statusRecorder.ts | 23 +++++++ 23 files changed, 646 insertions(+), 101 deletions(-) create mode 100644 packages/vscode/src/shared/notInstalled.ts create mode 100644 packages/vscode/tests/shared/notInstalled.test.ts create mode 100644 packages/vscode/tests/stacks/test/statusRecorder.ts diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 0225be1..96f9a61 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -23,6 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - **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.*`). - **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. +- **Not installed is a state, not an error — uniformly.** A folder or project whose dependencies are not installed (no `rstack`, no `@rstest/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 (`isMissingDependencyError`, on Node's `code`) because the IPC channel drops it — `NormalizedConfigResult` carries the verdict as data end to end, and `Project` branches on it. - 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 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. diff --git a/packages/vscode/src/shared/notInstalled.ts b/packages/vscode/src/shared/notInstalled.ts new file mode 100644 index 0000000..8a0832c --- /dev/null +++ b/packages/vscode/src/shared/notInstalled.ts @@ -0,0 +1,50 @@ +import { COMMAND_CATEGORY, type StackId, stackCommandTitle } from '../types'; + +/** + * The not-installed policy's words, once for all three stacks (AGENTS.md + * rules): a project whose dependencies are not installed is a `disabled` + * status whose reason names the way out, plus one `warn` line in the output + * channel. The stacks share the wording the way they share + * `formatVersionMismatch` — each keeps its own status machinery, but what + * the user reads is one sentence, not three near-copies. + * + * The trailing hint covers the recovery no watcher sees: an install that + * changes no lockfile (a fresh clone whose lockfile is already current) fires + * no detection pass, so the restart command is the way out and the status is + * where it has to be named (ADR 0002). + */ +const restartHint = (stack: StackId): string => + `then run "${COMMAND_CATEGORY}: ${stackCommandTitle(stack, 'restart')}" if this status stays`; + +/** The `disabled` reason for a package the stack needs and cannot find. */ +export const formatNotInstalledStatus = ( + stack: StackId, + packageName: string, +): string => + `${packageName} is not installed (node_modules missing) — install it, ${restartHint(stack)}`; + +/** + * The `disabled` reason for a config that evaluates but imports a package + * that is not there. `configPath` is workspace-relative: the status has no + * room for more. + */ +export const formatConfigDependencyMissingStatus = ( + stack: StackId, + configPath: string, +): string => + `${configPath} imports a package that is not installed — install the project dependencies, ${restartHint(stack)}`; + +/** + * The output-channel line: where the stack looked, plus the stack's own + * consequence — the same shape as the shared Node preflight message + * (adaptation 6), where each caller appends what the state means for it. + */ +export const formatNotInstalledLog = ( + packageName: string, + folderName: string, + searchedFrom: string, + consequence?: string, +): string => + `${packageName} is not installed in ${folderName} (node_modules missing); searched from ${searchedFrom}${ + consequence ? `; ${consequence}` : '' + }`; diff --git a/packages/vscode/src/stacks/fmt/index.ts b/packages/vscode/src/stacks/fmt/index.ts index bd20bc5..f05e46c 100644 --- a/packages/vscode/src/stacks/fmt/index.ts +++ b/packages/vscode/src/stacks/fmt/index.ts @@ -11,6 +11,10 @@ import { } from 'vscode-languageclient/node'; import { RSTACK_CONFIG_GLOB } from '../../detection'; import { getConfiguredNodeExecutable } from '../../shared/nodeExecutableSetting'; +import { + formatNotInstalledLog, + formatNotInstalledStatus, +} from '../../shared/notInstalled'; import { configuredNodeBelowFloor, NODE_EXECUTABLE_SETTING, @@ -299,12 +303,9 @@ class FmtFolderRuntime { // 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) — install it, then run "Rstack: Restart rs fmt" if this status stays', - ); + this.setState('disabled', formatNotInstalledStatus('fmt', 'rstack')); context.output.warn( - `rstack is not installed in ${this.folder.name} (node_modules missing); searched from ${folderRoot}`, + formatNotInstalledLog('rstack', this.folder.name, folderRoot), ); return; } diff --git a/packages/vscode/src/stacks/lint/RuntimeManager.ts b/packages/vscode/src/stacks/lint/RuntimeManager.ts index 2551c2c..a7f27b0 100644 --- a/packages/vscode/src/stacks/lint/RuntimeManager.ts +++ b/packages/vscode/src/stacks/lint/RuntimeManager.ts @@ -59,6 +59,8 @@ export interface DocumentResolutionFailure { readonly error: unknown; /** The core whose runtime failed to start; absent when resolution itself failed. */ readonly resolved?: ResolvedCoreRuntime; + /** The package directory of the runtime the document keeps (last-good), if any. */ + readonly keeping?: string; } export interface RuntimeManagerOptions { @@ -394,7 +396,9 @@ export class RuntimeManager { * category. Here the same event becomes a folder status entry (the stack * owns no UI chrome), so no deduplication is needed: a status is a value, * not a notification, and the controller replaces the document's previous - * one. The Output-channel line stays. + * one. The hook owns the whole report, the Output-channel line included, so + * its level and wording live in lint-owned code; upstream's line is kept + * only for a manager without a hook. */ private reportFailure( document: TextDocument, @@ -403,19 +407,22 @@ export class RuntimeManager { existing: RuntimeEntry | undefined, resolved?: ResolvedCoreRuntime, ): void { - const suffix = existing - ? ` (keeping ${existing.resolved.installation.packageDirectory} active)` - : ''; + const keeping = existing?.resolved.installation.packageDirectory; + if (this.options.onDocumentFailure) { + this.options.onDocumentFailure({ + document, + workspaceFolder, + error, + resolved, + keeping, + }); + return; + } + const suffix = keeping ? ` (keeping ${keeping} active)` : ''; this.logger.error( `Could not select an Rslint core for ${document.uri}${suffix}`, error, ); - this.options.onDocumentFailure?.({ - document, - workspaceFolder, - error, - resolved, - }); } private isCurrentDocument(document: TextDocument, epoch: number): boolean { diff --git a/packages/vscode/src/stacks/lint/index.ts b/packages/vscode/src/stacks/lint/index.ts index b3d31a3..84ecd64 100644 --- a/packages/vscode/src/stacks/lint/index.ts +++ b/packages/vscode/src/stacks/lint/index.ts @@ -6,6 +6,7 @@ import type { StackState, } from '../../types'; import { NODE_EXECUTABLE_SETTING } from '../../shared/nodeResolution'; +import { formatNotInstalledLog } from '../../shared/notInstalled'; import { CoreResolver, type ResolvedCoreRuntime } from './CoreResolver'; import { Logger } from './logger'; import { Rslint } from './Rslint'; @@ -17,6 +18,7 @@ import { attributeToCore, foldRslintFolderState, statusForRslintStartFailure, + isMissingRstackFailure, } from './status'; import { WorkspaceDocumentRouter } from './WorkspaceDocumentRouter'; @@ -198,7 +200,32 @@ class RslintController implements StackController { logger, { folderMode: (folder) => this.folderMode(folder), - onDocumentFailure: ({ document, workspaceFolder, error, resolved }) => { + onDocumentFailure: ({ + document, + workspaceFolder, + error, + resolved, + keeping, + }) => { + // The hook owns the report. The Output-channel line: a folder whose + // `rstack` is not installed is the not-installed state (AGENTS.md) + // — one warn line, no stack; anything else is upstream's error. + const suffix = keeping ? ` (keeping ${keeping} active)` : ''; + if (isMissingRstackFailure(error)) { + logger.warn( + formatNotInstalledLog( + 'rstack', + workspaceFolder.name, + workspaceFolder.uri.fsPath, + `${document.uri} will not lint until it is${suffix}`, + ), + ); + } else { + logger.error( + `Could not select an Rslint core for ${document.uri}${suffix}`, + error, + ); + } // Last-good semantics: the document keeps whatever runtime it had. // The failure is still the folder's worst news, so it is folded in // beside the runtimes rather than shown as a toast. A start failure diff --git a/packages/vscode/src/stacks/lint/status.ts b/packages/vscode/src/stacks/lint/status.ts index 0f99f4b..2e2b863 100644 --- a/packages/vscode/src/stacks/lint/status.ts +++ b/packages/vscode/src/stacks/lint/status.ts @@ -1,4 +1,5 @@ import type { StackState } from '../../types'; +import { formatNotInstalledStatus } from '../../shared/notInstalled'; import { RslintResolutionError } from './resolution'; export class RslintVersionMismatchError extends Error { @@ -8,18 +9,23 @@ export class RslintVersionMismatchError extends Error { } } +/** + * A bridged folder whose `rstack` is not installed — the not-installed state + * the three stacks report uniformly (AGENTS.md): a `disabled` status and a + * one-line warning, never a crash or a stack trace. `missing-core` is not it: + * rstack is there but unusable, which is worth the full error. + */ +export const isMissingRstackFailure = (error: unknown): boolean => + error instanceof RslintResolutionError && error.code === 'missing-rstack'; + export const statusForRslintStartFailure = (error: unknown): StackState => { if (error instanceof RslintVersionMismatchError) { return { kind: 'version-mismatch', detail: error.message }; } - if ( - error instanceof RslintResolutionError && - error.code === 'missing-rstack' - ) { + if (isMissingRstackFailure(error)) { return { kind: 'disabled', - reason: - 'rstack is not installed (node_modules missing) — install it, then restart Rslint if this status stays', + reason: formatNotInstalledStatus('rslint', 'rstack'), }; } return { diff --git a/packages/vscode/src/stacks/test/bridge.ts b/packages/vscode/src/stacks/test/bridge.ts index 581a941..58626ce 100644 --- a/packages/vscode/src/stacks/test/bridge.ts +++ b/packages/vscode/src/stacks/test/bridge.ts @@ -6,6 +6,7 @@ import { formatVersionMismatch, readPackageVersion, } from '../../shared/versionCheck'; +import { formatNotInstalledStatus } from '../../shared/notInstalled'; import { logger } from './logger'; import { status } from './status'; @@ -76,6 +77,12 @@ export function resolveRstackShim( `Cannot find the "rstack" package from ${configDir}. Rstest cannot be driven by "rstack.config.*" until the project dependencies are installed.`, ); } + // Latched like the version mismatch below, under the same key, so the + // status stays until this directory resolves or stops being a candidate. + status.notInstalled( + formatNotInstalledStatus('rstest', 'rstack'), + configDir, + ); return undefined; } @@ -110,6 +117,7 @@ export function resolveRstackShim( packageDirectory, version, }); + status.installed(configDir); status.versionOk(configDir); return { configFilePath, packageDirectory, version }; } diff --git a/packages/vscode/src/stacks/test/coreResolution.ts b/packages/vscode/src/stacks/test/coreResolution.ts index 1ff00bd..cb8a5bc 100644 --- a/packages/vscode/src/stacks/test/coreResolution.ts +++ b/packages/vscode/src/stacks/test/coreResolution.ts @@ -40,12 +40,31 @@ export function isModuleNotFoundError( ); } -export function formatCoreNotFoundMessage(searchedFrom: string): string { - return `Cannot find "@rstest/core" from ${searchedFrom}. Install the project dependencies, then refresh the Test Explorer. If Rstest is installed elsewhere, set "rstack.rstest.rstestPackagePath" to its package.json.`; -} - export function formatConfiguredCoreNotFoundMessage( configuredPackagePath: string, ): string { return `Cannot find "@rstest/core" at the configured "rstack.rstest.rstestPackagePath": ${configuredPackagePath}. Update the setting to point at an installed "@rstest/core" package.json.`; } + +// Whether a config evaluation failed because something it imports is not +// installed. Read from the error's `code` — Node's own classification, set by +// both loaders (`ERR_MODULE_NOT_FOUND` for ESM, `MODULE_NOT_FOUND` for CJS) — +// never from the message text. Any other failure (a syntax error in the +// config, a thrown plugin) is a real error. The check has to run in the +// worker, where the error is thrown: the IPC channel back to the extension +// host (`serialization: 'advanced'`) keeps an Error's message and stack but +// drops its `code`, so the classification is carried as data instead +// (`NormalizedConfigResult`). +export function isMissingDependencyError(error: unknown): boolean { + if (!(error instanceof Error)) return false; + const { code } = error as NodeJS.ErrnoException; + return code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND'; +} + +/** `cause` is the loader's own text, which names the specifier and the importer. */ +export function formatConfigDependencyMissingMessage( + configFilePath: string, + cause: string, +): string { + return `Cannot load ${configFilePath}: ${cause}. Install the project dependencies to enable Rstest for this config.`; +} diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index 78c7459..ad35d21 100644 --- a/packages/vscode/src/stacks/test/master.ts +++ b/packages/vscode/src/stacks/test/master.ts @@ -17,9 +17,12 @@ import { getConfiguredNodeExecutable, } from '../../shared/nodeExecutableSetting'; import { CONFIG_SECTION, getConfigValue } from './config'; +import { + formatNotInstalledLog, + formatNotInstalledStatus, +} from '../../shared/notInstalled'; import { formatConfiguredCoreNotFoundMessage, - formatCoreNotFoundMessage, isModuleNotFoundError, ReportedRstestResolutionError, } from './coreResolution'; @@ -40,6 +43,15 @@ import { TestRunReporter } from './testRunReporter'; import { toErrorMessage } from './utils'; import type { Worker } from './worker'; +// Both arguments are literals, so the status is one string for the master's +// lifetime; the log's consequence is the one recovery the shared line cannot +// know about — the setting that points Rstest at a core installed elsewhere. +const CORE_NOT_INSTALLED_STATUS = formatNotInstalledStatus( + 'rstest', + '@rstest/core', +); +const CORE_NOT_INSTALLED_CONSEQUENCE = `install the project dependencies, or set "${CONFIG_SECTION}.rstestPackagePath" to an installed @rstest/core package.json`; + export const runningWorkers = new Set>(); /** @@ -313,11 +325,26 @@ export class RstestApi { formatConfiguredCoreNotFoundMessage(configuredPackagePath), ); } - logger.error(formatCoreNotFoundMessage(fromDir)); + this.reportCoreNotInstalled(fromDir); return undefined; } } + // The not-installed policy (AGENTS.md): a `disabled` status naming the way + // out plus one warn line — the normal state of a repository whose + // dependencies are not installed yet, never a notification. + private reportCoreNotInstalled(searchedFrom: string): void { + logger.warn( + formatNotInstalledLog( + '@rstest/core', + this.workspace.name, + searchedFrom, + CORE_NOT_INSTALLED_CONSEQUENCE, + ), + ); + status.notInstalled(CORE_NOT_INSTALLED_STATUS, this.statusSource); + } + // Returns '' when resolution failed. Every such branch has already reported // itself — silently for a missing core, with a notification otherwise — so // callers must fail quietly rather than report again. @@ -356,9 +383,7 @@ export class RstestApi { this.rstestResolutionDir, ); if (!found) { - // The normal state of a repository whose dependencies are not - // installed yet: output channel only, never a notification. - logger.error(formatCoreNotFoundMessage(this.rstestResolutionDir)); + this.reportCoreNotInstalled(this.rstestResolutionDir); return ''; } corePackageJsonPath = found; @@ -384,6 +409,8 @@ export class RstestApi { // and a mismatch re-latched now — for a root that may never come back — // would have nothing left to clear it. if (!this.disposed) { + // The core resolved: this root's missing install, if any, is over. + status.installed(this.statusSource); if ( !reportVersionCheck( status, @@ -423,7 +450,7 @@ export class RstestApi { this.rstestResolutionDir, ); if (!pkgJsonPath) { - logger.error(formatCoreNotFoundMessage(this.rstestResolutionDir)); + this.reportCoreNotInstalled(this.rstestResolutionDir); } } if (!pkgJsonPath) return undefined; @@ -439,12 +466,12 @@ export class RstestApi { public async getNormalizedConfig() { const worker = await this.createChildProcess(); - const config = await worker.getNormalizedConfig({ + const result = await worker.getNormalizedConfig({ rstestPath: this.resolveRstestPath(), configFilePath: this.configFilePath, }); worker.$close(); - return config; + return result; } public async listTests(include?: string[]) { diff --git a/packages/vscode/src/stacks/test/project.ts b/packages/vscode/src/stacks/test/project.ts index 61849b0..1ea50bf 100644 --- a/packages/vscode/src/stacks/test/project.ts +++ b/packages/vscode/src/stacks/test/project.ts @@ -6,7 +6,11 @@ import vscode from 'vscode'; import { RSTACK_CONFIG_NAMES } from '../../detection'; import { resolveRstackShim } from './bridge'; import { watchConfigValue } from './config'; -import { ReportedRstestResolutionError } from './coreResolution'; +import { formatConfigDependencyMissingStatus } from '../../shared/notInstalled'; +import { + formatConfigDependencyMissingMessage, + ReportedRstestResolutionError, +} from './coreResolution'; import { logger } from './logger'; import { RstestApi } from './master'; import { type ChildProjectRef, computeCoveredConfigs } from './projectCoverage'; @@ -21,6 +25,10 @@ import { ProjectFolder, TestFile, TestFolder, testData } from './testTree'; // `rstest.config.ts`. const DEFAULT_ROOT_CONFIG_RE = /^(?:rstest|rstack)\.config\.[mc]?[tj]s$/; +/** A config path as the user sees it: relative to its workspace folder. */ +const relativeTo = (folder: vscode.WorkspaceFolder, uri: vscode.Uri): string => + path.relative(folder.uri.fsPath, uri.fsPath); + /** * Where a `Project`'s config comes from. The worker-cwd decoupling adaptation * plus the rstack bridge are both expressed here: upstream had only a config @@ -396,10 +404,7 @@ export class WorkspaceManager implements vscode.Disposable { // at the workspace root): show its test files directly, with no project node. if (activeProjects.size === 1) { const [[, project]] = activeProjects; - const relative = path.relative( - this.workspaceFolder.uri.fsPath, - project.sourceUri.fsPath, - ); + const relative = relativeTo(this.workspaceFolder, project.sourceUri); if (DEFAULT_ROOT_CONFIG_RE.test(relative)) { project.refresh(collection, null); return; @@ -452,10 +457,7 @@ export class WorkspaceManager implements vscode.Disposable { const root: TreeNode = { children: new Map() }; for (const project of projects.values()) { - const relative = path.relative( - this.workspaceFolder.uri.fsPath, - project.sourceUri.fsPath, - ); + const relative = relativeTo(this.workspaceFolder, project.sourceUri); let node = root; for (const segment of relative.split(path.sep)) { let next = node.children.get(segment); @@ -587,12 +589,17 @@ export class Project implements vscode.Disposable { void this.api .getNormalizedConfig() - .then((config) => { + .then((result) => { if (this.cancellationSource.token.isCancellationRequested) return; - this.root = vscode.Uri.file(config.root); - this.include = config.include; - this.exclude = config.exclude; - this.childProjects = config.childProjects; + if (!result.ok) { + this.reportMissingDependency(result.message); + return; + } + status.installed(this.sourceUri.toString()); + this.root = vscode.Uri.file(result.root); + this.include = result.include; + this.exclude = result.exclude; + this.childProjects = result.childProjects; this.applyWatch(); this.onConfigResolved?.(); }) @@ -607,6 +614,26 @@ export class Project implements vscode.Disposable { }); } + // The config imports a package that is not installed: the not-installed + // state (AGENTS.md), one step past a missing `@rstest/core` — some install + // *above* the project satisfied the shim, so the config itself is what + // failed. A scaffolded template beside its generator is the usual shape. + // Latched under this project's key, which `dispose` forgets. + private reportMissingDependency(cause: string): void { + this.configLoadFailed = true; + logger.warn( + formatConfigDependencyMissingMessage(this.sourceUri.fsPath, cause), + ); + status.notInstalled( + formatConfigDependencyMissingStatus( + 'rstest', + relativeTo(this.workspaceFolder, this.sourceUri), + ), + this.sourceUri.toString(), + ); + this.onConfigResolved?.(); + } + /** The config file path Rstest is asked to load (`-c`). */ get configFilePath(): string { return this.configFileUri.fsPath; diff --git a/packages/vscode/src/stacks/test/status.ts b/packages/vscode/src/stacks/test/status.ts index 06ba883..95a8718 100644 --- a/packages/vscode/src/stacks/test/status.ts +++ b/packages/vscode/src/stacks/test/status.ts @@ -44,6 +44,13 @@ class StatusHolder implements StatusReporter { // stack, which is exactly the lifetime of the resolution it reports on. #crashes = new Map(); #mismatches = new Map(); + // A root whose dependencies are not installed — `@rstest/core` missing, the + // bridge's `rstack` missing, or a config importing a package that is not + // there. The uniform not-installed policy (AGENTS.md): a `disabled` status + // with the way out in its reason, as fmt and lint report it, never a crash + // and never a notification. Cleared by the resolution that succeeds under + // the same key (`installed`) or by `forget`. + #notInstalled = new Map(); #lastRunningDetail: string | undefined; get stack() { @@ -54,6 +61,7 @@ class StatusHolder implements StatusReporter { this.#reporter = reporter; this.#crashes.clear(); this.#mismatches.clear(); + this.#notInstalled.clear(); this.#lastRunningDetail = undefined; } @@ -62,13 +70,18 @@ class StatusHolder implements StatusReporter { } get #latched(): boolean { - return this.#crashes.size > 0 || this.#mismatches.size > 0; + return ( + this.#crashes.size > 0 || + this.#mismatches.size > 0 || + this.#notInstalled.size > 0 + ); } /** - * Worst live state first: a crash outranks a version mismatch. Within one - * severity the oldest unrecovered entry wins, keeping the display stable - * while other roots come and go. + * Worst live state first: a crash outranks a version mismatch, which + * outranks a missing install — the same rank the fmt and lint folds use. + * Within one severity the oldest unrecovered entry wins, keeping the + * display stable while other roots come and go. */ #paintOrRun(): void { const [crash] = this.#crashes.values(); @@ -81,6 +94,11 @@ class StatusHolder implements StatusReporter { this.#reporter?.versionMismatch(mismatch); return; } + const [notInstalled] = this.#notInstalled.values(); + if (notInstalled !== undefined) { + this.#reporter?.report({ kind: 'disabled', reason: notInstalled }); + return; + } this.#reporter?.running(this.#lastRunningDetail); } @@ -121,6 +139,24 @@ class StatusHolder implements StatusReporter { this.#paintOrRun(); } + /** + * A root's dependencies are not installed: `disabled`, with the way out. + * Unlike the crash and mismatch latches this one is re-raised on every + * refresh pass and worker spawn with the same words, so an unchanged entry + * is not repainted. + */ + notInstalled(reason: string, source = ''): void { + if (this.#notInstalled.get(source) === reason) return; + this.#notInstalled.set(source, reason); + this.#paintOrRun(); + } + + /** A resolution under that root succeeded: its missing install is over. */ + installed(source = ''): void { + if (!this.#notInstalled.delete(source)) return; + this.#paintOrRun(); + } + /** * A resolution root went away (its config file or workspace folder was * removed) without recovering: its failures must not outlive it and keep @@ -129,7 +165,8 @@ class StatusHolder implements StatusReporter { forget(source: string): void { const hadCrash = this.#crashes.delete(source); const hadMismatch = this.#mismatches.delete(source); - if (hadCrash || hadMismatch) this.#paintOrRun(); + const hadNotInstalled = this.#notInstalled.delete(source); + if (hadCrash || hadMismatch || hadNotInstalled) this.#paintOrRun(); } } diff --git a/packages/vscode/src/stacks/test/types.ts b/packages/vscode/src/stacks/test/types.ts index ddd541e..0167078 100644 --- a/packages/vscode/src/stacks/test/types.ts +++ b/packages/vscode/src/stacks/test/types.ts @@ -7,3 +7,20 @@ export type WorkerInitOptions = RstestConfig & { rstestPath: string; command?: 'run' | 'list' | 'watch'; }; + +/** + * What the worker answers `getNormalizedConfig` with. A config that fails to + * evaluate because a dependency is not installed is a result, not a rejection: + * the IPC channel would strip the error's `code` (see + * `isMissingDependencyError`), so the worker classifies it and reports the + * loader's message as data. + */ +export type NormalizedConfigResult = + | { + ok: true; + root: string; + include: string[]; + exclude: string[]; + childProjects: { configFilePath: string | null; root: string | null }[]; + } + | { ok: false; reason: 'missing-dependency'; message: string }; diff --git a/packages/vscode/src/stacks/test/worker/index.ts b/packages/vscode/src/stacks/test/worker/index.ts index 514bed7..414067d 100644 --- a/packages/vscode/src/stacks/test/worker/index.ts +++ b/packages/vscode/src/stacks/test/worker/index.ts @@ -1,7 +1,8 @@ import { pathToFileURL } from 'node:url'; import { createBirpc } from 'birpc'; import type { TestRunReporter } from '../testRunReporter'; -import type { WorkerInitOptions } from '../types'; +import { isMissingDependencyError } from '../coreResolution'; +import type { NormalizedConfigResult, WorkerInitOptions } from '../types'; import { logger } from './logger'; import { CoverageReporter, ProgressLogger, ProgressReporter } from './reporter'; @@ -56,23 +57,41 @@ export class Worker { return { rstest, projects }; } - public async getNormalizedConfig(options: WorkerInitOptions) { - const { rstest, projects } = await this.init(options); - return { - root: rstest.context.normalizedConfig.root, - include: rstest.context.normalizedConfig.include, - exclude: rstest.context.normalizedConfig.exclude.patterns, - // Sub-projects this config aggregates via `projects`. Empty for a leaf - // config. The extension uses these to avoid registering a child config - // as its own top-level project when a parent already covers it - // (otherwise the same test files show up twice). A file-based child is - // identified by its config file; inline children only have a root. - // `null` (not `undefined`) so the fields survive the IPC JSON round-trip. - childProjects: projects.map((project) => ({ - configFilePath: project.configFilePath ?? null, - root: project.config.root ?? null, - })), - }; + public async getNormalizedConfig( + options: WorkerInitOptions, + ): Promise { + try { + const { rstest, projects } = await this.init(options); + return { + ok: true, + root: rstest.context.normalizedConfig.root, + include: rstest.context.normalizedConfig.include, + exclude: rstest.context.normalizedConfig.exclude.patterns, + // Sub-projects this config aggregates via `projects`. Empty for a + // leaf config. The extension uses these to avoid registering a child + // config as its own top-level project when a parent already covers + // it (otherwise the same test files show up twice). A file-based + // child is identified by its config file; inline children only have + // a root. `null` (not `undefined`) so the fields survive the IPC + // JSON round-trip. + childProjects: projects.map((project) => ({ + configFilePath: project.configFilePath ?? null, + root: project.config.root ?? null, + })), + }; + } catch (error) { + // Classified here and not in the master: `code` does not survive the + // IPC round-trip. Only this unprompted, per-config evaluation gets the + // treatment — a run or list the user asked for reports its failure. + if (isMissingDependencyError(error)) { + return { + ok: false, + reason: 'missing-dependency', + message: (error as Error).message, + }; + } + throw error; + } } public async runTest(data: WorkerInitOptions) { diff --git a/packages/vscode/src/types.ts b/packages/vscode/src/types.ts index 1a03418..a71bfd5 100644 --- a/packages/vscode/src/types.ts +++ b/packages/vscode/src/types.ts @@ -26,6 +26,19 @@ export const stackCommand = ( verb: 'restart' | 'output.focus', ): string => `rstack.${stack}.${verb}`; +/** The `category` every contributed command shares in package.json. */ +export const COMMAND_CATEGORY = 'Rstack'; + +/** + * The title side of `stackCommand`, spelled once for the same reason: a + * status that tells the user which command to run has to say what the + * Command Palette shows (`: `), and a status naming a + * command that was renamed is not a type error. `tests/extension.test.ts` + * checks the manifest against this. + */ +export const stackCommandTitle = (stack: StackId, verb: 'restart'): string => + `${verb === 'restart' ? 'Restart' : verb} ${STACK_LABELS[stack]}`; + /** * Per-stack state machine surfaced by the status bar hover. * diff --git a/packages/vscode/tests/extension.test.ts b/packages/vscode/tests/extension.test.ts index b392bb2..5f8515d 100644 --- a/packages/vscode/tests/extension.test.ts +++ b/packages/vscode/tests/extension.test.ts @@ -8,6 +8,12 @@ */ import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; import type vscode from 'vscode'; +import { + COMMAND_CATEGORY, + STACK_IDS, + stackCommand, + stackCommandTitle, +} from '../src/types'; interface FakeController { readonly restartOnSettings?: readonly string[]; @@ -448,7 +454,7 @@ describe('the shell restart command', () => { // adds back by symmetry. const manifest = require('../package.json') as { contributes: { - commands: Array<{ command: string }>; + commands: Array<{ command: string; title: string; category?: string }>; menus: { commandPalette: Array<{ command: string; when: string }> }; }; }; @@ -461,13 +467,24 @@ describe('the shell restart command', () => { // The per-stack ones are the opposite: they only make sense for a stack // that is up, so each is gated on its own context key. - for (const stack of ['rslint', 'rstest', 'fmt']) { - expect(harness.commands.has(`rstack.${stack}.restart`)).toBe(true); + for (const stack of STACK_IDS) { + const command = stackCommand(stack, 'restart'); + expect(harness.commands.has(command)).toBe(true); expect( manifest.contributes.menus.commandPalette.find( - (entry) => entry.command === `rstack.${stack}.restart`, + (entry) => entry.command === command, )?.when, ).toBe(`rstack.${stack}.active`); + // The not-installed statuses tell the user to run this command by its + // palette name (`shared/notInstalled.ts`); the manifest is the truth. + expect( + manifest.contributes.commands.find( + (entry) => entry.command === command, + ), + ).toMatchObject({ + title: stackCommandTitle(stack, 'restart'), + category: COMMAND_CATEGORY, + }); } }); diff --git a/packages/vscode/tests/shared/notInstalled.test.ts b/packages/vscode/tests/shared/notInstalled.test.ts new file mode 100644 index 0000000..3d1c886 --- /dev/null +++ b/packages/vscode/tests/shared/notInstalled.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from '@rstest/core'; +import { + formatConfigDependencyMissingStatus, + formatNotInstalledLog, + formatNotInstalledStatus, +} from '../../src/shared/notInstalled'; + +// One wording for the three stacks: the restart hint names the stack's own +// command exactly as the Command Palette shows it (`category: title` in +// package.json), so the user can type what the status says. +describe('not-installed wording', () => { + it('names each stack’s restart command in the status reason', () => { + expect(formatNotInstalledStatus('fmt', 'rstack')).toBe( + 'rstack is not installed (node_modules missing) — install it, then run "Rstack: Restart rs fmt" if this status stays', + ); + expect(formatNotInstalledStatus('rslint', 'rstack')).toContain( + '"Rstack: Restart Rslint"', + ); + expect(formatNotInstalledStatus('rstest', '@rstest/core')).toBe( + '@rstest/core is not installed (node_modules missing) — install it, then run "Rstack: Restart Rstest" if this status stays', + ); + }); + + it('names the config and the same way out for a missing config import', () => { + expect( + formatConfigDependencyMissingStatus( + 'rstest', + 'templates/app/rstack.config.ts', + ), + ).toBe( + 'templates/app/rstack.config.ts imports a package that is not installed — install the project dependencies, then run "Rstack: Restart Rstest" if this status stays', + ); + }); + + it('logs where the stack looked', () => { + expect(formatNotInstalledLog('rstack', 'app', '/repo/app')).toBe( + 'rstack is not installed in app (node_modules missing); searched from /repo/app', + ); + }); +}); diff --git a/packages/vscode/tests/stacks/lint/status.test.ts b/packages/vscode/tests/stacks/lint/status.test.ts index 8db589e..998d715 100644 --- a/packages/vscode/tests/stacks/lint/status.test.ts +++ b/packages/vscode/tests/stacks/lint/status.test.ts @@ -4,6 +4,7 @@ import { aggregateFolderStates, attributeToCore, foldRslintFolderState, + isMissingRstackFailure, RslintVersionMismatchError, runningRslintStatus, statusForRslintStartFailure, @@ -18,6 +19,22 @@ describe('Rslint status classification', () => { ).toMatchObject({ kind: 'disabled' }); }); + it('tells the not-installed state apart from an unusable install', () => { + // The same boundary the status uses: only a missing `rstack` is the + // warn-level, disabled state; a present-but-broken one is an error. + expect( + isMissingRstackFailure( + new RslintResolutionError('missing-rstack', 'missing rstack'), + ), + ).toBe(true); + expect( + isMissingRstackFailure( + new RslintResolutionError('missing-core', 'missing core'), + ), + ).toBe(false); + expect(isMissingRstackFailure(new Error('missing rstack'))).toBe(false); + }); + it('classifies missing core and worker failures as crashes', () => { expect( statusForRslintStartFailure( diff --git a/packages/vscode/tests/stacks/test/bridge.test.ts b/packages/vscode/tests/stacks/test/bridge.test.ts index 9ab73e2..73b8935 100644 --- a/packages/vscode/tests/stacks/test/bridge.test.ts +++ b/packages/vscode/tests/stacks/test/bridge.test.ts @@ -2,10 +2,10 @@ 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 type { StackState, StatusReporter } from '../../../src/types'; import { resolveRstackShim } from '../../../src/stacks/test/bridge'; import { logger } from '../../../src/stacks/test/logger'; import { status } from '../../../src/stacks/test/status'; +import { createStatusRecorder } from './statusRecorder'; // Resolution is exercised for real (a temporary `node_modules/rstack` tree) // rather than by mocking `nodeRequire`: the whole point of the bridge is that @@ -13,7 +13,7 @@ import { status } from '../../../src/stacks/test/status'; // resolver would only assert itself. const logged: string[] = []; -const reported: StackState[] = []; +const { reporter, reported } = createStatusRecorder(); const channel = { debug: (message: string) => logged.push(message), @@ -24,16 +24,6 @@ const channel = { dispose: () => {}, }; -const reporter: StatusReporter = { - stack: 'rstest', - report: (state) => reported.push(state), - starting: (detail) => reported.push({ kind: 'starting', detail }), - running: (detail) => reported.push({ kind: 'running', detail }), - crashed: (detail) => reported.push({ kind: 'crashed', detail }), - versionMismatch: (detail) => - reported.push({ kind: 'version-mismatch', detail }), -}; - const tmpDirs: string[] = []; // `os.tmpdir()` is a symlink on macOS (`/var` -> `/private/var`) and Node's @@ -114,12 +104,37 @@ describe('resolveRstackShim', () => { expect(fs.existsSync(shim!.configFilePath)).toBe(true); }); - it('reports nothing when the rstack package is not installed', () => { + it('reports a disabled status, not a crash, when the rstack package is not installed', () => { const root = makeTmpDir(); expect(resolveRstackShim(root)).toBeUndefined(); expect(logged.join('\n')).toContain('Cannot find the "rstack" package'); - expect(reported).toEqual([]); + // The uniform not-installed policy: the same `disabled` shape fmt and + // lint report, with the restart command as the way out. + expect(reported).toEqual([ + { + kind: 'disabled', + reason: + 'rstack is not installed (node_modules missing) — install it, then run "Rstack: Restart Rstest" if this status stays', + }, + ]); + }); + + it('clears the disabled status once the directory resolves', () => { + const configDir = createWorkspace(); + const rstackDir = path.join(configDir, 'node_modules', 'rstack'); + const parked = `${rstackDir}.parked`; + fs.renameSync(rstackDir, parked); + + expect(resolveRstackShim(configDir)).toBeUndefined(); + expect(reported.map((state) => state.kind)).toEqual(['disabled']); + + fs.renameSync(parked, rstackDir); + expect(resolveRstackShim(configDir)).toBeDefined(); + expect(reported.map((state) => state.kind)).toEqual([ + 'disabled', + 'running', + ]); }); it('stays silent on a repeated failure', () => { diff --git a/packages/vscode/tests/stacks/test/coreResolution.test.ts b/packages/vscode/tests/stacks/test/coreResolution.test.ts index 4bb9061..bd546f7 100644 --- a/packages/vscode/tests/stacks/test/coreResolution.test.ts +++ b/packages/vscode/tests/stacks/test/coreResolution.test.ts @@ -3,8 +3,9 @@ import os from 'node:os'; import path from 'node:path'; import { describe, expect, it } from '@rstest/core'; import { + formatConfigDependencyMissingMessage, formatConfiguredCoreNotFoundMessage, - formatCoreNotFoundMessage, + isMissingDependencyError, isModuleNotFoundError, } from '../../../src/stacks/test/coreResolution'; @@ -62,8 +63,57 @@ describe('core-not-found messages', () => { ); expect(message).toContain('/repo/vendor/core/package.json'); expect(message).not.toContain('Install the project dependencies'); - expect(formatCoreNotFoundMessage('/repo/app')).toContain( - 'Install the project dependencies', + }); +}); + +describe('isMissingDependencyError', () => { + // Same reasoning as `resolveError`: the predicate reads a code Node owns, + // so the errors come from Node's own loaders. + const importError = async (specifier: string): Promise<unknown> => { + try { + await import(specifier); + } catch (e) { + return e; + } + throw new Error(`expected "${specifier}" not to import`); + }; + + it('should detect a package an ESM config failed to import', async () => { + expect( + isMissingDependencyError( + await importError('@rstest/definitely-not-installed'), + ), + ).toBe(true); + }); + + it('should detect a package a CJS config failed to require', () => { + expect( + isMissingDependencyError( + resolveError('@rstest/definitely-not-installed', __dirname), + ), + ).toBe(true); + }); + + it('should leave every other failure to the full error report', () => { + expect(isMissingDependencyError(new SyntaxError('Unexpected token'))).toBe( + false, + ); + expect(isMissingDependencyError(new Error("Cannot find package 'x'"))).toBe( + false, + ); + expect(isMissingDependencyError("Cannot find package 'x'")).toBe(false); + expect(isMissingDependencyError(undefined)).toBe(false); + }); +}); + +describe('formatConfigDependencyMissingMessage', () => { + it("should name the config, the loader's own words and the way out", () => { + const message = formatConfigDependencyMissingMessage( + '/repo/templates/app/rstack.config.ts', + "Cannot find package '@rsbuild/plugin-react' imported from /repo/templates/app/rstack.config.ts", ); + expect(message).toContain('/repo/templates/app/rstack.config.ts'); + expect(message).toContain("Cannot find package '@rsbuild/plugin-react'"); + expect(message).toContain('Install the project dependencies'); }); }); diff --git a/packages/vscode/tests/stacks/test/master.test.ts b/packages/vscode/tests/stacks/test/master.test.ts index 296b2d0..8d024d0 100644 --- a/packages/vscode/tests/stacks/test/master.test.ts +++ b/packages/vscode/tests/stacks/test/master.test.ts @@ -12,6 +12,7 @@ import { } from '../../../src/shared/nodeResolution'; import { status } from '../../../src/stacks/test/status'; import type { StatusReporter } from '../../../src/types'; +import { createStatusRecorder } from './statusRecorder'; // The Rstest runner injects its own `@rstest/core` into every resolution path so // that test files can import it, which makes "the project has no @rstest/core" @@ -59,13 +60,14 @@ rs.mock('../../../src/stacks/test/nodeRequire', () => { // output channel, and the terminal a "Run in Terminal" would open. const shownMessages: string[] = []; const loggedErrors: string[] = []; +const loggedWarnings: string[] = []; const createdTerminals: string[] = []; const settings: Record<string, unknown> = {}; const channel = { debug: () => {}, info: () => {}, - warn: () => {}, + warn: (message: string) => loggedWarnings.push(message), error: (message: string) => loggedErrors.push(message), show: () => {}, dispose: () => {}, @@ -223,15 +225,33 @@ describe('RstestApi with a missing @rstest/core', () => { for (const key of Object.keys(settings)) delete settings[key]; }); - it('should log an actionable message instead of notifying, while discovering projects', async () => { - await expect(createApi().getNormalizedConfig()).rejects.toThrow( - 'Failed to resolve rstest path', - ); + it('should warn with the way out instead of notifying, while discovering projects', async () => { + // The uniform not-installed policy: a `disabled` status with the way + // out plus one warn line, never a crash and never a notification. + loggedWarnings.length = 0; + const { reporter, reported } = createStatusRecorder(); + status.bind(reporter); + try { + await expect(createApi().getNormalizedConfig()).rejects.toThrow( + 'Failed to resolve rstest path', + ); + } finally { + status.unbind(); + } expect(shownMessages).toEqual([]); - const logged = loggedErrors.join('\n'); - expect(logged).toContain(`Cannot find "@rstest/core" from ${noCoreDir}`); - expect(logged).toContain('Install the project dependencies'); + expect(loggedErrors).toEqual([]); + const logged = loggedWarnings.join('\n'); + expect(logged).toContain('@rstest/core is not installed'); + expect(logged).toContain(`searched from ${noCoreDir}`); + expect(logged).toContain('rstestPackagePath'); expect(logged).not.toContain('Require stack'); + expect(reported).toEqual([ + { + kind: 'disabled', + reason: + '@rstest/core is not installed (node_modules missing) — install it, then run "Rstack: Restart Rstest" if this status stays', + }, + ]); }); it('should stay silent while listing tests', async () => { diff --git a/packages/vscode/tests/stacks/test/project.test.ts b/packages/vscode/tests/stacks/test/project.test.ts index 6957972..940b530 100644 --- a/packages/vscode/tests/stacks/test/project.test.ts +++ b/packages/vscode/tests/stacks/test/project.test.ts @@ -2,6 +2,9 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; import { ReportedRstestResolutionError } from '../../../src/stacks/test/coreResolution'; import { logger } from '../../../src/stacks/test/logger'; +import { status } from '../../../src/stacks/test/status'; +import type { NormalizedConfigResult } from '../../../src/stacks/test/types'; +import { createStatusRecorder } from './statusRecorder'; // The worker-cwd decoupling adaptation pinned at its only site: upstream derived the // worker spawn cwd inside `Project` as `dirname(configFileUri)`, so a `Project` @@ -16,6 +19,7 @@ const apiCalls: { rstestResolutionDir: string; }[] = []; let normalizedConfigFailure: unknown; +let normalizedConfigResult: NormalizedConfigResult | undefined; rs.mock('../../../src/stacks/test/master', () => { class RstestApi { @@ -34,6 +38,9 @@ rs.mock('../../../src/stacks/test/master', () => { if (normalizedConfigFailure) { return Promise.reject(normalizedConfigFailure); } + if (normalizedConfigResult) { + return Promise.resolve(normalizedConfigResult); + } return new Promise<never>(() => {}); } dispose() {} @@ -44,10 +51,11 @@ rs.mock('../../../src/stacks/test/master', () => { // One log-channel double for both the vscode mock and `logger.bind`, so the // assertions below observe every stack log line. const loggedErrors: string[] = []; +const loggedWarnings: string[] = []; const channel = { debug: () => {}, info: () => {}, - warn: () => {}, + warn: (message: string) => loggedWarnings.push(message), error: (message: string) => loggedErrors.push(message), show: () => {}, dispose: () => {}, @@ -124,7 +132,9 @@ const collection = { beforeEach(() => { normalizedConfigFailure = undefined; + normalizedConfigResult = undefined; loggedErrors.length = 0; + loggedWarnings.length = 0; logger.bind(channel as never); }); @@ -204,4 +214,54 @@ describe('Project config/cwd/package-resolution decoupling', () => { expect(project.configLoadFailed).toBe(true); expect(loggedErrors).toEqual([]); }); + + it('reports a config whose dependency is not installed as one warning line', async () => { + // A scaffolded template beside its generator: its own dependencies are + // never installed, but the walk-up finds the generator's `rstack`, so the + // shim loads and the config's own import is what fails. + const rstackConfig = uri( + path.join('/repo', 'templates', 'app', 'rstack.config.ts'), + ); + // The worker's verdict, as data: it classified the failure where the + // error's `code` still existed. + normalizedConfigResult = { + ok: false, + reason: 'missing-dependency', + message: `Cannot find package '@rsbuild/plugin-react' imported from ${rstackConfig.fsPath}`, + }; + const { reporter, reported } = createStatusRecorder(); + status.bind(reporter); + + const { project } = await createProject({ + sourceUri: rstackConfig, + configFileUri: uri('/repo/node_modules/rstack/dist/rstestConfig.js'), + cwd: path.dirname(rstackConfig.fsPath), + rstestResolutionDir: '/repo/node_modules/rstack', + isBridge: true, + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(project.configLoadFailed).toBe(true); + expect(loggedErrors).toEqual([]); + expect(loggedWarnings).toHaveLength(1); + expect(loggedWarnings[0]).toContain(rstackConfig.fsPath); + expect(loggedWarnings[0]).toContain( + "Cannot find package '@rsbuild/plugin-react'", + ); + expect(loggedWarnings[0]).toContain('Install the project dependencies'); + // The status bar side: `disabled` naming the config (workspace-relative) + // and the way out — the same shape fmt and lint report. + expect(reported).toEqual([ + { + kind: 'disabled', + reason: + 'templates/app/rstack.config.ts imports a package that is not installed — install the project dependencies, then run "Rstack: Restart Rstest" if this status stays', + }, + ]); + + // Disposal forgets the latch, so the detection-driven retry starts clean. + project.dispose(); + expect(reported.at(-1)).toEqual({ kind: 'running', detail: undefined }); + status.unbind(); + }); }); diff --git a/packages/vscode/tests/stacks/test/status.test.ts b/packages/vscode/tests/stacks/test/status.test.ts index c3a96cb..41cdd4b 100644 --- a/packages/vscode/tests/stacks/test/status.test.ts +++ b/packages/vscode/tests/stacks/test/status.test.ts @@ -92,8 +92,52 @@ describe('StatusHolder failure latches', () => { it('drops stale latches on bind', () => { bindRecorder(); status.versionMismatch('core too old', '/a'); + status.notInstalled('core missing', '/b'); const calls = bindRecorder(); status.running(); expect(calls).toEqual(['running:']); }); + + it('paints a missing install as disabled and keeps it across running repaints', () => { + const calls = bindRecorder(); + status.notInstalled('core missing', '/a'); + status.running('2 folders'); + status.starting(); + expect(calls).toEqual(['report:disabled']); + }); + + it('ranks a missing install below a mismatch and a crash', () => { + const calls = bindRecorder(); + status.notInstalled('core missing', '/a'); + status.versionMismatch('core too old', '/b'); + status.crashed('spawn ENOENT', '/c'); + status.workerSpawned('/c'); + status.versionOk('/b'); + expect(calls).toEqual([ + 'report:disabled', + 'mismatch:core too old', + 'crashed:spawn ENOENT', + 'mismatch:core too old', + 'report:disabled', + ]); + }); + + it('clears one root’s missing install without touching another’s', () => { + const calls = bindRecorder(); + status.notInstalled('core missing', '/a'); + status.installed('/b'); + status.running('2 folders'); + expect(calls).toEqual(['report:disabled']); + + status.installed('/a'); + expect(calls).toEqual(['report:disabled', 'running:2 folders']); + }); + + it('forgets a removed root’s missing install and repaints', () => { + const calls = bindRecorder(); + status.notInstalled('core missing', '/a'); + status.running('1 folder'); + status.forget('/a'); + expect(calls).toEqual(['report:disabled', 'running:1 folder']); + }); }); diff --git a/packages/vscode/tests/stacks/test/statusRecorder.ts b/packages/vscode/tests/stacks/test/statusRecorder.ts new file mode 100644 index 0000000..44b7859 --- /dev/null +++ b/packages/vscode/tests/stacks/test/statusRecorder.ts @@ -0,0 +1,23 @@ +import type { StackState, StatusReporter } from '../../../src/types'; + +/** + * A `StatusReporter` that records every state it is handed, in the shape the + * shell's status bar would receive. One recorder for every suite that binds + * the Rstest `status` singleton, so the double is not retyped per file. + */ +export const createStatusRecorder = (): { + reporter: StatusReporter; + reported: StackState[]; +} => { + const reported: StackState[] = []; + const reporter: StatusReporter = { + stack: 'rstest', + report: (state) => reported.push(state), + starting: (detail) => reported.push({ kind: 'starting', detail }), + running: (detail) => reported.push({ kind: 'running', detail }), + crashed: (detail) => reported.push({ kind: 'crashed', detail }), + versionMismatch: (detail) => + reported.push({ kind: 'version-mismatch', detail }), + }; + return { reporter, reported }; +}; From a35984a5287d22f971bf09e5640386600bd3563f Mon Sep 17 00:00:00 2001 From: fi3ework <fi3ework@gmail.com> Date: Mon, 24 Aug 2026 16:00:16 +0800 Subject: [PATCH 2/5] 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. --- packages/vscode/AGENTS.md | 4 +- packages/vscode/src/stacks/lint/index.ts | 17 ++++--- packages/vscode/src/stacks/lint/resolution.ts | 5 +- packages/vscode/src/stacks/lint/status.ts | 46 ++++++++++++----- packages/vscode/src/stacks/test/bridge.ts | 15 ++++-- .../vscode/src/stacks/test/worker/index.ts | 5 ++ .../vscode/tests/stacks/lint/status.test.ts | 49 ++++++++++++------- .../vscode/tests/stacks/test/bridge.test.ts | 39 +++++++++++++-- 8 files changed, 131 insertions(+), 49 deletions(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 96f9a61..98e75ac 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -23,7 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - **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.*`). - **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. -- **Not installed is a state, not an error — uniformly.** A folder or project whose dependencies are not installed (no `rstack`, no `@rstest/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 (`isMissingDependencyError`, on Node's `code`) because the IPC channel drops it — `NormalizedConfigResult` carries the verdict as data end to end, and `Project` branches on it. +- **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 (`isMissingDependencyError`, on Node's `code`) because the IPC channel drops it — `NormalizedConfigResult` carries the verdict as data end to end, and `Project` branches on it. - 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 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. @@ -39,7 +39,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - 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. - **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. -- **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. +- **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. - 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. - 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. - 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`. diff --git a/packages/vscode/src/stacks/lint/index.ts b/packages/vscode/src/stacks/lint/index.ts index 84ecd64..5b601c5 100644 --- a/packages/vscode/src/stacks/lint/index.ts +++ b/packages/vscode/src/stacks/lint/index.ts @@ -18,7 +18,7 @@ import { attributeToCore, foldRslintFolderState, statusForRslintStartFailure, - isMissingRstackFailure, + missingPackageOf, } from './status'; import { WorkspaceDocumentRouter } from './WorkspaceDocumentRouter'; @@ -208,19 +208,22 @@ class RslintController implements StackController { keeping, }) => { // The hook owns the report. The Output-channel line: a folder whose - // `rstack` is not installed is the not-installed state (AGENTS.md) - // — one warn line, no stack; anything else is upstream's error. - const suffix = keeping ? ` (keeping ${keeping} active)` : ''; - if (isMissingRstackFailure(error)) { + // `rstack` or `@rslint/core` is not installed is the not-installed + // state (AGENTS.md) — one warn line, no stack; anything else is + // upstream's error. A document with a last-good runtime still + // lints, so its consequence says what it keeps, not "will not". + const missing = missingPackageOf(error); + if (missing !== undefined) { logger.warn( formatNotInstalledLog( - 'rstack', + missing, workspaceFolder.name, workspaceFolder.uri.fsPath, - `${document.uri} will not lint until it is${suffix}`, + `${document.uri} ${keeping ? `keeps ${keeping}` : 'will not lint'} until it is installed`, ), ); } else { + const suffix = keeping ? ` (keeping ${keeping} active)` : ''; logger.error( `Could not select an Rslint core for ${document.uri}${suffix}`, error, diff --git a/packages/vscode/src/stacks/lint/resolution.ts b/packages/vscode/src/stacks/lint/resolution.ts index ad32e8c..d8527ff 100644 --- a/packages/vscode/src/stacks/lint/resolution.ts +++ b/packages/vscode/src/stacks/lint/resolution.ts @@ -79,8 +79,11 @@ function resolveConfiguredCore( try { if (!fs.statSync(packageJsonPath).isFile()) throw new Error('not a file'); } catch (error) { + // Not `missing-core`: the user pointed `corePath` at this directory, so + // the fix is correcting the setting, not installing dependencies — it must + // not take the not-installed state (`missingPackageOf`). throw new RslintResolutionError( - 'missing-core', + 'invalid-package', `Could not access @rslint/core at ${directory}`, { cause: error }, ); diff --git a/packages/vscode/src/stacks/lint/status.ts b/packages/vscode/src/stacks/lint/status.ts index 2e2b863..5765bdd 100644 --- a/packages/vscode/src/stacks/lint/status.ts +++ b/packages/vscode/src/stacks/lint/status.ts @@ -1,6 +1,10 @@ import type { StackState } from '../../types'; import { formatNotInstalledStatus } from '../../shared/notInstalled'; -import { RslintResolutionError } from './resolution'; +import type { SupportedPackage } from '../../shared/versionCheck'; +import { + RslintResolutionError, + type RslintResolutionErrorCode, +} from './resolution'; export class RslintVersionMismatchError extends Error { constructor(message: string) { @@ -10,22 +14,39 @@ export class RslintVersionMismatchError extends Error { } /** - * A bridged folder whose `rstack` is not installed — the not-installed state - * the three stacks report uniformly (AGENTS.md): a `disabled` status and a - * one-line warning, never a crash or a stack trace. `missing-core` is not it: - * rstack is there but unusable, which is worth the full error. + * The package whose absence makes this failure the not-installed state the + * three stacks report uniformly (AGENTS.md) — a `disabled` status and a + * one-line warning, never a crash or a stack trace. `missing-rstack` is a + * bridged folder without `rstack`; `missing-core` is a native folder on a + * fresh clone (a bridged folder reaches it only when rstack's own dependency + * is gone — an interrupted install, where installing is still the fix). + * `missing-shim` stays an error: the package is there, its version is wrong. + * A misconfigured `corePath` is `invalid-package`, also an error: the fix is + * the setting, not an install. */ -export const isMissingRstackFailure = (error: unknown): boolean => - error instanceof RslintResolutionError && error.code === 'missing-rstack'; +const NOT_INSTALLED_PACKAGE: Partial< + Record<RslintResolutionErrorCode, SupportedPackage> +> = { + 'missing-rstack': 'rstack', + 'missing-core': '@rslint/core', +}; + +export const missingPackageOf = ( + error: unknown, +): SupportedPackage | undefined => + error instanceof RslintResolutionError + ? NOT_INSTALLED_PACKAGE[error.code] + : undefined; export const statusForRslintStartFailure = (error: unknown): StackState => { if (error instanceof RslintVersionMismatchError) { return { kind: 'version-mismatch', detail: error.message }; } - if (isMissingRstackFailure(error)) { + const missing = missingPackageOf(error); + if (missing !== undefined) { return { kind: 'disabled', - reason: formatNotInstalledStatus('rslint', 'rstack'), + reason: formatNotInstalledStatus('rslint', missing), }; } return { @@ -62,9 +83,10 @@ const RSLINT_IDLE_DETAIL = 'idle'; /** * Complete on purpose: a new state cannot be added without ranking itself, so * nothing silently falls through to `running`. `disabled` outranks `running` - * as in the fmt stack's table: at folder level it only ever means "no `rstack` - * installed, this bridged folder will never lint", a fact worth showing over a - * healthy runtime or sibling folder — unlike the shell's kill switch. + * as in the fmt stack's table: at folder level it only ever means "a package + * this folder needs is not installed, so it will not lint" (`missingPackageOf`), + * a fact worth showing over a healthy runtime or sibling folder — unlike the + * shell's kill switch. */ const STATE_RANK: Readonly<Record<StackState['kind'], number>> = { crashed: 5, diff --git a/packages/vscode/src/stacks/test/bridge.ts b/packages/vscode/src/stacks/test/bridge.ts index 58626ce..efebe28 100644 --- a/packages/vscode/src/stacks/test/bridge.ts +++ b/packages/vscode/src/stacks/test/bridge.ts @@ -86,14 +86,22 @@ export function resolveRstackShim( return undefined; } + // The package exists, so the not-installed latch is over even when the + // install turns out to be unusable below (missing shim, unsupported + // version) — those states carry their own reports. + status.installed(configDir); + const packageDirectory = path.dirname(packageJsonPath); const configFilePath = path.join(packageDirectory, SHIM_RELATIVE_PATH); if (!existsSync(configFilePath)) { + const message = `The installed "rstack" package has no ${SHIM_RELATIVE_PATH} (looked in ${packageJsonPath}). Upgrade "rstack" to a version that ships the Rstest config shim.`; if (!silent) { - logger.error( - `The installed "rstack" package has no ${SHIM_RELATIVE_PATH} (looked in ${packageJsonPath}). Upgrade "rstack" to a version that ships the Rstest config shim.`, - ); + logger.error(message); } + // Same latch as the floor check below: the fix is upgrading rstack, and + // without a report of its own this branch would paint `running` over an + // install that cannot drive Rstest. + status.versionMismatch(message, configDir); return undefined; } @@ -117,7 +125,6 @@ export function resolveRstackShim( packageDirectory, version, }); - status.installed(configDir); status.versionOk(configDir); return { configFilePath, packageDirectory, version }; } diff --git a/packages/vscode/src/stacks/test/worker/index.ts b/packages/vscode/src/stacks/test/worker/index.ts index 414067d..10389c3 100644 --- a/packages/vscode/src/stacks/test/worker/index.ts +++ b/packages/vscode/src/stacks/test/worker/index.ts @@ -60,6 +60,11 @@ export class Worker { public async getNormalizedConfig( options: WorkerInitOptions, ): Promise<NormalizedConfigResult> { + // The core loads outside the classified region: a broken `@rstest/core` + // install failing to import is the real error to report, not "the config + // imports an uninstalled package". Once this import succeeds, `init()`'s + // own import of the same path is served from the module cache. + await import(normalizeImportPath(options.rstestPath)); try { const { rstest, projects } = await this.init(options); return { diff --git a/packages/vscode/tests/stacks/lint/status.test.ts b/packages/vscode/tests/stacks/lint/status.test.ts index 998d715..55de03f 100644 --- a/packages/vscode/tests/stacks/lint/status.test.ts +++ b/packages/vscode/tests/stacks/lint/status.test.ts @@ -4,47 +4,58 @@ import { aggregateFolderStates, attributeToCore, foldRslintFolderState, - isMissingRstackFailure, + missingPackageOf, RslintVersionMismatchError, runningRslintStatus, statusForRslintStartFailure, } from '../../../src/stacks/lint/status'; describe('Rslint status classification', () => { - it('disables a bridged folder whose rstack package is missing', () => { + it('disables a folder whose package is not installed', () => { + // Both not-installed shapes take the uniform disabled state (AGENTS.md): + // a bridged folder without rstack, a fresh clone without the core. expect( statusForRslintStartFailure( new RslintResolutionError('missing-rstack', 'missing rstack'), ), ).toMatchObject({ kind: 'disabled' }); + expect( + statusForRslintStartFailure( + new RslintResolutionError('missing-core', 'missing core'), + ), + ).toMatchObject({ kind: 'disabled' }); }); - it('tells the not-installed state apart from an unusable install', () => { - // The same boundary the status uses: only a missing `rstack` is the - // warn-level, disabled state; a present-but-broken one is an error. + it('names the package a not-installed failure is missing', () => { + // The same boundary the status and the warn line use: a missing package + // is the disabled state; a present-but-broken install is an error. expect( - isMissingRstackFailure( + missingPackageOf( new RslintResolutionError('missing-rstack', 'missing rstack'), ), - ).toBe(true); + ).toBe('rstack'); expect( - isMissingRstackFailure( + missingPackageOf( new RslintResolutionError('missing-core', 'missing core'), ), - ).toBe(false); - expect(isMissingRstackFailure(new Error('missing rstack'))).toBe(false); + ).toBe('@rslint/core'); + expect( + missingPackageOf(new RslintResolutionError('missing-shim', 'no shim')), + ).toBe(undefined); + expect(missingPackageOf(new Error('missing rstack'))).toBe(undefined); }); - it('classifies missing core and worker failures as crashes', () => { - expect( - statusForRslintStartFailure( - new RslintResolutionError('missing-core', 'missing core'), - ), - ).toEqual({ kind: 'crashed', detail: 'missing core' }); + it('classifies worker and misconfiguration failures as crashes', () => { expect(statusForRslintStartFailure(new Error('worker stopped'))).toEqual({ kind: 'crashed', detail: 'worker stopped', }); + // A wrong `corePath` setting is fixed in the setting, not by an install. + expect( + statusForRslintStartFailure( + new RslintResolutionError('invalid-package', 'not a core'), + ), + ).toEqual({ kind: 'crashed', detail: 'not a core' }); }); it('classifies package and automatic Node floors as version mismatches', () => { @@ -146,9 +157,9 @@ describe('foldRslintFolderState', () => { }); it('lets a bridged folder that lost rstack outrank its live runtime', () => { - // Inside a folder `disabled` only ever means "missing rstack" — a - // failure the user must see, not the shell's kill switch — so, unlike the - // cross-folder rank, it beats a healthy runtime. + // Inside a folder `disabled` only ever means "a package is not + // installed" — a failure the user must see, not the shell's kill switch — + // so, unlike the cross-folder rank, it beats a healthy runtime. expect( foldRslintFolderState([ { kind: 'running' }, diff --git a/packages/vscode/tests/stacks/test/bridge.test.ts b/packages/vscode/tests/stacks/test/bridge.test.ts index 73b8935..f108ecd 100644 --- a/packages/vscode/tests/stacks/test/bridge.test.ts +++ b/packages/vscode/tests/stacks/test/bridge.test.ts @@ -71,6 +71,14 @@ const createWorkspace = ({ return configDir; }; +/** Hides the workspace's `node_modules/rstack`; returns the restore step. */ +const parkRstack = (configDir: string): (() => void) => { + const rstackDir = path.join(configDir, 'node_modules', 'rstack'); + const parked = `${rstackDir}.parked`; + fs.renameSync(rstackDir, parked); + return () => fs.renameSync(parked, rstackDir); +}; + beforeEach(() => { logged.length = 0; reported.length = 0; @@ -122,14 +130,12 @@ describe('resolveRstackShim', () => { it('clears the disabled status once the directory resolves', () => { const configDir = createWorkspace(); - const rstackDir = path.join(configDir, 'node_modules', 'rstack'); - const parked = `${rstackDir}.parked`; - fs.renameSync(rstackDir, parked); + const restore = parkRstack(configDir); expect(resolveRstackShim(configDir)).toBeUndefined(); expect(reported.map((state) => state.kind)).toEqual(['disabled']); - fs.renameSync(parked, rstackDir); + restore(); expect(resolveRstackShim(configDir)).toBeDefined(); expect(reported.map((state) => state.kind)).toEqual([ 'disabled', @@ -149,6 +155,31 @@ describe('resolveRstackShim', () => { expect(resolveRstackShim(configDir)).toBeUndefined(); expect(logged.join('\n')).toContain('rstestConfig.js'); + // Unusable-but-present must not paint `running`: the missing shim is an + // upgrade problem, latched like the version floor below. + expect(reported.map((state) => state.kind)).toEqual(['version-mismatch']); + }); + + it('clears the not-installed latch once the package exists, even unusable', () => { + // First pass: no rstack at all. Second pass: a partial install without + // the shim — "not installed" would now be a lie, and the shim failure + // carries its own report. + const configDir = createWorkspace({ shim: false }); + const restore = parkRstack(configDir); + + expect(resolveRstackShim(configDir)).toBeUndefined(); + expect(reported.map((state) => state.kind)).toEqual(['disabled']); + + restore(); + expect(resolveRstackShim(configDir)).toBeUndefined(); + // The latch clears the moment the package resolves, then the shim verdict + // lands — all within one synchronous pass, so the middle `running` never + // reaches the (asynchronously rendered) status bar. + expect(reported.map((state) => state.kind)).toEqual([ + 'disabled', + 'running', + 'version-mismatch', + ]); }); it('refuses an rstack older than the support matrix floor', () => { From 9066a5b1dc44baffed2c986c060eb1eede4ff6c7 Mon Sep 17 00:00:00 2001 From: fi3ework <fi3ework@gmail.com> Date: Mon, 24 Aug 2026 16:20:56 +0800 Subject: [PATCH 3/5] fix(vscode): classify only bare package imports as not installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/vscode/AGENTS.md | 2 +- packages/vscode/src/shared/notInstalled.ts | 19 ++++- packages/vscode/src/stacks/test/bridge.ts | 10 +-- .../vscode/src/stacks/test/coreResolution.ts | 71 +++++++++++-------- packages/vscode/src/stacks/test/project.ts | 10 +-- packages/vscode/src/stacks/test/status.ts | 16 +++++ packages/vscode/src/stacks/test/types.ts | 4 +- .../vscode/src/stacks/test/worker/index.ts | 11 ++- .../vscode/tests/shared/notInstalled.test.ts | 13 ++++ .../vscode/tests/stacks/test/bridge.test.ts | 24 +++++-- .../tests/stacks/test/coreResolution.test.ts | 68 ++++++++++-------- .../vscode/tests/stacks/test/status.test.ts | 14 ++++ 12 files changed, 177 insertions(+), 85 deletions(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 98e75ac..1706a30 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -23,7 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - **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.*`). - **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. -- **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 (`isMissingDependencyError`, on Node's `code`) because the IPC channel drops it — `NormalizedConfigResult` carries the verdict as data end to end, and `Project` branches on it. +- **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` plus a bare — package-name — specifier, so a typo'd relative import 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. - 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 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. diff --git a/packages/vscode/src/shared/notInstalled.ts b/packages/vscode/src/shared/notInstalled.ts index 8a0832c..b5d3753 100644 --- a/packages/vscode/src/shared/notInstalled.ts +++ b/packages/vscode/src/shared/notInstalled.ts @@ -1,4 +1,9 @@ -import { COMMAND_CATEGORY, type StackId, stackCommandTitle } from '../types'; +import { + COMMAND_CATEGORY, + STACK_LABELS, + type StackId, + stackCommandTitle, +} from '../types'; /** * The not-installed policy's words, once for all three stacks (AGENTS.md @@ -34,6 +39,18 @@ export const formatConfigDependencyMissingStatus = ( ): string => `${configPath} imports a package that is not installed — install the project dependencies, ${restartHint(stack)}`; +/** + * The output-channel line for a config that imports a package that is not + * installed. `cause` is the loader's own first line, which names the + * specifier and the importer. + */ +export const formatConfigDependencyMissingLog = ( + stack: StackId, + configPath: string, + cause: string, +): string => + `Cannot load ${configPath}: ${cause}. Install the project dependencies to enable ${STACK_LABELS[stack]} for this config.`; + /** * The output-channel line: where the stack looked, plus the stack's own * consequence — the same shape as the shared Node preflight message diff --git a/packages/vscode/src/stacks/test/bridge.ts b/packages/vscode/src/stacks/test/bridge.ts index efebe28..2d0f901 100644 --- a/packages/vscode/src/stacks/test/bridge.ts +++ b/packages/vscode/src/stacks/test/bridge.ts @@ -79,6 +79,8 @@ export function resolveRstackShim( } // Latched like the version mismatch below, under the same key, so the // status stays until this directory resolves or stops being a candidate. + // A stale mismatch from a previously-present install is superseded by + // the latch itself (one source, one verdict). status.notInstalled( formatNotInstalledStatus('rstest', 'rstack'), configDir, @@ -86,11 +88,6 @@ export function resolveRstackShim( return undefined; } - // The package exists, so the not-installed latch is over even when the - // install turns out to be unusable below (missing shim, unsupported - // version) — those states carry their own reports. - status.installed(configDir); - const packageDirectory = path.dirname(packageJsonPath); const configFilePath = path.join(packageDirectory, SHIM_RELATIVE_PATH); if (!existsSync(configFilePath)) { @@ -125,6 +122,9 @@ export function resolveRstackShim( packageDirectory, version, }); + // At most one of the two latches can be live (one source, one verdict), so + // this repaints once, whichever failure the previous pass observed. status.versionOk(configDir); + status.installed(configDir); return { configFilePath, packageDirectory, version }; } diff --git a/packages/vscode/src/stacks/test/coreResolution.ts b/packages/vscode/src/stacks/test/coreResolution.ts index cb8a5bc..a873659 100644 --- a/packages/vscode/src/stacks/test/coreResolution.ts +++ b/packages/vscode/src/stacks/test/coreResolution.ts @@ -1,15 +1,19 @@ /** - * Helpers for reporting a failed `@rstest/core` resolution. + * Classifying and reporting failed Rstest resolutions — the host-side helpers + * for a `@rstest/core` that cannot be resolved, and the worker-side + * classifier for a config whose own import failed + * (`missingDependencyCauseOf`). * - * Both messages replace Node's own `MODULE_NOT_FOUND` text, which embeds the - * require stack of whoever called `require.resolve` — for a bundled extension - * its `dist` path plus the VS Code extension host — and says nothing about what - * to do. They differ in where they end up: an uninstalled core is the normal - * state of a freshly cloned repository and is resolved for every config file - * without the user asking, so it is only logged; a `rstestPackagePath` that - * does not resolve is a setting the user has to fix, so it is notified. + * Every message here replaces Node's own `MODULE_NOT_FOUND` text, which + * embeds a multi-line require stack and says nothing about what to do. An + * uninstalled core is the normal state of a freshly cloned repository and is + * resolved for every config file without the user asking, so it is only + * logged; a `rstestPackagePath` that does not resolve is a setting the user + * has to fix, so it is notified. */ +import path from 'node:path'; + /** * Resolution failed after the actionable error was already logged or shown. * Callers still reject so project initialization stops, but must not report the @@ -46,25 +50,36 @@ export function formatConfiguredCoreNotFoundMessage( return `Cannot find "@rstest/core" at the configured "rstack.rstest.rstestPackagePath": ${configuredPackagePath}. Update the setting to point at an installed "@rstest/core" package.json.`; } -// Whether a config evaluation failed because something it imports is not -// installed. Read from the error's `code` — Node's own classification, set by -// both loaders (`ERR_MODULE_NOT_FOUND` for ESM, `MODULE_NOT_FOUND` for CJS) — -// never from the message text. Any other failure (a syntax error in the -// config, a thrown plugin) is a real error. The check has to run in the -// worker, where the error is thrown: the IPC channel back to the extension -// host (`serialization: 'advanced'`) keeps an Error's message and stack but -// drops its `code`, so the classification is carried as data instead -// (`NormalizedConfigResult`). -export function isMissingDependencyError(error: unknown): boolean { - if (!(error instanceof Error)) return false; +// The one-line cause when a config evaluation failed on a package that is +// not installed, or `undefined` for a real error. Gated on the error's +// `code` — Node's own classification (`ERR_MODULE_NOT_FOUND` for ESM, +// `MODULE_NOT_FOUND` for CJS) — but the code alone is too broad: a typo'd +// relative import fails with the same codes, and installing dependencies +// cannot fix it, so only a bare specifier — a package name, read from the +// message since CJS carries no structured one — counts, and anything +// unrecognized fails towards the full error report. The check has to run in +// the worker, where the error is thrown: the IPC channel back to the +// extension host (`serialization: 'advanced'`) drops the `code`, so the +// verdict travels as data (`NormalizedConfigResult`). Only the first line +// comes back: the rest of a CJS message is the require stack, and the +// not-installed state is one warn line without one. +export function missingDependencyCauseOf(error: unknown): string | undefined { + if (!(error instanceof Error)) return undefined; const { code } = error as NodeJS.ErrnoException; - return code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND'; -} - -/** `cause` is the loader's own text, which names the specifier and the importer. */ -export function formatConfigDependencyMissingMessage( - configFilePath: string, - cause: string, -): string { - return `Cannot load ${configFilePath}: ${cause}. Install the project dependencies to enable Rstest for this config.`; + if (code !== 'ERR_MODULE_NOT_FOUND' && code !== 'MODULE_NOT_FOUND') { + return undefined; + } + const [firstLine] = error.message.split('\n', 1); + const specifier = /^Cannot find (?:package|module) '([^']+)'/.exec( + firstLine, + )?.[1]; + if ( + specifier === undefined || + specifier.startsWith('.') || + specifier.startsWith('file:') || + path.isAbsolute(specifier) + ) { + return undefined; + } + return firstLine; } diff --git a/packages/vscode/src/stacks/test/project.ts b/packages/vscode/src/stacks/test/project.ts index 1ea50bf..718d16f 100644 --- a/packages/vscode/src/stacks/test/project.ts +++ b/packages/vscode/src/stacks/test/project.ts @@ -6,11 +6,11 @@ import vscode from 'vscode'; import { RSTACK_CONFIG_NAMES } from '../../detection'; import { resolveRstackShim } from './bridge'; import { watchConfigValue } from './config'; -import { formatConfigDependencyMissingStatus } from '../../shared/notInstalled'; import { - formatConfigDependencyMissingMessage, - ReportedRstestResolutionError, -} from './coreResolution'; + formatConfigDependencyMissingLog, + formatConfigDependencyMissingStatus, +} from '../../shared/notInstalled'; +import { ReportedRstestResolutionError } from './coreResolution'; import { logger } from './logger'; import { RstestApi } from './master'; import { type ChildProjectRef, computeCoveredConfigs } from './projectCoverage'; @@ -622,7 +622,7 @@ export class Project implements vscode.Disposable { private reportMissingDependency(cause: string): void { this.configLoadFailed = true; logger.warn( - formatConfigDependencyMissingMessage(this.sourceUri.fsPath, cause), + formatConfigDependencyMissingLog('rstest', this.sourceUri.fsPath, cause), ); status.notInstalled( formatConfigDependencyMissingStatus( diff --git a/packages/vscode/src/stacks/test/status.ts b/packages/vscode/src/stacks/test/status.ts index 95a8718..2b25b17 100644 --- a/packages/vscode/src/stacks/test/status.ts +++ b/packages/vscode/src/stacks/test/status.ts @@ -117,12 +117,27 @@ class StatusHolder implements StatusReporter { this.#reporter?.running(detail); } + /** + * One source, one verdict: a newly observed failure kind replaces whatever + * other kind the same source had latched, so a raise site never has to know + * the other tables exist. Without this, a lower-ranked observation could + * not win — a root whose `rstack` was outdated and is now removed would + * keep painting the stale upgrade hint over "not installed". + */ + #supersede(keep: Map<string, string>, source: string): void { + for (const latch of [this.#crashes, this.#mismatches, this.#notInstalled]) { + if (latch !== keep) latch.delete(source); + } + } + crashed(detail: string, source = ''): void { + this.#supersede(this.#crashes, source); this.#crashes.set(source, detail); this.#paintOrRun(); } versionMismatch(detail: string, source = ''): void { + this.#supersede(this.#mismatches, source); this.#mismatches.set(source, detail); this.#paintOrRun(); } @@ -147,6 +162,7 @@ class StatusHolder implements StatusReporter { */ notInstalled(reason: string, source = ''): void { if (this.#notInstalled.get(source) === reason) return; + this.#supersede(this.#notInstalled, source); this.#notInstalled.set(source, reason); this.#paintOrRun(); } diff --git a/packages/vscode/src/stacks/test/types.ts b/packages/vscode/src/stacks/test/types.ts index 0167078..6795120 100644 --- a/packages/vscode/src/stacks/test/types.ts +++ b/packages/vscode/src/stacks/test/types.ts @@ -12,8 +12,8 @@ export type WorkerInitOptions = RstestConfig & { * What the worker answers `getNormalizedConfig` with. A config that fails to * evaluate because a dependency is not installed is a result, not a rejection: * the IPC channel would strip the error's `code` (see - * `isMissingDependencyError`), so the worker classifies it and reports the - * loader's message as data. + * `missingDependencyCauseOf`), so the worker classifies it and reports the + * loader's own first line as data. */ export type NormalizedConfigResult = | { diff --git a/packages/vscode/src/stacks/test/worker/index.ts b/packages/vscode/src/stacks/test/worker/index.ts index 10389c3..7c159e0 100644 --- a/packages/vscode/src/stacks/test/worker/index.ts +++ b/packages/vscode/src/stacks/test/worker/index.ts @@ -1,7 +1,7 @@ import { pathToFileURL } from 'node:url'; import { createBirpc } from 'birpc'; import type { TestRunReporter } from '../testRunReporter'; -import { isMissingDependencyError } from '../coreResolution'; +import { missingDependencyCauseOf } from '../coreResolution'; import type { NormalizedConfigResult, WorkerInitOptions } from '../types'; import { logger } from './logger'; import { CoverageReporter, ProgressLogger, ProgressReporter } from './reporter'; @@ -88,12 +88,9 @@ export class Worker { // Classified here and not in the master: `code` does not survive the // IPC round-trip. Only this unprompted, per-config evaluation gets the // treatment — a run or list the user asked for reports its failure. - if (isMissingDependencyError(error)) { - return { - ok: false, - reason: 'missing-dependency', - message: (error as Error).message, - }; + const cause = missingDependencyCauseOf(error); + if (cause !== undefined) { + return { ok: false, reason: 'missing-dependency', message: cause }; } throw error; } diff --git a/packages/vscode/tests/shared/notInstalled.test.ts b/packages/vscode/tests/shared/notInstalled.test.ts index 3d1c886..0c81480 100644 --- a/packages/vscode/tests/shared/notInstalled.test.ts +++ b/packages/vscode/tests/shared/notInstalled.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from '@rstest/core'; import { + formatConfigDependencyMissingLog, formatConfigDependencyMissingStatus, formatNotInstalledLog, formatNotInstalledStatus, @@ -37,4 +38,16 @@ describe('not-installed wording', () => { 'rstack is not installed in app (node_modules missing); searched from /repo/app', ); }); + + it("logs the config, the loader's own words and the way out", () => { + expect( + formatConfigDependencyMissingLog( + 'rstest', + '/repo/templates/app/rstack.config.ts', + "Cannot find package '@rsbuild/plugin-react' imported from /repo/templates/app/rstack.config.ts", + ), + ).toBe( + "Cannot load /repo/templates/app/rstack.config.ts: Cannot find package '@rsbuild/plugin-react' imported from /repo/templates/app/rstack.config.ts. Install the project dependencies to enable Rstest for this config.", + ); + }); }); diff --git a/packages/vscode/tests/stacks/test/bridge.test.ts b/packages/vscode/tests/stacks/test/bridge.test.ts index f108ecd..b7ee9f4 100644 --- a/packages/vscode/tests/stacks/test/bridge.test.ts +++ b/packages/vscode/tests/stacks/test/bridge.test.ts @@ -162,8 +162,8 @@ describe('resolveRstackShim', () => { it('clears the not-installed latch once the package exists, even unusable', () => { // First pass: no rstack at all. Second pass: a partial install without - // the shim — "not installed" would now be a lie, and the shim failure - // carries its own report. + // the shim — "not installed" would now be a lie, and the shim verdict + // supersedes it in one paint. const configDir = createWorkspace({ shim: false }); const restore = parkRstack(configDir); @@ -172,16 +172,28 @@ describe('resolveRstackShim', () => { restore(); expect(resolveRstackShim(configDir)).toBeUndefined(); - // The latch clears the moment the package resolves, then the shim verdict - // lands — all within one synchronous pass, so the middle `running` never - // reaches the (asynchronously rendered) status bar. expect(reported.map((state) => state.kind)).toEqual([ 'disabled', - 'running', 'version-mismatch', ]); }); + it('replaces a stale mismatch with the disabled state when rstack disappears', () => { + // A shim-less (or below-floor) install latches a mismatch; if the package + // is then removed, the higher-ranked mismatch must not keep painting an + // upgrade hint over "not installed". + const configDir = createWorkspace({ shim: false }); + expect(resolveRstackShim(configDir)).toBeUndefined(); + expect(reported.map((state) => state.kind)).toEqual(['version-mismatch']); + + parkRstack(configDir); + expect(resolveRstackShim(configDir)).toBeUndefined(); + expect(reported.map((state) => state.kind)).toEqual([ + 'version-mismatch', + 'disabled', + ]); + }); + it('refuses an rstack older than the support matrix floor', () => { const configDir = createWorkspace({ version: '0.6.0' }); diff --git a/packages/vscode/tests/stacks/test/coreResolution.test.ts b/packages/vscode/tests/stacks/test/coreResolution.test.ts index bd546f7..eacc362 100644 --- a/packages/vscode/tests/stacks/test/coreResolution.test.ts +++ b/packages/vscode/tests/stacks/test/coreResolution.test.ts @@ -3,10 +3,9 @@ import os from 'node:os'; import path from 'node:path'; import { describe, expect, it } from '@rstest/core'; import { - formatConfigDependencyMissingMessage, formatConfiguredCoreNotFoundMessage, - isMissingDependencyError, isModuleNotFoundError, + missingDependencyCauseOf, } from '../../../src/stacks/test/coreResolution'; // Resolve for real rather than hand-building an error object: the predicate @@ -66,9 +65,9 @@ describe('core-not-found messages', () => { }); }); -describe('isMissingDependencyError', () => { - // Same reasoning as `resolveError`: the predicate reads a code Node owns, - // so the errors come from Node's own loaders. +describe('missingDependencyCauseOf', () => { + // Same reasoning as `resolveError`: the classifier reads a code and a + // message Node owns, so the errors come from Node's own loaders. const importError = async (specifier: string): Promise<unknown> => { try { await import(specifier); @@ -78,42 +77,51 @@ describe('isMissingDependencyError', () => { throw new Error(`expected "${specifier}" not to import`); }; - it('should detect a package an ESM config failed to import', async () => { + it('should name a package an ESM config failed to import', async () => { expect( - isMissingDependencyError( + missingDependencyCauseOf( await importError('@rstest/definitely-not-installed'), ), - ).toBe(true); + ).toContain("'@rstest/definitely-not-installed'"); }); - it('should detect a package a CJS config failed to require', () => { + it('should keep a CJS failure to one line, without the require stack', () => { + const cause = missingDependencyCauseOf( + resolveError('@rstest/definitely-not-installed', __dirname), + ); + expect(cause).toContain("'@rstest/definitely-not-installed'"); + // The not-installed warn is one line, no stack (AGENTS.md); Node's + // MODULE_NOT_FOUND message embeds a multi-line `Require stack:`. + expect(cause).not.toContain('\n'); + expect(cause).not.toContain('Require stack'); + }); + + it('should leave a missing relative or absolute import to the error report', () => { + // A typo'd `./helper` or a missing generated file is a source problem — + // installing dependencies cannot fix it, so it must not be classified as + // the not-installed state. The ESM loader reports relative imports as + // absolute paths, which the absolute case stands in for. + expect( + missingDependencyCauseOf(resolveError('./definitely-missing', __dirname)), + ).toBe(undefined); expect( - isMissingDependencyError( - resolveError('@rstest/definitely-not-installed', __dirname), + missingDependencyCauseOf( + resolveError( + path.join(os.tmpdir(), 'definitely-missing.js'), + os.tmpdir(), + ), ), - ).toBe(true); + ).toBe(undefined); }); it('should leave every other failure to the full error report', () => { - expect(isMissingDependencyError(new SyntaxError('Unexpected token'))).toBe( - false, - ); - expect(isMissingDependencyError(new Error("Cannot find package 'x'"))).toBe( - false, + expect(missingDependencyCauseOf(new SyntaxError('Unexpected token'))).toBe( + undefined, ); - expect(isMissingDependencyError("Cannot find package 'x'")).toBe(false); - expect(isMissingDependencyError(undefined)).toBe(false); - }); -}); - -describe('formatConfigDependencyMissingMessage', () => { - it("should name the config, the loader's own words and the way out", () => { - const message = formatConfigDependencyMissingMessage( - '/repo/templates/app/rstack.config.ts', - "Cannot find package '@rsbuild/plugin-react' imported from /repo/templates/app/rstack.config.ts", + expect(missingDependencyCauseOf(new Error("Cannot find package 'x'"))).toBe( + undefined, ); - expect(message).toContain('/repo/templates/app/rstack.config.ts'); - expect(message).toContain("Cannot find package '@rsbuild/plugin-react'"); - expect(message).toContain('Install the project dependencies'); + expect(missingDependencyCauseOf("Cannot find package 'x'")).toBe(undefined); + expect(missingDependencyCauseOf(undefined)).toBe(undefined); }); }); diff --git a/packages/vscode/tests/stacks/test/status.test.ts b/packages/vscode/tests/stacks/test/status.test.ts index 41cdd4b..96d24ef 100644 --- a/packages/vscode/tests/stacks/test/status.test.ts +++ b/packages/vscode/tests/stacks/test/status.test.ts @@ -49,6 +49,20 @@ describe('StatusHolder failure latches', () => { expect(calls).toEqual(['crashed:spawn ENOENT', 'running:']); }); + it('lets a new verdict supersede another kind latched by the same root', () => { + // One source, one verdict: without this, the higher-ranked stale entry + // would keep painting over the newer observation — an outdated rstack + // that is then removed must show "not installed", not the upgrade hint. + const calls = bindRecorder(); + status.versionMismatch('rstack too old', '/a'); + status.notInstalled('rstack is not installed', '/a'); + expect(calls).toEqual(['mismatch:rstack too old', 'report:disabled']); + + status.crashed('spawn ENOENT', '/a'); + status.notInstalled('rstack is not installed', '/a'); + expect(calls.slice(2)).toEqual(['crashed:spawn ENOENT', 'report:disabled']); + }); + it('outranks a mismatch with a crash and falls back on recovery', () => { const calls = bindRecorder(); status.versionMismatch('core too old', '/a'); From 8b832e761262cd61113dd8b87d7158af61c9ef6c Mon Sep 17 00:00:00 2001 From: fi3ework <fi3ework@gmail.com> Date: Mon, 24 Aug 2026 16:45:33 +0800 Subject: [PATCH 4/5] fix(vscode): keep independent failure facts from masking each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/vscode/AGENTS.md | 2 +- packages/vscode/src/stacks/test/bridge.ts | 21 +++++-- .../vscode/src/stacks/test/coreResolution.ts | 23 +++++++- packages/vscode/src/stacks/test/project.ts | 15 ++++- packages/vscode/src/stacks/test/status.ts | 34 +++++------ .../vscode/src/stacks/test/worker/index.ts | 4 +- .../vscode/tests/stacks/test/bridge.test.ts | 4 +- .../tests/stacks/test/coreResolution.test.ts | 57 +++++++++++++------ .../vscode/tests/stacks/test/status.test.ts | 20 +++++-- 9 files changed, 130 insertions(+), 50 deletions(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 1706a30..c6bf045 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -23,7 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - **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.*`). - **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. -- **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` plus a bare — package-name — specifier, so a typo'd relative import 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. +- **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. - 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 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. diff --git a/packages/vscode/src/stacks/test/bridge.ts b/packages/vscode/src/stacks/test/bridge.ts index 2d0f901..c80f55e 100644 --- a/packages/vscode/src/stacks/test/bridge.ts +++ b/packages/vscode/src/stacks/test/bridge.ts @@ -6,7 +6,10 @@ import { formatVersionMismatch, readPackageVersion, } from '../../shared/versionCheck'; -import { formatNotInstalledStatus } from '../../shared/notInstalled'; +import { + formatNotInstalledLog, + formatNotInstalledStatus, +} from '../../shared/notInstalled'; import { logger } from './logger'; import { status } from './status'; @@ -74,13 +77,18 @@ export function resolveRstackShim( if (packageJsonPath === undefined) { if (!silent) { logger.warn( - `Cannot find the "rstack" package from ${configDir}. Rstest cannot be driven by "rstack.config.*" until the project dependencies are installed.`, + formatNotInstalledLog( + 'rstack', + path.basename(configDir), + configDir, + 'Rstest cannot be driven by "rstack.config.*" until it is installed', + ), ); } // Latched like the version mismatch below, under the same key, so the // status stays until this directory resolves or stops being a candidate. - // A stale mismatch from a previously-present install is superseded by - // the latch itself (one source, one verdict). + // A stale mismatch from a previously-present install is retired by the + // latch itself (a package-state observation restates its root). status.notInstalled( formatNotInstalledStatus('rstest', 'rstack'), configDir, @@ -122,8 +130,9 @@ export function resolveRstackShim( packageDirectory, version, }); - // At most one of the two latches can be live (one source, one verdict), so - // this repaints once, whichever failure the previous pass observed. + // At most one of the two latches can be live (a package-state observation + // restates its root), so this repaints once, whichever failure the + // previous pass observed. status.versionOk(configDir); status.installed(configDir); return { configFilePath, packageDirectory, version }; diff --git a/packages/vscode/src/stacks/test/coreResolution.ts b/packages/vscode/src/stacks/test/coreResolution.ts index a873659..7a63b41 100644 --- a/packages/vscode/src/stacks/test/coreResolution.ts +++ b/packages/vscode/src/stacks/test/coreResolution.ts @@ -5,7 +5,9 @@ * (`missingDependencyCauseOf`). * * Every message here replaces Node's own `MODULE_NOT_FOUND` text, which - * embeds a multi-line require stack and says nothing about what to do. An + * embeds a multi-line require stack and says nothing about what to do; the + * classifier also touches the filesystem once, to check whether a failing + * subpath's package really is absent. An * uninstalled core is the normal state of a freshly cloned repository and is * resolved for every config file without the user asking, so it is only * logged; a `rstestPackagePath` that does not resolve is a setting the user @@ -13,6 +15,7 @@ */ import path from 'node:path'; +import { findPackageJsonUncached } from '../../shared/packageResolve'; /** * Resolution failed after the actionable error was already logged or shown. @@ -63,7 +66,10 @@ export function formatConfiguredCoreNotFoundMessage( // verdict travels as data (`NormalizedConfigResult`). Only the first line // comes back: the rest of a CJS message is the require stack, and the // not-installed state is one warn line without one. -export function missingDependencyCauseOf(error: unknown): string | undefined { +export function missingDependencyCauseOf( + error: unknown, + resolveFrom: string, +): string | undefined { if (!(error instanceof Error)) return undefined; const { code } = error as NodeJS.ErrnoException; if (code !== 'ERR_MODULE_NOT_FOUND' && code !== 'MODULE_NOT_FOUND') { @@ -81,5 +87,18 @@ export function missingDependencyCauseOf(error: unknown): string | undefined { ) { return undefined; } + // `installed-package/missing-subpath` wears the same bare shape, but the + // package itself is there — installing dependencies cannot fix it either, + // so a subpath is checked against the physical `node_modules` with the + // same uncached walk-up every stack resolves packages with. + const packageName = specifier.startsWith('@') + ? specifier.split('/').slice(0, 2).join('/') + : specifier.split('/', 1)[0]; + if ( + packageName !== specifier && + findPackageJsonUncached(packageName, resolveFrom) !== undefined + ) { + return undefined; + } return firstLine; } diff --git a/packages/vscode/src/stacks/test/project.ts b/packages/vscode/src/stacks/test/project.ts index 718d16f..1189c8f 100644 --- a/packages/vscode/src/stacks/test/project.ts +++ b/packages/vscode/src/stacks/test/project.ts @@ -595,7 +595,7 @@ export class Project implements vscode.Disposable { this.reportMissingDependency(result.message); return; } - status.installed(this.sourceUri.toString()); + status.installed(this.configDependencyStatusSource); this.root = vscode.Uri.file(result.root); this.include = result.include; this.exclude = result.exclude; @@ -614,6 +614,16 @@ export class Project implements vscode.Disposable { }); } + /** + * The latch key for this project's config-dependency verdict — its own + * namespace for the same reasons as `RstestApi.nodeRuntimeStatusSource`: + * a different fact than the core checks sharing the bare source URI, and + * one that must die with the project (`dispose` forgets it). + */ + private get configDependencyStatusSource(): string { + return `config-deps:${this.sourceUri.toString()}`; + } + // The config imports a package that is not installed: the not-installed // state (AGENTS.md), one step past a missing `@rstest/core` — some install // *above* the project satisfied the shim, so the config itself is what @@ -629,7 +639,7 @@ export class Project implements vscode.Disposable { 'rstest', relativeTo(this.workspaceFolder, this.sourceUri), ), - this.sourceUri.toString(), + this.configDependencyStatusSource, ); this.onConfigResolved?.(); } @@ -699,6 +709,7 @@ export class Project implements vscode.Disposable { // its own entries. Bridge *resolution* failures latch under the config // directory instead and are reconciled by `syncBridgeProjects`, not here. status.forget(this.sourceUri.toString()); + status.forget(this.configDependencyStatusSource); } get collection() { return this.testItem?.children || this.parentCollection; diff --git a/packages/vscode/src/stacks/test/status.ts b/packages/vscode/src/stacks/test/status.ts index 2b25b17..69f84ce 100644 --- a/packages/vscode/src/stacks/test/status.ts +++ b/packages/vscode/src/stacks/test/status.ts @@ -33,8 +33,9 @@ class StatusHolder implements StatusReporter { // URI (unique per project, so sibling configs in one directory stay // independent), a bridge shim resolution under its config directory, a // fact about the extension host itself under `NODE_RUNTIME_STATUS_SOURCE` - // below, and a project's configured-runtime advisory under a - // `node-runtime:`-prefixed URI; the four namespaces never collide. + // below, a project's configured-runtime advisory under a + // `node-runtime:`-prefixed URI, and a project's config-dependency verdict + // under a `config-deps:`-prefixed URI; the namespaces never collide. // A recovery observed under one key must not clear another key's // live failure. An entry is cleared by the code path that observes the // corresponding recovery (a worker that actually spawned, a version check @@ -117,27 +118,28 @@ class StatusHolder implements StatusReporter { this.#reporter?.running(detail); } + crashed(detail: string, source = ''): void { + this.#crashes.set(source, detail); + this.#paintOrRun(); + } + /** - * One source, one verdict: a newly observed failure kind replaces whatever - * other kind the same source had latched, so a raise site never has to know - * the other tables exist. Without this, a lower-ranked observation could - * not win — a root whose `rstack` was outdated and is now removed would - * keep painting the stale upgrade hint over "not installed". + * A package-state observation (`versionMismatch` / `notInstalled`) is a + * full restatement of its root: the two are mutually exclusive facts about + * the same package, and either also retires a previous crash — the worker + * that failed ran against a state that no longer holds, and its only other + * exit, `workerSpawned`, cannot happen while the package is unusable. + * `crashed` above supersedes nothing: it is a fact about the worker, + * arriving while the package state stays whatever it was. */ - #supersede(keep: Map<string, string>, source: string): void { + #restatePackageState(keep: Map<string, string>, source: string): void { for (const latch of [this.#crashes, this.#mismatches, this.#notInstalled]) { if (latch !== keep) latch.delete(source); } } - crashed(detail: string, source = ''): void { - this.#supersede(this.#crashes, source); - this.#crashes.set(source, detail); - this.#paintOrRun(); - } - versionMismatch(detail: string, source = ''): void { - this.#supersede(this.#mismatches, source); + this.#restatePackageState(this.#mismatches, source); this.#mismatches.set(source, detail); this.#paintOrRun(); } @@ -162,7 +164,7 @@ class StatusHolder implements StatusReporter { */ notInstalled(reason: string, source = ''): void { if (this.#notInstalled.get(source) === reason) return; - this.#supersede(this.#notInstalled, source); + this.#restatePackageState(this.#notInstalled, source); this.#notInstalled.set(source, reason); this.#paintOrRun(); } diff --git a/packages/vscode/src/stacks/test/worker/index.ts b/packages/vscode/src/stacks/test/worker/index.ts index 7c159e0..7d310c5 100644 --- a/packages/vscode/src/stacks/test/worker/index.ts +++ b/packages/vscode/src/stacks/test/worker/index.ts @@ -88,7 +88,9 @@ export class Worker { // Classified here and not in the master: `code` does not survive the // IPC round-trip. Only this unprompted, per-config evaluation gets the // treatment — a run or list the user asked for reports its failure. - const cause = missingDependencyCauseOf(error); + // The worker's spawn cwd is the project root (adaptation #5), which is + // where the config's dependencies are installed. + const cause = missingDependencyCauseOf(error, process.cwd()); if (cause !== undefined) { return { ok: false, reason: 'missing-dependency', message: cause }; } diff --git a/packages/vscode/tests/stacks/test/bridge.test.ts b/packages/vscode/tests/stacks/test/bridge.test.ts index b7ee9f4..00aee1e 100644 --- a/packages/vscode/tests/stacks/test/bridge.test.ts +++ b/packages/vscode/tests/stacks/test/bridge.test.ts @@ -116,7 +116,9 @@ describe('resolveRstackShim', () => { const root = makeTmpDir(); expect(resolveRstackShim(root)).toBeUndefined(); - expect(logged.join('\n')).toContain('Cannot find the "rstack" package'); + // The shared not-installed wording, with the bridge's own consequence. + expect(logged.join('\n')).toContain('rstack is not installed'); + expect(logged.join('\n')).toContain('Rstest cannot be driven'); // The uniform not-installed policy: the same `disabled` shape fmt and // lint report, with the restart command as the way out. expect(reported).toEqual([ diff --git a/packages/vscode/tests/stacks/test/coreResolution.test.ts b/packages/vscode/tests/stacks/test/coreResolution.test.ts index eacc362..8d654bb 100644 --- a/packages/vscode/tests/stacks/test/coreResolution.test.ts +++ b/packages/vscode/tests/stacks/test/coreResolution.test.ts @@ -66,6 +66,11 @@ describe('core-not-found messages', () => { }); describe('missingDependencyCauseOf', () => { + // The classifier resolves from the worker's cwd — the project root; the + // test directory stands in for it. + const classify = (error: unknown, from = __dirname) => + missingDependencyCauseOf(error, from); + // Same reasoning as `resolveError`: the classifier reads a code and a // message Node owns, so the errors come from Node's own loaders. const importError = async (specifier: string): Promise<unknown> => { @@ -79,14 +84,12 @@ describe('missingDependencyCauseOf', () => { it('should name a package an ESM config failed to import', async () => { expect( - missingDependencyCauseOf( - await importError('@rstest/definitely-not-installed'), - ), + classify(await importError('@rstest/definitely-not-installed')), ).toContain("'@rstest/definitely-not-installed'"); }); it('should keep a CJS failure to one line, without the require stack', () => { - const cause = missingDependencyCauseOf( + const cause = classify( resolveError('@rstest/definitely-not-installed', __dirname), ); expect(cause).toContain("'@rstest/definitely-not-installed'"); @@ -101,11 +104,11 @@ describe('missingDependencyCauseOf', () => { // installing dependencies cannot fix it, so it must not be classified as // the not-installed state. The ESM loader reports relative imports as // absolute paths, which the absolute case stands in for. + expect(classify(resolveError('./definitely-missing', __dirname))).toBe( + undefined, + ); expect( - missingDependencyCauseOf(resolveError('./definitely-missing', __dirname)), - ).toBe(undefined); - expect( - missingDependencyCauseOf( + classify( resolveError( path.join(os.tmpdir(), 'definitely-missing.js'), os.tmpdir(), @@ -114,14 +117,36 @@ describe('missingDependencyCauseOf', () => { ).toBe(undefined); }); + it('should tell a missing subpath of an installed package from a missing one', () => { + // `require('installed-package/missing')` fails with the same code and a + // bare-looking specifier, but the package is there — that is a source + // error, not the not-installed state. The same subpath under a package + // that is really absent still is. + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'rstest-vscode-')); + try { + const pkgDir = path.join(root, 'node_modules', 'installed-package'); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, 'package.json'), + '{"name":"installed-package","version":"1.0.0","main":"./index.js"}', + ); + fs.writeFileSync(path.join(pkgDir, 'index.js'), 'module.exports = {};\n'); + + expect( + classify(resolveError('installed-package/missing', root), root), + ).toBe(undefined); + expect( + classify(resolveError('not-installed-package/missing', root), root), + ).toContain("'not-installed-package/missing'"); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + it('should leave every other failure to the full error report', () => { - expect(missingDependencyCauseOf(new SyntaxError('Unexpected token'))).toBe( - undefined, - ); - expect(missingDependencyCauseOf(new Error("Cannot find package 'x'"))).toBe( - undefined, - ); - expect(missingDependencyCauseOf("Cannot find package 'x'")).toBe(undefined); - expect(missingDependencyCauseOf(undefined)).toBe(undefined); + expect(classify(new SyntaxError('Unexpected token'))).toBe(undefined); + expect(classify(new Error("Cannot find package 'x'"))).toBe(undefined); + expect(classify("Cannot find package 'x'")).toBe(undefined); + expect(classify(undefined)).toBe(undefined); }); }); diff --git a/packages/vscode/tests/stacks/test/status.test.ts b/packages/vscode/tests/stacks/test/status.test.ts index 96d24ef..b1d8d00 100644 --- a/packages/vscode/tests/stacks/test/status.test.ts +++ b/packages/vscode/tests/stacks/test/status.test.ts @@ -49,18 +49,28 @@ describe('StatusHolder failure latches', () => { expect(calls).toEqual(['crashed:spawn ENOENT', 'running:']); }); - it('lets a new verdict supersede another kind latched by the same root', () => { - // One source, one verdict: without this, the higher-ranked stale entry - // would keep painting over the newer observation — an outdated rstack - // that is then removed must show "not installed", not the upgrade hint. + it('lets the package-state verdicts supersede each other per root', () => { + // Mismatch and not-installed describe the same fact — the root's package + // — and are mutually exclusive: without the supersession the stale + // higher-ranked mismatch would keep painting the upgrade hint after the + // package is removed. const calls = bindRecorder(); status.versionMismatch('rstack too old', '/a'); status.notInstalled('rstack is not installed', '/a'); expect(calls).toEqual(['mismatch:rstack too old', 'report:disabled']); + status.versionMismatch('rstack too old', '/a'); + expect(calls.slice(2)).toEqual(['mismatch:rstack too old']); + }); + + it('lets a package-state observation retire a stale crash', () => { + // The crash's only other exit is `workerSpawned`, which cannot happen + // while the package is unusable — a fresh resolution verdict restates + // the root, so the crash must not outlive it and paint over `disabled`. + const calls = bindRecorder(); status.crashed('spawn ENOENT', '/a'); status.notInstalled('rstack is not installed', '/a'); - expect(calls.slice(2)).toEqual(['crashed:spawn ENOENT', 'report:disabled']); + expect(calls).toEqual(['crashed:spawn ENOENT', 'report:disabled']); }); it('outranks a mismatch with a crash and falls back on recovery', () => { From 82adc5995e2c905be35cef983278d8ffc4c70056 Mon Sep 17 00:00:00 2001 From: fi3ework <fi3ework@gmail.com> Date: Mon, 24 Aug 2026 16:54:23 +0800 Subject: [PATCH 5/5] docs(vscode): scope the config-import case to Rstest, tracked in #30 --- packages/vscode/AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index c6bf045..960cf7f 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -23,7 +23,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - **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.*`). - **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. -- **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. +- **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). - 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 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.