fix(vscode): address review findings on quiet classification, latches, color marker

Three review findings, each verified before fixing:

- The missing-cwd spawn refusal threw a plain Error, so callers re-logged
  the already-warned stale-project state as an error with a stack. The
  guard now throws ReportedRstestResolutionError (which gained an optional
  message), and the four catch sites above RstestApi share one
  logUnlessReported helper next to the class instead of re-deciding.
- The re-raise dedupe in versionMismatch/notInstalled short-circuited
  before the package-state restatement, so a crash latched between two
  identical verdicts survived a retry that aborted before spawning. Both
  observations now fold into one #observePackageState that restates
  first and skips only the repaint.
- retractForceColorIfDisabled left RSTACK_FORCE_COLOR_INJECTED in the env
  on the no-NO_COLOR path; the marker is now removed once the decision is
  complete, so pool processes and user test code never observe it.

Author: fi3ework

packages/vscode/src/stacks/test/coreResolution.ts

@@ -1,7 +1,9 @@
 /**
- * Classifying and reporting failed Rstest resolutions — the host-side helpers
- * for a `@rstest/core` that cannot be resolved. (The worker-side classifier
- * for a config whose own import failed is `shared/missingDependency.ts`.)
+ * Classifying and reporting failed Rstest worker setups — the host-side
+ * helpers for a `@rstest/core` that cannot be resolved, and the
+ * already-reported marker for any setup failure whose actionable state was
+ * logged where it was observed. (The worker-side classifier for a config
+ * whose own import failed is `shared/missingDependency.ts`.)
  *
  * 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
@@ -11,18 +13,28 @@
  * has to fix, so it is notified.
  */
 
+import { logger } from './logger';
+
 /**
- * Resolution failed after the actionable error was already logged or shown.
- * Callers still reject so project initialization stops, but must not report the
- * same failure again.
+ * The worker could not be set up, and the actionable state was already logged
+ * or shown — a core that did not resolve, or a spawn refused because the
+ * project directory is gone. Callers still reject so the operation stops, but
+ * must not report the same failure again: catch sites log through
+ * `logUnlessReported` below instead of re-deciding.
  */
 export class ReportedRstestResolutionError extends Error {
-  constructor() {
-    super('Failed to resolve rstest path');
+  constructor(message = 'Failed to resolve rstest path') {
+    super(message);
     this.name = 'ReportedRstestResolutionError';
   }
 }
 
+/** The catch-site half of the contract above. */
+export function logUnlessReported(message: string, error: unknown): void {
+  if (error instanceof ReportedRstestResolutionError) return;
+  logger.error(message, error);
+}
+
 // Whether `specifier` itself is what could not be found. `MODULE_NOT_FOUND`
 // alone is too broad: a package that is installed but whose entry file is gone
 // (an interrupted install, or a workspace link that has not been built) throws

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

@@ -4,6 +4,7 @@ import type {
   StackContext,
   StackController,
 } from '../../types';
+import { logUnlessReported } from './coreResolution';
 import { RstestDiagnostics } from './diagnostics';
 import { TestErrorStore, testMessageText } from './errorStore';
 import { logger } from './logger';
@@ -486,7 +487,7 @@ class Rstest implements vscode.Disposable {
         request.include ?? gatherTestItems(this.ctrl.items, false),
       );
     } catch (error) {
-      logger.error('Error running tests:', error);
+      logUnlessReported('Error running tests:', error);
     } finally {
       run.end();
     }

packages/vscode/src/stacks/test/master.ts

@@ -685,7 +685,7 @@ export class RstestApi {
     if (this.cwdIsGone()) {
       const message = this.missingCwdMessage();
       logger.warn(message);
-      throw new Error(message);
+      throw new ReportedRstestResolutionError(message);
     }
     // Resolved once per spawn and handed back to the caller: the callers'
     // worker requests need the same path, and re-resolving would repeat the

packages/vscode/src/stacks/test/project.ts

@@ -10,7 +10,7 @@ import {
   formatConfigDependencyMissingLog,
   formatConfigDependencyMissingStatus,
 } from '../../shared/notInstalled';
-import { ReportedRstestResolutionError } from './coreResolution';
+import { logUnlessReported } from './coreResolution';
 import { logger } from './logger';
 import { RstestApi } from './master';
 import { type ChildProjectRef, computeCoveredConfigs } from './projectCoverage';
@@ -606,9 +606,7 @@ export class Project implements vscode.Disposable {
       .catch((error) => {
         if (this.cancellationSource.token.isCancellationRequested) return;
         this.configLoadFailed = true;
-        if (!(error instanceof ReportedRstestResolutionError)) {
-          logger.error('Failed to initialize project config', error);
-        }
+        logUnlessReported('Failed to initialize project config', error);
         // Let the manager settle its tree even when a config fails to load.
         this.onConfigResolved?.();
       });
@@ -788,7 +786,10 @@ export class Project implements vscode.Disposable {
               })
               .catch((error) => {
                 if (!token.isCancellationRequested) {
-                  logger.error('Failed to update runtime test list', error);
+                  logUnlessReported(
+                    'Failed to update runtime test list',
+                    error,
+                  );
                 }
               });
           };
@@ -821,7 +822,7 @@ export class Project implements vscode.Disposable {
           });
         } catch (error) {
           if (!token.isCancellationRequested) {
-            logger.error('Failed to collect test files', error);
+            logUnlessReported('Failed to collect test files', error);
           }
         } finally {
           if (this.testItem) {

packages/vscode/src/stacks/test/shared/colorEnv.ts

@@ -42,11 +42,14 @@ export function injectForceColor(env: NodeJS.ProcessEnv): void {
 
 /**
  * Retract an earlier injection if the loaded config set `NO_COLOR`. Call in
- * the worker, right after the config has been evaluated.
+ * the worker, right after the config has been evaluated. The marker is
+ * removed either way: the decision is complete, and pool processes, user
+ * test code and their children must not observe an extension-only variable
+ * the bare CLI never supplies.
  */
 export function retractForceColorIfDisabled(env: NodeJS.ProcessEnv): void {
   if (env[INJECTED_MARKER] === '1' && env.NO_COLOR !== undefined) {
     delete env.FORCE_COLOR;
-    delete env[INJECTED_MARKER];
   }
+  delete env[INJECTED_MARKER];
 }

packages/vscode/src/stacks/test/status.ts

@@ -131,20 +131,29 @@ class StatusHolder implements StatusReporter {
    * 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.
+   *
+   * Both observations are re-raised on every refresh pass with the same
+   * words (the bridge re-resolves per pass), so an unchanged observation is
+   * not repainted. The restatement still runs first: a re-raise means a
+   * fresh resolution pass ran, so a crash latched in between — whose worker
+   * is gone — is retired even when the verdict itself did not change.
    */
-  #restatePackageState(keep: Map<string, string>, source: string): void {
-    for (const latch of [this.#crashes, this.#mismatches, this.#notInstalled]) {
-      if (latch !== keep) latch.delete(source);
+  #observePackageState(
+    latch: Map<string, string>,
+    detail: string,
+    source: string,
+  ): void {
+    let retired = false;
+    for (const other of [this.#crashes, this.#mismatches, this.#notInstalled]) {
+      if (other !== latch && other.delete(source)) retired = true;
     }
+    if (!retired && latch.get(source) === detail) return;
+    latch.set(source, detail);
+    this.#paintOrRun();
   }
 
-  // Like `notInstalled`, re-raised on every refresh pass with the same words
-  // (the bridge re-resolves per pass), so an unchanged entry is not restated.
   versionMismatch(detail: string, source = ''): void {
-    if (this.#mismatches.get(source) === detail) return;
-    this.#restatePackageState(this.#mismatches, source);
-    this.#mismatches.set(source, detail);
-    this.#paintOrRun();
+    this.#observePackageState(this.#mismatches, detail, source);
   }
 
   /** A worker process came up: that root's previous spawn failure is over. */
@@ -157,7 +166,7 @@ class StatusHolder implements StatusReporter {
    * A package version check passed. A version was read, so the package is
    * necessarily installed: this one observation ends both a previous
    * mismatch and a previous missing install — the recovery-side restatement
-   * mirroring `#restatePackageState`, so a success site cannot forget half
+   * mirroring `#observePackageState`, so a success site cannot forget half
    * the clearing (`crashed` stays: it is a fact about the worker, not the
    * package). `installed` below survives for the `config-deps:` namespace,
    * which has no version verdict.
@@ -168,17 +177,9 @@ class StatusHolder implements StatusReporter {
     if (hadMismatch || hadNotInstalled) 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.
-   */
+  /** A root's dependencies are not installed: `disabled`, with the way out. */
   notInstalled(reason: string, source = ''): void {
-    if (this.#notInstalled.get(source) === reason) return;
-    this.#restatePackageState(this.#notInstalled, source);
-    this.#notInstalled.set(source, reason);
-    this.#paintOrRun();
+    this.#observePackageState(this.#notInstalled, reason, source);
   }
 
   /** A resolution under that root succeeded: its missing install is over. */

packages/vscode/tests/stacks/test/colorEnv.test.ts

@@ -29,20 +29,25 @@ describe('injectForceColor', () => {
 });
 
 describe('retractForceColorIfDisabled', () => {
+  // Both paths also assert the internal marker is gone: pool processes and
+  // user test code must not observe an extension-only variable the bare CLI
+  // never supplies.
   it('retracts the injection when the config set NO_COLOR', () => {
     const env: NodeJS.ProcessEnv = {};
     injectForceColor(env);
     env.NO_COLOR = '1'; // config load
     retractForceColorIfDisabled(env);
     expect(env.FORCE_COLOR).toBeUndefined();
     expect(env.NO_COLOR).toBe('1');
+    expect(env.RSTACK_FORCE_COLOR_INJECTED).toBeUndefined();
   });
 
   it('leaves the injection alone when the config set nothing', () => {
     const env: NodeJS.ProcessEnv = {};
     injectForceColor(env);
     retractForceColorIfDisabled(env);
     expect(env.FORCE_COLOR).toBe('1');
+    expect(env.RSTACK_FORCE_COLOR_INJECTED).toBeUndefined();
   });
 
   it('never touches a user-set FORCE_COLOR (that conflict warns in the bare CLI too)', () => {

packages/vscode/tests/stacks/test/master.test.ts

@@ -494,7 +494,12 @@ describe('RstestApi worker spawn failures', () => {
     api = createApi(cwd);
     fs.rmSync(cwd, { recursive: true, force: true });
 
-    await expect(api.createChildProcess()).rejects.toThrow('no longer exists');
+    // The reported-error class keeps the quiet classification through the
+    // callers — `logUnlessReported` must not re-log this as a failure.
+    await expect(api.createChildProcess()).rejects.toMatchObject({
+      name: 'ReportedRstestResolutionError',
+      message: expect.stringContaining('no longer exists'),
+    });
 
     expect(shownMessages).toEqual([]);
     expect(reported).toEqual([]);

packages/vscode/tests/stacks/test/status.test.ts

@@ -63,6 +63,31 @@ describe('StatusHolder failure latches', () => {
     expect(calls.slice(2)).toEqual(['mismatch:rstack too old']);
   });
 
+  it('retires a stale crash even when the re-raised verdict is unchanged', () => {
+    // A re-raise means a fresh resolution pass ran, so the crashed worker is
+    // gone — e.g. a retry that aborts before spawning (occupied debug port)
+    // never reaches `workerSpawned`, and only the restatement can clear the
+    // old spawn failure. The dedupe must not short-circuit past it.
+    const calls = bindRecorder();
+    status.versionMismatch('core too old', '/a');
+    status.crashed('spawn ENOENT', '/a');
+    status.versionMismatch('core too old', '/a');
+    expect(calls).toEqual([
+      'mismatch:core too old',
+      'crashed:spawn ENOENT',
+      'mismatch:core too old',
+    ]);
+  });
+
+  it('stays silent on an identical re-raise with nothing else latched', () => {
+    const calls = bindRecorder();
+    status.notInstalled('core missing', '/a');
+    status.notInstalled('core missing', '/a');
+    status.versionMismatch('core too old', '/b');
+    status.versionMismatch('core too old', '/b');
+    expect(calls).toEqual(['report:disabled', 'mismatch:core 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