Skip to content

Commit b50d2e3

Browse files
committed
test(gate): cover the presence-mode staleness checkpoint and two long-untested reject arms
Brings packages/loopover-engine/src/review/screenshot-table-gate.ts and src/signals/change-guardrail.ts (and both re-export shims) to 100% statement, branch and function coverage. Three branches had no test on any identity: - evaluateScreenshotTableGate's PRESENCE-mode freshness checkpoint. Matrix mode's identical correlation was pinned by #8866's tests, but presence mode runs it through a separate return site and never saw a headSha in a test. Presence mode is the DEFAULT shape, so the miss meant one table pasted on push #1 could hold the gate green for every later push -- the exact regression the checkpoint exists to stop. Six tests: first-push checkpoint, stale-on-new-head (asserting no checkpoint is re-issued, which would launder the staleness away after one extra push), re-affirmation with fresh URLs, custom message on the stale path, no-headSha degradation, and same-head replay. - guardrailPathMatches' empty-path skip, a deliberate divergence from matchesAny's fail-safe: the boolean form matches an empty path under an over-complex glob, the structured form must not, because its output is rendered verbatim into public review text and audit metadata. - extractTableRows' non-table reject arm, which is what stops ordinary PR prose (and a shell pipe inside backticks) from parsing as table rows. Mirrored across both import identities so a sharded, flag-merged coverage upload cannot report a branch as uncovered on one copy.
1 parent 3616282 commit b50d2e3

3 files changed

Lines changed: 244 additions & 1 deletion

File tree

test/unit/change-guardrail.test.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,25 @@ describe("change-guardrail glob matching", () => {
123123
expect(isGuardrailHit(["unrelated/file.ts"], [pathological])).toBe(true);
124124
});
125125

126+
it("INVARIANT: guardrailPathMatches SKIPS an empty changed path — deliberately diverging from matchesAny's fail-safe", () => {
127+
// matchesAny("") returns true for an over-complex glob (asserted above): fail-safe means "match everything
128+
// rather than silently disable a maintainer's guardrail". The STRUCTURED form cannot inherit that, because
129+
// its output is rendered verbatim into public review text and audit metadata — emitting `{ path: "" }` puts
130+
// a blank filename in front of a maintainer, which is worse than useless. So an empty path is dropped here
131+
// even though the boolean form would report it, and that asymmetry is intentional, not an oversight.
132+
const pathological = `${"**/".repeat(12)}*.ts`;
133+
expect(matchesAny("", [pathological])).toBe(true); // the boolean form still fails safe
134+
expect(guardrailPathMatches([""], [pathological])).toEqual([]); // ...the structured one does not invent a row
135+
136+
// The skip is per-path, not a whole-call bail: real paths alongside an empty one are still reported.
137+
expect(guardrailPathMatches(["", "src/scoring/x.ts", ""], ["src/scoring/**"])).toEqual([
138+
{ path: "src/scoring/x.ts", glob: "src/scoring/**" },
139+
]);
140+
141+
// ...and it holds for ordinary (safe) globs too, so this is a property of the path, not of the glob.
142+
expect(guardrailPathMatches([""], ["**"])).toEqual([]);
143+
});
144+
126145
it("SECURITY (ReDoS): a glob AT the safe cap (2 wildcards) still compiles and matches NORMALLY (not the fail-safe path)", () => {
127146
// Exactly 2 stars: at the cap, still safely compiled/evaluated — proves the cap is inclusive, not exclusive,
128147
// and that ordinary (non-pathological) multi-wildcard globs keep their real matching semantics. This is also

test/unit/screenshot-table-gate-engine.test.ts

Lines changed: 144 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {
1717
requiredScreenshotMatrixPairs,
1818
type ScreenshotMatrixPair,
1919
} from "../../packages/loopover-engine/src/review/screenshot-table-gate";
20-
import { matchesAnyWithExclusions } from "../../packages/loopover-engine/src/signals/change-guardrail";
20+
import { guardrailPathMatches as engineGuardrailPathMatches, matchesAnyWithExclusions } from "../../packages/loopover-engine/src/signals/change-guardrail";
2121
import type { ScreenshotTableGateConfig } from "../../packages/loopover-engine/src/types/manifest-deps-types";
2222

2323
function config(overrides: Partial<ScreenshotTableGateConfig> = {}): ScreenshotTableGateConfig {
@@ -341,6 +341,23 @@ describe("extractTableRows", () => {
341341

342342
expect(extractTableRows(body)).toHaveLength(38);
343343
});
344+
345+
it("INVARIANT: a line pair that is not header+separator is skipped, so prose and pipe-bearing text are never rows", () => {
346+
// The scan walks EVERY adjacent line pair, so the reject arm is what stops ordinary PR-description prose
347+
// from being parsed as a table. Three distinct rejection shapes, each of which alone would produce a bogus
348+
// row if the guard were dropped:
349+
expect(extractTableRows("just prose\nmore prose")).toEqual([]); // neither line is pipe-delimited
350+
expect(extractTableRows("| a | b |\nnot a separator\n| 1 | 2 |")).toEqual([]); // header, but no separator under it
351+
expect(extractTableRows("intro text\n| --- | --- |\n| 1 | 2 |")).toEqual([]); // separator with no header above it
352+
353+
// A pipe inside prose (a shell command, a regex alternation) is the realistic false positive this prevents.
354+
expect(extractTableRows("run `cat x | grep y` to check\nand then rerun it")).toEqual([]);
355+
356+
// ...and the reject arm does not consume a REAL table that follows the rejected prose.
357+
expect(extractTableRows("some intro prose\n\n| before | after |\n| --- | --- |\n| a.png | b.png |")).toEqual([
358+
["a.png", "b.png"],
359+
]);
360+
});
344361
});
345362

346363
describe("missingScreenshotMatrixPairs (#4535)", () => {
@@ -748,6 +765,122 @@ describe("evaluateScreenshotTableGate", () => {
748765
expect(result).toEqual({ violated: false, reason: null });
749766
});
750767
});
768+
769+
// Matrix mode's staleness checkpoint is covered above; PRESENCE mode runs the identical
770+
// evidenceFreshnessForHead correlation through a separate return site, and that site had no headSha test at
771+
// all. The gap matters more here than in matrix mode: presence mode is the DEFAULT shape (any image-bearing
772+
// table passes), so a stale-evidence miss means one table pasted on push #1 keeps the gate green for every
773+
// later push — exactly the #stale-screenshot-table-fix regression, silently un-pinned.
774+
describe("presence mode staleness (#stale-screenshot-table-fix)", () => {
775+
it("issues a head-keyed checkpoint on the first satisfying push, so a later push has something to compare against", () => {
776+
const push1 = evaluateScreenshotTableGate({
777+
config: config({ enabled: true }),
778+
prBody: TABLE_BODY,
779+
prLabels: [],
780+
changedFiles: ["apps/ui/src/App.tsx"],
781+
headSha: "presence-push-1",
782+
});
783+
expect(push1.violated).toBe(false);
784+
expect(push1.presenceModeSatisfiedState?.headSha).toBe("presence-push-1");
785+
expect(push1.presenceModeSatisfiedState?.evidenceFingerprint).toEqual(expect.any(String));
786+
});
787+
788+
it("REGRESSION: the SAME table on a new head is stale — it must not keep passing the gate across pushes", () => {
789+
const push1 = evaluateScreenshotTableGate({
790+
config: config({ enabled: true }),
791+
prBody: TABLE_BODY,
792+
prLabels: [],
793+
changedFiles: ["apps/ui/src/App.tsx"],
794+
headSha: "presence-push-1",
795+
});
796+
const staleOnPush2 = evaluateScreenshotTableGate({
797+
config: config({ enabled: true }),
798+
prBody: TABLE_BODY, // byte-identical body: no re-affirmation
799+
prLabels: [],
800+
changedFiles: ["apps/ui/src/App.tsx"],
801+
headSha: "presence-push-2",
802+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
803+
});
804+
expect(staleOnPush2.violated).toBe(true);
805+
expect(staleOnPush2.reason).toContain(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE);
806+
// No checkpoint is issued on a stale result — re-issuing one would let push #3 compare against push #2
807+
// and read as "fresh", laundering the staleness away after a single extra push.
808+
expect(staleOnPush2.presenceModeSatisfiedState).toBeUndefined();
809+
});
810+
811+
it("a re-affirmed table (fresh image URLs) on the new head is NOT stale, and re-checkpoints to that head", () => {
812+
const push1 = evaluateScreenshotTableGate({
813+
config: config({ enabled: true }),
814+
prBody: TABLE_BODY,
815+
prLabels: [],
816+
changedFiles: ["apps/ui/src/App.tsx"],
817+
headSha: "presence-push-1",
818+
});
819+
const push2 = evaluateScreenshotTableGate({
820+
config: config({ enabled: true }),
821+
prBody: TABLE_BODY.replaceAll(".png", "-v2.png"), // a fresh upload gets a fresh URL
822+
prLabels: [],
823+
changedFiles: ["apps/ui/src/App.tsx"],
824+
headSha: "presence-push-2",
825+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
826+
});
827+
expect(push2.violated).toBe(false);
828+
expect(push2.presenceModeSatisfiedState?.headSha).toBe("presence-push-2");
829+
});
830+
831+
it("a custom config.message wins over the default contract text on the stale path too", () => {
832+
const push1 = evaluateScreenshotTableGate({
833+
config: config({ enabled: true }),
834+
prBody: TABLE_BODY,
835+
prLabels: [],
836+
changedFiles: [],
837+
headSha: "presence-push-1",
838+
});
839+
const stale = evaluateScreenshotTableGate({
840+
config: config({ enabled: true, message: "custom contract text" }),
841+
prBody: TABLE_BODY,
842+
prLabels: [],
843+
changedFiles: [],
844+
headSha: "presence-push-2",
845+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
846+
});
847+
expect(stale).toEqual({ violated: true, reason: "custom contract text" });
848+
});
849+
850+
it("INVARIANT: no headSha ⇒ byte-identical to pre-fix behavior — no checkpoint issued, no staleness possible", () => {
851+
// The caller may have no persistence wired up. Degrading to the old always-pass shape is deliberate:
852+
// without a head SHA there is nothing to correlate, and inventing a violation would fail closed on a
853+
// deployment that never opted in.
854+
const result = evaluateScreenshotTableGate({
855+
config: config({ enabled: true }),
856+
prBody: TABLE_BODY,
857+
prLabels: [],
858+
changedFiles: [],
859+
presenceModeSatisfied: { headSha: "old", evidenceFingerprint: "anything" },
860+
});
861+
expect(result).toEqual({ violated: false, reason: null });
862+
});
863+
864+
it("INVARIANT: the SAME head re-evaluated is never stale — a webhook replay must not flip a passing gate", () => {
865+
const push1 = evaluateScreenshotTableGate({
866+
config: config({ enabled: true }),
867+
prBody: TABLE_BODY,
868+
prLabels: [],
869+
changedFiles: [],
870+
headSha: "presence-push-1",
871+
});
872+
const replay = evaluateScreenshotTableGate({
873+
config: config({ enabled: true }),
874+
prBody: TABLE_BODY,
875+
prLabels: [],
876+
changedFiles: [],
877+
headSha: "presence-push-1",
878+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
879+
});
880+
expect(replay.violated).toBe(false);
881+
expect(replay.presenceModeSatisfiedState?.headSha).toBe("presence-push-1");
882+
});
883+
});
751884
});
752885

753886
describe("extractTableRowImageUrls (#4366)", () => {
@@ -820,3 +953,13 @@ describe("matchesAnyWithExclusions via the engine path (#9434)", () => {
820953
expect(result).toBe(true);
821954
});
822955
});
956+
957+
// Mirrored from the app suite so BOTH import identities own this branch (CI shards + merges by flag).
958+
describe("guardrailPathMatches empty-path skip via the engine path", () => {
959+
it("INVARIANT: an empty changed path is dropped rather than rendered as a blank filename", () => {
960+
expect(engineGuardrailPathMatches([""], ["**"])).toEqual([]);
961+
expect(engineGuardrailPathMatches(["", "src/scoring/x.ts"], ["src/scoring/**"])).toEqual([
962+
{ path: "src/scoring/x.ts", glob: "src/scoring/**" },
963+
]);
964+
});
965+
});

test/unit/screenshot-table-gate.test.ts

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -822,3 +822,84 @@ describe("whenPaths exclusions via the src re-export (#9434)", () => {
822822
expect(hasCommittedImageFile(["docs/logo.png"], ["apps/ui/**"])).toBe(false);
823823
});
824824
});
825+
826+
// Mirrored from the engine suite so BOTH import identities own these branches: CI shards the coverage run and
827+
// merges by flag, so a branch proven only through one identity can read as uncovered on the other.
828+
describe("extractTableRows rejects non-table line pairs", () => {
829+
it("INVARIANT: a line pair that is not header+separator is skipped, so prose and pipe-bearing text are never rows", () => {
830+
expect(extractTableRows("just prose\nmore prose")).toEqual([]); // neither line is pipe-delimited
831+
expect(extractTableRows("| a | b |\nnot a separator\n| 1 | 2 |")).toEqual([]); // header, but no separator under it
832+
expect(extractTableRows("intro text\n| --- | --- |\n| 1 | 2 |")).toEqual([]); // separator with no header above it
833+
expect(extractTableRows("run `cat x | grep y` to check\nand then rerun it")).toEqual([]); // a pipe inside prose
834+
expect(extractTableRows("some intro prose\n\n| before | after |\n| --- | --- |\n| a.png | b.png |")).toEqual([
835+
["a.png", "b.png"],
836+
]);
837+
});
838+
});
839+
840+
// Presence mode is the DEFAULT gate shape (any image-bearing table passes), and its staleness correlation ran
841+
// through a separate return site from matrix mode's — one that had no headSha test at all. Untested, a stale
842+
// -evidence miss means one table pasted on push #1 holds the gate green for every later push.
843+
describe("evaluateScreenshotTableGate presence-mode staleness (#stale-screenshot-table-fix)", () => {
844+
const satisfying = { config: config({ enabled: true }), prBody: TABLE_BODY, prLabels: [] as string[], changedFiles: ["apps/ui/src/App.tsx"] };
845+
846+
it("issues a head-keyed checkpoint on the first satisfying push", () => {
847+
const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" });
848+
expect(push1.violated).toBe(false);
849+
expect(push1.presenceModeSatisfiedState?.headSha).toBe("presence-push-1");
850+
expect(push1.presenceModeSatisfiedState?.evidenceFingerprint).toEqual(expect.any(String));
851+
});
852+
853+
it("REGRESSION: the SAME table on a new head is stale — it must not keep passing the gate across pushes", () => {
854+
const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" });
855+
const stale = evaluateScreenshotTableGate({
856+
...satisfying, // byte-identical body: no re-affirmation
857+
headSha: "presence-push-2",
858+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
859+
});
860+
expect(stale.violated).toBe(true);
861+
expect(stale.reason).toContain(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE);
862+
// No checkpoint on a stale result — re-issuing one would let push #3 compare against push #2 and read as
863+
// fresh, laundering the staleness away after a single extra push.
864+
expect(stale.presenceModeSatisfiedState).toBeUndefined();
865+
});
866+
867+
it("a re-affirmed table (fresh image URLs) on the new head is NOT stale, and re-checkpoints to that head", () => {
868+
const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" });
869+
const push2 = evaluateScreenshotTableGate({
870+
...satisfying,
871+
prBody: TABLE_BODY.replaceAll(".png", "-v2.png"), // a fresh upload gets a fresh URL
872+
headSha: "presence-push-2",
873+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
874+
});
875+
expect(push2.violated).toBe(false);
876+
expect(push2.presenceModeSatisfiedState?.headSha).toBe("presence-push-2");
877+
});
878+
879+
it("a custom config.message wins over the default contract text on the stale path too", () => {
880+
const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" });
881+
const stale = evaluateScreenshotTableGate({
882+
...satisfying,
883+
config: config({ enabled: true, message: "custom contract text" }),
884+
headSha: "presence-push-2",
885+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
886+
});
887+
expect(stale).toEqual({ violated: true, reason: "custom contract text" });
888+
});
889+
890+
it("INVARIANT: no headSha ⇒ byte-identical to pre-fix behavior — no checkpoint issued, no staleness possible", () => {
891+
const result = evaluateScreenshotTableGate({ ...satisfying, presenceModeSatisfied: { headSha: "old", evidenceFingerprint: "anything" } });
892+
expect(result).toEqual({ violated: false, reason: null });
893+
});
894+
895+
it("INVARIANT: the SAME head re-evaluated is never stale — a webhook replay must not flip a passing gate", () => {
896+
const push1 = evaluateScreenshotTableGate({ ...satisfying, headSha: "presence-push-1" });
897+
const replay = evaluateScreenshotTableGate({
898+
...satisfying,
899+
headSha: "presence-push-1",
900+
presenceModeSatisfied: push1.presenceModeSatisfiedState,
901+
});
902+
expect(replay.violated).toBe(false);
903+
expect(replay.presenceModeSatisfiedState?.headSha).toBe("presence-push-1");
904+
});
905+
});

0 commit comments

Comments
 (0)