Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions packages/loopover-engine/src/review/screenshot-table-gate.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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.`);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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),
Expand Down
47 changes: 46 additions & 1 deletion packages/loopover-engine/test/screenshot-table-gate.test.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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);
});
31 changes: 31 additions & 0 deletions test/unit/screenshot-table-gate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)", () => {
Expand Down