diff --git a/doc/connections/GITHUB.md b/doc/connections/GITHUB.md index a374f8616c8..84d952e9b00 100644 --- a/doc/connections/GITHUB.md +++ b/doc/connections/GITHUB.md @@ -241,7 +241,7 @@ recovery of the existing App instead of a second creation. Direct webhook and manual existing-App recovery remain available. Connecting does not prove that an agent runtime can execute a review. -### One editable response per request +### One main response per pull request A pull request task can retain both its main discussion and inline finding threads. Bot tool discovery lists each endpoint/catalog tool once across those @@ -249,10 +249,13 @@ threads. Publishing an inline finding therefore does not duplicate the tool catalog on the task's next run. Both conversation mappings remain available for authorized follow-up replies. -After accepting authorized GitHub work, Paperclip posts **Working on this…** -before starting the agent. Issue and PR discussion comments use the same GitHub -comment API; inline-review conversations receive an inline reply. Description -mentions and automatic tasks also receive a working comment. No new eyes +The main PR conversation shares one App-owned comment across pushes, mentions, +and repeat reviews. A new request updates that comment to **Review queued…**; +agent progress and the final assessment update the same comment. Its identity +survives restarts and reconnects. Existing request comments can be adopted without +creating another main comment. Issue conversations and inline-review threads +retain one editable response per request. Description mentions and automatic +tasks also receive an acknowledgement. No new eyes reaction is added; cleanup still handles receipts created by older versions. The agent's `update_comment` tool edits the current request's working comment. @@ -275,7 +278,11 @@ App-owned markers recover uncertain creation without posting duplicates. Deleted or no-longer-owned comments are not recreated or edited. Coalesced runs settle a deleted earlier working comment without blocking the current response, review findings, or check publication. Failed runs -update the same comment when no final reply is confirmed or unresolved. A run +update the same comment when no final reply is confirmed or unresolved. Maintenance +also reconciles acknowledged requests when recovery stops before a publication +is staged. Requests saved on a blocked task report that work is paused and stay +open for resumed progress or stopped-run cleanup. An older +run cannot replace a newer run's response. A run that ends without a final reply clears a remaining working state honestly. Repository restrictions, person authorization and governed tool checks remain in effect for edits. Existing disabled or quarantined tools stay disabled. @@ -290,14 +297,17 @@ Private images use the exact unchanged source's authenticated GitHub rendering; repository/thread identity and body hashes remain bound across restarts. Signed image URLs and credentials are never added to durable attachment descriptors. -A manual message naming another connected GitHub bot in the same company does -not wake this bot through its thread subscription. Paperclip filters that +A manual message beginning with another recipient's @mention does not wake this +bot through its thread subscription, including Apps outside Paperclip such as +`@greptile review`. Mentions of other connected GitHub bots are also filtered +elsewhere in the body. Paperclip filters that delivery before creating task work or adding an acknowledgement. Explicitly mentioning both bots allows both to receive the request, subject to their normal authorization and repository checks. -Unaddressed follow-ups, human mentions and unknown handles retain the existing -subscription behavior. The routing rule does not change automatic issue or PR +Unaddressed follow-ups and incidental human or unknown mentions retain the existing +subscription behavior. Explicitly mentioning this bot admits a request even when +another recipient is named. The routing rule does not change automatic issue or PR event policies. Archived bots and identities from other companies or providers are excluded from the routing lookup. diff --git a/doc/connections/UNDERSTANDING-GITHUB-PR-REVIEW-BOTS.md b/doc/connections/UNDERSTANDING-GITHUB-PR-REVIEW-BOTS.md index 0f4a1ec976d..9ee6800f8e9 100644 --- a/doc/connections/UNDERSTANDING-GITHUB-PR-REVIEW-BOTS.md +++ b/doc/connections/UNDERSTANDING-GITHUB-PR-REVIEW-BOTS.md @@ -255,16 +255,20 @@ discussion, it uses `comment`. For a review, `submit_review` publishes the assessment summary and updates the check. It should not add another comment just to announce that the review is complete. -Paperclip posts one “Working on this…” comment when it accepts a request. The -agent can periodically edit that comment with `update_comment` as it makes +Paperclip keeps one main comment per bot per PR, beginning with “Review queued…”. +Pushes and repeat requests reuse it. The agent can periodically edit it with `update_comment` as it makes progress. Its final `comment` or review summary replaces the same comment; -inline-only reviews close it with a link to the finding. Coalesced requests have -their existing working comments updated to point to the response. New requests +inline-only reviews close it with a link to the finding. Coalesced PR requests +share the main comment; issue and inline threads keep per-request responses. New requests no longer create eyes reactions. The runner's final text stays inside the Paperclip task. If a run ends without a tool reply, Paperclip updates the working comment with a safe notice and a link to the task. A real question can link to its answer form in Paperclip. +Maintenance clears stale working states after recovery stops, and requests saved +while the task is blocked show a paused notice. A leading mention addressed to +another recipient, such as `@greptile review`, does not wake this bot unless the +message also explicitly mentions it. A confirmed reply suppresses fallback notices. Pending or uncertain tool delivery prevents a duplicate reply while its receipt is resolved. A check alone does not count as a conversation reply. diff --git a/maps/agent-access-map/chat-channels.md b/maps/agent-access-map/chat-channels.md index 68cb515a5b1..2786b70922e 100644 --- a/maps/agent-access-map/chat-channels.md +++ b/maps/agent-access-map/chat-channels.md @@ -110,6 +110,16 @@ not task completion. Source: PostgreSQL-backed provider fixtures; this entry does not claim live provider qualification for the new retry path. +GitHub main-response identity (2026-10-19): PR requests share a company-, App-, +repository- and PR-bound comment. Agents still supply no comment destination. +Current sender, assignee, repository and publication authority are checked before +edits, including maintenance failure notices. Leading mentions for another +recipient are filtered before task work; explicit mentions of this bot still +follow normal access checks. Provider-fixture coverage is not live GitHub proof. +Native terminal reconciliation resolves the accepted run identity through the +existing chat provenance resolver; an older run's reply cannot complete a newer +request merely because both requests share the same PR comment. + Speko browser sessions use `POST /api/companies/{companyId}/voice-sessions`, with GET inspection/notification and POST `/{sessionId}/end` operations below that collection. These routes require board identity; ordinary agent credentials diff --git a/maps/feature-map/chat-channels.md b/maps/feature-map/chat-channels.md index 82a0ee9b166..a030f504689 100644 --- a/maps/feature-map/chat-channels.md +++ b/maps/feature-map/chat-channels.md @@ -40,6 +40,16 @@ Setup is covered in [connection setup](./connection-setup.md). Automated: [channel integration](../../server/src/__tests__/chat-channels.integration.test.ts) uses provider fixtures; it does not prove a live Slack, Discord, Teams, Telegram, GitHub, email, or Photon delivery. +GitHub review comment regression (2026-10-19): repeated PR review requests reuse +one main App comment, including across restart and reconnect. Test a leading +request for an external bot (`@greptile review`) and verify it creates no wake or +acknowledgement. Stop execution before a milestone is staged, run maintenance, +and verify the existing comment reports the stop; saved requests on a blocked +task show paused state. Preserve newer progress and confirmed or uncertain replies. +Repeat terminal reconciliation with native identity and no legacy wake fields. +Check delayed acknowledgements, check-only assessments followed by formal reviews, +and combined requests with separate pre-upgrade placeholders. + Manual: For each supported provider being changed, send a unique message, verify the intended agent/company receives it, and inspect the actual external reply in the original thread. Repeat with an unlinked/unauthorized sender, duplicate inbound delivery, and one supported attachment. Record provider-specific results separately. For Slack, enable **Require at-mention** and **Allow direct messages**. Send a diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 4a6af5b4cb5..66baee1c4e4 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2480,7 +2480,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { await send(); await send(); const rows = await db.select().from(chatDeliveries).where(and(eq(chatDeliveries.endpointId, f.endpoint.id), eq(chatDeliveries.providerEventId, `${threadId}:41902`))); expect(rows).toHaveLength(1); - expect(rows[0]).toMatchObject({ state: "filtered", principalId: null, redactedError: "GitHub message explicitly mentions a different connected bot" }); + expect(rows[0]).toMatchObject({ state: "filtered", principalId: null, redactedError: "GitHub message explicitly addresses a different recipient" }); expect(rows[0].normalizedEvent).toHaveProperty("filtering.contentRetained", false); expect(JSON.stringify(rows[0].normalizedEvent)).not.toContain("tell me a joke"); expect(await readIssueCommentsByIssueId(f.conversation.issueId!)).toHaveLength(1); @@ -2488,11 +2488,11 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { expect(f.runtime.endpoints.get(f.endpoint.id)?.reactions).toEqual([]); expect(f.responseWrites).toHaveLength(1); expect(await f.service.listActivity(f.endpoint.id)).toEqual(expect.arrayContaining([ - expect.objectContaining({ status: "filtered", detail: "GitHub message explicitly mentions a different connected bot" }), + expect.objectContaining({ status: "filtered", detail: "GitHub message explicitly addresses a different recipient" }), ])); }); - it.each(["Thanks, continue", "@octocat thanks", "@unknown-bot help", "@gonzo-other help", "x@gonzo help"])( - "retains subscribed GitHub follow-ups without another exact connected bot mention: %s", async text => { + it.each(["Thanks, continue", "Thanks @octocat", "Please ask @unknown-bot for context", "Check @gonzo-other's advice", "x@gonzo help"])( + "retains subscribed GitHub follow-ups with incidental human or unknown mentions: %s", async text => { const f = await subscribedGitHubFixture(); await addPeerGitHubBot(f); const wakeCount = f.wakeup.mock.calls.length; @@ -2518,7 +2518,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { await db.update(chatEndpoints).set({ botUsername: "gonzo[bot]", status: kind === "archived" ? "archived" : "active" }).where(eq(chatEndpoints.id, peer.id)); const wakeCount = f.wakeup.mock.calls.length; await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread: f.thread, - trigger: "subscribed_message", message: makeMessage({ id: "41902", text: "@gonzo help", userId: "42", userName: "octocat" }) }); + trigger: "subscribed_message", message: makeMessage({ id: "41902", text: "Please consider @gonzo's advice", userId: "42", userName: "octocat" }) }); expect(await readIssueCommentsByIssueId(f.conversation.issueId!)).toHaveLength(2); expect(f.wakeup).toHaveBeenCalledTimes(wakeCount + 1); }); @@ -2599,6 +2599,267 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { )))[0]; return { ...f, service, head, assessment, provider, action, inline, checks, writes }; } + it("reuses one main GitHub PR comment across review requests", async () => { + const f = await publicationRetryFixture(); + await f.service.execute(f.session, "submit_review", f.assessment, "first-review"); + const originalId = [...f.responseComments.keys()][0]; + // Restart after a provider write whose shared receipt was lost. The + // App-owned marker, rather than process memory, must recover the target. + await db.update(chatActions).set({ result: null }).where(and(eq(chatActions.endpointId, f.endpoint.id), + eq(chatActions.kind, "github_pr_response_comment"))); + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, f.run.id)); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + await githubResponseCommentService(db, f.providerFetch).processPending(); + const [next] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.assignedAgentId, + status: "running", runtimeMode: "native", contextSnapshot: await chatWakeContext({ + endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", + }) }).returning(); + const session = { ...f.session, runId: next.id }; + await f.service.execute(session, "begin_review", { reviewedCommit: f.head }); + await f.service.execute(session, "submit_review", { ...f.assessment, summary: "Second assessment" }, "second-review"); + expect([...f.responseComments.keys()]).toEqual([originalId]); + expect(f.responseComments.get(originalId)?.body).toContain("Second assessment"); + expect(f.responseWrites.filter(write => write.method === "POST")).toHaveLength(1); + }); + it("ignores a review request addressed to an external GitHub bot", async () => { + const f = await subscribedGitHubFixture("github:paperclipai/paperclip:419"); + const wakeCount = f.wakeup.mock.calls.length; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread: f.thread, + trigger: "subscribed_message", message: makeMessage({ id: "41902", text: "@greptile review", userId: "42", userName: "octocat" }) }); + const [delivery] = await db.select().from(chatDeliveries).where(and(eq(chatDeliveries.endpointId, f.endpoint.id), + eq(chatDeliveries.providerEventId, `${f.thread.id}:41902`))); + expect(delivery.state).toBe("filtered"); + expect(f.wakeup).toHaveBeenCalledTimes(wakeCount); + expect(await readIssueCommentsByIssueId(f.conversation.issueId!)).toHaveLength(1); + expect(f.responseWrites).toHaveLength(1); + }); + it("retains a provider-verified GitHub mention when its plain text uses an alias", async () => { + const f = await subscribedGitHubFixture("github:paperclipai/paperclip:419"); + const wakeCount = f.wakeup.mock.calls.length; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread: f.thread, + trigger: "mention", message: makeMessage({ id: "41902", text: "@rendered-alias review", userId: "42", userName: "octocat", mentioned: true }) }); + expect(f.wakeup).toHaveBeenCalledTimes(wakeCount + 1); + expect(await readIssueCommentsByIssueId(f.conversation.issueId!)).toHaveLength(2); + }); + it("reconciles a GitHub working comment when execution stops without a publication", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + await db.update(heartbeatRuns).set({ status: "failed", errorCode: "execution_finalization_deadline_exceeded" }) + .where(eq(heartbeatRuns.id, f.run.id)); + await db.update(issues).set({ status: "blocked" }).where(eq(issues.id, f.conversation.issueId!)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect(f.responseComments.size).toBe(1); + expect([...f.responseComments.values()][0].body).toContain("stopped before completing"); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect(f.responseComments.size).toBe(1); + expect([...f.responseComments.values()][0].body).toContain("paused"); + }); + it("preserves newer GitHub PR progress when an older run fails", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + await db.update(heartbeatRuns).set({ status: "failed" }).where(eq(heartbeatRuns.id, f.run.id)); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + const [next] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.assignedAgentId, + status: "running", runtimeMode: "native", contextSnapshot: await chatWakeContext({ + endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", + }) }).returning(); + await githubChatReviewService(db, f.providerFetch).execute({ ...f.session, runId: next.id }, "update_comment", + { body: "Reviewing the new request", idempotencyKey: "new-progress" }); + await expect(githubResponseCommentService(db, f.providerFetch).failRun({ ...f.session, endpointId: f.endpoint.id, + body: "Old run failed" })).rejects.toMatchObject({ status: 403 }); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect(f.responseComments.size).toBe(1); + expect([...f.responseComments.values()][0].body).toContain("Reviewing the new request"); + }); + it("adopts an existing GitHub PR placeholder and retires earlier duplicate placeholders", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + const original = [...f.responseComments.values()][0]; + original.body = original.body.replace(/\n/, ""); + await db.delete(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_pr_response_comment"))); + // Reconcile a pre-upgrade acknowledgement before its run completes. + const [first] = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_response_comment"))); + await db.update(chatActions).set({ status: "received" }).where(eq(chatActions.id, first.id)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect(f.responseComments.size).toBe(1); + expect(original.body).toContain("paperclip-pr-response:"); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + const [second] = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), + eq(chatActions.kind, "github_response_comment"), sql`${chatActions.id} <> ${first.id}`)); + // Simulate the second request's persisted pre-upgrade provider receipt. + const duplicateId = 799999; + f.responseComments.set(duplicateId, { ...original, id: duplicateId, + body: original.body.replace(/\n/, ""), + html_url: `${original.html_url}-legacy`, + }); + await db.update(chatActions).set({ status: "received", result: { ...second.result, id: String(duplicateId) } }).where(eq(chatActions.id, second.id)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect(f.responseComments.get(duplicateId)?.body).toContain("current review"); + expect(f.responseComments.get(duplicateId)?.body).not.toContain("queued"); + expect(f.responseWrites.filter(write => write.method === "POST")).toHaveLength(1); + await githubChatReviewService(db, f.providerFetch).execute(f.session, "comment", { body: "Final result", idempotencyKey: "legacy-final" }); + expect(original.body).toContain("Final result"); + }); + it("keeps a newer GitHub request open when its acknowledgement follows older progress", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + const [link] = await db.select().from(chatMessageLinks).where(and(eq(chatMessageLinks.endpointId, f.endpoint.id), eq(chatMessageLinks.providerMessageId, "41802"))); + await db.update(chatActions).set({ status: "received", result: null }).where(and(eq(chatActions.deliveryId, link.deliveryId!), eq(chatActions.kind, "github_response_comment"))); + await githubChatReviewService(db, f.providerFetch).execute(f.session, "update_comment", { body: "Older run progress", idempotencyKey: "old-progress" }); + await githubResponseCommentService(db, f.providerFetch).tryAcknowledge(link.deliveryId!); + const [response] = await db.select().from(chatActions).where(and(eq(chatActions.deliveryId, link.deliveryId!), eq(chatActions.kind, "github_response_comment"))); + expect(response.result?.final).toBe(false); + const [next] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.assignedAgentId, + status: "running", runtimeMode: "native", contextSnapshot: await chatWakeContext({ + endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", + }) }).returning(); + const result = await githubChatReviewService(db, f.providerFetch).execute({ ...f.session, runId: next.id }, "update_comment", + { body: "Newer request progress", idempotencyKey: "new-progress" }); + expect(result.status).toBe("processed"); + expect([...f.responseComments.values()][0].body).toContain("Newer request progress"); + }); + it.each([false, true])("closes a GitHub placeholder after a formal review (legacy receipt: %s)", async legacy => { + const f = await publicationRetryFixture(); + const formal = { id: 991, html_url: "https://github.com/paperclipai/paperclip/pull/418#pullrequestreview-991" }; + if (legacy) { + await db.insert(chatActions).values({ companyId: f.companyId, endpointId: f.endpoint.id, conversationId: f.conversation.id, + kind: "github_review_publication", providerActionId: randomUUID(), status: "processed", + payload: { operation: "formal_review", session: f.session }, result: { id: String(formal.id), url: formal.html_url } }); + } else { + const config = await f.management.configuration(f.endpoint.id, "owner-user"); + await f.management.saveConfiguration(f.endpoint.id, { expectedRevision: config.revision, + configuration: { ...config.configuration, defaults: { ...config.configuration.defaults, + allowApprove: true, publishSummary: false, publishInline: false } } }, "owner-user"); + f.setSupplementalProviderFetch(async (input, init) => String(input).includes("/pulls/418/reviews") + ? Response.json(init?.method === "POST" ? formal : []) : f.provider(input, init)); + await f.service.execute(f.session, "submit_review", f.assessment, "score-only"); + await f.service.execute(f.session, "formal_review", { event: "APPROVE", reviewedCommit: f.head, body: "Reviewed", idempotencyKey: "formal-only" }); + } + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, f.run.id)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + const body = [...f.responseComments.values()][0].body; + expect(body).not.toMatch(/Review queued|Working on this/); + expect(body).toContain("paperclip-response-state:final"); + }); + it("retires a legacy coalesced GitHub placeholder before replacing its receipt", async () => { + const f = await publicationRetryFixture(); + const original = { ...[...f.responseComments.values()][0] }; + const [first] = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_response_comment"))); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + const duplicateId = 799999; + f.responseComments.set(duplicateId, { ...original, id: duplicateId, + body: original.body.replace(/\n/, ""), + html_url: "https://github.com/paperclipai/paperclip/pull/418#issuecomment-799999" }); + await db.update(chatActions).set({ result: { ...first.result, id: String(duplicateId) } }).where(eq(chatActions.id, first.id)); + const links = await db.select().from(chatMessageLinks).where(and(eq(chatMessageLinks.endpointId, f.endpoint.id), eq(chatMessageLinks.direction, "inbound"))); + const latest = await chatWakeContext({ endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802" }); + await db.update(heartbeatRuns).set({ contextSnapshot: { ...latest, wakeCommentIds: links.map(link => link.commentId) } }).where(eq(heartbeatRuns.id, f.run.id)); + await f.service.execute(f.session, "begin_review", { reviewedCommit: f.head }); + await f.service.execute(f.session, "submit_review", f.assessment, "coalesced-upgrade"); + expect(f.responseComments.get(duplicateId)?.body).toContain("current review"); + expect(f.responseComments.get(duplicateId)?.body).not.toMatch(/Review queued|Working on this/); + expect([...f.responseComments.values()].some(comment => comment.body.includes("Advisory review"))).toBe(true); + }); + it("does not handle a blocked new GitHub request with an older run's final reply", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + await githubChatReviewService(db, f.providerFetch).execute(f.session, "comment", { body: "Earlier result", idempotencyKey: "earlier-result" }); + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, f.run.id)); + await db.update(issues).set({ status: "blocked" }).where(eq(issues.id, f.conversation.issueId!)); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "New request", userId: "42", userName: "octocat", mentioned: true }) }); + await githubResponseCommentService(db, f.providerFetch).processPending(); + const body = [...f.responseComments.values()][0].body; + expect(body).toContain("paused"); + expect(body).not.toContain("has been handled"); + expect(f.responseComments.size).toBe(1); + }); + it.each([["failed", false], ["failed", true], ["queued", false], ["queued", true]] as const)( + "resumes a paused GitHub request and reconciles stopped execution (prior run: %s, progress: %s)", async (status, progress) => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + await db.update(heartbeatRuns).set({ status }).where(eq(heartbeatRuns.id, f.run.id)); + await db.update(issues).set({ status: "blocked" }).where(eq(issues.id, f.conversation.issueId!)); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "New request", userId: "42", userName: "octocat", mentioned: true }) }); + const responses = githubResponseCommentService(db, f.providerFetch); + await responses.processPending(); + expect([...f.responseComments.values()][0].body).toContain("paused"); + const pausedWrites = f.responseWrites.length; + await responses.processPending(); + await responses.processPending(); + expect(f.responseWrites).toHaveLength(pausedWrites); + await db.update(issues).set({ status: "in_progress" }).where(eq(issues.id, f.conversation.issueId!)); + const [next] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.assignedAgentId, + status: "running", runtimeMode: "native", contextSnapshot: await chatWakeContext({ + endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", + }) }).returning(); + const session = { ...f.session, runId: next.id }; + if (progress) { + expect(await githubChatReviewService(db, f.providerFetch).execute(session, "update_comment", + { body: "Resumed request progress", idempotencyKey: "resumed-progress" })).toMatchObject({ status: "processed" }); + expect([...f.responseComments.values()][0].body).toContain("Resumed request progress"); + } + await db.update(heartbeatRuns).set({ status: "failed" }).where(eq(heartbeatRuns.id, next.id)); + await responses.failRun({ ...session, endpointId: f.endpoint.id, body: "Resumed work stopped" }); + expect(f.responseComments.size).toBe(1); + expect([...f.responseComments.values()][0].body).toContain("Resumed work stopped"); + }); + it("preserves a final GitHub assessment while settling an incomplete request receipt", async () => { + const f = await publicationRetryFixture(); + await f.service.execute(f.session, "submit_review", f.assessment, "published-assessment"); + const before = [...f.responseComments.values()][0].body; + const [response] = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_response_comment"))); + await db.update(chatActions).set({ result: { ...response.result, final: false } }).where(eq(chatActions.id, response.id)); + await db.update(heartbeatRuns).set({ status: "succeeded", updatedAt: new Date() }).where(eq(heartbeatRuns.id, f.run.id)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect([...f.responseComments.values()][0].body).toBe(before); + const [settled] = await db.select().from(chatActions).where(eq(chatActions.id, response.id)); + expect(settled.result?.final).toBe(true); + }); + it("reconciles stopped GitHub work from native identity without legacy wake fields", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + await initializeRunIdentity(db, { companyId: f.companyId, runId: f.run.id, issueId: f.conversation.issueId!, + responsibleUserId: "owner-user", messageIds: [String(f.run.contextSnapshot?.wakeCommentId)], cause: "instruction" }); + await db.update(heartbeatRuns).set({ nativeIssueId: f.conversation.issueId, contextSnapshot: {}, status: "failed", updatedAt: new Date() }) + .where(eq(heartbeatRuns.id, f.run.id)); + await db.update(issues).set({ status: "in_progress" }).where(eq(issues.id, f.conversation.issueId!)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect([...f.responseComments.values()][0].body).toContain("stopped before completing"); + expect(f.responseComments.size).toBe(1); + }); + it("retires a legacy GitHub placeholder when its acknowledgement is superseded", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + const original = { ...[...f.responseComments.values()][0] }; + const [first] = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_response_comment"))); + const duplicateId = 799999; + f.responseComments.set(duplicateId, { ...original, id: duplicateId, + body: original.body.replace(/\n/, ""), + html_url: "https://github.com/paperclipai/paperclip/pull/418#issuecomment-799999" }); + await db.update(chatActions).set({ status: "received", result: { ...first.result, id: String(duplicateId) } }).where(eq(chatActions.id, first.id)); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "mention", message: makeMessage({ id: "41802", text: "Review again", userId: "42", userName: "octocat", mentioned: true }) }); + const links = await db.select().from(chatMessageLinks).where(and(eq(chatMessageLinks.endpointId, f.endpoint.id), eq(chatMessageLinks.direction, "inbound"))); + const latest = await chatWakeContext({ endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802" }); + await db.update(heartbeatRuns).set({ contextSnapshot: { ...latest, wakeCommentIds: links.map(link => link.commentId) } }).where(eq(heartbeatRuns.id, f.run.id)); + await githubChatReviewService(db, f.providerFetch).execute(f.session, "update_comment", { body: "Newer progress", idempotencyKey: "latest-progress" }); + await githubResponseCommentService(db, f.providerFetch).tryAcknowledge(first.deliveryId!); + expect(f.responseComments.get(duplicateId)?.body).toContain("current review"); + expect([...f.responseComments.values()].some(comment => comment.body.includes("Newer progress"))).toBe(true); + const [settled] = await db.select().from(chatActions).where(eq(chatActions.id, first.id)); + expect(settled.result?.final).toBe(false); + }); it.each([false, true])("recovers an approved queued GitHub review without another approval (revoked: %s)", async revoked => { const f = await publicationRetryFixture(); let unavailable = true; @@ -3193,7 +3454,8 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const f = await toolReplyFixture(threadId); const service = githubChatReviewService(db, f.providerFetch); expect(f.responseWrites).toHaveLength(1); - expect(f.responseWrites[0]).toMatchObject({ method: "POST", body: expect.stringContaining("Working on this") }); + expect(f.responseWrites[0]).toMatchObject({ method: "POST", body: expect.stringContaining( + threadId === "github:paperclipai/paperclip:418" ? "Review queued" : "Working on this") }); expect(f.runtime.endpoints.get(f.endpoint.id)?.reactions).toEqual([]); const progress = await service.execute(f.session, "update_comment", { body: "Checking the request", idempotencyKey: "progress-1" }); expect(progress.status).toBe("processed"); @@ -3319,7 +3581,7 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, trigger: "mention", message: makeMessage({ id: "41802", text: "Review this too", userId: "42", userName: "octocat", mentioned: true }) }); - await expect.poll(async () => { await githubResponseCommentService(db, f.providerFetch).processPending(); return f.responseComments.size; }, { timeout: 10_000 }).toBe(2); + await expect.poll(async () => { await githubResponseCommentService(db, f.providerFetch).processPending(); return f.responseWrites.length; }, { timeout: 10_000 }).toBe(2); f.responseComments.delete([...f.responseComments.keys()][0]); const links = await db.select().from(chatMessageLinks).where(and(eq(chatMessageLinks.endpointId, f.endpoint.id), eq(chatMessageLinks.direction, "inbound"))); const latest = await chatWakeContext({ endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802" }); @@ -3353,13 +3615,15 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { findings: [{ key: "note", path: "src/math.ts", line: 1, side: "RIGHT", severity: "error", category: "correctness", body: "Example finding" }], }, "inline-only"); expect(inlineWrites).toHaveLength(1); - expect(f.responseWrites.map(write => write.method)).toEqual(deletedSibling ? ["POST", "POST", "PATCH"] : ["POST", "PATCH"]); - expect(f.responseComments.size).toBe(1); + expect(f.responseWrites.map(write => write.method)).toEqual(["POST", "PATCH"]); + expect(f.responseComments.size).toBe(deletedSibling ? 0 : 1); const comment = [...f.responseComments.values()][0]; - expect(comment.body).toContain("Review complete."); - expect(comment.body).toContain("https://github.com/paperclipai/paperclip/pull/418"); - expect(comment.body).not.toContain("Disabled summary"); - expect(comment.body).toContain("paperclip-response-state:final"); + if (!deletedSibling) { + expect(comment.body).toContain("Review complete."); + expect(comment.body).toContain("https://github.com/paperclipai/paperclip/pull/418"); + expect(comment.body).not.toContain("Disabled summary"); + expect(comment.body).toContain("paperclip-response-state:final"); + } await expect(githubRunReplyState(db, { ...f.session, endpointId: f.endpoint.id })).resolves.toBe("confirmed"); }); it.each(["during_summary", "after_summary"] as const)("keeps partial review delivery authoritative when the head changes %s", async phase => { diff --git a/server/src/services/chat-channels/github/response-target.ts b/server/src/services/chat-channels/github/response-target.ts new file mode 100644 index 00000000000..1e955b14482 --- /dev/null +++ b/server/src/services/chat-channels/github/response-target.ts @@ -0,0 +1,81 @@ +import { createHash } from "node:crypto"; +import { and, asc, eq } from "drizzle-orm"; +import { chatActions, chatConversations, chatDeliveries, chatEndpoints, type Db } from "@paperclipai/db"; +import { forbidden, HttpError } from "../../../errors.js"; + +export type GitHubResponseSource = { + endpoint: typeof chatEndpoints.$inferSelect; + conversation: typeof chatConversations.$inferSelect; + delivery: typeof chatDeliveries.$inferSelect; + number: number; + replyId: string | null; + repositoryId?: string; +}; + +export const isMainPullResponse = (source: GitHubResponseSource) => + !source.replyId && /^github:[^:]+:[1-9][0-9]*$/.test(source.conversation.externalThreadId); + +export const requestResponseMarker = (source: GitHubResponseSource) => ``; + +export const pullResponseMarker = (source: GitHubResponseSource) => ``; + +/** The per-request ledger remains intact; only the main PR's publication target + * is shared. Callers serialize mutations with the repository publication lease. */ +export async function stagePullResponse(db: Db, source: GitHubResponseSource) { + const key = `github_pr_response:${source.conversation.externalThreadId.toLowerCase()}`; + const [inserted] = await db.insert(chatActions).values({ + companyId: source.endpoint.companyId, endpointId: source.endpoint.id, + conversationId: source.conversation.id, kind: "github_pr_response_comment", providerActionId: key, + payload: { botExternalId: source.endpoint.botExternalId, agentId: source.endpoint.assignedAgentId }, status: "processed", + }).onConflictDoNothing().returning(); + const target = inserted ?? (await db.select().from(chatActions).where(and( + eq(chatActions.companyId, source.endpoint.companyId), eq(chatActions.endpointId, source.endpoint.id), + eq(chatActions.providerActionId, key), + )))[0]; + if (!target || target.conversationId !== source.conversation.id || + target.payload.botExternalId !== source.endpoint.botExternalId || target.payload.agentId !== source.endpoint.assignedAgentId) + throw forbidden("GitHub main response binding changed"); + return target; +} + +/** Adopt only comments identified by this conversation's durable request ledger + * or the previous PR summary marker, never an arbitrary comment by the App. */ +export async function legacyPullResponseMarkers(db: Db, source: GitHubResponseSource) { + const rows = await db.select({ deliveryId: chatActions.deliveryId }).from(chatActions).where(and( + eq(chatActions.companyId, source.endpoint.companyId), eq(chatActions.endpointId, source.endpoint.id), + eq(chatActions.conversationId, source.conversation.id), eq(chatActions.kind, "github_response_comment"), + )).orderBy(asc(chatActions.createdAt)); + // Per-request markers include the company and endpoint, and remain provable + // after a receipt was lost between the provider write and the local commit. + const summary = source.repositoryId ? [``] : []; + return [...summary, ...rows.filter(row => row.deliveryId).map(row => requestResponseMarker({ ...source, delivery: { ...source.delivery, id: row.deliveryId! } }))]; +} + +/** Retire the proven old placeholder before its only receipt is replaced. */ +export async function retireLegacyResponsePlaceholder( + source: GitHubResponseSource, + result: typeof chatActions.$inferSelect["result"], + current: { id: string; url: string }, + api: { request(path: string, options?: { method?: string; body?: unknown }): Promise }, + assertCurrent: () => Promise, +) { + if (!result?.id || String(result.id) === current.id || result.final === true) return; + if (!/^[1-9][0-9]*$/.test(String(result.id))) throw forbidden("Invalid GitHub response receipt"); + const item = source.replyId ? "/pulls/comments" : "/issues/comments"; + try { + await assertCurrent(); + const old = await api.request<{ id: number; body?: string; user?: { login?: string } }>(`${item}/${result.id}`); + if (old.user?.login !== source.endpoint.botUsername || !old.body?.includes(requestResponseMarker(source))) + throw forbidden("GitHub response is no longer owned by this App"); + await assertCurrent(); + await api.request(`${item}/${old.id}`, { method: "PATCH", body: { + body: `See the [current review](${current.url}).\n\n${requestResponseMarker(source)}\n`, + } }); + } catch (error) { + const details = error instanceof HttpError ? error.details as { providerStatus?: number } | undefined : undefined; + if (details?.providerStatus !== 404) throw error; + } +} diff --git a/server/src/services/chat-channels/inbound/admission.ts b/server/src/services/chat-channels/inbound/admission.ts index 69ea9cdac5e..34329d81557 100644 --- a/server/src/services/chat-channels/inbound/admission.ts +++ b/server/src/services/chat-channels/inbound/admission.ts @@ -9,7 +9,7 @@ import type { EndpointRow, DbTransaction, DbOrTransaction, InboundRuntimeContext import { notifyChatDeliveryWork } from "../../chat-work-notifications.js"; import { - githubMessageAddressesAnotherBot, + githubMessageAddressesAnotherRecipient, } from "../../chat-github-events.js"; import { @@ -173,15 +173,16 @@ export function createInboundAdmission(db: Db, options: InboundAdmissionOptions) message.isMention !== true; // GitHub sends the same comment to every installed App. Subscribed // conversations are a fallback for unaddressed replies, not permission - // to wake every bot when the person names a different connected App. + // to wake every bot when the person addresses a different recipient. // Recheck inside durable admission so recovered deliveries follow the // same company-scoped routing decision before any task or reaction. const githubAddressedElsewhere = endpointAccepting && endpoint.provider === "github" && + !addressed && !githubAutomatic && !githubIssue && - (await githubMessageAddressesAnotherBot(tx, { + (await githubMessageAddressesAnotherRecipient(tx, { id: endpoint.id, companyId: endpoint.companyId, botUsername: currentEndpoint.botUsername, @@ -201,7 +202,7 @@ export function createInboundAdmission(db: Db, options: InboundAdmissionOptions) ? "Connection activation changed before admission" : "Connection is not active" : githubAddressedElsewhere - ? "GitHub message explicitly mentions a different connected bot" + ? "GitHub message explicitly addresses a different recipient" : mentionRequired ? "Message did not @mention the bot" : endpoint.provider === "telegram" && !thread.isDM && !addressed diff --git a/server/src/services/chat-github-events.ts b/server/src/services/chat-github-events.ts index cf3effdb4c3..4766843513a 100644 --- a/server/src/services/chat-github-events.ts +++ b/server/src/services/chat-github-events.ts @@ -42,14 +42,16 @@ export function githubBodyMentionsBot(body: string, username: string | null): bo /** * A thread subscription must not turn a request for another connected bot - * into work for this bot. Human mentions still follow ordinary thread policy, - * and explicitly mentioning this bot (including alongside another) wins. */ -export async function githubMessageAddressesAnotherBot( + * into work for this bot. A leading mention explicitly selects a recipient, + * including Apps outside Paperclip. Incidental human mentions still follow + * thread policy, and explicitly mentioning this bot always wins. */ +export async function githubMessageAddressesAnotherRecipient( db: Db | Parameters[0]>[0], endpoint: Pick, body: string, ): Promise { if (!body.includes("@") || githubBodyMentionsBot(body, endpoint.botUsername)) return false; + if (/^\s*@[a-z\d](?:[a-z\d-]{0,38})(?:\[bot\])?(?![\w-])/i.test(body)) return true; const others = await db .select({ username: chatEndpoints.botUsername }) .from(chatEndpoints) diff --git a/server/src/services/chat-github-response-comments.ts b/server/src/services/chat-github-response-comments.ts index a20877fe3e0..04216356621 100644 --- a/server/src/services/chat-github-response-comments.ts +++ b/server/src/services/chat-github-response-comments.ts @@ -1,4 +1,3 @@ -import { createHash } from "node:crypto"; import { and, asc, desc, eq, inArray, or, sql } from "drizzle-orm"; import { chatActions, chatConversations, chatDeliveries, chatEndpoints, @@ -17,30 +16,27 @@ import { githubRunReplyState } from "./chat-run-publications.js"; import { logActivity } from "./activity-log.js"; import { boundRunIssueId } from "./run-task-binding.js"; import { chatRunSourceMessageId } from "./chat-channels/run-origin.js"; +import { isMainPullResponse, legacyPullResponseMarkers, pullResponseMarker, requestResponseMarker, + retireLegacyResponsePlaceholder, stagePullResponse, type GitHubResponseSource } from "./chat-channels/github/response-target.js"; type Transaction = Parameters[0]>[0]; type Api = { request(path: string, options?: { method?: string; body?: unknown }): Promise }; type Lease = { commit(write: (tx: Transaction) => Promise): Promise }; -type Source = { - endpoint: typeof chatEndpoints.$inferSelect; - conversation: typeof chatConversations.$inferSelect; - delivery: typeof chatDeliveries.$inferSelect; - number: number; - replyId: string | null; -}; +type Source = GitHubResponseSource; type Comment = { id: number; body?: string; html_url: string; user?: { login?: string } }; const generation = (endpoint: Source["endpoint"]) => (endpoint.setup as { runtimeGeneration?: number }).runtimeGeneration ?? 0; const actionKey = (deliveryId: string) => `github_response:${deliveryId}`; -const responseMarker = (source: Source) => ``; +const responseMarker = (source: Source) => isMainPullResponse(source) ? pullResponseMarker(source) : requestResponseMarker(source); -async function findResponseComment(source: Source, api: Api): Promise { +async function findResponseComment(source: Source, api: Api, legacyMarkers: string[] = []): Promise { const list = source.replyId ? `/pulls/${source.number}/comments` : `/issues/${source.number}/comments`; + let legacy: Comment | null = null; for (let page = 1; page <= 100; page++) { const rows = await api.request(`${list}?per_page=100&page=${page}`); const found = rows.find(row => row.body?.includes(responseMarker(source)) && row.user?.login === source.endpoint.botUsername); if (found) return found; - if (rows.length < 100) return null; + legacy ??= rows.find(row => legacyMarkers.some(marker => row.body?.includes(marker)) && row.user?.login === source.endpoint.botUsername) ?? null; + if (rows.length < 100) return legacy; } throw conflict("GitHub history is too large to safely recover the response"); } @@ -71,13 +67,26 @@ async function stageResponse(db: Db, source: Source) { export async function writeGitHubResponseComment( db: Db, source: Source, api: Api, lease: Lease, - input: { body: string; final: boolean; versionAt?: Date; acknowledgement?: boolean }, + input: { body: string; final: boolean; versionAt?: Date; acknowledgement?: boolean; preserveFinal?: boolean }, assertCurrent: () => Promise, ) { await assertCurrent(); const response = await stageResponse(db, source); + const shared = isMainPullResponse(source); + const target = shared ? await stagePullResponse(db, source) : response; + const versionAt = input.versionAt ?? source.delivery.createdAt; + if (target.result?.id && Date.parse(String(target.result.versionAt)) > versionAt.getTime()) { + if (!input.acknowledgement) throw conflict("A newer GitHub response has already been published"); + if (shared) await retireLegacyResponsePlaceholder(source, response.result, + { id: String(target.result.id), url: String(target.result.url) }, api, assertCurrent); + await lease.commit(async tx => { await tx.update(chatActions).set({ status: "processed", result: { + ...response.result, id: target.result!.id, url: target.result!.url, + final: response.result?.final === true, code: "acknowledgement_superseded", versionAt: versionAt.toISOString(), + }, updatedAt: new Date() }).where(eq(chatActions.id, response.id)); }); + return { id: String(target.result.id), url: String(target.result.url) }; + } // A delayed acknowledgement or progress retry must never replace a result. - if (response.result?.id && (input.acknowledgement || (response.result.final === true && !input.final) || + if (response.result?.id && (!shared || response.result.id === target.result?.id) && (input.acknowledgement || (response.result.final === true && !input.final) || (input.versionAt && Date.parse(String(response.result.versionAt)) > input.versionAt.getTime()))) { if (!input.acknowledgement && response.result.final === true && !input.final) throw conflict("This GitHub response is already final"); @@ -88,30 +97,40 @@ export async function writeGitHubResponseComment( const item = source.replyId ? "/pulls/comments" : "/issues/comments"; await assertCurrent(); let previous: Comment | null = null; - if (response.result?.id) { + if (target.result?.id) { // Never recreate a deleted comment or edit an arbitrary caller-selected ID. - if (!/^[1-9][0-9]*$/.test(String(response.result.id))) throw forbidden("Invalid GitHub response receipt"); - previous = await api.request(`${item}/${response.result.id}`); + if (!/^[1-9][0-9]*$/.test(String(target.result.id))) throw forbidden("Invalid GitHub response receipt"); + previous = await api.request(`${item}/${target.result.id}`); if (!previous.body?.includes(marker) || previous.user?.login !== source.endpoint.botUsername) throw forbidden("GitHub response is no longer owned by this App"); } else { - previous = await findResponseComment(source, api); + previous = await findResponseComment(source, api, shared ? await legacyPullResponseMarkers(db, source) : []); } await assertCurrent(); - const alreadyFinal = previous?.body?.includes("") === true; - const body = `${projectSafeChatPublicationText(input.body)}\n\n${marker}${input.final ? "\n" : ""}`; - const posted = previous && (input.acknowledgement || (alreadyFinal && !input.final)) ? previous : await api.request( + const providerVersion = previous?.body?.match(//)?.[1]; + if (providerVersion && Date.parse(providerVersion) > versionAt.getTime()) throw conflict("A newer GitHub response has already been published"); + const sameRequest = !shared || previous?.body?.includes(requestResponseMarker(source)) === true; + const alreadyFinal = sameRequest && previous?.body?.includes("") === true; + const body = `${projectSafeChatPublicationText(input.body)}\n\n${marker}${shared ? `\n${requestResponseMarker(source)}` : ""}\n${input.final ? "\n" : ""}`; + const preserved = previous && (previous.body === body || (input.acknowledgement && sameRequest) || (alreadyFinal && !input.final) || + (input.preserveFinal && previous.body?.includes(""))) ? previous : null; + const posted = preserved && (!shared || preserved.body?.includes(marker)) ? preserved : await api.request( previous ? `${item}/${previous.id}` : source.replyId ? `/pulls/${source.number}/comments/${source.replyId}/replies` : list, - { method: previous ? "PATCH" : "POST", body: { body } }, + { method: previous ? "PATCH" : "POST", body: { body: preserved ? `${preserved.body}\n${marker}` : body } }, ); - if (!Number.isSafeInteger(posted.id) || posted.id <= 0 || !posted.html_url) + if (!posted || !Number.isSafeInteger(posted.id) || posted.id <= 0 || !posted.html_url) throw conflict("GitHub response delivery was not confirmed"); + if (shared) await retireLegacyResponsePlaceholder(source, response.result, + { id: String(posted.id), url: posted.html_url }, api, assertCurrent); await assertCurrent(); const receipt = { id: String(posted.id), url: posted.html_url }; await lease.commit(async tx => { + if (shared) await tx.update(chatActions).set({ result: { + ...receipt, final: input.final || alreadyFinal, versionAt: versionAt.toISOString(), deliveryId: source.delivery.id, + }, updatedAt: new Date() }).where(eq(chatActions.id, target.id)); await tx.update(chatActions).set({ status: "processed", result: { - ...receipt, final: input.final || alreadyFinal, versionAt: (input.versionAt ?? response.createdAt).toISOString(), + ...receipt, final: input.final || alreadyFinal, versionAt: versionAt.toISOString(), }, updatedAt: new Date() }).where(eq(chatActions.id, response.id)); await logActivity(tx as unknown as Db, { companyId: source.endpoint.companyId, actorType: "system", actorId: "github-response", action: "chat_github.response_updated", @@ -125,7 +144,7 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { async function context(deliveryId: string) { const [row] = await db.select({ endpoint: chatEndpoints, conversation: chatConversations, delivery: chatDeliveries, resource: chatEndpointResources, config: chatGitHubConfigurations, - assignee: issues.assigneeAgentId }).from(chatMessageLinks) + assignee: issues.assigneeAgentId, issueStatus: issues.status, sourceMessageId: chatMessageLinks.commentId }).from(chatMessageLinks) .innerJoin(chatDeliveries, and(eq(chatDeliveries.id, chatMessageLinks.deliveryId), eq(chatDeliveries.companyId, chatMessageLinks.companyId))) .innerJoin(chatConversations, and(eq(chatConversations.id, chatMessageLinks.conversationId), eq(chatConversations.companyId, chatMessageLinks.companyId))) .innerJoin(chatEndpoints, and(eq(chatEndpoints.id, chatConversations.endpointId), eq(chatEndpoints.companyId, chatConversations.companyId))) @@ -164,9 +183,13 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { }; await assertCurrent(); let onlyExisting = false; + let finalVersionAt: Date | undefined; + let requestRunning = false; + let requestPaused = false; + let preserveFinal = false; if (finalBody === undefined) { const existing = await stageResponse(db, source); - const [run] = await db.select().from(heartbeatRuns).innerJoin(chatMessageLinks, and( + const [requestRun] = await db.select().from(heartbeatRuns).innerJoin(chatMessageLinks, and( eq(chatMessageLinks.companyId, source.endpoint.companyId), eq(chatMessageLinks.endpointId, source.endpoint.id), eq(chatMessageLinks.deliveryId, deliveryId), eq(chatMessageLinks.direction, "inbound"), or(sql`${chatMessageLinks.commentId}::text = ${heartbeatRuns.contextSnapshot}->>'wakeCommentId'`, @@ -174,15 +197,42 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { )).where(and(eq(heartbeatRuns.companyId, source.endpoint.companyId), eq(heartbeatRuns.agentId, source.endpoint.assignedAgentId), sql`coalesce(${heartbeatRuns.nativeIssueId}::text, ${heartbeatRuns.contextSnapshot}->>'issueId') = ${source.conversation.issueId}`)) .orderBy(desc(heartbeatRuns.createdAt)).limit(1); - if (run && ["succeeded", "failed", "interrupted", "timed_out", "cancelled"].includes(run.heartbeat_runs.status)) { - const reply = await githubRunReplyState(db, { companyId: source.endpoint.companyId, endpointId: source.endpoint.id, - issueId: source.conversation.issueId!, runId: run.heartbeat_runs.id }); + const [latestRun] = await db.select().from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, source.endpoint.companyId), eq(heartbeatRuns.agentId, source.endpoint.assignedAgentId), + sql`coalesce(${heartbeatRuns.nativeIssueId}::text, ${heartbeatRuns.contextSnapshot}->>'issueId') = ${source.conversation.issueId}`, + )).orderBy(desc(heartbeatRuns.createdAt), desc(heartbeatRuns.id)).limit(1); + const originMessageId = latestRun?.activeIdentityContextId ? await chatRunSourceMessageId(db, latestRun, + { allowLegacyWakeComment: true, provider: "github" }) : null; + const ownsLatestRequest = !!latestRun && (requestRun?.heartbeat_runs.id === latestRun.id || + (!!source.sourceMessageId && originMessageId === source.sourceMessageId)); + requestRunning = ownsLatestRequest && latestRun?.status === "running"; + const terminal = latestRun && ["succeeded", "failed", "interrupted", "timed_out", "cancelled"].includes(latestRun.status); + const run = terminal && (ownsLatestRequest || source.issueStatus === "blocked") ? latestRun : null; + if (run) { + const reply = ownsLatestRequest ? await githubRunReplyState(db, { + companyId: source.endpoint.companyId, endpointId: source.endpoint.id, + issueId: source.conversation.issueId!, runId: run.id }) : "none"; if (reply === "unsettled") throw conflict("GitHub reply is still being resolved"); + if (existing.result?.final === true) return existing.result.id ? { id: String(existing.result.id), url: String(existing.result.url) } : undefined; + preserveFinal = reply === "confirmed"; onlyExisting = reply === "confirmed" && !existing.result?.id; + requestPaused = source.issueStatus === "blocked" && !ownsLatestRequest; finalBody = reply === "confirmed" ? "This request has been handled. See the response in this conversation." - : run.heartbeat_runs.status === "succeeded" ? "This turn ended without publishing a final response. See the Paperclip task for details." + : requestPaused ? "Work is paused. Your request is saved. See the Paperclip task for details." + : run.status === "succeeded" ? "This turn ended without publishing a final response. See the Paperclip task for details." : "This turn stopped before completing. See the Paperclip task for details."; - } else if (existing.result?.id) return { id: String(existing.result.id), url: String(existing.result.url) }; + finalVersionAt = new Date(Math.max(run.updatedAt.getTime(), source.delivery.createdAt.getTime())); + } else if (source.issueStatus === "blocked" && latestRun?.status !== "running") { + requestPaused = true; + finalBody = "Work is paused. Your request is saved. See the Paperclip task for details."; + finalVersionAt = source.delivery.createdAt; + } else if (existing.result?.id && (!isMainPullResponse(source) || + (await stagePullResponse(db, source)).result?.id === existing.result.id)) { + // Rotate no-op maintenance checks so a page of newer queued asks + // cannot hide other conversations that need terminal reconciliation. + await lease.commit(async tx => { await tx.update(chatActions).set({ updatedAt: new Date() }).where(eq(chatActions.id, existing.id)); }); + return { id: String(existing.result.id), url: String(existing.result.url) }; + } if (finalBody === undefined) { const [fast] = await db.select({ request: fastResponseRequests, publicationState: chatPublications.state }) .from(fastResponseRequests).leftJoin(chatPublications, and( @@ -216,8 +266,9 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { return; } return writeGitHubResponseComment(db, source, api, lease, - { body: finalBody ?? "Working on this…", final: finalBody !== undefined, - acknowledgement: finalBody === undefined, ...(finalBody !== undefined ? { versionAt: new Date() } : {}) }, assertCurrent); + { body: finalBody ?? (isMainPullResponse(source) && !requestRunning ? "Review queued…" : "Working on this…"), final: finalBody !== undefined && !requestPaused, + acknowledgement: finalBody === undefined, preserveFinal, + ...(finalBody !== undefined ? { versionAt: finalVersionAt ?? new Date() } : {}) }, assertCurrent); }); } async function requestsForRun(input: { companyId: string; endpointId: string; issueId: string; runId: string }) { @@ -245,6 +296,16 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { throw forbidden("Coalesced GitHub response belongs to another conversation"); const [response] = await db.select().from(chatActions).where(and(eq(chatActions.deliveryId, link.deliveryId), eq(chatActions.kind, "github_response_comment"))); if (!response || response.result?.final === true) continue; + if (isMainPullResponse(primary) && !input.closePrimary) { + // Every coalesced request shares the published main comment. Settle + // receipts without replacing the assessment with a link to itself. + await retireLegacyResponsePlaceholder(sibling, response.result, receipt, api, + async () => { await assertCurrent(); await context(link.deliveryId!); }); + await lease.commit(async tx => { await tx.update(chatActions).set({ status: "processed", + result: { ...response.result, ...receipt, final: true }, updatedAt: new Date(), + }).where(eq(chatActions.id, response.id)); }); + continue; + } // Never create a sibling placeholder just to close it. Mark a failed // acknowledgement settled so its maintenance retry cannot post later. if (!response.result?.id && !(await findResponseComment(sibling, api))) { @@ -290,6 +351,11 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { source.endpoint.id !== input.endpointId || source.conversation.issueId !== input.issueId || boundRunIssueId(current) !== input.issueId) throw forbidden("GitHub failure authority changed"); + const [latest] = await db.select({ id: heartbeatRuns.id }).from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, input.companyId), eq(heartbeatRuns.agentId, source.endpoint.assignedAgentId), + sql`coalesce(${heartbeatRuns.nativeIssueId}::text, ${heartbeatRuns.contextSnapshot}->>'issueId') = ${input.issueId}`, + )).orderBy(desc(heartbeatRuns.createdAt), desc(heartbeatRuns.id)).limit(1); + if (latest?.id !== run.id) throw forbidden("A newer GitHub run owns the response"); if ((await responseForRequest())?.result?.final === true) throw forbidden("GitHub request already has a final response"); if (await githubRunReplyState(db, input) !== "none") throw conflict("GitHub reply is still being resolved"); }; @@ -312,20 +378,44 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { or(eq(chatActions.status, "received"), and(eq(chatActions.status, "failed"), sql`${chatActions.result}->>'retryable' = 'true'`, sql`(${chatActions.result}->>'retryAt')::timestamptz <= now()`)))).orderBy(asc(chatActions.createdAt)).limit(limit); for (const row of rows) if (row.deliveryId) await tryAcknowledge(row.deliveryId); - return rows.length; + // Recovery can stop execution before a milestone publication is staged. + // Revisit acknowledged requests with terminal execution, even after a + // restart; successful acknowledgement is not successful task completion. + const stopped = await db.select({ deliveryId: chatActions.deliveryId }).from(chatActions) + .innerJoin(chatConversations, and(eq(chatConversations.id, chatActions.conversationId), eq(chatConversations.companyId, chatActions.companyId))) + .innerJoin(chatEndpoints, and(eq(chatEndpoints.id, chatActions.endpointId), eq(chatEndpoints.companyId, chatActions.companyId))) + .innerJoin(issues, and(eq(issues.id, chatConversations.issueId), eq(issues.companyId, chatActions.companyId))) + .where(and(eq(chatActions.kind, "github_response_comment"), eq(chatActions.status, "processed"), + sql`coalesce(${chatActions.result}->>'final', 'false') <> 'true'`, + sql`exists (select 1 from ${heartbeatRuns} r where r.company_id = ${chatActions.companyId} + and r.agent_id = ${chatEndpoints.assignedAgentId} + and coalesce(r.native_issue_id::text, r.context_snapshot->>'issueId') = ${chatConversations.issueId}::text + and r.status in ('succeeded', 'failed', 'interrupted', 'timed_out', 'cancelled') + and not exists (select 1 from ${heartbeatRuns} newer where newer.company_id = r.company_id + and newer.agent_id = r.agent_id + and coalesce(newer.native_issue_id::text, newer.context_snapshot->>'issueId') = ${chatConversations.issueId}::text + and (newer.created_at, newer.id) > (r.created_at, r.id)) + and (${issues.status} = 'blocked' or r.active_identity_context_id is not null or exists (select 1 from ${chatMessageLinks} l + where l.company_id = ${chatActions.companyId} and l.endpoint_id = ${chatActions.endpointId} + and l.delivery_id = ${chatActions.deliveryId} and l.direction = 'inbound' + and (l.comment_id::text = r.context_snapshot->>'wakeCommentId' + or coalesce(r.context_snapshot->'wakeCommentIds', '[]'::jsonb) ? l.comment_id::text))))`, + )).orderBy(asc(chatActions.updatedAt)).limit(limit); + for (const row of stopped) if (row.deliveryId) await tryAcknowledge(row.deliveryId); + return rows.length + stopped.length; } async function tryAcknowledge(deliveryId: string) { try { return await acknowledge(deliveryId); } catch (error) { const [row] = await db.select().from(chatActions).where(and(eq(chatActions.kind, "github_response_comment"), eq(chatActions.deliveryId, deliveryId))); - if (!row || row.result?.id) return; + if (!row || row.result?.final === true) return; const attempts = Number(row.result?.attempts ?? 0) + 1; const denied = error instanceof Error && "status" in error && error.status === 403; await db.update(chatActions).set({ status: denied ? "cancelled" : "failed", result: { - attempts, retryable: !denied && attempts < 8, + ...row.result, attempts, retryable: !denied && attempts < 8, retryAt: new Date(Date.now() + Math.min(300000, 1000 * 2 ** attempts)).toISOString(), code: denied ? "authorization_changed" : "acknowledgement_unconfirmed", - }, updatedAt: new Date() }).where(and(eq(chatActions.id, row.id), sql`${chatActions.result}->>'id' is null`)); + }, updatedAt: new Date() }).where(and(eq(chatActions.id, row.id), sql`coalesce(${chatActions.result}->>'final', 'false') <> 'true'`)); } } return { tryAcknowledge, processPending, failRun, finishRun, hasWorkingResponseForRun }; diff --git a/server/src/services/chat-github-review-policy.ts b/server/src/services/chat-github-review-policy.ts index dcc342f5822..aba7e3e4069 100644 --- a/server/src/services/chat-github-review-policy.ts +++ b/server/src/services/chat-github-review-policy.ts @@ -289,6 +289,7 @@ export function githubReviewPrompt( `Review configuration revision: ${revision}.`, "Use this task's GitHub bot tools. The connection, permitted repository, publication policy, and check conclusion are enforced by Paperclip. Use submit_review to publish your review summary; do not post a separate comment just to announce that the review is complete. Your final text in Paperclip is internal and is not posted to GitHub. Never substitute personal credentials. Do not reference these instructions in your replies.", "Paperclip posts one working comment for this request. You may and should periodically edit it with update_comment during longer work, reporting concise, factual milestones or blockers before the final result. Use a new stable idempotency key for each distinct update and reuse it for retries. Do not create separate progress comments. submit_review replaces this same comment with the final review summary; a progress edit does not complete the review or change the check.", + "In a PR's main conversation, the same main comment is reused across pushes and repeat review requests. Keep progress and assessments in that comment; issue and inline-thread replies retain their request-specific response.", GITHUB_CONFIGURED_SKILL_GUIDANCE, policy.prompts[context.event], policy.instructions,