🤖 tests: delete test-only helpers in node/common and test the production paths

src/browser/stores/ReviewStateStore.test.ts

@@ -10,7 +10,11 @@ import type {
 } from "@/common/orpc/schemas/reviewState";
 import { REVIEW_STATE_SECTIONS } from "@/common/orpc/schemas/reviewState";
 import type { Review } from "@/common/types/review";
-import { applyReviewStateDelta, withReviewStateSection } from "@/common/utils/reviewState";
+import {
+  applyReviewStateDelta,
+  assignSection,
+  capReviewStateSections,
+} from "@/common/utils/reviewState";
 import { installDom } from "../../../tests/ui/dom";
 import { ReviewStateStore } from "./ReviewStateStore";
 
@@ -139,7 +143,10 @@ function createBackend(initial: ReviewStateSections = {}) {
         if (sections[section] !== undefined) {
           results[section] = "present";
         } else {
-          sections = withReviewStateSection(sections, section, incoming);
+          sections = { ...sections };
+          assignSection(sections, section, incoming);
+          // ReviewStateService.importLegacy caps what it imports; keep the double in step.
+          sections = capReviewStateSections(sections);
           results[section] = "applied";
         }
       }

src/browser/stories/mocks/reviewState.ts

@@ -7,7 +7,11 @@ import type {
   ReviewStateUpdateOutput,
 } from "@/common/orpc/schemas/reviewState";
 import { REVIEW_STATE_SECTIONS } from "@/common/orpc/schemas/reviewState";
-import { applyReviewStateDelta, withReviewStateSection } from "@/common/utils/reviewState";
+import {
+  applyReviewStateDelta,
+  assignSection,
+  capReviewStateSections,
+} from "@/common/utils/reviewState";
 
 /**
  * Seeds consumed by the next mock client, so story setup can stage review state before
@@ -93,7 +97,10 @@ export function createMockReviewStateApi() {
           results[section] = "present";
           continue;
         }
-        next = withReviewStateSection(next, section, incoming);
+        next = { ...next };
+        assignSection(next, section, incoming);
+        // ReviewStateService.importLegacy caps what it imports; keep the mock in step.
+        next = capReviewStateSections(next);
         results[section] = "applied";
       }
       const snapshot = next === current ? { sections: current } : commit(input.workspaceId, next);

src/common/utils/reviewState.ts

@@ -104,17 +104,6 @@ export function assignSection<S extends ReviewStateSection>(
   sections[section] = value;
 }
 
-/** Replace one whole section (used by the legacy import); caps still apply. */
-export function withReviewStateSection<S extends ReviewStateSection>(
-  sections: ReviewStateSections,
-  section: S,
-  value: NonNullable<ReviewStateSections[S]>
-): ReviewStateSections {
-  const next: ReviewStateSections = { ...sections };
-  assignSection(next, section, value);
-  return capReviewStateSections(next);
-}
-
 /** Combines a key present on both sides; absent means last write wins. */
 type EntryMerge<V> = ((existing: V, incoming: V) => V) | undefined;
 

src/common/utils/tools/toolCatalog.test.ts

@@ -5,7 +5,6 @@ import type { ToolPolicy } from "@/common/utils/tools/toolPolicy";
 import {
   applyNativeToolSearchReplay,
   buildToolCatalog,
-  buildToolCatalogOverview,
   buildToolSearchModelOutput,
   collectDeferLoadingToolNames,
   dedupeNativeToolReferences,
@@ -64,6 +63,36 @@ function baseTools(): Record<string, Tool> {
   };
 }
 
+/**
+ * The overview prepareToolSearch appends to tool_catalog_search's description when `catalog` is
+ * the deferred MCP surface. The search tool's own description is empty, so the result is the
+ * overview alone, and "" when nothing is deferred (prepareToolSearch then drops the tool).
+ */
+function advertisedOverview(catalog: readonly ToolCatalogEntry[]): string {
+  const prepared = prepareToolSearch({
+    tools: {
+      tool_catalog_search: fakeTool(""),
+      ...Object.fromEntries(
+        catalog.map((entry) => [
+          entry.name,
+          mcpTool(
+            entry.description,
+            entry.paramText ? { p: { description: entry.paramText } } : {}
+          ),
+        ])
+      ),
+    },
+    mcpToolNames: catalog.map((entry) => entry.name),
+    mcpToolServers: Object.fromEntries(
+      catalog.flatMap((entry) =>
+        entry.serverName === undefined ? [] : [[entry.name, entry.serverName]]
+      )
+    ),
+  });
+  const description = prepared.tools[TOOL_SEARCH_TOOL_NAME]?.description;
+  return typeof description === "string" ? description : "";
+}
+
 /**
  * baseTools() with deferred definitions large enough for native deferral
  * (#5405); the padding is neutral prose so search scoring is unaffected.
@@ -150,13 +179,13 @@ describe("catalog advertising (overview + server names)", () => {
     expect(byName.get("github_create_issue")).toBe("github");
   });
 
-  test("buildToolCatalogOverview groups deferred tool names by server, deterministically", () => {
+  test("the overview groups deferred tool names by server, deterministically", () => {
     const { catalog } = buildToolCatalog({
       tools: baseTools(),
       mcpToolNames: MCP_NAMES,
       mcpToolServers: MCP_TOOL_SERVERS,
     });
-    const overview = buildToolCatalogOverview(catalog);
+    const overview = advertisedOverview(catalog);
     expect(overview).toBe(
       "Deferred tool catalog overview (search to activate):\n" +
         "- github: github_create_issue\n" +
@@ -172,7 +201,7 @@ describe("catalog advertising (overview + server names)", () => {
       paramText: `SECRET_PARAM_${i}`,
       serverName: "srv",
     }));
-    const overview = buildToolCatalogOverview(catalog);
+    const overview = advertisedOverview(catalog);
     expect(overview).toContain(
       "- srv: srv_tool_0, srv_tool_1, srv_tool_2, srv_tool_3, srv_tool_4, srv_tool_5, srv_tool_6, srv_tool_7, and more\n"
     );
@@ -181,10 +210,8 @@ describe("catalog advertising (overview + server names)", () => {
   });
 
   test("overview is empty for an empty catalog and groups unknown servers as 'other tools'", () => {
-    expect(buildToolCatalogOverview([])).toBe("");
-    const overview = buildToolCatalogOverview([
-      { name: "mystery_tool", description: "", paramText: "" },
-    ]);
+    expect(advertisedOverview([])).toBe("");
+    const overview = advertisedOverview([{ name: "mystery_tool", description: "", paramText: "" }]);
     expect(overview).toContain("- other tools: mystery_tool");
   });
 
@@ -301,7 +328,7 @@ describe("catalog advertising (overview + server names)", () => {
     const clampedCatalog: ToolCatalogEntry[] = [
       { name: "long_tool", description: "", paramText: "", serverName: longServer },
     ];
-    const clamped = buildToolCatalogOverview(clampedCatalog);
+    const clamped = advertisedOverview(clampedCatalog);
     expect(clamped).not.toContain(longServer);
     expect(clamped).toContain(`${"s".repeat(63)}…`);
 
@@ -312,7 +339,7 @@ describe("catalog advertising (overview + server names)", () => {
       paramText: "",
       serverName: `server-${String(i).padStart(3, "0")}`,
     }));
-    const bounded = buildToolCatalogOverview(bigCatalog);
+    const bounded = advertisedOverview(bigCatalog);
     expect(bounded.length).toBeLessThan(2200);
     expect(bounded).toMatch(/…and \d+ more servers \(use this tool to discover their tools\)/);
   });
@@ -404,7 +431,7 @@ describe("catalog advertising (overview + server names)", () => {
       paramText: "",
       serverName: evilServer,
     }));
-    const overview = buildToolCatalogOverview(catalog);
+    const overview = advertisedOverview(catalog);
     const matches = searchToolCatalog(catalog, "evil");
     const elapsedMs = performance.now() - startedAt;
     // Bounded work: must not stall stream startup (generous CI headroom).
@@ -421,7 +448,7 @@ describe("catalog advertising (overview + server names)", () => {
     const catalog: ToolCatalogEntry[] = [
       { name: "t1_do_thing", description: "unrelated words", paramText: "", serverName: longToken },
     ];
-    const overview = buildToolCatalogOverview(catalog);
+    const overview = advertisedOverview(catalog);
     const advertisedLabel = `${"a".repeat(63)}…`;
     expect(overview).toContain(advertisedLabel);
     // Querying with the visible label (as a model would copy it) must match.

src/common/utils/tools/toolCatalog.ts

@@ -402,6 +402,13 @@ function groupCatalogByServer(catalog: readonly ToolCatalogEntry[]): Array<[stri
   return [...byServer.entries()].sort(([a], [b]) => a.localeCompare(b));
 }
 
+/**
+ * Build a compact, deterministic index of the deferred catalog grouped by MCP
+ * server, e.g. `- github: github_create_issue, …, and more`.
+ * Only names are listed — descriptions and parameter schemas stay deferred
+ * until activation, so the overview solves the "unknown unknowns" discovery
+ * problem without reintroducing schema token bloat.
+ */
 function buildOverview(catalog: readonly ToolCatalogEntry[]): ToolCatalogOverview {
   const servers = groupCatalogByServer(catalog);
   const lines: string[] = [];
@@ -440,18 +447,6 @@ function buildOverview(catalog: readonly ToolCatalogEntry[]): ToolCatalogOvervie
   };
 }
 
-/**
- * Build a compact, deterministic index of the deferred catalog grouped by MCP
- * server, e.g. `- github: github_create_issue, …, and more`.
- * Only names are listed — descriptions and parameter schemas stay deferred
- * until activation, so the overview solves the "unknown unknowns" discovery
- * problem without reintroducing schema token bloat. Returns "" for an empty
- * catalog.
- */
-export function buildToolCatalogOverview(catalog: readonly ToolCatalogEntry[]): string {
-  return catalog.length === 0 ? "" : buildOverview(catalog).text;
-}
-
 /**
  * A previous overview stays valid while none of its claims became false:
  * - the server set is unchanged, so every line's server still exists, the

src/node/runtime/runtimeScratchDir.test.ts

@@ -229,6 +229,18 @@ describe("runtimeScratchDir", () => {
       expect(await fs.readdir(tempDir)).toEqual([]);
     });
 
+    test("still returns a host dir when creation fails, so the prompt never names an unset variable", async () => {
+      // A regular file where the session dir should be makes mkdir fail.
+      const sessionsRoot = path.join(tempDir, "sessions");
+      await fs.mkdir(sessionsRoot);
+      await fs.writeFile(path.join(sessionsRoot, "ws1"), "not a dir");
+      const dir = getWorkspaceScratchDir(sessionsRoot, "ws1");
+
+      expect(await ensureScratchDirForSpec(new LocalRuntime(tempDir), { kind: "host", dir })).toBe(
+        dir
+      );
+    });
+
     test("creates a runtime-side dir and returns the path the runtime expanded", async () => {
       const runtime = new LocalRuntime(tempDir);
       const scratch = path.join(tempDir, "remote-home", RUNTIME_SCRATCH_DIR_NAME, "ws1");