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/); });