diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 53ae77a5c9..3b375ed77e 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2588,6 +2588,77 @@ export async function countRecentAuditEventsForActorAndTarget(env: Env, actor: s return row.count; } +/** Shared by every `targetKey` literal-prefix `LIKE` scan below ({@link countRecentAuditEventsForActorInRepo}, + * {@link findHottestReviewTargetForRepo}) so a repo name containing a SQL `LIKE` wildcard (`%`/`_`) is always + * matched literally, never as a pattern -- e.g. `owner/foo_bar` must never spuriously match `owner/fooXbar#...`'s + * targets. */ +function escapeSqlLikePattern(value: string): string { + return value.replace(/[\\%_]/g, "\\$&"); +} + +/** + * Repo-scoped sibling of {@link countRecentAuditEventsForActorAndTarget} (#review-nag-cross-pr-carryover): counts + * one actor's matching events across EVERY target within `repoFullName` (their current PR/issue plus every other + * one they've touched), not just the single `targetKey` the caller happens to be evaluating. The per-target count + * lets a contributor who exhausts a cooldown on PR A reset to a clean slate simply by opening a fresh PR B (a new + * `issue.number` is a new `targetKey`) -- this is the fix: the running count now follows the ACTOR through the + * repo, mirroring how the contributor blacklist and moderation-rules ban tally already persist by login rather + * than by thread. Reuses the same literal-prefix `LIKE ... ESCAPE` scoping as {@link findHottestReviewTargetForRepo} + * so `owner/foo_bar` can never spuriously match `owner/fooXbar#...`'s targets. + */ +export async function countRecentAuditEventsForActorInRepo(env: Env, actor: string, eventType: string, repoFullName: string, sinceIso: string): Promise { + const db = getDb(env.DB); + const targetPrefixPattern = `${escapeSqlLikePattern(repoFullName)}#%`; + const [row] = await db + .select({ count: sql`count(*)` }) + .from(auditEvents) + .where( + and( + eq(auditEvents.actor, actor), + eq(auditEvents.eventType, eventType), + sql`${auditEvents.targetKey} LIKE ${targetPrefixPattern} ESCAPE '\\'`, + gte(auditEvents.createdAt, sinceIso), + ), + ); + /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */ + if (!row) return 0; + return row.count; +} + +/** + * Variant of {@link countRecentAuditEventsForActorInRepo} for a `targetKey` shape that carries a THIRD segment + * after `repo#issueNumber` (e.g. maybeThrottleMonitoredMentions's `owner/repo#123#mention:someLogin`): scopes + * across every PR/issue NUMBER in the repo (the same repo-wide carryover fix) while still pinning to one EXACT + * `targetKeySuffix`, so independently-budgeted sub-targets (one per monitored login) never bleed into each + * other's count. Pass the suffix literally, e.g. `mention:someLogin` -- both `repoFullName` and `targetKeySuffix` + * are escaped before embedding, so neither can smuggle in a stray SQL `LIKE` wildcard. + */ +export async function countRecentAuditEventsForActorInRepoWithTargetSuffix( + env: Env, + actor: string, + eventType: string, + repoFullName: string, + targetKeySuffix: string, + sinceIso: string, +): Promise { + const db = getDb(env.DB); + const targetPattern = `${escapeSqlLikePattern(repoFullName)}#%#${escapeSqlLikePattern(targetKeySuffix)}`; + const [row] = await db + .select({ count: sql`count(*)` }) + .from(auditEvents) + .where( + and( + eq(auditEvents.actor, actor), + eq(auditEvents.eventType, eventType), + sql`${auditEvents.targetKey} LIKE ${targetPattern} ESCAPE '\\'`, + gte(auditEvents.createdAt, sinceIso), + ), + ); + /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */ + if (!row) return 0; + return row.count; +} + /** #orb-ci-stuck-repeat / #orb-retry-storm ops-alerts signal: the single PR within `repoFullName` that published * the most review surfaces in the last `sinceIso`-bounded window, and how many. `github_app.pr_public_surface_ * published` is a genuine INSERT-only event (never upserted) recorded once per successful publish pass @@ -2596,10 +2667,6 @@ export async function countRecentAuditEventsForActorAndTarget(env: Env, actor: s * overwrites rather than accumulates), this correctly counts repeat publishes even when the head SHA never * changes -- exactly the shape of a stuck-CI or sweep retry-storm bleed. Returns null when the repo published * no surfaces in the window at all. */ -function escapeSqlLikePattern(value: string): string { - return value.replace(/[\\%_]/g, "\\$&"); -} - export async function findHottestReviewTargetForRepo( env: Env, repoFullName: string, diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 22306fa55e..515cf42594 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -57,6 +57,8 @@ import { recordAgentCommandFeedback, recordAuditEvent, countRecentAuditEventsForActorAndTarget, + countRecentAuditEventsForActorInRepo, + countRecentAuditEventsForActorInRepoWithTargetSuffix, hasAuditEventForDelivery, recordGateBlockOutcome, getGateBlockOutcome, @@ -12236,10 +12238,18 @@ const REVIEW_NAG_PING_EVENT_TYPE = "github_app.review_nag_ping"; /** * Review-request nagging cooldown (#2463, anti-abuse): throttle a thread's OWN author repeatedly pinging - * @gittensory for review on the SAME PR/issue. Runs BEFORE maybeProcessGittensoryMentionCommand below so a - * throttled ping short-circuits ahead of the normal answer-card dispatch — under the threshold this just - * records the ping (via the shared audit-events ledger, scoped by targetKey so the count never mixes threads) - * and falls through unchanged; only crossing the threshold applies the repo's configured policy. + * @gittensory for review. Runs BEFORE maybeProcessGittensoryMentionCommand below so a throttled ping + * short-circuits ahead of the normal answer-card dispatch — under the threshold this just records the ping + * (still tagged with the THIS thread's own targetKey, so a per-thread audit trail is preserved) and falls + * through unchanged; only crossing the threshold applies the repo's configured policy. + * + * The running count is scoped to the ACTOR across the WHOLE repo (#review-nag-cross-pr-carryover), not to one + * `targetKey` — a contributor who exhausts their pings on PR A and opens a fresh PR B carries the count over + * instead of resetting to a clean 0/maxPings slate, mirroring how the contributor blacklist and moderation-rules + * ban tally already persist by login rather than by thread. This also makes enforcement immediate rather than + * merely cumulative: because the count already reflects every prior target, the very FIRST ping on PR B can + * already cross `maxPings` on its own — there's no need for a separate "still on cooldown" table, since the + * audit-events ledger read at the new repo scope already IS that persistent per-actor state. * * Deliberately scoped to the THREAD'S OWN author (`issue.user.login === commenter`): a third party pinging on * someone else's PR/issue must never throttle or close the AUTHOR's unrelated work — this mirrors the standing @@ -12289,7 +12299,10 @@ async function maybeThrottleReviewNagPing( /* 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 = 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); + // Repo-wide, not per-target (#review-nag-cross-pr-carryover): counts every @gittensory ping this actor has + // sent anywhere in this repo within the window, so exhausting the budget on PR A already shows up on PR B's + // very first ping instead of restarting at 0/maxPings just because the targetKey (issue.number) is new. + const priorPings = await countRecentAuditEventsForActorInRepo(env, commenter, REVIEW_NAG_PING_EVENT_TYPE, repoFullName, sinceIso); const pingCount = priorPings + 1; // this ping counts too // Always record the ping first so the running count reflects reality even when the rest of this handler @@ -12424,6 +12437,14 @@ function bodyMentionsLogin(body: string, login: string): boolean { * don't share one budget. Runs regardless of whether the comment also contains an `@gittensory` mention/command * — mentioning a maintainer is never a bot command, so this must not gate or interact with command dispatch. * Off (`reviewNagMonitoredMentions` empty/absent, the default) is a complete no-op — no extra reads at all. + * + * Like {@link maybeThrottleReviewNagPing}, the running count is scoped to the ACTOR across the WHOLE repo + * (#review-nag-cross-pr-carryover) rather than to one `targetKey`, so exhausting the budget mentioning @maintainer + * on PR A carries over to PR B instead of resetting. Because a mentioned login's own budget must stay independent + * of every OTHER monitored login's budget (the "don't share one budget" design above), the repo-wide count is + * additionally pinned to this one login's `mention:` targetKey suffix via + * {@link countRecentAuditEventsForActorInRepoWithTargetSuffix} — carryover happens across PRs, never across + * different mentioned logins. */ async function maybeThrottleMonitoredMentions( env: Env, @@ -12457,13 +12478,25 @@ async function maybeThrottleMonitoredMentions( const mentionedLogin = monitoredLogins.find((login) => bodyMentionsLogin(body, login)); if (!mentionedLogin) return false; - const targetKey = `${repoFullName}#${issue.number}#mention:${mentionedLogin.toLowerCase()}`; + // The per-login suffix is shared between the full targetKey (below, for the recordAuditEvent audit trail) and + // the repo-wide count's suffix filter, so a naming drift between the two can never silently under/over-count. + const mentionTargetSuffix = `mention:${mentionedLogin.toLowerCase()}`; + const targetKey = `${repoFullName}#${issue.number}#${mentionTargetSuffix}`; /* 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 = 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, MONITORED_MENTION_PING_EVENT_TYPE, targetKey, sinceIso); + // Repo-wide, not per-target (#review-nag-cross-pr-carryover), but still pinned to THIS mentioned login's own + // suffix so independently-budgeted mentioned logins never bleed into each other's count. + const priorPings = await countRecentAuditEventsForActorInRepoWithTargetSuffix( + env, + commenter, + MONITORED_MENTION_PING_EVENT_TYPE, + repoFullName, + mentionTargetSuffix, + sinceIso, + ); const pingCount = priorPings + 1; await recordAuditEvent(env, { diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index 9b555abef3..29299d62a3 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -4,6 +4,8 @@ import { countRecentDeadLetters, countRecentDeadLettersByType, countRecentAuditEventsForActorAndTarget, + countRecentAuditEventsForActorInRepo, + countRecentAuditEventsForActorInRepoWithTargetSuffix, findHottestReviewTargetForRepo, hasAuditEventForDelivery, getLatestScorePreview, @@ -581,6 +583,67 @@ describe("database row parser hardening", () => { expect(await countRecentAuditEventsForActorAndTarget(env, "chatty", "github_app.review_nag_ping", "owner/repo#1", "2026-06-24T13:00:00.000Z")).toBe(0); // none after the cutoff → count(*) returns 0 }); + it("countRecentAuditEventsForActorInRepo counts one actor's events across EVERY target within a repo, not just one targetKey (#review-nag-cross-pr-carryover)", async () => { + const env = createTestEnv(); + // Same actor, TWO different targets (PR #1 and PR #2) within the same repo -- both must count toward the total. + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T10:00:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T10:05:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/repo#2", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + // A different actor in the SAME repo must not be counted (the actor filter). + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "someone-else", targetKey: "owner/repo#3", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + // The SAME actor pinging a DIFFERENT repo must not be counted (the repo-prefix scope). + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/other-repo#1", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + // An unrelated event type on the same actor+repo must not be counted (the eventType filter). + await recordAuditEvent(env, { eventType: "github_app.agent_command_replied", actor: "chatty", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + + expect(await countRecentAuditEventsForActorInRepo(env, "chatty", "github_app.review_nag_ping", "owner/repo", "2026-06-24T09:00:00.000Z")).toBe(3); + expect(await countRecentAuditEventsForActorInRepo(env, "chatty", "github_app.review_nag_ping", "owner/repo", "2026-06-24T11:00:00.000Z")).toBe(1); // only the 12:00 owner/repo#2 ping + expect(await countRecentAuditEventsForActorInRepo(env, "chatty", "github_app.review_nag_ping", "owner/repo", "2026-06-24T13:00:00.000Z")).toBe(0); // none after the cutoff → count(*) returns 0 + }); + + it("countRecentAuditEventsForActorInRepo treats repo names as literal LIKE prefixes (regression mirroring findHottestReviewTargetForRepo's #review-burst-scope-pollution fix)", async () => { + const env = createTestEnv(); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/foo_bar#1", outcome: "completed", createdAt: "2026-06-24T10:00:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/foo_bar#2", outcome: "completed", createdAt: "2026-06-24T10:05:00.000Z" }); + // owner/fooXbar is a DIFFERENT repo that would spuriously match "owner/foo_bar#%" if `_` were left as a SQL + // wildcard instead of being escaped -- must not leak into owner/foo_bar's count. + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/fooXbar#99", outcome: "completed", createdAt: "2026-06-24T10:00:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/fooXbar#99", outcome: "completed", createdAt: "2026-06-24T10:05:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/fooXbar#99", outcome: "completed", createdAt: "2026-06-24T10:10:00.000Z" }); + + expect(await countRecentAuditEventsForActorInRepo(env, "chatty", "github_app.review_nag_ping", "owner/foo_bar", "2026-06-24T09:00:00.000Z")).toBe(2); + }); + + it("countRecentAuditEventsForActorInRepoWithTargetSuffix counts across every PR/issue number in a repo while still pinning to ONE exact targetKey suffix (#review-nag-cross-pr-carryover)", async () => { + const env = createTestEnv(); + // Two different PR numbers within the SAME repo, both suffixed "#mention:jsonbored" -- both count. + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/repo#1#mention:jsonbored", outcome: "completed", createdAt: "2026-06-24T10:00:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/repo#2#mention:jsonbored", outcome: "completed", createdAt: "2026-06-24T10:05:00.000Z" }); + // A DIFFERENT mentioned login's suffix on the SAME repo+actor must NOT bleed into the "jsonbored" count -- + // this is the independent-budget guarantee the plain repo-prefix countRecentAuditEventsForActorInRepo can't + // provide on its own. + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/repo#3#mention:other-maintainer", outcome: "completed", createdAt: "2026-06-24T10:07:00.000Z" }); + // A different actor with the SAME suffix must not be counted (the actor filter). + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "someone-else", targetKey: "owner/repo#4#mention:jsonbored", outcome: "completed", createdAt: "2026-06-24T10:08:00.000Z" }); + // A different repo with the SAME suffix must not be counted (the repo-prefix scope). + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/other-repo#1#mention:jsonbored", outcome: "completed", createdAt: "2026-06-24T10:09:00.000Z" }); + + expect(await countRecentAuditEventsForActorInRepoWithTargetSuffix(env, "chatty", "github_app.monitored_mention_ping", "owner/repo", "mention:jsonbored", "2026-06-24T09:00:00.000Z")).toBe(2); + expect(await countRecentAuditEventsForActorInRepoWithTargetSuffix(env, "chatty", "github_app.monitored_mention_ping", "owner/repo", "mention:other-maintainer", "2026-06-24T09:00:00.000Z")).toBe(1); + expect(await countRecentAuditEventsForActorInRepoWithTargetSuffix(env, "chatty", "github_app.monitored_mention_ping", "owner/repo", "mention:jsonbored", "2026-06-24T10:06:00.000Z")).toBe(0); // cutoff after both matching pings + }); + + it("countRecentAuditEventsForActorInRepoWithTargetSuffix escapes BOTH the repo prefix and the suffix before embedding them in the LIKE pattern (regression mirroring countRecentAuditEventsForActorInRepo's escaping fix)", async () => { + const env = createTestEnv(); + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/foo_bar#1#mention:some_login", outcome: "completed", createdAt: "2026-06-24T10:00:00.000Z" }); + // Neither a repo-name collision (fooXbar vs foo_bar) NOR a suffix collision (someXlogin vs some_login) may + // leak in if `_` were left as an unescaped SQL wildcard in either segment. + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/fooXbar#2#mention:some_login", outcome: "completed", createdAt: "2026-06-24T10:01:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "owner/foo_bar#3#mention:someXlogin", outcome: "completed", createdAt: "2026-06-24T10:02:00.000Z" }); + + expect(await countRecentAuditEventsForActorInRepoWithTargetSuffix(env, "chatty", "github_app.monitored_mention_ping", "owner/foo_bar", "mention:some_login", "2026-06-24T09:00:00.000Z")).toBe(1); + }); + it("findHottestReviewTargetForRepo returns the PR with the most published surfaces in the window, scoped to ONE repo (#orb-ci-stuck-repeat)", async () => { const env = createTestEnv(); const publish = (targetKey: string, createdAt: string) => diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9ab01ebac8..53ac8c4874 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -20224,6 +20224,47 @@ describe("queue processors", () => { expect(closeAudit?.n).toBeGreaterThanOrEqual(1); }); + it("REGRESSION (#review-nag-cross-pr-carryover): a contributor who exhausted their pings on PR A carries the count over to a BRAND-NEW PR B instead of resetting to a clean 0/maxPings slate", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3, autonomy: { close: "auto", label: "auto" } }); + // PR A: "chatty" already sent 3 pings (the full budget) and PR A was closed for it -- this is the exact + // state left behind by the "close policy on a PR thread" scenario above. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 220, title: "PR A (already closed)", state: "closed", user: { login: "chatty" }, head: { sha: "sha220" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#220", outcome: "completed" }); + } + // PR B: a BRAND-NEW PR from the SAME contributor -- a new issue.number means a new targetKey the old + // per-target count would treat as a clean slate. Only ONE ping is sent here. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 221, title: "PR B (brand new)", state: "open", user: { login: "chatty" }, head: { sha: "sha221" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(221, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-carryover-pr-b", + 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: 221, title: "PR B (brand new)", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + // Under the OLD per-targetKey count, this is ping 1/3 on PR B alone -- under threshold, no action. The + // FIX counts every prior ping across the whole repo, so this single PR-B ping is already #4 overall + // (3 carried over from PR A + this one), crossing maxPings=3 on the very first PR-B ping. + expect(seen.closed).toBe(true); + expect(seen.comments.some((c) => c.includes("chatty") && c.includes("4 times"))).toBe(true); + const prA = await env.DB.prepare("select state from pull_requests where number = 220").first<{ state: string }>(); + expect(prA?.state).toBe("closed"); // PR A is untouched by this second evaluation + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); + expect(closeAudit?.n).toBeGreaterThanOrEqual(1); + }); + it("close policy degrades to hold on an ISSUE thread (no closeIssue primitive yet)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); @@ -20723,6 +20764,86 @@ describe("queue processors", () => { // #label-scoping: close: "auto" alone (no broad label: "auto") is sufficient for the label AND the close. }); + it("REGRESSION (#review-nag-cross-pr-carryover): a contributor who exhausted their @-mention pings for ONE login on PR A carries that login's count over to a BRAND-NEW PR B", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3, reviewNagMonitoredMentions: ["JSONbored"], autonomy: { close: "auto" } }); + // PR A: "chatty" already sent 3 pings mentioning @JSONbored (the full budget) and PR A was closed for it. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 320, title: "PR A (already closed)", state: "closed", user: { login: "chatty" }, head: { sha: "sha320" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "JSONbored/gittensory#320#mention:jsonbored", outcome: "completed" }); + } + // PR B: a BRAND-NEW PR from the SAME contributor mentioning the SAME login. A new issue.number is a new + // targetKey the old per-target count would treat as a clean slate. Only ONE mention is sent here. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 321, title: "PR B (brand new)", state: "open", user: { login: "chatty" }, head: { sha: "sha321" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubMonitoredMentionFetch(321, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "mention-carryover-pr-b", + 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: 321, title: "PR B (brand new)", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@JSONbored please look at this", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + // Under the OLD per-targetKey count, this is mention-ping 1/3 on PR B alone -- under threshold, no action. + // The FIX counts every prior @JSONbored mention-ping across the whole repo, so this single PR-B ping is + // already #4 overall (3 carried over from PR A + this one), crossing maxPings=3 on the very first ping. + expect(seen.closed).toBe(true); + const prA = await env.DB.prepare("select state from pull_requests where number = 320").first<{ state: string }>(); + expect(prA?.state).toBe("closed"); // PR A is untouched by this second evaluation + }); + + it("REGRESSION (#review-nag-cross-pr-carryover): a DIFFERENT monitored login mentioned on PR B keeps its own independent budget, unaffected by another login's exhausted count", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + reviewNagPolicy: "close", + reviewNagMaxPings: 3, + reviewNagMonitoredMentions: ["JSONbored", "other-maintainer"], + autonomy: { close: "auto" }, + }); + // PR A: "chatty" already exhausted the @JSONbored budget (3 pings) -- same seed as the carryover test above. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 322, title: "PR A (JSONbored exhausted)", state: "closed", user: { login: "chatty" }, head: { sha: "sha322" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.monitored_mention_ping", actor: "chatty", targetKey: "JSONbored/gittensory#322#mention:jsonbored", outcome: "completed" }); + } + // PR B: the SAME contributor mentions a DIFFERENT monitored login ("other-maintainer") for the FIRST time. + // If the repo-wide carryover fix accidentally merged every mentioned login into one shared count, this + // single ping would incorrectly already be "#4" and get throttled -- it must instead be a fresh 1/3. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 323, title: "PR B (different login)", state: "open", user: { login: "chatty" }, head: { sha: "sha323" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubMonitoredMentionFetch(323, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "mention-independent-login-pr-b", + 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: 323, title: "PR B (different login)", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@other-maintainer could you take a look?", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); // "other-maintainer"'s own budget is untouched by @JSONbored's exhausted count + const mentionPings = await env.DB.prepare( + "select count(*) as n from audit_events where event_type = 'github_app.monitored_mention_ping' and target_key = 'JSONbored/gittensory#323#mention:other-maintainer'", + ).first<{ n: number }>(); + expect(mentionPings?.n).toBe(1); // recorded as ping 1/3 for THIS login, not folded into @JSONbored's tally + }); + it("does NOT throttle the repo owner, an admin login, an automation bot, or an exempt login", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), ADMIN_GITHUB_LOGINS: "fleet-admin" }); await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 1, reviewNagMonitoredMentions: ["JSONbored"], autoCloseExemptLogins: ["trusted-regular"] });