From 9f2cb516ab3aed65013a963b495b560852c6d7f1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 09:35:30 -0700 Subject: [PATCH] fix(github): exclude the panel's per-pass timestamp from the comment idempotency check (#9069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR panel embeds `Review updated: `, re-stamped from `reviewedAt ?? new Date()` on every render. createOrUpdateIssueCommentWithMarker compared raw bodies, so the panel could never match its own posted body and the idempotency skip was unreachable for it: every re-gate tick PATCHed GitHub purely to move a clock, and each PATCH generated an inbound issue_comment.edited delivery that ingress then classified as our own noise and discarded. That loop accounted for 79,612 of ~309,600 lifetime webhook deliveries (26%), plus the matching outbound writes against the REST rate limit. Compare through comparableCommentBody, which normalizes only that generated line. Compare-only: the posted body keeps its real timestamp, and a body whose content genuinely changed still PATCHes with the fresh one. The surviving timestamp then means "the review last changed at X" rather than "we last looked at X". Also makes the #6724 no-op accounting honest — pr_public_surface_published was being recorded on every clock-only pass, inflating the public reviews-completed count and the review-burst anomaly counter. markPullRequestSurfacePublished stays outside that branch, so the surface stamp still advances and the repair sweeps are unaffected. Closes #9069 --- src/github/comments.ts | 33 +++++++++++++- test/unit/github-comments.test.ts | 74 ++++++++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/src/github/comments.ts b/src/github/comments.ts index f12ffd8e6f..885898f6c1 100644 --- a/src/github/comments.ts +++ b/src/github/comments.ts @@ -26,6 +26,30 @@ export function closeExplanationMarker(closeKind: string | undefined): string { // `batch.length < 100` early-exit below still keeps a short comment list to a single request. const COMMENT_SEARCH_PAGE_LIMIT = 10; +// The PR panel embeds a per-pass `Review updated: ` line (review/unified-comment.ts), +// re-stamped from `reviewedAt ?? new Date()` on every render. Comparing raw bodies in the idempotency check +// below therefore NEVER matched for the panel, defeating the skip entirely: every re-gate tick PATCHed GitHub +// (a write + rate-limit cost) purely to move a clock, and each PATCH generated an inbound +// `issue_comment.edited` delivery that ingress then classified as our own noise and discarded -- ~26% of all +// lifetime webhook traffic (79,612 of ~309,600 deliveries) was this loop feeding itself (#9069). +// +// Normalizing the timestamp out of BOTH sides restores the skip. Deliberately COMPARE-ONLY: the body actually +// posted keeps its real timestamp, and whenever some other part of the body does change, the PATCH carries the +// fresh one along with it. The surviving timestamp then reads as "the review last CHANGED at X" rather than +// "we last looked at X" -- the more useful meaning, and the one the line's own wording already implies. +// +// Bounded to `[^<]*` so it can only ever match this exact generated line: every caller-supplied string that +// reaches a comment body is HTML-angle-escaped first (escapePublicHtmlAngles), so contributor text cannot +// forge a `` wrapper here. Comments without the line (close explanations, visual follow-ups) are +// untouched, keeping their comparison byte-exact as before. +const VOLATILE_REVIEW_TIMESTAMP_LINE = /^Review updated: [^<]*<\/sub>$/gm; +const VOLATILE_REVIEW_TIMESTAMP_PLACEHOLDER = "Review updated:"; + +/** Body projection used ONLY for the idempotency equality check -- never for what gets posted. */ +export function comparableCommentBody(body: string): string { + return body.replace(VOLATILE_REVIEW_TIMESTAMP_LINE, VOLATILE_REVIEW_TIMESTAMP_PLACEHOLDER); +} + type IssueComment = { id: number; body?: string | null; @@ -138,11 +162,16 @@ async function createOrUpdateIssueCommentWithMarker( } const canonical = canonicalMarkerComment(existing); if (canonical) { - // Idempotency (#4): skip the PATCH when the rendered body is byte-identical to what's already posted. The + // Idempotency (#4): skip the PATCH when the rendered body matches what's already posted. The // re-gate sweep re-renders the same surface every cycle for an unchanged PR; without this, every cycle PATCHes // GitHub (a write + rate-limit cost) for no visible change. Defense-in-depth alongside the head_sha publish // marker — also collapses a duplicate webhook delivery for the same commit. - if (canonical.body === body) { + // #9069: compared through comparableCommentBody so the panel's per-pass "Review updated" timestamp — which + // changes on every render and made this check unreachable for the panel — no longer counts as a change. + /* v8 ignore next -- `?? ""` is a type-level guard only: the marker filter above requires + * `comment.body?.includes(candidate)`, so any comment that becomes `canonical` provably has a non-empty + * string body. Kept because IssueComment types `body` as `string | null | undefined`. */ + if (comparableCommentBody(canonical.body ?? "") === comparableCommentBody(body)) { await deleteDuplicateMarkerComments(octokit, owner, repo, existing, canonical.id); return { id: canonical.id, ...(canonical.html_url !== undefined ? { html_url: canonical.html_url } : {}), changed: false }; } diff --git a/test/unit/github-comments.test.ts b/test/unit/github-comments.test.ts index d21f29dad1..b9ca33dd21 100644 --- a/test/unit/github-comments.test.ts +++ b/test/unit/github-comments.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { closeExplanationMarker, createOrUpdateCloseExplanationComment, createOrUpdatePrIntelligenceComment, createOrUpdateVisualFollowupComment, PR_INTELLIGENCE_COMMENT_MARKER, VISUAL_FOLLOWUP_COMMENT_MARKER } from "../../src/github/comments"; +import { closeExplanationMarker, comparableCommentBody, createOrUpdateCloseExplanationComment, createOrUpdatePrIntelligenceComment, createOrUpdateVisualFollowupComment, PR_INTELLIGENCE_COMMENT_MARKER, VISUAL_FOLLOWUP_COMMENT_MARKER } from "../../src/github/comments"; import { createTestEnv } from "../helpers/d1"; import { generatePrivateKeyPem } from "../helpers/github-app-key"; @@ -420,6 +420,78 @@ describe("GitHub PR intelligence comments", () => { expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); }); + it("skips the PATCH when only the panel's per-pass 'Review updated' timestamp differs (#9069)", async () => { + const privateKey = await generatePrivateKeyPem(); + const posted = `${PR_INTELLIGENCE_COMMENT_MARKER}\n### result\nReview updated: 2026-07-26 15:15:11 UTC\nunchanged verdict`; + const rerendered = `${PR_INTELLIGENCE_COMMENT_MARKER}\n### result\nReview updated: 2026-07-26 16:41:02 UTC\nunchanged verdict`; + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push(`${init?.method ?? "GET"} ${url}`); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/12/comments") && (init?.method ?? "GET") === "GET") { + return Response.json([{ id: 303, body: posted, html_url: "https://github.com/comment/303", user: { login: "loopover-orb[bot]", type: "Bot" } }]); + } + return new Response("not found", { status: 404 }); + }); + + const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, rerendered); + + // The whole point of #9069: a clock-only delta is NOT a content change, so no GitHub write and no + // self-inflicted issue_comment.edited delivery. changed:false also keeps the #6724 no-op accounting honest. + expect(result).toEqual({ id: 303, html_url: "https://github.com/comment/303", changed: false }); + expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); + }); + + it("still PATCHes when real content changes alongside the timestamp (#9069 does not suppress real updates)", async () => { + const privateKey = await generatePrivateKeyPem(); + const posted = `${PR_INTELLIGENCE_COMMENT_MARKER}\n### result\nReview updated: 2026-07-26 15:15:11 UTC\nCI failing`; + const rerendered = `${PR_INTELLIGENCE_COMMENT_MARKER}\n### result\nReview updated: 2026-07-26 16:41:02 UTC\nCI green`; + const calls: string[] = []; + let patchedBody = ""; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push(`${init?.method ?? "GET"} ${url}`); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/12/comments") && (init?.method ?? "GET") === "GET") { + return Response.json([{ id: 404, body: posted, user: { login: "loopover-orb[bot]", type: "Bot" } }]); + } + if (url.includes("/issues/comments/404") && init?.method === "PATCH") { + patchedBody = (JSON.parse(String(init.body)) as { body: string }).body; + return Response.json({ id: 404 }); + } + return new Response("not found", { status: 404 }); + }); + + const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, rerendered); + + expect(result?.changed).toBe(true); + expect(calls.some((call) => call.startsWith("PATCH "))).toBe(true); + // Compare-only normalization: the body actually posted carries the REAL fresh timestamp, not the placeholder. + expect(patchedBody).toContain("Review updated: 2026-07-26 16:41:02 UTC"); + }); + + it("INVARIANT (#9069): normalization is compare-only and confined to the generated timestamp line", () => { + // Regression guard for the exact loop that produced ~26% of lifetime webhook traffic: two renders of one + // unchanged panel must compare equal, while any real content delta must not. + const at = (stamp: string) => `${PR_INTELLIGENCE_COMMENT_MARKER}\n### result\nReview updated: ${stamp}\nverdict`; + expect(comparableCommentBody(at("2026-07-26 15:15:11 UTC"))).toBe(comparableCommentBody(at("2027-01-01 00:00:00 UTC"))); + expect(comparableCommentBody(at("x"))).not.toBe(comparableCommentBody(`${PR_INTELLIGENCE_COMMENT_MARKER}\n### result\nReview updated: x\nDIFFERENT`)); + + // Bodies without the line (close explanations, visual follow-ups) stay byte-exact — no accidental widening. + const plain = `${PR_INTELLIGENCE_COMMENT_MARKER}\nclose explanation`; + expect(comparableCommentBody(plain)).toBe(plain); + + // Must not swallow a same-named line carrying different surrounding text, and must not match across a + // forged `<` (contributor text is angle-escaped upstream, so `[^<]*` can only ever span the real stamp). + const forged = `${PR_INTELLIGENCE_COMMENT_MARKER}\nReview updated: ainjectedReview updated: b`; + expect(comparableCommentBody(forged)).toBe(forged); // neither line is on its own line → untouched + + // Multi-occurrence safety: the /g regex must normalize every standalone occurrence, not just the first. + const twice = `Review updated: one\nmid\nReview updated: two`; + expect(comparableCommentBody(twice)).toBe("Review updated:\nmid\nReview updated:"); + }); + it("rejects invalid repository names before calling GitHub", async () => { await expect(createOrUpdatePrIntelligenceComment(createTestEnv(), 123, "invalid", 12, "body")).rejects.toThrow(/Invalid repository full name/); });