diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 01f43f27f..6d46c72ce 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -42,7 +42,7 @@ import { type ModerationTier, } from "../settings/moderation-rules"; import { incr } from "../selfhost/metrics"; -import { shouldWaitForOlderSiblings } from "../review/merge-train"; +import { MERGE_TRAIN_MAX_WAIT_MS, shouldWaitForOlderSiblings } from "../review/merge-train"; import { capturePostHogError } from "../selfhost/posthog"; import { claimContributorCapLock, releaseContributorCapLock } from "../queue/transient-locks"; import { buildDecisionRecord, persistDecisionRecord, type DecisionRecord } from "../review/decision-record"; @@ -60,6 +60,7 @@ const AGENT_ACTOR = "loopover"; // retried PR, so it should page sooner. A 1h window catches a wedge well inside its first hour -- far ahead of // both the 24h staleness cap and the 4h it took a human to notice the confirmed #8735 incident (57 denials, // 5 PRs blocked). +const MERGE_TRAIN_WAIT_COMMENT_EVENT_TYPE = "agent.action.merge_train_wait_comment"; const MERGE_TRAIN_WEDGE_ALERT_THRESHOLD = 5; const MERGE_TRAIN_WEDGE_WINDOW_MS = 60 * 60 * 1000; const MERGE_TRAIN_WEDGE_EVENT_TYPE = "agent.action.merge_train_blocked"; @@ -732,6 +733,43 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE } } if (ctx.mergeTrainMode === "enforce") { + // #merge-train-honest-comment (observed live on JSONbored/loopover#9837): the published surface said + // the PR was MERGING -- the planner's disposition legitimately concluded wouldMerge before this + // step-8 check ran -- and then this branch denied it with an AUDIT-ONLY record. Publicly the PR + // claimed an action that never happened, which reads as the bot silently breaking its word. + // + // Post the truth once per (waiting PR, blocker) pair. Dedup via the AUDIT TRAIL, not a transient + // lock: claimTransientLock fails OPEN ({acquired:true}) on any deployment without a transient + // cache bound, which would repeat this comment on every denial pass -- and the comment's own audit + // row is already the exact "did we say this" fact, durable on every deployment. The target key + // carries the blocker so a NEW blocker (a genuinely new fact) gets its own line. Fail-open on the + // comment itself: a post failure never blocks the denial bookkeeping. + const waitCommentTargetKey = `${ctx.repoFullName}#${ctx.pullNumber}:merge-train-wait-comment-for-${decision.blockingPr}`; + const alreadyToldSinceIso = new Date(Date.now() - MERGE_TRAIN_MAX_WAIT_MS).toISOString(); + const alreadyTold = await countRecentAuditEventsForActorAndTarget(env, AGENT_ACTOR, MERGE_TRAIN_WAIT_COMMENT_EVENT_TYPE, waitCommentTargetKey, alreadyToldSinceIso).catch(() => 0); + if (alreadyTold === 0) { + const posted = await createIssueComment( + env, + ctx.installationId, + ctx.repoFullName, + ctx.pullNumber, + `Queued in the merge train behind #${decision.blockingPr}, which touches overlapping work and was opened first. ` + + `This PR merges automatically once #${decision.blockingPr} completes (or leaves the train). No action needed. ` + + `This is an automated maintenance action.`, + ).then(() => true).catch(() => false); + // Recorded ONLY after a successful post: a failed post leaves no row, so the next pass retries + // rather than believing the contributor was told when they never were. + if (posted) { + await recordAuditEvent(env, { + eventType: MERGE_TRAIN_WAIT_COMMENT_EVENT_TYPE, + actor: AGENT_ACTOR, + targetKey: waitCommentTargetKey, + outcome: "completed", + detail: `told the contributor this PR is queued behind #${decision.blockingPr}`, + metadata: { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, blockingPr: decision.blockingPr }, + }).catch(() => undefined); + } + } await audit("denied", `merge train: waiting for older mergeable sibling #${decision.blockingPr} — action not executed`); continue; } diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 3944865e9..a49cb9997 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -2271,6 +2271,83 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train expect(mergePullRequest).not.toHaveBeenCalled(); }); + it("REGRESSION (#merge-train-honest-comment): an enforce-mode train denial tells the contributor, once", async () => { + // Observed live on JSONbored/loopover#9837: the published surface said the PR was MERGING (the planner + // legitimately concluded wouldMerge before the executor's train check), then the denial was recorded + // audit-only -- publicly the PR claimed an action that never happened. The denial must say so on the PR, + // and repeated passes behind the SAME blocker must not repeat the comment (transient-lock dedup). + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older overlapping sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T08:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T10:00:00.000Z" }); + + await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); + expect(createIssueComment).toHaveBeenCalledTimes(1); + const [, , , commentPr, commentBody] = vi.mocked(createIssueComment).mock.calls[0]!; + expect(commentPr).toBe(7); + expect(commentBody).toContain("merge train"); + expect(commentBody).toContain("#3"); + expect(commentBody).toContain("merges automatically"); + + // Second pass behind the SAME blocker: still denied, but no second comment. + await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); + expect(createIssueComment).toHaveBeenCalledTimes(1); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("INVARIANT (#merge-train-honest-comment): a failed AUDIT write does not break the denial — the comment already went out", async () => { + // The dedup row's own write is best-effort. If it fails, the worst case is one duplicate comment on a + // later pass; the denial bookkeeping and the merge suppression must be unaffected. Fail-OPEN, deliberately: + // a D1 hiccup must never turn "queued behind a sibling" into an unhandled throw that skips the deny. + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older overlapping sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T08:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T10:00:00.000Z" }); + + // Fail ONLY the wait-comment bookkeeping row, leaving every other audit write (including the denial's own) + // working -- mocking them all would prove nothing about this branch. + // Capture the real implementation BEFORE spying: referencing the module property inside the mock would + // resolve to the spy itself and recurse forever. + const realRecordAuditEvent = repositoriesModule.recordAuditEvent; + const auditSpy = vi + .spyOn(repositoriesModule, "recordAuditEvent") + .mockImplementation(async (envArg, event) => + event.eventType === "agent.action.merge_train_wait_comment" + ? Promise.reject(new Error("D1 write error")) + : realRecordAuditEvent(envArg, event), + ); + + const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); + auditSpy.mockRestore(); + + expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "denied" }); + expect(createIssueComment).toHaveBeenCalledTimes(1); // the contributor WAS told + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("INVARIANT (#merge-train-honest-comment): a FAILED comment post records nothing, so the next pass retries", async () => { + // The dedup row is written only after a successful post. If a transient GitHub failure ate the comment + // and we recorded it anyway, the contributor would be permanently un-told while the audit trail claimed + // otherwise -- the exact "believed we said it" failure the audit-trail dedup exists to avoid. + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older overlapping sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T08:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T10:00:00.000Z" }); + const trainCtx = ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }); + + vi.mocked(createIssueComment).mockRejectedValueOnce(new Error("502 from GitHub")); + const first = await executeAgentMaintenanceActions(env, trainCtx, [merge]); + expect(first[0]?.outcome).toBe("denied"); // the denial itself is unaffected by the comment failure + expect(createIssueComment).toHaveBeenCalledTimes(1); + + // Nothing was recorded, so the NEXT pass tries again rather than believing the contributor was told. + vi.mocked(createIssueComment).mockResolvedValueOnce(undefined as never); + await executeAgentMaintenanceActions(env, trainCtx, [merge]); + expect(createIssueComment).toHaveBeenCalledTimes(2); + + // And once it succeeds, it stops. + await executeAgentMaintenanceActions(env, trainCtx, [merge]); + expect(createIssueComment).toHaveBeenCalledTimes(2); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + it("does NOT hold an enforce-mode merge behind an older sibling that shares no linked issue or changed file (#selfhost-merge-train-overlap)", async () => { const env = createTestEnv({}); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Unrelated older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #99", created_at: "2026-07-05T08:00:00.000Z" });