Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/github/comments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ const LEGACY_AGENT_COMMAND_COMMENT_MARKER = "<!-- gittensory-agent-command -->";
* 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 = "<!-- loopover:visual-unrelated-followup -->";
// #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 `<!-- loopover:close-explanation:${closeKind ?? "close"} -->`;
}
// 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
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 5 additions & 1 deletion src/services/agent-action-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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": {
Expand Down
10 changes: 9 additions & 1 deletion test/unit/agent-action-executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof import("../../src/github/comments")>()),
// #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),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions test/unit/agent-approval-queue.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { beforeEach, describe, expect, it, vi } from "vitest";

vi.mock("../../src/github/comments", async (importOriginal) => ({
...(await importOriginal<typeof import("../../src/github/comments")>()),
// #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" })),
Expand Down
64 changes: 63 additions & 1 deletion test/unit/github-comments.test.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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("<!-- loopover:close-explanation:close -->");
});
});
6 changes: 6 additions & 0 deletions test/unit/routes-agent-approval.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { beforeEach, describe, expect, it, vi } from "vitest";

vi.mock("../../src/github/comments", async (importOriginal) => ({
...(await importOriginal<typeof import("../../src/github/comments")>()),
// #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" })),
Expand Down
Loading