fix(mcp): move pack validation out of unit suite (#10065)

scripts/build/mcpPublishedFilesClosure.ts

@@ -0,0 +1,132 @@
+/**
+ * Shared MCP publish-path helpers (#3578 / #3821).
+ *
+ * Unit tests use the static `files` allowlist walker (no subprocess).
+ * The pack-artifact gate uses the same helpers against a real
+ * `npm pack --dry-run --ignore-scripts` file list so concurrent unit
+ * suites never shell out to `npm pack`.
+ */
+
+import fs from "node:fs";
+import path from "node:path";
+
+import { normalizeArtifactPath } from "./pack-artifact-policy.ts";
+
+/** Co-located test / spec paths that must never ship in the npm tarball. */
+export const PACK_ARTIFACT_TEST_FILE_RE = /(?:^|\/)__tests__\/|\.(?:test|spec)\.[cm]?[jt]sx?$/;
+
+/** Negations that must stay in package.json `files` (static unit guard). */
+export const REQUIRED_PACKAGE_FILES_TEST_NEGATIONS: readonly string[] = [
+  "!**/__tests__/**",
+  "!**/*.test.ts",
+  "!**/*.test.tsx",
+  "!**/*.test.js",
+  "!**/*.test.mjs",
+  "!**/*.spec.ts",
+  "!**/*.spec.tsx",
+];
+
+/** Spot-check file from the original #3578 bug report. */
+export const MCP_CLOSURE_SPOT_CHECK_PATH = "src/lib/combos/steps.ts";
+
+function resolveImport(root: string, fromFile: string, spec: string): string | null {
+  let base: string;
+  if (spec.startsWith("@/")) base = path.join("src", spec.slice(2));
+  else if (spec.startsWith("@omniroute/open-sse/"))
+    base = path.join("open-sse", spec.slice("@omniroute/open-sse/".length));
+  else if (spec === "@omniroute/open-sse") base = path.join("open-sse", "index");
+  else if (spec.startsWith("./") || spec.startsWith("../"))
+    base = path.join(path.dirname(fromFile), spec);
+  else return null; // bare package — not our source
+  base = base.replace(/\.(ts|tsx|js|mjs)$/, "");
+  const cands = [
+    base + ".ts",
+    base + ".tsx",
+    path.join(base, "index.ts"),
+    path.join(base, "index.tsx"),
+    base + ".js",
+    base + ".mjs",
+  ];
+  for (const c of cands) if (fs.existsSync(path.join(root, c))) return c;
+  return null;
+}
+
+/**
+ * Transitive import closure of the MCP server entrypoints under `src/` + `open-sse/`.
+ */
+export function computeMcpClosure(root: string = process.cwd()): string[] {
+  const roots: string[] = [];
+  for (const f of fs.readdirSync(path.join(root, "open-sse/mcp-server"))) {
+    if (f.endsWith(".ts")) roots.push("open-sse/mcp-server/" + f);
+  }
+  for (const d of ["open-sse/mcp-server/tools", "open-sse/mcp-server/schemas"]) {
+    const abs = path.join(root, d);
+    if (fs.existsSync(abs))
+      for (const f of fs.readdirSync(abs)) if (f.endsWith(".ts")) roots.push(d + "/" + f);
+  }
+
+  const seen = new Set<string>();
+  const stack = [...roots];
+  const importRe =
+    /(?:import|export)[^"']*?from\s*["']([^"']+)["']|import\s*\(\s*["']([^"']+)["']\s*\)/g;
+  while (stack.length) {
+    const f = stack.pop() as string;
+    if (seen.has(f)) continue;
+    seen.add(f);
+    let src: string;
+    try {
+      src = fs.readFileSync(path.join(root, f), "utf8");
+    } catch {
+      continue;
+    }
+    let m: RegExpExecArray | null;
+    while ((m = importRe.exec(src))) {
+      const spec = m[1] || m[2];
+      if (!spec) continue;
+      const r = resolveImport(root, f, spec);
+      if (r && !seen.has(r)) stack.push(r);
+    }
+  }
+  return [...seen].filter((f) => f.startsWith("src/") || f.startsWith("open-sse/"));
+}
+
+/** Whether `file` is covered by a package.json `files` allowlist entry. */
+export function isCoveredByFiles(file: string, filesEntries: string[]): boolean {
+  for (const entry of filesEntries) {
+    if (entry.startsWith("!")) continue; // negations are not positive coverage
+    if (entry.endsWith("/")) {
+      if (file === entry.slice(0, -1) || file.startsWith(entry)) return true;
+    } else if (file === entry || file.startsWith(entry + "/")) {
+      return true;
+    }
+  }
+  return false;
+}
+
+/** Packed paths that look like test / spec files (over-inclusion). */
+export function findLeakedTestArtifactPaths(filePaths: string[]): string[] {
+  return filePaths
+    .map(normalizeArtifactPath)
+    .filter(Boolean)
+    .filter((filePath) => PACK_ARTIFACT_TEST_FILE_RE.test(filePath))
+    .sort();
+}
+
+/** MCP closure members missing from a packed (or candidate) path set. */
+export function findMissingMcpClosurePaths(
+  packedPaths: string[],
+  closurePaths: string[] = computeMcpClosure()
+): string[] {
+  const packed = new Set(packedPaths.map(normalizeArtifactPath).filter(Boolean));
+  return closurePaths
+    .map(normalizeArtifactPath)
+    .filter(Boolean)
+    .filter((filePath) => !packed.has(filePath))
+    .sort();
+}
+
+/** Required `files` negation entries that are absent from package.json. */
+export function findMissingPackageFilesTestNegations(filesEntries: string[]): string[] {
+  const present = new Set(filesEntries);
+  return REQUIRED_PACKAGE_FILES_TEST_NEGATIONS.filter((entry) => !present.has(entry));
+}

scripts/build/pack-artifact-policy.ts

@@ -228,6 +228,59 @@ export function normalizeArtifactPath(filePath: string): string {
     .replace(/\/{2,}/g, "/");
 }
 
+/** Extract complete JSON values from npm's mixed stdout/stderr-style output. */
+export function parseJsonValuesOutput(output: string): unknown[] {
+  const values: unknown[] = [];
+  for (let start = 0; start < output.length; start++) {
+    if (output[start] !== "[" && output[start] !== "{") continue;
+
+    const stack: string[] = [];
+    let inString = false;
+    let escaped = false;
+    for (let end = start; end < output.length; end++) {
+      const char = output[end];
+      if (inString) {
+        if (escaped) escaped = false;
+        else if (char === "\\") escaped = true;
+        else if (char === '"') inString = false;
+        continue;
+      }
+      if (char === '"') {
+        inString = true;
+      } else if (char === "[" || char === "{") {
+        stack.push(char);
+      } else if (char === "]" || char === "}") {
+        const expectedOpen = char === "]" ? "[" : "{";
+        if (stack.at(-1) !== expectedOpen) break;
+        stack.pop();
+        if (stack.length === 0) {
+          try {
+            const parsed: unknown = JSON.parse(output.slice(start, end + 1));
+            values.push(parsed);
+            start = end;
+          } catch {
+            // This bracket pair was not a complete JSON value; continue scanning.
+          }
+          break;
+        }
+      }
+    }
+  }
+  return values;
+}
+
+/** Extract the first matching JSON array from npm's mixed stdout/stderr-style output. */
+export function parseJsonArrayOutput(
+  output: string,
+  matches: (parsed: unknown[]) => boolean = () => true
+): unknown[] {
+  const parsed = parseJsonValuesOutput(output).find(
+    (value): value is unknown[] => Array.isArray(value) && matches(value)
+  );
+  if (!parsed) throw new Error("Expected a valid JSON array in command output.");
+  return parsed;
+}
+
 /**
  * Paths that are NEVER publishable, whatever the allowlist says.
  *

scripts/build/validate-pack-artifact.ts

@@ -1,16 +1,23 @@
 #!/usr/bin/env node
 
-import { execFileSync } from "node:child_process";
+import { execFileSync, spawnSync } from "node:child_process";
 import { existsSync } from "node:fs";
 import { dirname, join } from "node:path";
 import { fileURLToPath } from "node:url";
 
+import {
+  MCP_CLOSURE_SPOT_CHECK_PATH,
+  computeMcpClosure,
+  findLeakedTestArtifactPaths,
+  findMissingMcpClosurePaths,
+} from "./mcpPublishedFilesClosure.ts";
 import {
   PACK_ARTIFACT_ALLOWED_EXACT_PATHS,
   PACK_ARTIFACT_ALLOWED_PATH_PREFIXES,
   PACK_ARTIFACT_REQUIRED_PATHS,
   findMissingArtifactPaths,
   findUnexpectedArtifactPaths,
+  parseJsonValuesOutput,
 } from "./pack-artifact-policy.ts";
 
 const __filename: string = fileURLToPath(import.meta.url);
@@ -24,12 +31,29 @@ function runNpm(args: string[], stdio: "inherit" | "pipe" = "pipe"): string {
   const command = npmExecPath && !isBunRuntime ? process.execPath : npmCommand;
   const commandArgs = npmExecPath && !isBunRuntime ? [npmExecPath, ...args] : args;
 
-  return execFileSync(command, commandArgs, {
+  if (stdio === "inherit") {
+    execFileSync(command, commandArgs, {
+      cwd: ROOT,
+      encoding: "utf8",
+      stdio: "inherit",
+      maxBuffer: 64 * 1024 * 1024,
+    });
+    return "";
+  }
+
+  const result = spawnSync(command, commandArgs, {
     cwd: ROOT,
     encoding: "utf8",
-    stdio: stdio === "inherit" ? "inherit" : ["ignore", "pipe", "pipe"],
+    stdio: ["ignore", "pipe", "pipe"],
     maxBuffer: 64 * 1024 * 1024,
   });
+  if (result.error) throw result.error;
+  if (result.status !== 0) {
+    throw new Error(
+      (result.stderr || result.stdout || `npm exited with status ${result.status}`).trim()
+    );
+  }
+  return `${result.stdout || ""}\n${result.stderr || ""}`;
 }
 
 function ensureAppStagingReady(): void {
@@ -43,15 +67,39 @@ function ensureAppStagingReady(): void {
   runNpm(["run", "build:cli"], "inherit");
 }
 
-function runPackDryRun(): any {
+type PackReport = {
+  files: Array<{ path: string }>;
+  filename?: string;
+  entryCount?: number;
+  size?: number;
+  unpackedSize?: number;
+};
+
+function findPackReport(value: unknown): PackReport | null {
+  if (Array.isArray(value)) {
+    for (const item of value) {
+      const report = findPackReport(item);
+      if (report) return report;
+    }
+    return null;
+  }
+  if (typeof value !== "object" || value === null) return null;
+
+  const record = value as Record<string, unknown>;
+  if (Array.isArray(record.files)) return record as unknown as PackReport;
+  for (const child of Object.values(record)) {
+    const report = findPackReport(child);
+    if (report) return report;
+  }
+  return null;
+}
+
+function runPackDryRun(): PackReport {
   const output = runNpm(["pack", "--dry-run", "--json", "--ignore-scripts"]);
 
-  const jsonStart = output.indexOf("[");
-  const jsonEnd = output.lastIndexOf("]");
-  const jsonPayload =
-    jsonStart >= 0 && jsonEnd > jsonStart ? output.slice(jsonStart, jsonEnd + 1) : output;
-  const parsed = JSON.parse(jsonPayload);
-  const packReport = Array.isArray(parsed) ? parsed[0] : null;
+  const packReport = parseJsonValuesOutput(output)
+    .map(findPackReport)
+    .find((report): report is PackReport => report !== null);
 
   if (!packReport || !Array.isArray(packReport.files)) {
     throw new Error("npm pack --dry-run --json did not return the expected files[] payload.");
@@ -78,17 +126,17 @@ function formatBytes(bytes: number): string {
 }
 
 // --policy-only: skip the build (ensureAppStagingReady → build:cli) and the
-// required-runtime-files check (which needs the built dist/), running ONLY the
-// unexpected-files allowlist check. The unexpected files (e.g. stray bin/*.sh) are
-// SOURCE files that `npm pack --dry-run` lists regardless of build, so this catches
-// the "new file leaked into the tarball" regression cheaply on the fast-path (PR→release),
-// instead of only on the release PR's full Package Artifact job. See incident v3.8.36 (#5029).
+// required-runtime-files check (which needs the built dist/). Source-side policy checks
+// still run against the real `npm pack --dry-run` file list: unexpected files (e.g. stray
+// bin/*.sh), test/spec leaks, and missing MCP closure files. This catches source regressions
+// cheaply on the fast-path (PR→release), instead of only on the release PR's full Package
+// Artifact job. See incident v3.8.36 (#5029).
 const POLICY_ONLY = process.argv.includes("--policy-only");
 
 try {
   if (!POLICY_ONLY) ensureAppStagingReady();
   const packReport = runPackDryRun();
-  const artifactPaths: string[] = packReport.files.map((file: any) => file.path);
+  const artifactPaths: string[] = packReport.files.map((file) => file.path);
   const unexpectedPaths: string[] = findUnexpectedArtifactPaths(artifactPaths, {
     exactPaths: PACK_ARTIFACT_ALLOWED_EXACT_PATHS,
     prefixPaths: PACK_ARTIFACT_ALLOWED_PATH_PREFIXES,
@@ -97,11 +145,20 @@ try {
     ? []
     : findMissingArtifactPaths(artifactPaths, PACK_ARTIFACT_REQUIRED_PATHS);
 
+  // #3821 — broad `files` prefixes (open-sse/, src/lib/, ...) would otherwise allow
+  // co-located *.test.* / __tests__ leaks; ban them explicitly on the real pack list.
+  const leakedTestPaths: string[] = findLeakedTestArtifactPaths(artifactPaths);
+
+  // #3578 — MCP runs from published TypeScript source; every reachable file must pack.
+  const mcpClosure: string[] = computeMcpClosure(ROOT);
+  const missingMcpPaths: string[] = findMissingMcpClosurePaths(artifactPaths, mcpClosure);
+
   console.log("📦 npm pack artifact summary");
   console.log(`   File:          ${packReport.filename}`);
   console.log(`   Entry count:   ${packReport.entryCount}`);
   console.log(`   Packed size:   ${formatBytes(packReport.size)}`);
   console.log(`   Unpacked size: ${formatBytes(packReport.unpackedSize)}`);
+  console.log(`   MCP closure:   ${mcpClosure.length} source files checked`);
 
   if (unexpectedPaths.length > 0) {
     console.error("\n❌ Unexpected files were found in the npm publish artifact:");
@@ -117,7 +174,33 @@ try {
     }
   }
 
-  if (unexpectedPaths.length > 0 || missingRequiredPaths.length > 0) {
+  if (leakedTestPaths.length > 0) {
+    console.error(
+      "\n❌ Test/spec files leaked into the npm publish artifact (tighten package.json files negations):"
+    );
+    for (const leakedPath of leakedTestPaths) {
+      console.error(`   - ${leakedPath}`);
+    }
+  }
+
+  if (missingMcpPaths.length > 0) {
+    console.error(
+      "\n❌ MCP-reachable source files are missing from the npm publish artifact (would 404 --mcp):"
+    );
+    for (const missingPath of missingMcpPaths) {
+      console.error(`   - ${missingPath}`);
+    }
+    if (missingMcpPaths.includes(MCP_CLOSURE_SPOT_CHECK_PATH)) {
+      console.error(`   (includes the #3578 bug file ${MCP_CLOSURE_SPOT_CHECK_PATH})`);
+    }
+  }
+
+  if (
+    unexpectedPaths.length > 0 ||
+    missingRequiredPaths.length > 0 ||
+    leakedTestPaths.length > 0 ||
+    missingMcpPaths.length > 0
+  ) {
     process.exit(1);
   }