From 056016d307fdecb270e976088f094f6f9ae3d1e2 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sun, 26 Jul 2026 09:54:20 +0800 Subject: [PATCH] fix(orb): backport the webhook-redelivery guard to two throttle functions maybeThrottleReviewNagPing and maybeThrottleMonitoredMentions never received the hasAuditEventForDelivery redelivery guard that maybeThrottleLoopOverCommand and maybeThrottleIntentRouting carry, so a redelivered issue_comment double-counted a ping toward the throttle (and could re-post a cooldown/close). Adds the same guard to both; a redelivery is now suppressed, counted once. Co-Authored-By: Claude Opus 4.8 --- src/queue/processors.ts | 11 +++++++++++ test/unit/queue-5.test.ts | 28 ++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index f4623c4de7..8f686eaf4e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -13314,6 +13314,12 @@ async function maybeThrottleReviewNagPing( if (isAutoCloseExempt(commenter, settings.autoCloseExemptLogins)) return false; const targetKey = `${repoFullName}#${issue.number}`; + // #8681: webhook-redelivery guard, backported from maybeThrottleLoopOverCommand/maybeThrottleIntentRouting. + // GitHub can redeliver the same issue_comment event; without this, a redelivery re-counts this ping toward + // the threshold (and can re-post the cooldown/close). The original delivery already recorded its ping under + // this deliveryId, so short-circuit the replay entirely. + const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + if (await hasAuditEventForDelivery(env, commenter, REVIEW_NAG_PING_EVENT_TYPE, targetKey, deliveryId, redeliverySinceIso)) return true; /* 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. */ @@ -13503,6 +13509,11 @@ async function maybeThrottleMonitoredMentions( // 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}`; + // #8681: webhook-redelivery guard, backported from maybeThrottleLoopOverCommand/maybeThrottleIntentRouting. + // A redelivered issue_comment event must not re-count this mention toward the threshold; the original + // delivery already recorded it under this deliveryId + per-login targetKey, so short-circuit the replay. + const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + if (await hasAuditEventForDelivery(env, commenter, MONITORED_MENTION_PING_EVENT_TYPE, targetKey, deliveryId, redeliverySinceIso)) return true; /* 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. */ diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 47e29bfe8c..9b0ea4a96c 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -464,6 +464,26 @@ describe("queue processors", () => { expect(closeAudit?.n).toBeGreaterThanOrEqual(1); }); + it("REGRESSION (#8681): a redelivered issue_comment does NOT double-count a review-nag ping", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewNagPolicy: "close", reviewNagMaxPings: 5 } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 231, title: "Redelivery", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(231, seen); + const payload = { + action: "created" as const, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" as const } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 231, title: "Redelivery", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@loopover help", user: { login: "chatty", type: "User" as const }, author_association: "NONE" }, + }; + await processJob(env, { type: "github-webhook", deliveryId: "nag-redelivery-same", eventName: "issue_comment", payload }); + await processJob(env, { type: "github-webhook", deliveryId: "nag-redelivery-same", eventName: "issue_comment", payload }); + // #8681: the webhook-redelivery guard suppresses the replay, so two deliveries of one real ping record once. + 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); + }); + 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 upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewNagPolicy: "close", reviewNagMaxPings: 3 } }); @@ -1110,10 +1130,10 @@ describe("queue processors", () => { await processJob(env, { type: "github-webhook", deliveryId: "mention-redelivery-same", eventName: "issue_comment", payload }); await processJob(env, { type: "github-webhook", deliveryId: "mention-redelivery-same", eventName: "issue_comment", payload }); const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.monitored_mention_ping'").first<{ n: number }>(); - // NOTE: unlike #2560's per-command limiter, review-nag/monitored-mention ping recording does not itself - // dedup by deliveryId -- it always records. This assertion documents CURRENT behavior (2 pings from 2 - // deliveries) rather than asserting an idempotency guarantee this handler does not provide. - expect(pings?.n).toBe(2); + // #8681: monitored-mention recording now carries the same webhook-redelivery guard as #2560's per-command + // limiter -- a redelivered issue_comment (same deliveryId) is suppressed rather than counted a second + // time, so two deliveries of one real mention record exactly ONE ping. + expect(pings?.n).toBe(1); }); });