From e55bc2df65d2a73125d1ed4c02fff69bc67c078a Mon Sep 17 00:00:00 2001 From: Dotta Date: Fri, 30 Oct 2026 16:19:27 -0500 Subject: [PATCH] fix(server): schedule only outstanding merge confirmations Co-Authored-By: Paperclip --- ...026-10-30-merged-pr-confirmations-sleep.md | 83 ++++++ .../merged-pr-confirmation-sweep.test.ts | 11 +- .../merged-pr-confirmation-work.test.ts | 278 ++++++++++++++++++ .../server-startup-feedback-export.test.ts | 5 +- server/src/index.ts | 17 +- .../chat-confirmation-notifications.ts | 2 + .../services/delivery-work-notifications.ts | 1 + .../src/services/issue-thread-interactions.ts | 101 ++++--- server/src/services/issues.ts | 4 + .../merged-pr-confirmation-notifications.ts | 7 + .../services/merged-pr-confirmation-work.ts | 30 ++ 11 files changed, 474 insertions(+), 65 deletions(-) create mode 100644 doc/plans/2026-10-30-merged-pr-confirmations-sleep.md create mode 100644 server/src/__tests__/merged-pr-confirmation-work.test.ts create mode 100644 server/src/services/merged-pr-confirmation-notifications.ts create mode 100644 server/src/services/merged-pr-confirmation-work.ts diff --git a/doc/plans/2026-10-30-merged-pr-confirmations-sleep.md b/doc/plans/2026-10-30-merged-pr-confirmations-sleep.md new file mode 100644 index 00000000000..fb8b091f841 --- /dev/null +++ b/doc/plans/2026-10-30-merged-pr-confirmations-sleep.md @@ -0,0 +1,83 @@ +# Merged pull request confirmation scheduling + +The merged-PR confirmation duty previously ran from every heartbeat maintenance +callback, including when there were no merge requests. It loaded all pending +confirmation cards and then applied the merge-only parser in memory. + +The interaction service still owns merge-only eligibility, provider access, +automatic acceptance, current resolver authority, audit, and continuation. +The worker registers the existing scoped-deadline helper with a dedicated delivery +topic. Shared scheduling receives interaction/company identities and deadlines; +it receives no GitHub payloads. No database migration is required. + +## Durable truth and producers + +Pending `request_confirmation` interaction rows remain the durable work source. +The candidate query excludes terminal and hidden issues, linked tool actions, +and text without merge intent or a PR reference. The existing fail-closed parser +then excludes additional actions, governed actions, and document/attachment targets. +No accepted or cancelled history keeps a deadline alive. + +Ordinary creation registers the topic before inserting a confirmation in its +transaction. Existing confirmation-resolution notifications invalidate its issue +before acceptance, rejection, withdrawal, supersession, and administrative expiry. +Issue status and visibility changes invalidate that issue before their update. +The signal helper joins caller-owned outer transactions; nested savepoint release +is not a commit. Startup reconstructs existing candidates. Direct tool, secret, +and OAuth interaction inserts do not create eligible merge-only work. Company +portability does not import pending interaction rows. + +The supported writer model is the instance's ordinary service/API process. +Independent SQL writers must use the same transaction-aware notification contract +or explicitly invalidate/restart the owning worker. Process-local notifications +do not establish a cross-replica ingress guarantee. No permanent database listener +or empty recovery scan is added. + +## External observation and retries + +The existing normalized `applyPullRequestEvent` to +`sweepMergedPullRequestConfirmations` seam remains available. A normalized merge +hint checks matching references in its company; unrelated tenants and PRs do not +cause provider reads. Existing provider/binding authorization remains unchanged. + +A pending candidate can reference a PR whose external changes have no dependable +webhook coverage. Each such candidate retains a five-minute observation deadline. +Resolution, withdrawal, supersession, hiding, or terminal issue state removes the +obligation. Provider/read failures and unknown states retain the candidate, using +the existing scoped helper's 30-second exponential backoff capped at five minutes. +These retries do not scan completed history. Removing candidate reconciliation +requires a proven durable delivery/replay guarantee for every referenced PR, +including the first event after idle and supported non-Cloud installations. + +This is candidate-backed external observation. It does not eliminate all polling, +register a Cloud wake alarm, or prove that the host/database suspends. Existing +sleep-safety blockers remain in force. + +## Acceptance evidence + +Focused tests use real PostgreSQL, the actual service producers, transaction +signals, coordinator, scheduler, acceptance transaction, and persisted audit and +chat continuation outbox. GitHub state and the direct heartbeat wake callback are +controlled simulation boundaries. The tests do not claim live GitHub delivery or +real agent execution. + +Cases cover empty/configured idle and retained terminal history with no recurring +SQL/provider work across old cadence windows; the first ordinary creation; +outer commit/rollback; exact candidate and failure deadlines; withdrawal; +provider failure across drain; standby/resume; restart; tenant-scoped and duplicate +merge hints; latest/superseded cards during provider observation; hidden/terminal +issues; URL targets; no-continuation policy; and shutdown during an in-flight read. +Existing startup and normalized GitHub event tests exercise the changed consumers. + +## Existing limitations outside this migration + +Direct non-chat continuation still uses the existing postcommit `wakeup` call. +If that call fails after acceptance commits, the previous periodic sweep also +skipped the accepted row and did not reconstruct the missing wake. This migration +preserves that behavior; a general durable interaction-continuation contract is +a separate follow-up. Real chat bindings retain the existing transactional +`interaction_wakeup` outbox, verified through ordinary creation and acceptance. + +The separate review-authority issue tracked in public issue #17704 and PR #17711 +remains owned there. The worker isolates candidate failures and preserves current +review authorization; it does not grant a system actor new review authority. diff --git a/server/src/__tests__/merged-pr-confirmation-sweep.test.ts b/server/src/__tests__/merged-pr-confirmation-sweep.test.ts index 1e4d56436e7..d9cf9880357 100644 --- a/server/src/__tests__/merged-pr-confirmation-sweep.test.ts +++ b/server/src/__tests__/merged-pr-confirmation-sweep.test.ts @@ -1,3 +1,4 @@ +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; import { randomUUID } from "node:crypto"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { inArray } from "drizzle-orm"; @@ -131,7 +132,8 @@ describe("merged pull-request confirmation extraction", () => { }); }); -const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const external = resolveExternalTestDatabaseUrl("PAPERCLIP_MERGE_SWEEP_TEST_DATABASE_URL"); +const embeddedPostgresSupport = external ? { supported: true } : await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; describeEmbeddedPostgres("merged pull-request confirmation sweep", () => { @@ -139,8 +141,8 @@ describeEmbeddedPostgres("merged pull-request confirmation sweep", () => { let tempDb: Awaited> | null = null; beforeAll(async () => { - tempDb = await startEmbeddedPostgresTestDatabase("paperclip-merged-pr-confirmations-"); - db = createDb(tempDb.connectionString); + if (!external) tempDb = await startEmbeddedPostgresTestDatabase("paperclip-merged-pr-confirmations-"); + db = createDb(external ?? tempDb!.connectionString, { maxConnections: 6 }); }, 20_000); afterEach(async () => { @@ -153,6 +155,7 @@ describeEmbeddedPostgres("merged pull-request confirmation sweep", () => { }); afterAll(async () => { + await db?.$client.end({ timeout: 1 }); await tempDb?.cleanup(); }); @@ -276,7 +279,7 @@ describeEmbeddedPostgres("merged pull-request confirmation sweep", () => { repo: "paperclip", number: 39, }])).resolves.toEqual({ - checked: 6, + checked: 3, candidates: 3, accepted: 2, woken: 2, diff --git a/server/src/__tests__/merged-pr-confirmation-work.test.ts b/server/src/__tests__/merged-pr-confirmation-work.test.ts new file mode 100644 index 00000000000..51195e6157b --- /dev/null +++ b/server/src/__tests__/merged-pr-confirmation-work.test.ts @@ -0,0 +1,278 @@ +import { randomUUID } from "node:crypto"; +import { setTimeout as delay } from "node:timers/promises"; +import { eq, sql } from "drizzle-orm"; +import { activityLog, agents, chatActions, chatConversations, chatEndpoints, chatPublications, companies, createDb, issues, issueThreadInteractions, toolApplications, toolConnections, type Db } from "@paperclipai/db"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { createDeliveryWorkCoordinator } from "../services/delivery-work-coordinator.js"; +import { issueThreadInteractionService } from "../services/issue-thread-interactions.js"; +import { issueService } from "../services/issues.js"; +import { registerMergeConfirmationWork, MERGE_CONFIRMATION_RECONCILE_MS } from "../services/merged-pr-confirmation-work.js"; +import { idleWorkSnapshot } from "../services/task-admission.js"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; + +const external = resolveExternalTestDatabaseUrl("PAPERCLIP_MERGE_WORK_TEST_DATABASE_URL"); +const support = external ? { supported: true } : await getEmbeddedPostgresTestSupport(); +(support.supported ? describe : describe.skip)("merge confirmation work with PostgreSQL and simulated GitHub", () => { + let temporary: Awaited> | undefined; + let db: ReturnType; + let companyId: string, issueId: string, agentId: string; + let coordinator: ReturnType | undefined; + let draining = false, standby = false; + const errors: unknown[] = []; + const provider = vi.fn(async (_companyId: string, _reference: { number: number }): Promise<"open" | "merged" | "unknown"> => "merged"); + const wakeup = vi.fn(async () => ({ id: "simulated-durable-wake" })); + beforeAll(async () => { + if (!external) temporary = await startEmbeddedPostgresTestDatabase("merged-pr-work-"); + db = createDb(external ?? temporary!.connectionString, { maxConnections: 6, idleTimeoutSeconds: 0, maxLifetimeSeconds: 0 }); + }, 30_000); + beforeEach(async () => { + errors.length = 0; draining = false; standby = false; + provider.mockReset().mockResolvedValue("merged"); wakeup.mockClear(); + companyId = randomUUID(); agentId = randomUUID(); issueId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Merge worker", issuePrefix: `M${companyId.slice(0, 6)}` }); + await db.insert(agents).values({ id: agentId, companyId, name: "Coder", role: "engineer", status: "idle", adapterType: "codex_local" }); + await db.insert(issues).values({ id: issueId, companyId, title: "Merge task", status: "todo", assigneeAgentId: agentId }); + await Promise.all(Array.from({ length: 6 }, () => db.execute(sql`select pg_sleep(0.01)`))); + vi.useFakeTimers({ toFake: ["Date", "setTimeout", "clearTimeout"], shouldAdvanceTime: false }); + }); + afterEach(async () => { + await coordinator?.stop(); coordinator = undefined; + vi.useRealTimers(); vi.restoreAllMocks(); + await db.delete(chatActions); await db.delete(chatPublications); await db.delete(chatConversations); await db.delete(chatEndpoints); + await db.delete(toolConnections); await db.delete(toolApplications); + await db.delete(activityLog); await db.delete(issueThreadInteractions); await db.delete(issues); + await db.delete(agents); await db.delete(companies); + }); + afterAll(async () => { await db?.$client.end({ timeout: 1 }); await temporary?.cleanup(); }); + const service = (database: Db = db) => issueThreadInteractionService(database, { resolvePullRequestState: provider, wakeup }); + const input = (number = 39) => ({ kind: "request_confirmation" as const, title: `Merge paperclipai/paperclip#${number}?`, + resolverPolicy: "human_only" as const, continuationPolicy: "wake_assignee_on_accept" as const, + payload: { version: 1 as const, prompt: "Merge the PR?" } }); + const create = (database: Db = db, number = 39) => service(database).create({ id: issueId, companyId }, input(number), { agentId }); + const stored = (id: string) => db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.id, id)).then(rows => rows[0]); + async function start() { + coordinator = createDeliveryWorkCoordinator({ owner: db, canRun: () => !draining && !standby, + canReconcile: () => !standby, onError: error => errors.push(error) }); + const worker = registerMergeConfirmationWork(coordinator, service(), () => !draining && !standby); + await worker.ready; + return worker; + } + async function waitFor(check: () => Promise | void) { + for (let attempt = 0; attempt < 100; attempt++) { + await vi.advanceTimersByTimeAsync(1000); + await delay(10); + try { await check(); return; } catch (error) { if (attempt === 99) throw error; } + } + } + const settle = () => waitFor(() => expect(idleWorkSnapshot().active).toBe(0)); + async function quiet() { + await waitFor(() => { expect(idleWorkSnapshot().active).toBe(0); expect(coordinator!.nextWakeAt()).toBeNull(); }); + const reads = vi.spyOn(db, "select"), transactions = vi.spyOn(db, "transaction"), executes = vi.spyOn(db, "execute"); + const calls = provider.mock.calls.length; + await vi.advanceTimersByTimeAsync(3 * MERGE_CONFIRMATION_RECONCILE_MS); + expect(reads).not.toHaveBeenCalled(); expect(transactions).not.toHaveBeenCalled(); expect(executes).not.toHaveBeenCalled(); + expect(provider).toHaveBeenCalledTimes(calls); + reads.mockRestore(); transactions.mockRestore(); executes.mockRestore(); + } + + it("settles empty and configured idle, then ordinary creation accepts, audits, wakes once and becomes quiet with retained history", async () => { + await start(); await quiet(); expect(provider).not.toHaveBeenCalled(); + const card = await create(); + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); + await quiet(); + expect(wakeup).toHaveBeenCalledTimes(1); + expect(wakeup).toHaveBeenCalledWith(agentId, expect.objectContaining({ idempotencyKey: `interaction:${card.id}:accepted`, allowRunCoalescing: false })); + expect(await db.select().from(activityLog)).toEqual(expect.arrayContaining([expect.objectContaining({ + action: "issue.thread_interaction_accepted", actorId: "system:pr-merged", details: expect.objectContaining({ interactionId: card.id }), + })])); + expect(errors).toEqual([]); + }); + + it("reconciles an open candidate at its deadline, never early, then stops after merge", async () => { + provider.mockResolvedValue("open"); await start(); const card = await create(); + await waitFor(() => expect(provider).toHaveBeenCalledTimes(1)); await settle(); + const dueAt = coordinator!.nextWakeAt()!; + expect(dueAt).toBeGreaterThan(Date.now()); + await vi.advanceTimersByTimeAsync(dueAt - Date.now() - 1); expect(provider).toHaveBeenCalledTimes(1); + provider.mockResolvedValue("merged"); await vi.advanceTimersByTimeAsync(1); + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + expect(provider).toHaveBeenCalledTimes(2); expect(wakeup).toHaveBeenCalledTimes(1); + }); + + it("manual cancellation removes the candidate deadline without provider work", async () => { + provider.mockResolvedValue("open"); await start(); const card = await create(); + await waitFor(() => expect(provider).toHaveBeenCalledTimes(1)); await settle(); + await service().withdrawInteraction({ id: issueId, companyId }, card.id, {}, { agentId }); + await quiet(); expect(provider).toHaveBeenCalledTimes(1); expect(wakeup).not.toHaveBeenCalled(); + expect((await stored(card.id)).status).toBe("cancelled"); + }); + + it("does not accept before caller-owned outer commit and forgets a rolled-back create", async () => { + await start(); await quiet(); + let release!: () => void, created!: () => void; + const barrier = new Promise(resolve => { release = resolve; }); + const inserted = new Promise(resolve => { created = resolve; }); + let id = ""; + const transaction = db.transaction(async tx => { + id = (await create(tx as unknown as Db)).id; created(); await barrier; + }); + await inserted; + await vi.advanceTimersByTimeAsync(10_000); expect(provider).not.toHaveBeenCalled(); + expect(await stored(id)).toBeUndefined(); release(); await transaction; + await waitFor(async () => expect((await stored(id)).status).toBe("accepted")); await quiet(); + const prior = provider.mock.calls.length; + await expect(db.transaction(async tx => { await create(tx as unknown as Db, 41); throw new Error("rollback"); })).rejects.toThrow("rollback"); + await quiet(); expect(provider).toHaveBeenCalledTimes(prior); expect(errors).toEqual([]); + }); + + it("retains provider failures across drain, resumes without overlap, and clears retry after success", async () => { + provider.mockRejectedValue(new Error("GitHub unavailable")); await start(); const card = await create(); + await waitFor(() => expect(errors).toHaveLength(1)); await settle(); + expect((await stored(card.id)).status).toBe("pending"); + draining = true; + const reads = vi.spyOn(db, "select"), calls = provider.mock.calls.length; + await vi.advanceTimersByTimeAsync(120_000); + expect(reads).not.toHaveBeenCalled(); expect(provider).toHaveBeenCalledTimes(calls); reads.mockRestore(); + draining = false; provider.mockResolvedValue("merged"); + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + expect(wakeup).toHaveBeenCalledTimes(1); + }); + + it("keeps first work committed during drain and startup during standby until resume", async () => { + standby = true; await start(); const card = await create(); + await vi.advanceTimersByTimeAsync(60_000); expect(provider).not.toHaveBeenCalled(); + standby = false; draining = true; + await vi.advanceTimersByTimeAsync(60_000); expect(provider).not.toHaveBeenCalled(); + draining = false; + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + }); + + it("reconstructs pending cards on restart and ignores terminal or hidden tasks", async () => { + provider.mockResolvedValue("open"); const card = await create(); await start(); + await waitFor(() => expect(provider).toHaveBeenCalledTimes(1)); await settle(); + await coordinator!.stop(); coordinator = undefined; provider.mockResolvedValue("merged"); + await start(); await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + const hidden = await db.transaction(async tx => { + const card = await create(tx as unknown as Db, 41); + await issueService(tx as unknown as Db).update(issueId, { hiddenAt: new Date() }); + return card; + }); + await quiet(); expect((await stored(hidden.id)).status).toBe("pending"); + await issueService(db).update(issueId, { hiddenAt: null }); + await waitFor(async () => expect((await stored(hidden.id)).status).toBe("accepted")); await quiet(); + }); + + it("preserves a real chat binding's durable continuation outbox on automatic acceptance", async () => { + const applicationId = randomUUID(), connectionId = randomUUID(), endpointId = randomUUID(); + await db.insert(toolApplications).values({ id: applicationId, companyId, applicationKey: `chat:slack:${endpointId}`, name: "Slack", type: "chat", status: "active" }); + await db.insert(toolConnections).values({ id: connectionId, companyId, applicationId, name: "Slack", uid: endpointId, + connectionPurpose: "channel", transport: "chat_sdk", status: "active", enabled: true }); + await db.insert(chatEndpoints).values({ id: endpointId, companyId, connectionId, provider: "slack", publicId: endpointId, + assignedAgentId: agentId, status: "active" }); + const [conversation] = await db.insert(chatConversations).values({ companyId, endpointId, issueId, + externalConversationId: "merge-channel", externalThreadId: "merge-thread", externalLabel: "Merge thread", state: "active" }).returning(); + await start(); await quiet(); const card = await create(); + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + expect(await db.select().from(chatActions).where(eq(chatActions.kind, "interaction_wakeup"))).toEqual([ + expect.objectContaining({ companyId, endpointId, conversationId: conversation.id, providerActionId: `interaction_wakeup:${card.id}`, + payload: expect.objectContaining({ interactionId: card.id, issueId, agentId, interactionStatus: "accepted" }) }), + ]); + expect(await db.select().from(chatPublications)).toEqual(expect.arrayContaining([ + expect.objectContaining({ idempotencyKey: `interaction:${card.id}:${endpointId}`, state: "cancelled" }), + ])); + expect(errors).toEqual([]); + }); + + it("does not lose a newer card created during provider observation or wake the superseded card", async () => { + let release!: () => void, observed!: () => void; + const barrier = new Promise(resolve => { release = resolve; }); + const observing = new Promise(resolve => { observed = resolve; }); + provider.mockImplementationOnce(async () => { observed(); await barrier; return "merged"; }); + await start(); const old = await create(); + await vi.advanceTimersByTimeAsync(1000); await observing; + const current = await create(db, 41); + expect((await stored(old.id)).status).toBe("expired"); + release(); + await waitFor(async () => expect((await stored(current.id)).status).toBe("accepted")); await quiet(); + expect(wakeup).toHaveBeenCalledTimes(1); + expect(wakeup).toHaveBeenCalledWith(agentId, expect.objectContaining({ idempotencyKey: `interaction:${current.id}:accepted` })); + expect(errors).toEqual([]); + }); + + it("keeps unknown provider results as retryable debt and does not act before retry", async () => { + provider.mockResolvedValue("unknown"); await start(); const card = await create(); + await waitFor(() => expect(errors).toHaveLength(1)); + const next = coordinator!.nextWakeAt()!; + await vi.advanceTimersByTimeAsync(next - Date.now() - 1); expect(provider).toHaveBeenCalledTimes(1); + provider.mockResolvedValue("merged"); await vi.advanceTimersByTimeAsync(1); + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + expect(provider).toHaveBeenCalledTimes(2); + }); + + it("honors terminal issues, pure merge URL targets and continuationPolicy none", async () => { + await start(); await quiet(); + const target = await service().create({ id: issueId, companyId }, { ...input(), title: "Merge the linked PR?", continuationPolicy: "none", + payload: { version: 1, prompt: "Merge the PR?", target: { type: "custom", key: "github-pr-39", href: "https://github.com/paperclipai/paperclip/pull/39" } } }, { agentId }); + await waitFor(async () => expect((await stored(target.id)).status).toBe("accepted")); await quiet(); + expect(wakeup).not.toHaveBeenCalled(); + provider.mockClear(); + const terminal = await db.transaction(async tx => { + const card = await create(tx as unknown as Db, 41); + await issueService(tx as unknown as Db).update(issueId, { status: "cancelled" }); + return card; + }); + await quiet(); expect((await stored(terminal.id)).status).toBe("expired"); + expect(provider).not.toHaveBeenCalled(); expect(wakeup).not.toHaveBeenCalled(); + }); + + it("rechecks hidden state under the acceptance lock when visibility changes during a provider read", async () => { + let release!: () => void, observed!: () => void; + const barrier = new Promise(resolve => { release = resolve; }); + const observing = new Promise(resolve => { observed = resolve; }); + provider.mockImplementationOnce(async () => { observed(); await barrier; return "merged"; }); + await start(); const card = await create(); await vi.advanceTimersByTimeAsync(1000); await observing; + await issueService(db).update(issueId, { hiddenAt: new Date() }); release(); await quiet(); + expect((await stored(card.id)).status).toBe("pending"); expect(wakeup).not.toHaveBeenCalled(); + await issueService(db).update(issueId, { hiddenAt: null }); + await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + expect(wakeup).toHaveBeenCalledTimes(1); expect(errors).toEqual([]); + }); + + it("unrelated pending confirmations do not retain worker SQL or provider work", async () => { + await start(); await quiet(); + await service().create({ id: issueId, companyId }, { ...input(), title: "Approve the plan?", payload: { version: 1, prompt: "Approve the plan?" } }, { agentId }); + await quiet(); expect(provider).not.toHaveBeenCalled(); expect(wakeup).not.toHaveBeenCalled(); + expect(errors).toEqual([]); + }); + + it("awaits an in-flight observation at shutdown and reconstructs its unaccepted card on restart", async () => { + let release!: () => void, observed!: () => void; + const barrier = new Promise(resolve => { release = resolve; }); + const observing = new Promise(resolve => { observed = resolve; }); + provider.mockImplementationOnce(async () => { observed(); await barrier; return "merged"; }); + await start(); const card = await create(); await vi.advanceTimersByTimeAsync(1000); await observing; + let stopped = false; + const stopping = coordinator!.stop().then(() => { stopped = true; }); + await delay(10); expect(stopped).toBe(false); release(); await stopping; coordinator = undefined; + expect((await stored(card.id)).status).toBe("pending"); expect(wakeup).not.toHaveBeenCalled(); + const reads = vi.spyOn(db, "select"); + await vi.advanceTimersByTimeAsync(2 * MERGE_CONFIRMATION_RECONCILE_MS); expect(reads).not.toHaveBeenCalled(); reads.mockRestore(); + await start(); await waitFor(async () => expect((await stored(card.id)).status).toBe("accepted")); await quiet(); + expect(wakeup).toHaveBeenCalledTimes(1); expect(errors).toEqual([]); + }); + + it("limits normalized merge hints to the owning company and matching PR and tolerates duplicates", async () => { + provider.mockResolvedValue("open"); const own = await create(); + const foreignCompany = randomUUID(), foreignIssue = randomUUID(); + await db.insert(companies).values({ id: foreignCompany, name: "Other company", issuePrefix: "OTH" }); + await db.insert(issues).values({ id: foreignIssue, companyId: foreignCompany, title: "Other", status: "todo" }); + const foreign = await service().create({ id: foreignIssue, companyId: foreignCompany }, input(), { userId: "board" }); + const local = service(); + const hint = [{ companyId, owner: "PAPERCLIPAI", repo: "paperclip", number: 39 }]; + expect((await local.sweepMergedPullRequestConfirmations(hint)).accepted).toBe(1); + expect((await stored(own.id)).status).toBe("accepted"); expect((await stored(foreign.id)).status).toBe("pending"); + expect(provider).not.toHaveBeenCalled(); + expect((await local.sweepMergedPullRequestConfirmations(hint)).accepted).toBe(0); expect(wakeup).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index b645d7437c8..9fc7234b2e6 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -180,6 +180,7 @@ const { const heartbeatServiceFactoryMock = vi.fn(() => heartbeatServiceMock); const issueThreadInteractionServiceMock = { sweepSupersededPendingRequestConfirmations: vi.fn(async () => ({ expired: 0 })), + mergedPullRequestConfirmationCandidates: vi.fn(async () => []), sweepMergedPullRequestConfirmations: vi.fn(async () => ({ checked: 0, candidates: 0, @@ -1334,7 +1335,9 @@ describe("startServer feedback export wiring", () => { expect(heartbeatServiceMock.tickAgentTimers).toHaveBeenCalledTimes(1); expect(externalObjectsServiceMock.refreshDueObjectsForActiveCompanies).toHaveBeenCalledTimes(1); - expect(issueThreadInteractionServiceMock.sweepMergedPullRequestConfirmations).toHaveBeenCalledTimes(1); + expect(issueThreadInteractionServiceMock.mergedPullRequestConfirmationCandidates).toHaveBeenCalledTimes(1); + expect(issueThreadInteractionServiceMock.sweepMergedPullRequestConfirmations).not.toHaveBeenCalled(); + expect(deliveryWorkRegisterMock.mock.calls.some(([queue]) => queue === DELIVERY_QUEUES.mergedPullRequestConfirmations)).toBe(true); expect(executionWorkspaceServiceMock.sweepTerminalWorkspaces).toHaveBeenCalledTimes(1); expect(routineServiceMock.tickScheduledTriggers).toHaveBeenCalledTimes(1); const routineWorkerIndex = deliveryWorkRegisterMock.mock.calls.findIndex(([queue]) => queue === DELIVERY_QUEUES.routineSchedules); diff --git a/server/src/index.ts b/server/src/index.ts index ae229f03518..c1804998ef5 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -1,3 +1,4 @@ +import { registerMergeConfirmationWork } from "./services/merged-pr-confirmation-work.js"; import { registerProviderTraceRetentionWork } from "./services/provider-trace-retention-work.js"; import { registerRunEvidenceMigrationWork } from "./services/run-evidence-migration-work.js"; import { registerSkillVersionRetentionWork } from "./services/skill-version-retention-work.js"; @@ -1600,19 +1601,8 @@ async function startServerWithDatabaseTeardown( : config.workspaceCompactionDelayHours * 60 * 60 * 1000, terminalWorkspaceInspectionBackoffMs: TERMINAL_WORKSPACE_RETRY_MS, }); - const scheduleMergedPullRequestConfirmationSweep = () => { - if (isIdleTaskDrainActive() || heartbeatSchedulerStopped) return; - trackHeartbeatSchedulerWork(mergedPullRequestConfirmations - .sweepMergedPullRequestConfirmations() - .then((result) => { - if (result.accepted > 0) { - logger.info(result, "accepted merge confirmations for merged pull requests"); - } - }) - .catch((err) => { - logger.error({ err }, "merged pull-request confirmation sweep failed"); - })); - }; + await registerMergeConfirmationWork(deliveryWork, mergedPullRequestConfirmations, + () => !isIdleTaskDrainActive() && !heartbeatSchedulerStopped && !isWarmStandby()).ready; const gitRestorePointWorker = registerGitRestorePointWork(deliveryWork, gitRestorePointRetentionService(db as any, { storage: storageService, retentionDays: config.gitRestorePointRetentionDays, @@ -2005,7 +1995,6 @@ async function startServerWithDatabaseTeardown( scheduleExternalObjectRefreshSweep(new Date()); if (isIdleTaskDrainActive() || heartbeatSchedulerStopped) return; - scheduleMergedPullRequestConfirmationSweep(); scheduleGitHubConnectionEventPoll(); scheduleGitHubConnectionContinuitySweep(); diff --git a/server/src/services/chat-confirmation-notifications.ts b/server/src/services/chat-confirmation-notifications.ts index e55d8c0d7ac..02cdfbee8c8 100644 --- a/server/src/services/chat-confirmation-notifications.ts +++ b/server/src/services/chat-confirmation-notifications.ts @@ -1,3 +1,4 @@ +import { notifyMergeConfirmationWork } from "./merged-pr-confirmation-notifications.js"; import { DELIVERY_QUEUES, notifyDeliveryWork } from "./delivery-work-notifications.js"; export type ChatConfirmationScope = { companyId: string; issueId?: string }; @@ -10,6 +11,7 @@ export async function notifyChatConfirmationWork(tx: object, scope: ChatConfirma /** Interaction resolution can release a callback parked while its human input * was pending. Register on the owning transaction before its resolution write. */ export async function notifyChatConfirmationResolution(tx: object, interaction: ChatConfirmationScope & { kind: string }) { + if (interaction.kind === "request_confirmation") await notifyMergeConfirmationWork(tx, interaction); if (["request_confirmation", "request_checkbox_confirmation", "suggest_tasks", "connection_intent"].includes(interaction.kind)) { await notifyChatConfirmationWork(tx, interaction); } diff --git a/server/src/services/delivery-work-notifications.ts b/server/src/services/delivery-work-notifications.ts index 9195f1fcb12..308a721cbc5 100644 --- a/server/src/services/delivery-work-notifications.ts +++ b/server/src/services/delivery-work-notifications.ts @@ -97,6 +97,7 @@ export const DELIVERY_QUEUES = { browser: "browser-use", email: "email-channels", museMaintenance: "muse-maintenance", + mergedPullRequestConfirmations: "merged-pull-request-confirmations", mcpEvents: "mcp-events", dotEvents: "dot-events", grokDispatch: "grok-dispatch", diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 59c14618342..b306f637e27 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -1,3 +1,4 @@ +import { notifyMergeConfirmationWork, type MergeConfirmationScope } from "./merged-pr-confirmation-notifications.js"; import { notifyNativeStatusReassessment } from "./native-status-reassessment-notifications.js"; import { notifyIssueReviewState } from "./issue-review-state-notifications.js"; import { notifyChatConfirmationResolution } from "./chat-confirmation-notifications.js"; @@ -501,6 +502,7 @@ type IssueResolutionContext = { companyId: string; status: string; executionRunId: string | null; + hiddenAt?: Date | null; workMode: string; assigneeAgentId: string | null; assigneeUserId: string | null; @@ -2074,6 +2076,32 @@ export function issueThreadInteractionService( ); return states; } + async function mergedPullRequestConfirmationCandidates(scope?: MergeConfirmationScope) { + const text = sql`concat_ws(' ', ${issueThreadInteractions.title}, ${issueThreadInteractions.summary}, + ${issueThreadInteractions.payload}->>'prompt', ${issueThreadInteractions.payload}->>'acceptLabel', + ${issueThreadInteractions.payload}->>'detailsMarkdown', ${issueThreadInteractions.payload}->'target'->>'key', + ${issueThreadInteractions.payload}->'target'->>'label', ${issueThreadInteractions.payload}->'target'->>'href')`; + const rows = await db.select({ interaction: issueThreadInteractions, issue: { + id: issues.id, companyId: issues.companyId, projectId: issues.projectId, goalId: issues.goalId, + status: issues.status, assigneeAgentId: issues.assigneeAgentId, + } }).from(issueThreadInteractions).innerJoin(issues, and( + eq(issueThreadInteractions.issueId, issues.id), eq(issueThreadInteractions.companyId, issues.companyId), + )).where(and( + eq(issueThreadInteractions.kind, "request_confirmation"), eq(issueThreadInteractions.status, "pending"), + isNull(issues.hiddenAt), sql`${issues.status} NOT IN ('done', 'cancelled')`, + scope ? eq(issueThreadInteractions.companyId, scope.companyId) : undefined, + scope?.issueId ? eq(issueThreadInteractions.issueId, scope.issueId) : undefined, + scope?.interactionId ? eq(issueThreadInteractions.id, scope.interactionId) : undefined, + sql`${text} ~* '(merge|merging)'`, + sql`${text} ~* '(github\\.com/[^ /]+/[^ /]+/pull/[1-9][0-9]*|[a-z0-9_.-]+/[a-z0-9_.-]+#[1-9][0-9]*)'`, + sql`NOT EXISTS (SELECT 1 FROM ${toolActionRequests} WHERE ${toolActionRequests.interactionId} = ${issueThreadInteractions.id})`, + )); + return rows.flatMap(row => { + const references = getMergeConfirmationPullRequestReferences(row.interaction); + return references.length ? [{ ...row, references }] : []; + }); + } + async function getIdempotentInteraction(args: { issueId: string; companyId: string; @@ -2271,6 +2299,7 @@ export function issueThreadInteractionService( companyId: issues.companyId, status: issues.status, executionRunId: issues.executionRunId, + hiddenAt: issues.hiddenAt, workMode: issues.workMode, assigneeAgentId: issues.assigneeAgentId, assigneeUserId: issues.assigneeUserId, @@ -2309,6 +2338,10 @@ export function issueThreadInteractionService( "Interaction has already been resolved", ); } + if (args.actor.systemId === "system:pr-merged" && + (issueContext.hiddenAt || getMergeConfirmationPullRequestReferences(lockedCurrent).length === 0)) { + throw conflict("Merge confirmation is no longer actionable"); + } await assertRequestConfirmationResolutionAllowedUnderLock( tx as unknown as Db, issueContext, @@ -2979,6 +3012,7 @@ export function issueThreadInteractionService( await emitInteractionResolvedTelemetry(db, interaction); return interaction; }, + mergedPullRequestConfirmationCandidates, sweepMergedPullRequestConfirmations: async ( mergedHints: Array<{ companyId: string; @@ -2986,51 +3020,17 @@ export function issueThreadInteractionService( repo: string; number: number; }> = [], + scope?: MergeConfirmationScope, + signal?: AbortSignal, ) => { - const rows = await db - .select({ - interaction: issueThreadInteractions, - issue: { - id: issues.id, - companyId: issues.companyId, - projectId: issues.projectId, - goalId: issues.goalId, - status: issues.status, - assigneeAgentId: issues.assigneeAgentId, - }, - }) - .from(issueThreadInteractions) - .innerJoin(issues, eq(issueThreadInteractions.issueId, issues.id)) - .where( - and( - eq(issueThreadInteractions.kind, "request_confirmation"), - eq(issueThreadInteractions.status, "pending"), - ), - ); - - const candidates = rows.flatMap((row) => { - const references = getMergeConfirmationPullRequestReferences( - row.interaction, - ); - return references.length > 0 ? [{ ...row, references }] : []; - }); - if (candidates.length === 0) { - return { checked: rows.length, candidates: 0, accepted: 0, woken: 0 }; - } - - const candidateIds = candidates.map(({ interaction }) => interaction.id); - const linkedToolActions = await db - .select({ interactionId: toolActionRequests.interactionId }) - .from(toolActionRequests) - .where(inArray(toolActionRequests.interactionId, candidateIds)); - const toolActionInteractionIds = new Set( - linkedToolActions - .map((row) => row.interactionId) - .filter((value): value is string => Boolean(value)), - ); - const eligible = candidates.filter( - ({ interaction }) => !toolActionInteractionIds.has(interaction.id), - ); + const candidates = await mergedPullRequestConfirmationCandidates(scope ?? + (mergedHints.length === 1 ? { companyId: mergedHints[0]!.companyId } : undefined)); + const eligible = mergedHints.length ? candidates.filter(candidate => mergedHints.some(hint => + hint.companyId === candidate.issue.companyId && candidate.references.some(reference => + hint.owner.toLowerCase() === reference.owner.toLowerCase() && + hint.repo.toLowerCase() === reference.repo.toLowerCase() && hint.number === reference.number), + )) : candidates; + if (eligible.length === 0) return { checked: 0, candidates: 0, accepted: 0, woken: 0 }; const checkedAt = now().getTime(); const cacheTtlMs = opts.pullRequestCacheTtlMs ?? 5 * 60 * 1000; @@ -3065,6 +3065,9 @@ export function issueThreadInteractionService( const refreshedStates = await resolvePullRequestStates([ ...uniqueReferences.values(), ]); + if (scope && [...refreshedStates.values()].some(state => state === "unknown")) { + throw new Error("Merge confirmation provider state is unavailable"); + } for (const [key, state] of refreshedStates) { setBoundedPullRequestCacheEntry(pullRequestStateCache, key, { state, @@ -3075,6 +3078,7 @@ export function issueThreadInteractionService( let accepted = 0; let woken = 0; for (const candidate of eligible) { + if (signal?.aborted) break; const allMerged = candidate.references.every((reference) => { const key = `${candidate.issue.companyId}:${reference.owner.toLowerCase()}/${reference.repo.toLowerCase()}#${reference.number}`; return pullRequestStateCache.get(key)?.state === "merged"; @@ -3111,12 +3115,16 @@ export function issueThreadInteractionService( if (resolved.interaction.status !== "accepted") continue; accepted += 1; - const wakeIssue = resolved.continuationIssue ?? candidate.issue; + const wakeIssue = resolved.continuationIssue ?? await db.select({ + id: issues.id, assigneeAgentId: issues.assigneeAgentId, status: issues.status, + }).from(issues).where(and(eq(issues.id, candidate.issue.id), + eq(issues.companyId, candidate.issue.companyId), isNull(issues.hiddenAt))).then(rows => rows[0]); const shouldWake = resolved.interaction.continuationPolicy === "wake_assignee" || resolved.interaction.continuationPolicy === "wake_assignee_on_accept"; if ( !opts.wakeup || + !wakeIssue || !shouldWake || !wakeIssue.assigneeAgentId || isTerminalIssueStatus(wakeIssue.status) @@ -3158,7 +3166,7 @@ export function issueThreadInteractionService( } return { - checked: rows.length, + checked: candidates.length, candidates: eligible.length, accepted, woken, @@ -3909,6 +3917,7 @@ export function issueThreadInteractionService( } else { await notifyNativeStatusReassessment(tx, { companyId: issue.companyId, issueId: issue.id }); } + if (data.kind === "request_confirmation") await notifyMergeConfirmationWork(tx, { companyId: issue.companyId, issueId: issue.id }); const [row] = await tx .insert(issueThreadInteractions) .values({ diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 42c65bb5d02..4178dc1a35c 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -1,3 +1,4 @@ +import { notifyMergeConfirmationWork } from "./merged-pr-confirmation-notifications.js"; import { notifyExternalConversationSettlement } from "./external-conversation-settlement-notifications.js"; import { notifyNativeRecoveryNotices } from "./native-recovery-notice-notifications.js"; import { notifyNativeRunProjection } from "./native-run-projection-notifications.js"; @@ -11491,6 +11492,9 @@ export function issueService(db: Db) { || issueData.parentId !== undefined || issueData.executionWorkspaceId !== undefined) { await notifyTerminalWorkspaceWork(tx, `issue:${id}`); } + if (patch.status !== undefined || patch.hiddenAt !== undefined) { + await notifyMergeConfirmationWork(tx, { companyId: receiptExisting.companyId, issueId: id }); + } if (patch.status !== undefined || patch.assigneeAgentId !== undefined || patch.assigneeUserId !== undefined || patch.hiddenAt !== undefined || patch.conversationAgentId !== undefined) { await notifyDeliveryWork(tx, DELIVERY_QUEUES.agentTimers); diff --git a/server/src/services/merged-pr-confirmation-notifications.ts b/server/src/services/merged-pr-confirmation-notifications.ts new file mode 100644 index 00000000000..34df4ecffb0 --- /dev/null +++ b/server/src/services/merged-pr-confirmation-notifications.ts @@ -0,0 +1,7 @@ +import { DELIVERY_QUEUES, notifyDeliveryWork } from "./delivery-work-notifications.js"; + +export type MergeConfirmationScope = { companyId: string; issueId?: string; interactionId?: string }; + +export function notifyMergeConfirmationWork(tx: object, scope: MergeConfirmationScope) { + return notifyDeliveryWork(tx, DELIVERY_QUEUES.mergedPullRequestConfirmations, JSON.stringify({ companyId: scope.companyId, issueId: scope.issueId, interactionId: scope.interactionId })); +} diff --git a/server/src/services/merged-pr-confirmation-work.ts b/server/src/services/merged-pr-confirmation-work.ts new file mode 100644 index 00000000000..bfccaeea79f --- /dev/null +++ b/server/src/services/merged-pr-confirmation-work.ts @@ -0,0 +1,30 @@ +import type { createDeliveryWorkCoordinator } from "./delivery-work-coordinator.js"; +import { DELIVERY_QUEUES } from "./delivery-work-notifications.js"; +import type { issueThreadInteractionService } from "./issue-thread-interactions.js"; +import type { MergeConfirmationScope } from "./merged-pr-confirmation-notifications.js"; +import { registerScopedDeadlineWork } from "./scoped-deadline-work.js"; + +export const MERGE_CONFIRMATION_RECONCILE_MS = 5 * 60_000; + +export function registerMergeConfirmationWork( + coordinator: ReturnType, + service: Pick, + "mergedPullRequestConfirmationCandidates" | "sweepMergedPullRequestConfirmations">, + canRun: () => boolean, +) { + return registerScopedDeadlineWork(coordinator, { + queue: DELIVERY_QUEUES.mergedPullRequestConfirmations, + decodeScope: hint => hint === "*" ? undefined : JSON.parse(hint) as MergeConfirmationScope, + matches: (entry, scope?: MergeConfirmationScope) => !scope || (entry.companyId === scope.companyId && + (!scope.issueId || entry.issueId === scope.issueId) && (!scope.interactionId || entry.id === scope.interactionId)), + load: async (scope?: MergeConfirmationScope) => (await service.mergedPullRequestConfirmationCandidates(scope)) + .map(({ interaction }) => ({ id: interaction.id, companyId: interaction.companyId, issueId: interaction.issueId, dueAt: Date.now() })), + check: async (entry, signal) => { + const scope = { companyId: entry.companyId, issueId: entry.issueId, interactionId: entry.id }; + await service.sweepMergedPullRequestConfirmations([], scope, signal); + if (signal?.aborted) return entry; + const remaining = await service.mergedPullRequestConfirmationCandidates(scope); + return remaining.length ? { ...entry, dueAt: Date.now() + MERGE_CONFIRMATION_RECONCILE_MS } : null; + }, + }, canRun); +}