From cb979ac84d32ff44e3cd17849a523050f4a14792 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:43:38 +0900 Subject: [PATCH] fix(engine): reject over-complex screenshotTableGate whenPaths globs at normalization 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 --- .../src/review/screenshot-table-gate.ts | 22 +++++++-- .../test/screenshot-table-gate.test.ts | 47 ++++++++++++++++++- test/unit/screenshot-table-gate.test.ts | 31 ++++++++++++ 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/packages/loopover-engine/src/review/screenshot-table-gate.ts b/packages/loopover-engine/src/review/screenshot-table-gate.ts index bc741505e..b1af5b8fd 100644 --- 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 `!` is compiled as ``, 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 index d48044537..79b70ef8e 100644 --- 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 index 47ad62e5e..a329e296c 100644 --- 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)", () => {