diff --git a/src/github/comments.ts b/src/github/comments.ts index eeb8227713..f12ffd8e6f 100644 --- a/src/github/comments.ts +++ b/src/github/comments.ts @@ -11,6 +11,14 @@ const LEGACY_AGENT_COMMAND_COMMENT_MARKER = ""; * thread from the sticky PR panel above (that comment stops being useful the moment the PR closes), so this * must never collapse into `PR_PANEL_COMMENT_MARKER`'s aliases the way the two constants above do. */ export const VISUAL_FOLLOWUP_COMMENT_MARKER = ""; +// #8803: the close-explanation comment's idempotency marker, parameterized by closeKind so distinct close +// reasons on one PR (rare, but e.g. a hard-rule close after an earlier heuristic close attempt) keep their +// own canonical comments. Routing the executor's closeComment through the marker helper means a +// comment-succeeded-close-failed retry PATCHes/skips the canonical comment instead of stacking an +// identical duplicate every failed cycle. +export function closeExplanationMarker(closeKind: string | undefined): string { + return ``; +} // Bound the marker-comment search at 10 pages (up to 1,000 comments), matching src/github's other pagination // caps (app.ts's MAX_WORKFLOW_RUN_LIST_PAGES, pr-actions.ts's REVIEW_PAGE_LIMIT). The old cap of 3 (300 comments) // let a PR/issue that accrued >300 comments before LoopOver's own marker comment hide it from this search, so @@ -45,6 +53,24 @@ export async function createOrUpdatePrIntelligenceComment( * updates the same comment instead of posting a confusing duplicate; a same-body repeat (a retried `closed` * webhook delivery) is a genuine no-op via the same byte-identical-body skip every other marker comment here * already gets. */ +/** #8803: idempotent close-explanation comment — the executor's close path routes here so a retry after a + * failed close never re-posts the identical "why we closed you" body (byte-identical → skip; changed → + * PATCH the canonical comment). createIfMissing stays true: the first attempt must always post. */ +export async function createOrUpdateCloseExplanationComment( + env: Env, + installationId: number, + repoFullName: string, + pullNumber: number, + body: string, + closeKind: string | undefined, +): Promise<{ id: number; html_url?: string; changed: boolean } | null> { + const marker = closeExplanationMarker(closeKind); + // The marker MUST live in the posted body — the search-side helper only finds comments whose body contains + // it (the intelligence/visual callers embed theirs the same way). An HTML comment renders invisibly, so the + // contributor-facing text is unchanged; a byte-identical replan skips, a reworded one PATCHes. + return createOrUpdateIssueCommentWithMarker(env, installationId, repoFullName, pullNumber, `${marker}\n${body}`, marker); +} + export async function createOrUpdateVisualFollowupComment( env: Env, installationId: number, diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 12135b15d0..10b20f9b8c 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -24,6 +24,7 @@ import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestAssignee } from "../github/assignees"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions"; +import { createOrUpdateCloseExplanationComment } from "../github/comments"; import { fetchPullRequestFreshness, pullRequestFreshnessDetail } from "../github/pr-freshness"; import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy"; import { boundStructuredCloseReasonsForPersistence, buildAgentActionAudit, formatAgentPermissionDenial, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness, type AgentActionMode } from "../settings/agent-execution"; @@ -1051,7 +1052,10 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: return; } case "close": - if (action.closeComment) await createIssueComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.closeComment); + // #8803: marker-idempotent — when the comment lands but the close call fails transiently, the retry's + // replan produces the identical closeComment; the marker helper skips/PATCHes the canonical comment + // instead of stacking a duplicate "why we closed you" every failed cycle. + if (action.closeComment) await createOrUpdateCloseExplanationComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.closeComment, action.closeKind); await closePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber); return; case "update_branch": { diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 802146a03f..31978bec33 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -10,6 +10,12 @@ vi.mock("../../src/github/pr-actions", () => ({ updatePullRequestBranch: vi.fn(async () => undefined), dismissLatestBotApproval: vi.fn(async () => ({ dismissed: true })), })); +vi.mock("../../src/github/comments", async (importOriginal) => ({ + ...(await importOriginal()), + // #8803: the close-explanation comment now routes through the marker helper; mock it like the sibling + // pr-actions primitives so no real fetch happens and call sites can assert on it. + createOrUpdateCloseExplanationComment: vi.fn(async () => ({ id: 2, changed: true })), +})); vi.mock("../../src/github/labels", () => ({ ensurePullRequestLabel: vi.fn(async () => ({ applied: true, created: false })), removePullRequestLabel: vi.fn(async () => undefined), @@ -58,6 +64,7 @@ import { ensurePullRequestAssignee } from "../../src/github/assignees"; import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; import { createInstallationToken } from "../../src/github/app"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; +import { createOrUpdateCloseExplanationComment } from "../../src/github/comments"; import { actionParams, applyModerationEscalationForRule, @@ -192,7 +199,8 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { // its own — a live sweep's approve plans no explicit pin, so this is the unpinned/live-sweep case (#2262). expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "lgtm", "sha7"); expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); - expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "closing"); + // #8803: the close explanation routes through the idempotent marker helper (closeKind-scoped marker). + expect(createOrUpdateCloseExplanationComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "closing", undefined); expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); expect(updatePullRequestBranch).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "sha7"); expect(fetchPullRequestFreshness).toHaveBeenCalledTimes(6); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 35f87d4616..adef8786c0 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -1,5 +1,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +vi.mock("../../src/github/comments", async (importOriginal) => ({ + ...(await importOriginal()), + // #8803: the close-explanation comment routes through the marker helper; mock it like the pr-actions + // primitives below so no real fetch (marker search) happens in these suites. + createOrUpdateCloseExplanationComment: vi.fn(async () => ({ id: 2, changed: true })), +})); vi.mock("../../src/github/pr-actions", () => ({ createPullRequestReview: vi.fn(async () => ({ id: 1 })), mergePullRequest: vi.fn(async () => ({ merged: true, sha: "merged-sha" })), diff --git a/test/unit/github-comments.test.ts b/test/unit/github-comments.test.ts index a3299a6bc2..d21f29dad1 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 { createOrUpdatePrIntelligenceComment, createOrUpdateVisualFollowupComment, PR_INTELLIGENCE_COMMENT_MARKER, VISUAL_FOLLOWUP_COMMENT_MARKER } from "../../src/github/comments"; +import { closeExplanationMarker, 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"; @@ -520,3 +520,65 @@ describe("createOrUpdateIssueCommentWithMarker repoFullName guard (#8311)", () = } }); }); + +describe("createOrUpdateCloseExplanationComment (#8803)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("first attempt POSTs the explanation with the closeKind-scoped marker embedded in the body", async () => { + const privateKey = await generatePrivateKeyPem(); + let postedBody = ""; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.includes("/issues/7/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/7/comments") && method === "POST") { + postedBody = (JSON.parse(String(init?.body)) as { body: string }).body; + return Response.json({ id: 55, html_url: "https://github.com/comment/55" }); + } + return new Response("not found", { status: 404 }); + }); + const result = await createOrUpdateCloseExplanationComment( + createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), + 123, + "JSONbored/gittensory", + 7, + "Closed: duplicate of #5.", + "heuristic", + ); + expect(result?.id).toBe(55); + expect(postedBody).toContain(closeExplanationMarker("heuristic")); + expect(postedBody).toContain("Closed: duplicate of #5."); + }); + + it("a retry with the identical body SKIPS (no duplicate 'why we closed you' comment) — the #8803 incident shape", async () => { + const privateKey = await generatePrivateKeyPem(); + const marker = closeExplanationMarker("blacklist"); + const posts: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.includes("/issues/7/comments") && method === "GET") { + // The first attempt's comment already landed; the close then failed and this is the replan's retry. + return Response.json([{ id: 90, user: { login: "loopover-orb[bot]", type: "Bot" }, body: `${marker}\nClosed: policy.` }]); + } + if (method === "POST" || method === "PATCH") { + posts.push(method); + return Response.json({ id: 91 }); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey, GITHUB_APP_SLUG: "loopover-orb" }); + const result = await createOrUpdateCloseExplanationComment(env, 123, "JSONbored/gittensory", 7, "Closed: policy.", "blacklist"); + expect(result?.changed).toBe(false); // byte-identical -> skip + expect(posts).toEqual([]); // neither a duplicate POST nor a pointless PATCH + }); + + it("distinct closeKinds use distinct markers so different close reasons keep separate canonical comments", () => { + expect(closeExplanationMarker("blacklist")).not.toBe(closeExplanationMarker("heuristic")); + expect(closeExplanationMarker(undefined)).toBe(""); + }); +}); diff --git a/test/unit/routes-agent-approval.test.ts b/test/unit/routes-agent-approval.test.ts index 6fff60182e..6a66ecaa3e 100644 --- a/test/unit/routes-agent-approval.test.ts +++ b/test/unit/routes-agent-approval.test.ts @@ -1,5 +1,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +vi.mock("../../src/github/comments", async (importOriginal) => ({ + ...(await importOriginal()), + // #8803: the close-explanation comment routes through the marker helper; mock it like the pr-actions + // primitives below so no real fetch (marker search) happens in these suites. + createOrUpdateCloseExplanationComment: vi.fn(async () => ({ id: 2, changed: true })), +})); vi.mock("../../src/github/pr-actions", () => ({ createPullRequestReview: vi.fn(async () => ({ id: 1 })), mergePullRequest: vi.fn(async () => ({ merged: true, sha: "merged-sha" })),