From 00260f23167bb4d73eac14e216433db9c482a508 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 06:13:28 -0700 Subject: [PATCH 1/3] fix(train): tell the contributor when the merge train defers a merge the surface already announced Observed live on JSONbored/loopover#9837: the published panel said the PR was MERGING -- the planner's disposition legitimately concluded wouldMerge before the executor's train check ran -- and then the enforce-mode train denial recorded itself AUDIT-ONLY. Publicly the PR claimed an action that never happened, which reads as the bot silently breaking its word (and, to an operator, as the old lost-merge bug recurring when it is actually FIFO queueing working as designed: 9837 was waiting on overlapping older sibling 9833, whose CI was still running). Post one comment naming the blocker and what happens next. Dedup via the AUDIT TRAIL, not a transient lock: claimTransientLock fails OPEN on any deployment without a transient cache bound, which would repeat the comment on every denial pass. The comment's own audit row (recorded only after a successful post, so a failed post retries next pass) is the durable "did we say this" fact everywhere, keyed per (waiting PR, blocker) so a NEW blocker gets its own line and the SAME blocker never repeats within the 24h train window. --- src/services/agent-action-executor.ts | 40 ++++++++++++++++++++++++- test/unit/agent-action-executor.test.ts | 23 ++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) 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..2df469f00 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -2271,6 +2271,29 @@ 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("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" }); From b8abaf2c02534a32431c09c0392980d64b41e9d3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 06:40:15 -0700 Subject: [PATCH 2/3] test(train): cover the failed-comment retry path Codecov flagged the two remaining lines: the branch where createIssueComment REJECTS. That path is the whole reason the audit row is written after the post rather than before -- a transient GitHub failure must leave no row, so the next pass retries instead of believing the contributor was told. Pins all three states: a failed post still denies and records nothing, the next pass retries, and once it succeeds it stops. --- test/unit/agent-action-executor.test.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 2df469f00..cb335fb88 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -2294,6 +2294,31 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train 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" }); From 4b9ced766d6d4b7bca9443ffb7a6499ae10a97e0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 07:35:30 -0700 Subject: [PATCH 3/3] test(train): cover the audit-write fail-safe on the wait comment Codecov's last uncovered line was the .catch() on the dedup row's own write. It is deliberately fail-open: if that best-effort write fails, the worst case is one duplicate comment on a later pass, and the denial plus merge suppression must be unaffected. Pinned by failing ONLY that event type, so the denial's own audit write still succeeds and the test proves the branch rather than the mock. --- test/unit/agent-action-executor.test.ts | 29 +++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index cb335fb88..a49cb9997 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -2294,6 +2294,35 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train 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