From f9526e8c65653a6e239510a18d0aaf636bcde297 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:41:23 -0700 Subject: [PATCH 1/3] fix(settings): cap review nag cooldown --- .gittensory.yml.example | 2 +- apps/gittensory-ui/public/openapi.json | 3 ++- src/db/repositories.ts | 10 ++++++++-- src/openapi/schemas.ts | 3 ++- src/queue/processors.ts | 3 ++- src/settings/agent-actions.ts | 2 ++ src/signals/focus-manifest.ts | 6 +++++- test/unit/data-spine.test.ts | 4 ++++ test/unit/focus-manifest.test.ts | 3 +++ test/unit/queue.test.ts | 23 +++++++++++++++++++++++ 10 files changed, 52 insertions(+), 7 deletions(-) diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 85b82fdbf8..e080bf7b12 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -325,7 +325,7 @@ settings: # a dedicated closeIssue primitive lands) with a clear reason. Off by default. # reviewNagPolicy: off # off | hold | close. Default: off. # reviewNagMaxPings: 3 # Positive integer. Pings above this within the cooldown window trigger the policy. Default: 3. - # reviewNagCooldownDays: 5 # Positive integer. Window the ping count is measured over. Default: 5. + # reviewNagCooldownDays: 5 # Positive integer up to 365. Window the ping count is measured over. Default: 5. # reviewNagLabel: review-nag-cooldown # Label applied alongside the hold/close action. Default: review-nag-cooldown. # Shared repo-scoped exemption list (#2463): GitHub logins never throttled/closed by gittensory's diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index 5cdb50d945..70abe84731 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -8618,7 +8618,8 @@ "reviewNagCooldownDays": { "type": "integer", "minimum": 0, - "exclusiveMinimum": true + "exclusiveMinimum": true, + "maximum": 365 }, "reviewNagLabel": { "type": "string" diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 3589785a29..277c4f0fe3 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -57,6 +57,7 @@ import { upstreamSourceSnapshots, webhookEvents, } from "./schema"; +import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; import type { Advisory, AdvisoryFinding, @@ -562,7 +563,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise contributorCapLabel: row.contributorCapLabel, reviewNagPolicy: normalizeReviewNagPolicy(row.reviewNagPolicy), reviewNagMaxPings: normalizePositiveIntWithDefault(row.reviewNagMaxPings, 3), - reviewNagCooldownDays: normalizePositiveIntWithDefault(row.reviewNagCooldownDays, 5), + reviewNagCooldownDays: normalizeReviewNagCooldownDays(row.reviewNagCooldownDays, 5), reviewNagLabel: row.reviewNagLabel, autoCloseExemptLogins: parseAutoCloseExemptLogins(row.autoCloseExemptLoginsJson), requireFreshRebaseWindowMinutes: normalizeOpenItemCap(row.requireFreshRebaseWindowMinutes), @@ -649,7 +650,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial(value, null)); } diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 81f3ea0bd7..9c5cc56c96 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; import { extendZodWithOpenApi } from "@asteasolutions/zod-to-openapi"; extendZodWithOpenApi(z); @@ -647,7 +648,7 @@ export const RepositorySettingsSchema = z contributorCapLabel: z.string().optional(), reviewNagPolicy: z.enum(["off", "hold", "close"]).optional(), reviewNagMaxPings: z.number().int().positive().optional(), - reviewNagCooldownDays: z.number().int().positive().optional(), + reviewNagCooldownDays: z.number().int().positive().max(MAX_REVIEW_NAG_COOLDOWN_DAYS).optional(), reviewNagLabel: z.string().optional(), autoCloseExemptLogins: z.array(z.string()).optional(), accountAgeThresholdDays: z.number().int().positive().nullable().optional(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e3005cae00..6f3a6db895 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -235,6 +235,7 @@ import { aiReviewCacheInputFingerprint } from "../review/ai-review-cache-input"; import { downgradeCloseToHold, downgradeMergeToHold, + MAX_REVIEW_NAG_COOLDOWN_DAYS, isProtectedAutomationAuthor, planAgentMaintenanceActions, type PlannedAgentAction, @@ -8814,7 +8815,7 @@ async function maybeThrottleReviewNagPing( /* v8 ignore next -- resolveRepositorySettings always resolves a concrete positive integer (NOT NULL DEFAULT 3); the undefined side is defensive against the field's optional TS type. */ const maxPings = settings.reviewNagMaxPings ?? 3; /* v8 ignore next -- resolveRepositorySettings always resolves a concrete positive integer (NOT NULL DEFAULT 5); the undefined side is defensive against the field's optional TS type. */ - const cooldownDays = settings.reviewNagCooldownDays ?? 5; + const cooldownDays = Math.min(settings.reviewNagCooldownDays ?? 5, MAX_REVIEW_NAG_COOLDOWN_DAYS); const sinceIso = new Date(Date.now() - cooldownDays * 24 * 60 * 60 * 1000).toISOString(); const priorPings = await countRecentAuditEventsForActorAndTarget(env, commenter, REVIEW_NAG_PING_EVENT_TYPE, targetKey, sinceIso); const pingCount = priorPings + 1; // this ping counts too diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 34a12b9871..7fc5832842 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -31,6 +31,8 @@ export const DEFAULT_CONTRIBUTOR_CAP_LABEL = "over-contributor-limit"; // configurable per-repo via `.gittensory.yml` (`settings.reviewNagLabel`); the planner uses the resolved label // and falls back to this default, mirroring DEFAULT_BLACKLIST_LABEL's shape. export const DEFAULT_REVIEW_NAG_LABEL = "review-nag-cooldown"; +// Keep the review-nag lookback operationally bounded so repo-controlled config cannot overflow Date arithmetic. +export const MAX_REVIEW_NAG_COOLDOWN_DAYS = 365; // A PR that PASSES the gate but touches a hard-guardrail path is NOT ready to auto-merge — it is withheld // for a human (the merge/approve/close dispositions are suppressed below). Labeling it `ready-to-merge` // would be misleading (the label promises an auto-merge that never happens), so a guarded passing PR gets diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 85d49f17b8..811e4754a9 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -4,6 +4,7 @@ import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy } from "../setting import { normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { mergeContributorBlacklists, normalizeContributorBlacklist } from "../settings/contributor-blacklist"; import { normalizeAutoCloseExemptLogins } from "../settings/auto-close-exempt"; +import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; @@ -859,7 +860,10 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) const reviewNagMaxPings = normalizeOptionalPositiveInteger(r.reviewNagMaxPings, "settings.reviewNagMaxPings", warnings); if (reviewNagMaxPings !== null) out.reviewNagMaxPings = reviewNagMaxPings; const reviewNagCooldownDays = normalizeOptionalPositiveInteger(r.reviewNagCooldownDays, "settings.reviewNagCooldownDays", warnings); - if (reviewNagCooldownDays !== null) out.reviewNagCooldownDays = reviewNagCooldownDays; + if (reviewNagCooldownDays !== null && reviewNagCooldownDays <= MAX_REVIEW_NAG_COOLDOWN_DAYS) out.reviewNagCooldownDays = reviewNagCooldownDays; + if (reviewNagCooldownDays !== null && reviewNagCooldownDays > MAX_REVIEW_NAG_COOLDOWN_DAYS) { + warnings.push(`Manifest field "settings.reviewNagCooldownDays" must be at most ${MAX_REVIEW_NAG_COOLDOWN_DAYS}; ignoring it.`); + } const reviewNagLabel = normalizeOptionalString(r.reviewNagLabel, "settings.reviewNagLabel", warnings); if (reviewNagLabel !== null) out.reviewNagLabel = reviewNagLabel; // Shared repo-scoped exemption list (#2463): only set it when at least one VALID login survives diff --git a/test/unit/data-spine.test.ts b/test/unit/data-spine.test.ts index 20c40c9e71..0271e1dbbf 100644 --- a/test/unit/data-spine.test.ts +++ b/test/unit/data-spine.test.ts @@ -366,6 +366,10 @@ describe("data spine repositories", () => { // back to its default rather than being silently coerced. await upsertRepositorySettings(env, { repoFullName: "owner/badnagrepo", reviewNagPolicy: "delete-everything" as never, reviewNagMaxPings: -1, reviewNagCooldownDays: 2.5 as never }); expect(await getRepositorySettings(env, "owner/badnagrepo")).toMatchObject({ reviewNagPolicy: "off", reviewNagMaxPings: 3, reviewNagCooldownDays: 5 }); + await upsertRepositorySettings(env, { repoFullName: "owner/bigwindowrepo", reviewNagMaxPings: 1_000, reviewNagCooldownDays: 1_000_000_000 }); + expect(await getRepositorySettings(env, "owner/bigwindowrepo")).toMatchObject({ reviewNagMaxPings: 1_000, reviewNagCooldownDays: 365 }); + await env.DB.prepare("update repository_settings set review_nag_cooldown_days = ? where repo_full_name = ?").bind(1_000_000_000, "owner/bigwindowrepo").run(); + expect(await getRepositorySettings(env, "owner/bigwindowrepo")).toMatchObject({ reviewNagMaxPings: 1_000, reviewNagCooldownDays: 365 }); expect(updated.slopAiAdvisory).toBe(false); expect(await getRepoSyncState(env, "missing/repo")).toBeNull(); expect(await getPullRequest(env, "owner/repo", 404)).toBeNull(); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 6846e63ada..f522a18631 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1382,6 +1382,9 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(invalid.warnings.some((w) => /settings\.reviewNagPolicy/.test(w))).toBe(true); expect(invalid.warnings.some((w) => /settings\.reviewNagMaxPings/.test(w))).toBe(true); expect(invalid.warnings.some((w) => /settings\.reviewNagCooldownDays/.test(w))).toBe(true); + const tooLarge = parseFocusManifest({ settings: { reviewNagCooldownDays: 366 } }); + expect(tooLarge.settings.reviewNagCooldownDays).toBeUndefined(); + expect(tooLarge.warnings.some((w) => /settings\.reviewNagCooldownDays/.test(w) && /365/.test(w))).toBe(true); }); it("parses + resolves the account-age throttle settings from the settings: block, overlaying the DB (#2561)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index aaf2139318..4ba58a6d75 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11976,6 +11976,29 @@ describe("queue processors", () => { expect(seen.closed).toBe(false); }); + it("caps an oversized review-nag cooldown before Date arithmetic (regression)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3, reviewNagCooldownDays: 1_000_000_000 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 206, title: "Huge cooldown", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(206, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-huge-cooldown", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 206, title: "Huge cooldown", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>(); + expect(pings?.n).toBe(1); + expect(seen.closed).toBe(false); + }); + it("records pings under the configured threshold without acting; the normal @gittensory reply still proceeds", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); From 5528d207c1edc83582e9d1fe4b7e42fbae815e4f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:39:31 -0700 Subject: [PATCH 2/3] fix(review): avoid a UI typecheck regression from a new agent-actions import focus-manifest.ts is part of the UI package's typechecked closure (its transitive deps get walked by apps/gittensory-ui's own tsc run). Importing MAX_REVIEW_NAG_COOLDOWN_DAYS from settings/agent-actions.ts pulled that module's own import of github/commands.ts -> utils/crypto.ts into the UI build for the first time, exposing a pre-existing latent Uint8Array/BufferSource type mismatch in crypto.ts that the UI's tsc had never previously reached. Duplicates the small constant locally in focus-manifest.ts instead of importing it, keeping db/repositories.ts's own import from agent-actions.ts (never part of the UI closure) unaffected. Also rebases the branch's merge commit into a linear rebase onto current main. --- src/signals/focus-manifest.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 811e4754a9..f624460b48 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -4,7 +4,6 @@ import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy } from "../setting import { normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { mergeContributorBlacklists, normalizeContributorBlacklist } from "../settings/contributor-blacklist"; import { normalizeAutoCloseExemptLogins } from "../settings/auto-close-exempt"; -import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions"; import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; @@ -742,6 +741,13 @@ function normalizeOptionalString(value: JsonValue | undefined, field: string, wa return null; } +// Keep the review-nag lookback operationally bounded so repo-controlled config cannot overflow Date +// arithmetic. Duplicated from settings/agent-actions.ts's own MAX_REVIEW_NAG_COOLDOWN_DAYS (same value, +// same rationale) rather than imported: this module is part of the UI package's typechecked closure, and +// agent-actions.ts transitively imports github/commands.ts -> utils/crypto.ts, pulling a heavier +// GitHub-App-specific dependency chain into the UI build for one small constant. +const MAX_REVIEW_NAG_COOLDOWN_DAYS = 365; + /** * Parse the optional `settings:` mapping — a partial repository-settings override. Only recognized * fields are kept; unknown/invalid values are dropped with a warning and never throw. From aa061ba5807f3c98b82006cd4da7fb159320566d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:49:19 -0700 Subject: [PATCH 3/3] 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. --- test/unit/queue.test.ts | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 4ba58a6d75..b1d1673db0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11976,12 +11976,30 @@ describe("queue processors", () => { expect(seen.closed).toBe(false); }); - it("caps an oversized review-nag cooldown before Date arithmetic (regression)", async () => { + 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 () => { + // upsertRepositorySettings/getRepositorySettings both clamp reviewNagCooldownDays on write AND read, so + // seeding an oversized value through the normal repository layer (even via a raw DB update bypassing the + // write-time clamp) can never actually reach maybeThrottleReviewNagPing uncapped -- the read-time clamp in + // getRepositorySettings neutralizes it first. Mock resolveRepositorySettings directly so this test proves + // processors.ts's OWN Math.min(reviewNagCooldownDays, MAX_REVIEW_NAG_COOLDOWN_DAYS) guard, not the DB layer. const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); - await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3, reviewNagCooldownDays: 1_000_000_000 }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3 }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 206, title: "Huge cooldown", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + // Three prior pings, all 400 DAYS ago -- outside the 365-day cap, but well within an uncapped + // "1,000,000,000-day" window. If the guard clamps correctly, these fall outside the window and don't + // count; if the guard were removed, the uncapped window would count all three, crossing maxPings=3. + vi.setSystemTime(new Date("2025-04-24T00:00:00.000Z")); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#206", outcome: "completed" }); + } + vi.setSystemTime(new Date("2026-05-29T00:00:00.000Z")); // ~400 days later + const baseSettings = await repositorySettingsModule.resolveRepositorySettings(env, "JSONbored/gittensory"); + const resolveSettingsSpy = vi + .spyOn(repositorySettingsModule, "resolveRepositorySettings") + .mockResolvedValueOnce({ ...baseSettings, reviewNagCooldownDays: 1_000_000_000 }); const seen = { comments: [] as string[], labels: [] as string[], closed: false }; stubReviewNagFetch(206, seen); + await processJob(env, { type: "github-webhook", deliveryId: "nag-huge-cooldown", @@ -11994,9 +12012,16 @@ describe("queue processors", () => { comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, }, }); - const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>(); - expect(pings?.n).toBe(1); + + // The 400-day-old pings fell outside the CAPPED 365-day window, so this is only the 1st ping this + // window — under maxPings=3, never throttled. An uncapped window would have counted all 3 prior pings + // (pingCount=4 > maxPings=3) and applied the cooldown instead. + 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 }>(); + expect(applied?.n).toBe(0); expect(seen.closed).toBe(false); + expect(seen.comments.some((c) => c.includes("cooldown limit"))).toBe(false); + expect(resolveSettingsSpy).toHaveBeenCalled(); + resolveSettingsSpy.mockRestore(); }); it("records pings under the configured threshold without acting; the normal @gittensory reply still proceeds", async () => {