Skip to content

Commit aa061ba

Browse files
committed
fix(review): make the review-nag cooldown-cap regression test actually exercise the guard
Gate review: the original regression test seeded an oversized reviewNagCooldownDays through upsertRepositorySettings, but both upsertRepositorySettings and getRepositorySettings already clamp that field on write AND read -- so the value read back inside resolveRepositorySettings was never actually oversized by the time maybeThrottleReviewNagPing saw it. The test could pass even with processors.ts's own Math.min(reviewNagCooldownDays, MAX_REVIEW_NAG_COOLDOWN_DAYS) guard removed entirely. Mocks resolveRepositorySettings directly (bypassing the DB/yml clamp layers entirely, not just the write-time one) to actually deliver an oversized value to the function under test, then proves the guard's effect behaviorally: three prior pings 400 days old fall outside the CORRECTLY capped 365-day window (no cooldown applied), whereas an uncapped "1-billion-day" window would count them and trip the threshold. Confirmed by mutation-testing: removing the Math.min throws a real RangeError (Invalid time value) building the Date.
1 parent 5528d20 commit aa061ba

1 file changed

Lines changed: 29 additions & 4 deletions

File tree

test/unit/queue.test.ts

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11976,12 +11976,30 @@ describe("queue processors", () => {
1197611976
expect(seen.closed).toBe(false);
1197711977
});
1197811978

11979-
it("caps an oversized review-nag cooldown before Date arithmetic (regression)", async () => {
11979+
it("REGRESSION (gate-flagged): caps an oversized review-nag cooldown at MAX_REVIEW_NAG_COOLDOWN_DAYS before Date arithmetic, even when the resolved settings object itself carries an oversized value", async () => {
11980+
// upsertRepositorySettings/getRepositorySettings both clamp reviewNagCooldownDays on write AND read, so
11981+
// seeding an oversized value through the normal repository layer (even via a raw DB update bypassing the
11982+
// write-time clamp) can never actually reach maybeThrottleReviewNagPing uncapped -- the read-time clamp in
11983+
// getRepositorySettings neutralizes it first. Mock resolveRepositorySettings directly so this test proves
11984+
// processors.ts's OWN Math.min(reviewNagCooldownDays, MAX_REVIEW_NAG_COOLDOWN_DAYS) guard, not the DB layer.
1198011985
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
11981-
await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3, reviewNagCooldownDays: 1_000_000_000 });
11986+
await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3 });
1198211987
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 206, title: "Huge cooldown", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" });
11988+
// Three prior pings, all 400 DAYS ago -- outside the 365-day cap, but well within an uncapped
11989+
// "1,000,000,000-day" window. If the guard clamps correctly, these fall outside the window and don't
11990+
// count; if the guard were removed, the uncapped window would count all three, crossing maxPings=3.
11991+
vi.setSystemTime(new Date("2025-04-24T00:00:00.000Z"));
11992+
for (let i = 0; i < 3; i += 1) {
11993+
await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#206", outcome: "completed" });
11994+
}
11995+
vi.setSystemTime(new Date("2026-05-29T00:00:00.000Z")); // ~400 days later
11996+
const baseSettings = await repositorySettingsModule.resolveRepositorySettings(env, "JSONbored/gittensory");
11997+
const resolveSettingsSpy = vi
11998+
.spyOn(repositorySettingsModule, "resolveRepositorySettings")
11999+
.mockResolvedValueOnce({ ...baseSettings, reviewNagCooldownDays: 1_000_000_000 });
1198312000
const seen = { comments: [] as string[], labels: [] as string[], closed: false };
1198412001
stubReviewNagFetch(206, seen);
12002+
1198512003
await processJob(env, {
1198612004
type: "github-webhook",
1198712005
deliveryId: "nag-huge-cooldown",
@@ -11994,9 +12012,16 @@ describe("queue processors", () => {
1199412012
comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" },
1199512013
},
1199612014
});
11997-
const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>();
11998-
expect(pings?.n).toBe(1);
12015+
12016+
// The 400-day-old pings fell outside the CAPPED 365-day window, so this is only the 1st ping this
12017+
// window — under maxPings=3, never throttled. An uncapped window would have counted all 3 prior pings
12018+
// (pingCount=4 > maxPings=3) and applied the cooldown instead.
12019+
const applied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ n: number }>();
12020+
expect(applied?.n).toBe(0);
1199912021
expect(seen.closed).toBe(false);
12022+
expect(seen.comments.some((c) => c.includes("cooldown limit"))).toBe(false);
12023+
expect(resolveSettingsSpy).toHaveBeenCalled();
12024+
resolveSettingsSpy.mockRestore();
1200012025
});
1200112026

1200212027
it("records pings under the configured threshold without acting; the normal @gittensory reply still proceeds", async () => {

0 commit comments

Comments
 (0)