fix(review): skip re-publishing a PR surface already current at its head (#1615)

apps/gittensory-ui/src/components/site/proof-of-power-stats.test.tsx

@@ -1,12 +1,14 @@
 import { render, screen, waitFor } from "@testing-library/react";
 import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
-import { describe, expect, it, vi } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
 import type { ReactNode } from "react";
 
 const { apiFetch } = vi.hoisted(() => ({ apiFetch: vi.fn() }));
 vi.mock("@/lib/api/request", () => ({ apiFetch: (...args: unknown[]) => apiFetch(...args) }));
 vi.mock("@/lib/api/origin", () => ({ getApiOrigin: () => "https://api.test" }));
 
+afterEach(() => vi.unstubAllGlobals());
+
 import { ProofOfPowerStats } from "@/components/site/proof-of-power-stats";
 import {
   formatStatsAgo,
@@ -121,8 +123,18 @@ describe("ProofOfPowerStats", () => {
   });
 
   it("settles the count-up on the real reviewed total (not stuck at 0 when rAF never fires)", async () => {
+    // Deterministic (#flake): force prefers-reduced-motion so useCountUp lands the final value synchronously on
+    // mount, instead of running the requestAnimationFrame tween. jsdom has no matchMedia, so the unfixed test took
+    // the animated path and raced the 3s findByText timeout under CI load. This still pins the intent — the count
+    // settles on the real reviewed total, never stuck at 0 — without depending on animation-frame timing.
+    vi.stubGlobal("matchMedia", () => ({
+      matches: true,
+      media: "(prefers-reduced-motion: reduce)",
+      addEventListener: () => {},
+      removeEventListener: () => {},
+    }));
     apiFetch.mockResolvedValue({ ok: true, status: 200, durationMs: 1, data: PAYLOAD });
     renderWithClient(<ProofOfPowerStats />);
-    expect(await screen.findByText("2,708", undefined, { timeout: 3000 })).toBeTruthy();
+    expect(await screen.findByText("2,708")).toBeTruthy();
   });
 });

migrations/0080_pr_last_published_surface_sha.sql

@@ -0,0 +1,7 @@
+-- Over-publish dedup (#4): the head SHA at which a PR's public surface (comment/label/check-run) was LAST
+-- published. The scheduled re-gate sweep skips re-reviewing + re-publishing a PR while
+-- last_published_surface_sha === head_sha (the surface is already current). Keyed to the head SHA so a push /
+-- rebase / force-push (new head) no longer matches → the next sweep re-reviews + re-publishes the new code.
+-- NULL = never published. gittensory-computed (publish-written); like approved_head_sha / merge_blocked_sha it is
+-- omitted from the GitHub-sync SET clause so a later sync cannot clobber it.
+ALTER TABLE pull_requests ADD COLUMN last_published_surface_sha TEXT;

src/db/repositories.ts

@@ -2627,6 +2627,20 @@ export async function markPullRequestApproved(env: Env, fullName: string, number
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
 }
 
+/** Over-publish dedup (#4): record the head SHA at which the PR's public surface was just published. The scheduled
+ *  re-gate sweep skips re-reviewing while last_published_surface_sha == headSha. Scoped to headSha so a later commit
+ *  (push/rebase/force-push — the live head no longer matches) re-publishes the new code without any manual reset.
+ *  The eq(headSha) in the WHERE is load-bearing: if the live head advanced between review and this write, the UPDATE
+ *  no-ops (never stamps a stale head) → the next sweep correctly re-reviews. Mirrors markPullRequestApproved. */
+export async function markPullRequestSurfacePublished(env: Env, fullName: string, number: number, headSha: string | null | undefined): Promise<void> {
+  if (!headSha) return; // no head to key the marker on → nothing to stamp (the caller's advisory had no head SHA)
+  const db = getDb(env.DB);
+  await db
+    .update(pullRequests)
+    .set({ lastPublishedSurfaceSha: headSha, updatedAt: nowIso() })
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
+}
+
 /** Sweep convergence: stamp the timestamp the scheduled re-gate sweep just recomputed this PR. A plain D1 UPDATE
  *  — NOT routed through the agent-action-executor chokepoint (#1258) — so it advances even when GitHub writes are
  *  suppressed (dry-run / paused). selectRegateCandidates orders the sweep by last_regated_at, so a just-regated PR
@@ -4161,6 +4175,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull
     approvedHeadSha: row.approvedHeadSha,
     // Read straight from the row, NEVER the GitHub payload — this is a gittensory-internal sweep marker.
     lastRegatedAt: row.lastRegatedAt,
+    lastPublishedSurfaceSha: row.lastPublishedSurfaceSha,
   };
 }
 

src/db/schema.ts

@@ -304,6 +304,12 @@ export const pullRequests = sqliteTable(
     // review WRITE that would bump updated_at is suppressed (dry-run / paused). gittensory-computed (sweep-written),
     // omitted from the GitHub-sync SET clause so a later sync cannot clobber it. (Mirrors approved_head_sha.)
     lastRegatedAt: text("last_regated_at"),
+    // Over-publish dedup: the head SHA at which the public surface (comment/label/check-run) was LAST published.
+    // The sweep skips re-reviewing + re-publishing a PR while last_published_surface_sha === headSha (already
+    // current). Keyed to head SHA → a push/rebase/force-push (new head) clears the match and the next sweep
+    // re-reviews + re-publishes. gittensory-computed (publish-written), omitted from the GitHub-sync SET clause so
+    // a later sync cannot clobber it. (Mirrors approved_head_sha.)
+    lastPublishedSurfaceSha: text("last_published_surface_sha"),
     createdAt: text("created_at").notNull().$defaultFn(() => nowIso()),
     updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()),
   },

src/github/comments.ts

@@ -72,6 +72,11 @@ async function createOrUpdateIssueCommentWithMarker(
     if (batch.length < 100) break;
   }
   if (existing) {
+    // Idempotency (#4): skip the PATCH when the rendered body is byte-identical to 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 (existing.body === body) return { id: existing.id, ...(existing.html_url !== undefined ? { html_url: existing.html_url } : {}) };
     const response = await octokit.request("PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}", {
       owner,
       repo,

src/queue/processors.ts

@@ -41,6 +41,7 @@ import {
   getCachedAiReview,
   putCachedAiReview,
   markPullRequestsRegated,
+  markPullRequestSurfacePublished,
   getLatestRegatedAt,
   claimRegateFanoutSlot,
   recordAgentCommandFeedback,
@@ -1508,6 +1509,16 @@ async function reReviewStoredPullRequest(
     /* v8 ignore next -- the row was just upserted above, so the re-read always returns it; `?? pr` is belt-and-suspenders fail-open. */
     pr = (await getPullRequest(env, repoFullName, prNumber)) ?? pr;
   }
+  // Over-publish dedup (#4): the resync above made pr.headSha the LIVE head. If the public surface was already
+  // published at this exact head, the verdict + comment are already current — skip the re-review + re-publish so the
+  // sweep stops re-publishing every open PR every ~2-min cycle. A never-published PR (NULL marker) or a drifted head
+  // (push/rebase/force-push → marker !== live head) falls THROUGH and re-reviews at the new head; the AI cache is
+  // head_sha-keyed too, so a rebase misses it and gets a fresh review. (Webhook synchronize/opened paths review
+  // directly and always re-stamp — this guard only gates the scheduled sweep.)
+  if (pr.lastPublishedSurfaceSha && pr.lastPublishedSurfaceSha === pr.headSha) {
+    console.log(JSON.stringify({ level: "info", event: "rereview_skipped_surface_current", deliveryId, repository: repoFullName, pullNumber: prNumber, headSha: pr.headSha }));
+    return;
+  }
   // Operator review flow: rebase-if-behind → wait for ALL CI to finish → only THEN review. Defers (returns) when
   // a rebase fired a synchronize, or CI is still running — the synchronize / CI-completion webhook re-triggers
   // once the head is current and CI has settled (the sweep backstops a missed event). REST-budget dedup
@@ -5328,6 +5339,13 @@ async function maybePublishPrPublicSurface(
       failedOutputs,
     },
   });
+  // Over-publish dedup (#4): stamp the head SHA we just published at, so the scheduled sweep skips re-reviewing +
+  // re-publishing this PR until its head changes (see the guard in reReviewStoredPullRequest). Reached only when at
+  // least one surface output actually published (the zero-output early-return above covers the suppressed/dry-run
+  // case). The helper no-ops on a null head, and its WHERE pins head_sha so a head that advanced mid-pass won't stamp.
+  await markPullRequestSurfacePublished(env, repoFullName, pr.number, advisory.headSha).catch((error) => {
+    console.error(JSON.stringify({ level: "warn", event: "surface_published_mark_failed", repoFullName, pullNumber: pr.number, error: errorMessage(error) }));
+  });
   return gateEvaluation;
 }
 

src/types.ts

@@ -453,6 +453,10 @@ export type PullRequestRecord = {
    *  review write that would bump updatedAt is suppressed (dry-run / paused). Sweep-written; read straight from
    *  the row (never the GitHub payload). */
   lastRegatedAt?: string | null | undefined;
+  /** Over-publish dedup: the head SHA at which the public surface was last published. The re-gate sweep skips
+   *  re-reviewing + re-publishing while lastPublishedSurfaceSha === headSha; a new commit (push/rebase/force-push)
+   *  clears the match so the surface re-publishes the new code. Publish-written; read straight from the row. */
+  lastPublishedSurfaceSha?: string | null | undefined;
 };
 
 export type IssueRecord = {

test/unit/db-parsers.test.ts

@@ -12,6 +12,7 @@ import {
   listRepoSyncStates,
   markPullRequestRegated,
   markPullRequestsRegated,
+  markPullRequestSurfacePublished,
   recordAuditEvent,
   recordWebhookEvent,
   upsertOfficialMinerDetection,
@@ -101,6 +102,25 @@ describe("database row parser hardening", () => {
     expect(after?.title).toBe("Stale PR"); // INVARIANT: a plain D1 UPDATE — it touches only the marker, not PR content
   });
 
+  it("markPullRequestSurfacePublished stamps last_published_surface_sha only at the matching live head (#4 over-publish dedup)", async () => {
+    const env = createTestEnv();
+    await upsertPullRequestFromGitHub(env, "owner/repo", { number: 9, title: "PR", state: "open", user: { login: "alice" }, head: { sha: "headA" }, labels: [] });
+
+    const before = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 9);
+    expect(before?.lastPublishedSurfaceSha ?? null).toBeNull(); // never published → marker absent
+
+    await markPullRequestSurfacePublished(env, "owner/repo", 9, null); // null head → no-op (the !headSha guard)
+    expect((await listPullRequests(env, "owner/repo")).find((p) => p.number === 9)?.lastPublishedSurfaceSha ?? null).toBeNull();
+
+    await markPullRequestSurfacePublished(env, "owner/repo", 9, "oldHead"); // stale head → WHERE head_sha mismatch → no-op
+    expect((await listPullRequests(env, "owner/repo")).find((p) => p.number === 9)?.lastPublishedSurfaceSha ?? null).toBeNull();
+
+    await markPullRequestSurfacePublished(env, "owner/repo", 9, "headA"); // matches the live head → stamps
+    const after = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 9);
+    expect(after?.lastPublishedSurfaceSha).toBe("headA");
+    expect(after?.title).toBe("PR"); // INVARIANT: touches only the marker, not PR content
+  });
+
   it("markPullRequestsRegated batch-stamps every candidate at dispatch and no-ops on an empty list (#audit-sweep-dispatch-stamp)", async () => {
     const env = createTestEnv();
     for (const number of [5, 6, 7]) {

test/unit/github-comments.test.ts

@@ -229,6 +229,46 @@ describe("GitHub PR intelligence comments", () => {
     expect(calls.some((call) => call.startsWith("POST ") && call.includes("/issues/12/comments"))).toBe(true);
   });
 
+  it("skips the PATCH when the existing sticky comment body is byte-identical (#4 idempotency), keeping html_url", async () => {
+    const privateKey = await generatePrivateKeyPem();
+    const body = `${PR_INTELLIGENCE_COMMENT_MARKER}\nidentical body`;
+    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: 101, body, html_url: "https://github.com/comment/101", user: { login: "gittensory[bot]", type: "Bot" } }]);
+      }
+      return new Response("not found", { status: 404 });
+    });
+
+    const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, body);
+
+    expect(result).toEqual({ id: 101, html_url: "https://github.com/comment/101" }); // html_url-present branch of the early return
+    expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); // identical body → NO GitHub write
+  });
+
+  it("skips the PATCH on an identical body even when the existing comment has no html_url (#4 idempotency)", async () => {
+    const privateKey = await generatePrivateKeyPem();
+    const body = `${PR_INTELLIGENCE_COMMENT_MARKER}\nidentical body`;
+    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: 202, body, user: { login: "gittensory[bot]", type: "Bot" } }]); // no html_url field
+      }
+      return new Response("not found", { status: 404 });
+    });
+
+    const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, body);
+
+    expect(result).toEqual({ id: 202 }); // html_url-absent branch → no html_url key on the early return
+    expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false);
+  });
+
   it("rejects invalid repository names before calling GitHub", async () => {
     await expect(createOrUpdatePrIntelligenceComment(createTestEnv(), 123, "invalid", 12, "body")).rejects.toThrow(/Invalid repository full name/);
   });