fix(engine): reject over-complex screenshotTableGate whenPaths globs at normalization (#10115)

shin-core · web-flow · commit afe6969abf65 · 2026-07-31T08:12:55.000Z
whenPaths back matchesAnyWithExclusions, whose include half (matchesAny) FAILS TOWARD MATCHING for a glob whose wildcard-group count exceeds the compiler cap -- correct for hardGuardrailGlobs (an over-complex guardrail glob still forces a human hold) but wrong for whenPaths, where matching everything means every PR in the repo is in scope for a close-tier visual gate. normalizeStringList validated type/emptiness/count/length but never wildcard count, so an ordinary monorepo scoping glob like apps/**/src/**/*.tsx (3 groups) silently scoped every PR, and a 3-group exclusion like !**/*.generated.* compiled to NEVER_MATCHES and excluded nothing -- both with no warning. Validate whenPaths at the normalizer with the same hasUnsafeWildcardCount predicate every other manifest glob surface already uses (mirrors focus-manifest's normalizeOptionalGlob): an over-complex entry is dropped with a warning naming the field and index, measured on the glob BODY so an exclusion is judged by the pattern matchesAnyWithExclusions actually compiles, and a bare '!' (which would mis-route into the include list) is dropped too. whenLabels / requireViewports / requireThemes normalization, matchesAny's fail-toward-matching semantics, MAX_GLOB_WILDCARD_GROUPS and hardGuardrailGlobs are all unchanged. Closes #9993
diff --git a/packages/loopover-engine/src/review/screenshot-table-gate.ts b/packages/loopover-engine/src/review/screenshot-table-gate.ts
@@ -1,4 +1,4 @@
-import { matchesAnyWithExclusions } from "../signals/change-guardrail.js";
+import { hasUnsafeWildcardCount, matchesAnyWithExclusions } from "../signals/change-guardrail.js";
 import type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types/manifest-deps-types.js";
 
 export type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types/manifest-deps-types.js";
@@ -39,7 +39,7 @@ export function isScreenshotTableGateAction(value: unknown): value is Screenshot
   return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value);
 }
 
-function normalizeStringList(value: unknown, field: string, max: number, maxChars: number, warnings: string[]): string[] {
+function normalizeStringList(value: unknown, field: string, max: number, maxChars: number, warnings: string[], glob = false): string[] {
   if (value === undefined) return [];
   if (!Array.isArray(value)) {
     warnings.push(`settings.requireScreenshotTable.${field} must be an array; ignoring it.`);
@@ -55,7 +55,21 @@ function normalizeStringList(value: unknown, field: string, max: number, maxChar
       warnings.push(`settings.requireScreenshotTable.${field}[${index}] must be a non-empty string; ignoring it.`);
       continue;
     }
-    out.push(item.trim().slice(0, maxChars));
+    const trimmed = item.trim().slice(0, maxChars);
+    if (glob) {
+      // #9993: whenPaths back matchesAnyWithExclusions, whose include half FAILS TOWARD MATCHING for an
+      // over-complex glob (matchesAny returns true for every path). An unvalidated `apps/**/src/**/*.tsx`
+      // (3 groups) would silently put every PR in scope for a close-tier gate. Reject the same shape the
+      // other manifest glob surfaces reject (mirrors focus-manifest's normalizeOptionalGlob), measured on
+      // the glob BODY: an exclusion `!<body>` is compiled as `<body>`, so a bare `!` has no body to compile
+      // and matchesAnyWithExclusions would mis-route it into the include list -- drop it too.
+      const globBody = trimmed.startsWith("!") ? trimmed.slice(1) : trimmed;
+      if (globBody === "" || hasUnsafeWildcardCount(globBody)) {
+        warnings.push(`settings.requireScreenshotTable.${field}[${index}] has too many wildcards to compile safely; ignoring it.`);
+        continue;
+      }
+    }
+    out.push(trimmed);
   }
   return out;
 }
@@ -93,7 +107,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str
   return {
     enabled,
     whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings),
-    whenPaths: normalizeStringList(record.whenPaths, "whenPaths", MAX_PATHS, MAX_PATH_CHARS, warnings),
+    whenPaths: normalizeStringList(record.whenPaths, "whenPaths", MAX_PATHS, MAX_PATH_CHARS, warnings, true),
     action,
     requireViewports: normalizeStringList(record.requireViewports, "requireViewports", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings),
     requireThemes: normalizeStringList(record.requireThemes, "requireThemes", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings),
diff --git a/packages/loopover-engine/test/screenshot-table-gate.test.ts b/packages/loopover-engine/test/screenshot-table-gate.test.ts
@@ -1,7 +1,7 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
 
-import { normalizeScreenshotTableGateConfig } from "../dist/review/screenshot-table-gate.js";
+import { isScreenshotTableGateInScope, normalizeScreenshotTableGateConfig } from "../dist/review/screenshot-table-gate.js";
 
 // #9996: the invalid-action warning was left naming only "close" or "advisory" after #9964 added the
 // non-destructive "block" tier -- an operator who mistyped the value was never told "block" exists.
@@ -27,3 +27,48 @@ test("normalizeScreenshotTableGateConfig resolves a valid block action with no w
   assert.equal(warnings.length, 0);
   assert.equal(config.action, "block");
 });
+
+// #9993: whenPaths back matchesAnyWithExclusions, whose include half FAILS TOWARD MATCHING for an
+// over-complex glob (matchesAny returns true for every path). An unvalidated 3-group glob such as
+// `apps/**/src/**/*.tsx` therefore silently put EVERY changed file in scope for a close-tier visual gate.
+// whenPaths is now validated with the same hasUnsafeWildcardCount predicate every other manifest glob uses.
+test("#9993: a 3-wildcard-group whenPaths glob is dropped, so an unrelated file is no longer in scope", () => {
+  const warnings: string[] = [];
+  // The over-complex glob alongside a valid one: today the over-complex glob matches README.md (fail-open),
+  // putting it in scope. After the fix only the valid glob survives and README.md is out of scope.
+  const config = normalizeScreenshotTableGateConfig(
+    { enabled: true, whenPaths: ["apps/**/src/**/*.tsx", "apps/ui/src/**"] },
+    warnings,
+  );
+  assert.deepEqual(config.whenPaths, ["apps/ui/src/**"]);
+  assert.ok(warnings.some((w) => w.includes("whenPaths[0]")));
+  assert.equal(isScreenshotTableGateInScope(config, [], ["README.md"]), false);
+  assert.equal(isScreenshotTableGateInScope(config, [], ["apps/ui/src/App.tsx"]), true);
+});
+
+test("#9993: an exclusion is judged by its glob BODY, and a bare `!` is dropped", () => {
+  const warnings: string[] = [];
+  // `!**/*.generated.*` has a 3-group body, so it is dropped (an unvalidated over-complex exclusion compiles
+  // to NEVER_MATCHES and excludes nothing). The valid include survives.
+  const config = normalizeScreenshotTableGateConfig(
+    { enabled: true, whenPaths: ["!**/*.generated.*", "apps/ui/src/**"] },
+    warnings,
+  );
+  assert.deepEqual(config.whenPaths, ["apps/ui/src/**"]);
+  assert.ok(warnings.some((w) => w.includes("whenPaths[0]")));
+
+  const bareBang: string[] = [];
+  const config2 = normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["!"] }, bareBang);
+  assert.deepEqual(config2.whenPaths, []);
+  assert.ok(bareBang.some((w) => w.includes("whenPaths")));
+});
+
+test("#9993: a 2-group whenPaths glob (and a valid exclusion) is preserved unchanged", () => {
+  const warnings: string[] = [];
+  const config = normalizeScreenshotTableGateConfig(
+    { enabled: true, whenPaths: ["apps/ui/public/**/*.json", "!node_modules/**"] },
+    warnings,
+  );
+  assert.deepEqual(config.whenPaths, ["apps/ui/public/**/*.json", "!node_modules/**"]);
+  assert.equal(warnings.filter((w) => w.includes("whenPaths")).length, 0);
+});
diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts
@@ -319,6 +319,37 @@ describe("normalizeScreenshotTableGateConfig", () => {
     expect(normalizeScreenshotTableGateConfig({ skillFileUrl: "x".repeat(301) }, []).skillFileUrl).toBeUndefined();
     expect(warnings.some((w) => w.includes("skillFileUrl"))).toBe(true);
   });
+
+  it("#9993: drops an over-complex whenPaths glob with a warning, so it can no longer scope every PR", () => {
+    // whenPaths back matchesAnyWithExclusions, whose include half FAILS TOWARD MATCHING for an over-complex
+    // glob — so an unvalidated `apps/**/src/**/*.tsx` (3 groups) silently put every PR in scope. It is now
+    // validated with the same hasUnsafeWildcardCount predicate the other manifest glob surfaces use.
+    const warnings: string[] = [];
+    const config = normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["apps/**/src/**/*.tsx", "apps/ui/src/**"] }, warnings);
+    expect(config.whenPaths).toEqual(["apps/ui/src/**"]);
+    expect(warnings.some((w) => w.includes("whenPaths[0]"))).toBe(true);
+    // The behaviour change the drop produces: an unrelated file is no longer in scope (it was, today).
+    expect(isScreenshotTableGateInScope(config, [], ["README.md"])).toBe(false);
+    expect(isScreenshotTableGateInScope(config, [], ["apps/ui/src/App.tsx"])).toBe(true);
+  });
+
+  it("#9993: measures an exclusion by its glob BODY and drops a bare `!`, keeping valid entries", () => {
+    const warnings: string[] = [];
+    // `!**/*.generated.*` has a 3-group body → dropped; the valid include survives.
+    expect(normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["!**/*.generated.*", "apps/ui/src/**"] }, warnings).whenPaths).toEqual(["apps/ui/src/**"]);
+    expect(warnings.some((w) => w.includes("whenPaths[0]"))).toBe(true);
+    // A bare `!` has no body to compile and would mis-route into the include list — dropped.
+    const bare: string[] = [];
+    expect(normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["!"] }, bare).whenPaths).toEqual([]);
+    expect(bare.some((w) => w.includes("whenPaths"))).toBe(true);
+  });
+
+  it("#9993: preserves a 2-group whenPaths glob and a valid exclusion unchanged", () => {
+    const warnings: string[] = [];
+    const config = normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["apps/ui/public/**/*.json", "!node_modules/**"] }, warnings);
+    expect(config.whenPaths).toEqual(["apps/ui/public/**/*.json", "!node_modules/**"]);
+    expect(warnings.filter((w) => w.includes("whenPaths"))).toHaveLength(0);
+  });
 });
 
 describe("requiredScreenshotMatrixPairs (#4535)", () => {
