From 7a4767017ed495f56b038a2bc45260686ac2ee18 Mon Sep 17 00:00:00 2001 From: Michael Nguyen <13559011+nguyenm7@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:18:15 -0700 Subject: [PATCH] fix(issues): base comment-wake decisions on post-insert issue state (#10068) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work; issues get commented on by both humans and agents, and the assignee is woken to act on new comments. > - #10050 added human-attributed issue comments for chat gateway plugins, with the host waking the issue's assignee the same way a board user's comment does. > - Greptile's review on #10050 flagged that the wakeup guard in `plugin-host-services.ts` decides whether to wake the assignee using the issue snapshot fetched *before* the comment was inserted. > - If another request closes, cancels, unassigns, or reassigns the issue in the window between that fetch and the wakeup call, the guard still acts on the stale snapshot — it can wake an agent for a now-terminal issue, or wake the old assignee instead of the new one. > - The PR discussion noted the HTTP add-comment route (`routes/issues.ts`) has the identical pattern outside its reopen/auto-approval branches, and deferred a fix to a follow-up covering both call sites — this PR is that follow-up. > - The fix re-fetches the issue immediately before the wake decision in both places, so the decision reflects the latest committed state instead of a pre-insert snapshot. ## Linked Issues or Issue Description Refs #10050 **Problem or motivation** Both the plugin-comment wakeup guard (`plugin-host-services.ts`) and the HTTP add-comment route's wakeup guard (`routes/issues.ts`, outside its reopen/auto-approval branches) decide whether to wake the issue's assignee using the issue state fetched before the comment was inserted. A concurrent close/unassign/reassign landing in that window is invisible to the guard, so it can enqueue a wakeup for a stale assignee or an issue that is no longer open. **Proposed solution** Re-fetch the issue immediately before the wake decision in both call sites, and base the assignee/status checks on that fresh read instead of the earlier snapshot. This shrinks the race window to essentially nothing (the fetch happens right before the fire-and-forget wakeup call), and any residual window is already covered by the heartbeat/checkout machinery re-validating issue status and assignee ownership when a woken run actually starts. **Alternatives considered** Wrap the whole comment-insert + wake-decision sequence in a single serializable transaction with row locking (rejected for this change — much larger blast radius across two already-complex handlers for a wakeup that is explicitly best-effort; the woken run's own re-validation already makes a stale wake degrade to a no-op rather than incorrect work). Leaving the plugin path fixed but not the HTTP route (rejected — that was the exact gap the original PR discussion flagged as needing a follow-up covering both call sites). **Roadmap alignment** Bug fix / hardening follow-up to #10050; no change to planned core roadmap items. ## What Changed - `server/src/services/plugin-host-services.ts`: `issues.createComment`'s assignee-wakeup guard now re-fetches the issue after the comment is inserted and bases the assignee/status checks on that fresh read, instead of the snapshot fetched before the insert. - `server/src/routes/issues.ts`: the `POST /issues/:id/comments` route's wakeup guard (outside the reopen/auto-approval branches, which already use post-mutation state) now does the same re-fetch before deciding whether — and whom — to wake. - Adds regression coverage for both: - `server/src/__tests__/plugin-orchestration-apis.test.ts`: a new embedded-Postgres test holds a row lock on the issue to deterministically force the race (comment-insert's internal update blocks until a concurrent transaction commits a cancellation), then asserts no wakeup is enqueued. - `server/src/__tests__/issue-comment-reopen-routes.test.ts`: two new mocked-service tests assert the route skips the wakeup when the fresh re-fetch shows the issue cancelled, and wakes the freshly reassigned agent (not the pre-insert snapshot's assignee) when the fresh re-fetch shows a different assignee. ## Verification - `pnpm --filter @paperclipai/server typecheck` — clean. - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/plugin-orchestration-apis.test.ts` — 13/13 (1 new). - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/issue-comment-reopen-routes.test.ts` — 74/74 (2 new). - `pnpm --filter @paperclipai/plugin-sdk exec vitest run tests/host-client-factory.test.ts` — 14/14. - Broader sweep of 38 `routes/issues.ts`-adjacent test files (447 tests) — all passing, confirming the added re-fetch doesn't change behavior for any existing reopen/auto-approval/interrupt/scheduled-retry/dependency-wake scenario. ## Risks Low risk. Both changes are additive guards around an existing best-effort, fire-and-forget wakeup (failures already logged, not thrown) — no change to the comment-write path itself, response shape, or status codes. The HTTP route's fix only touches the plain (non-reopen, non-auto-approval) wake-decision path; the reopen and auto-approval branches already used post-mutation state for the reasons documented inline and are unchanged. Adds one extra `SELECT` per comment on each call site, negligible relative to the existing query volume in both handlers. ## Model Used Claude Sonnet 5 (`claude-sonnet-5`), extended thinking, tool use enabled, via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I searched the GitHub PR list (open and recently closed) for similar PRs; found no duplicate — this is a direct follow-up to the review discussion on #10050 - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal ticket id - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: anicca Co-authored-by: Claude Sonnet 5 --- .../issue-comment-reopen-routes.test.ts | 82 +++++++++++++++++++ .../plugin-orchestration-apis.test.ts | 51 ++++++++++++ server/src/routes/issues.ts | 18 +++- server/src/services/plugin-host-services.ts | 75 ++++++++++------- 4 files changed, 196 insertions(+), 30 deletions(-) diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index cf22679188..8d130dc57d 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -1031,6 +1031,88 @@ describe.sequential("issue comment reopen routes", () => { )); }); + it("skips the assignee wakeup when the issue is concurrently cancelled while the comment is being written", async () => { + const app = await installActor(createApp()); + // First call is the route's pre-insert fetch; second is the wake-decision + // re-fetch. A stale wake decision would use the first (open, assigned) + // snapshot instead of the second (cancelled) one. + mockIssueService.getById + .mockResolvedValueOnce(makeIssue("in_progress")) + .mockResolvedValueOnce(makeIssue("cancelled")); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-race-cancel", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "Still working on this?", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Still working on this?" }); + + expect(res.status).toBe(201); + expect(mockIssueService.addComment).toHaveBeenCalled(); + // Give any (incorrect) fire-and-forget wakeup a moment to fire before asserting its absence. + await new Promise((resolve) => setTimeout(resolve, 25)); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("wakes the freshly reassigned agent, not the pre-insert snapshot's assignee, when the issue is concurrently reassigned", async () => { + const app = await installActor(createApp()); + const reassignedAgentId = "44444444-4444-4444-8444-444444444444"; + mockIssueService.getById + .mockResolvedValueOnce(makeIssue("in_progress")) + .mockResolvedValueOnce({ ...makeIssue("in_progress"), assigneeAgentId: reassignedAgentId }); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-race-reassign", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "Status update", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Status update" }); + + expect(res.status).toBe(201); + await waitForWakeup(() => expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith( + reassignedAgentId, + expect.objectContaining({ reason: "issue_commented" }), + )); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith( + "22222222-2222-4222-8222-222222222222", + expect.anything(), + ); + }); + + it("keeps the comment write successful and falls back to the in-hand snapshot when the wake re-fetch fails", async () => { + const app = await installActor(createApp()); + // The comment is already committed before the wake-decision re-fetch runs. + // If that best-effort re-fetch throws, the route must still return 201 for + // the persisted comment (a 5xx would invite a retry that duplicates it) and + // fall back to the in-hand snapshot so a legitimate wake isn't dropped. + mockIssueService.getById + .mockResolvedValueOnce(makeIssue("in_progress")) + .mockRejectedValueOnce(new Error("transient read failure")); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-refetch-fail", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "Still here?", + }); + + const res = await request(app) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Still here?" }); + + expect(res.status).toBe(201); + expect(mockIssueService.addComment).toHaveBeenCalled(); + await waitForWakeup(() => expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith( + "22222222-2222-4222-8222-222222222222", + expect.objectContaining({ reason: "issue_commented" }), + )); + }); + it("passes validated comment presentation fields to trusted board comment writes", async () => { const app = await installActor(createApp()); mockIssueService.getById.mockResolvedValue(makeIssue("todo")); diff --git a/server/src/__tests__/plugin-orchestration-apis.test.ts b/server/src/__tests__/plugin-orchestration-apis.test.ts index 71f22c181e..b54fe6e31d 100644 --- a/server/src/__tests__/plugin-orchestration-apis.test.ts +++ b/server/src/__tests__/plugin-orchestration-apis.test.ts @@ -903,6 +903,57 @@ describeEmbeddedPostgres("plugin orchestration APIs", () => { expect(run).toMatchObject({ agentId, companyId, status: "queued" }); }); + it("skips the assignee wakeup when the issue is cancelled concurrently with the comment being written", async () => { + const { companyId, agentId } = await seedCompanyAndAgent(); + const humanUserId = randomUUID(); + const issueId = randomUUID(); + await db.insert(companyMemberships).values({ + companyId, + principalType: "user", + principalId: humanUserId, + status: "active", + membershipRole: "owner", + }); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Needs human input", + status: "in_review", + priority: "medium", + assigneeAgentId: agentId, + }); + + const services = buildHostServices(db, "plugin-record-id", "paperclip.gateway", createEventBusStub()); + + // Hold a row lock on the issue so createComment's own `UPDATE issues SET + // updated_at` (inside addComment) blocks until this transaction commits a + // concurrent cancellation. That deterministically reproduces "another + // request closes the issue while the comment is being written" — the + // race Greptile flagged on the pre-insert `issue` snapshot — without + // relying on timing luck for the outcome, only for scheduling. + const lockAndCancelPromise = db.transaction(async (tx) => { + await tx.select().from(issues).where(eq(issues.id, issueId)).for("update"); + await new Promise((resolve) => setTimeout(resolve, 50)); + await tx.update(issues).set({ status: "cancelled" }).where(eq(issues.id, issueId)); + }); + + const commentPromise = services.issues.createComment({ + issueId, + companyId, + body: "Here's my answer", + actorUserId: humanUserId, + }); + + const [comment] = await Promise.all([commentPromise, lockAndCancelPromise]); + + expect(comment).toMatchObject({ + authorType: "user", + authorUserId: humanUserId, + body: "Here's my answer", + }); + await expect(db.select().from(agentWakeupRequests)).resolves.toHaveLength(0); + }); + // --------------------------------------------------------------------------- // LOOA-641 — interactions.respond / approvals.decide impersonation surface. // The host must independently re-verify the paired user's active membership diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 73b6ea87fc..b673573b47 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -12146,13 +12146,27 @@ export function issueRoutes( addWakeup(commentDecisionStageWakeup.agentId, commentDecisionStageWakeup.wakeup); } - const assigneeId = currentIssue.assigneeAgentId; + // Re-fetch immediately before deciding whether to wake anyone: outside + // the reopen/auto-approval branches above, `currentIssue` is still the + // snapshot read before the comment was inserted, so a concurrent + // close/unassign/reassign landing in that window would otherwise wake + // the wrong (or no-longer-relevant) agent off stale state. The comment + // is already committed, so a failed re-fetch is logged and falls back to + // the in-hand snapshot rather than aborting this best-effort wake block. + const wakeIssueSnapshot = (await svc.getById(currentIssue.id).catch((err) => { + logger.warn( + { err, issueId: currentIssue.id }, + "failed to re-fetch issue for comment wake decision; falling back to in-hand snapshot", + ); + return null; + })) ?? currentIssue; + const assigneeId = wakeIssueSnapshot.assigneeAgentId; const actorIsAgent = actor.actorType === "agent"; const selfComment = actorIsAgent && actor.actorId === assigneeId; // Re-derive closed-ness from the post-mutation issue so the auto-approval // transition (in_review -> done) suppresses a stale `issue_commented` wake // to the returnAssignee for an already-completed issue. - const skipWake = selfComment || isClosedIssueStatus(currentIssue.status); + const skipWake = selfComment || isClosedIssueStatus(wakeIssueSnapshot.status); if (assigneeId && (reopened || !skipWake)) { if (reopened) { addWakeup(assigneeId, { diff --git a/server/src/services/plugin-host-services.ts b/server/src/services/plugin-host-services.ts index 78400a43f5..3f975c18ef 100644 --- a/server/src/services/plugin-host-services.ts +++ b/server/src/services/plugin-host-services.ts @@ -2402,36 +2402,55 @@ export function buildHostServices( // handling here, just the core wake. An assignee-less or // closed-status issue is a silent no-op, matching the route's own // guard. - if ( - params.actorUserId - && issue.assigneeAgentId - && issue.status !== "done" - && issue.status !== "cancelled" - ) { - await heartbeat.wakeup(issue.assigneeAgentId, { - source: "automation", - triggerDetail: "system", - reason: "issue_commented", - payload: { + // + // The guard re-fetches the issue instead of trusting the pre-insert + // `issue` snapshot: a concurrent close/unassign/reassign landing + // between the initial fetch and here would otherwise wake the wrong + // (or no-longer-relevant) agent off stale state. + // + // The comment is already committed above, so this best-effort wake + // must never change that outcome: a failed re-fetch is logged and + // falls back to the in-hand snapshot rather than rejecting + // createComment — a rejection would surface to the caller as a failed + // write and invite a retry that inserts a duplicate comment. + if (params.actorUserId) { + const postCommentIssue = (await issues.getById(issue.id).catch((err) => { + logger.warn( + { err, issueId: issue.id, commentId: comment.id }, + "failed to re-fetch issue for plugin-relayed human comment wake; falling back to pre-insert snapshot", + ); + return null; + })) ?? issue; + if ( + postCommentIssue.assigneeAgentId + && postCommentIssue.status !== "done" + && postCommentIssue.status !== "cancelled" + ) { + await heartbeat.wakeup(postCommentIssue.assigneeAgentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_commented", + payload: { + issueId: issue.id, + commentId: comment.id, + mutation: "comment", + }, + requestedByActorType: "user", + requestedByActorId: params.actorUserId, + contextSnapshot: { + issueId: issue.id, + taskId: issue.id, + sourceCommentId: comment.id, + wakeReason: "issue_commented", + source: `plugin:${pluginKey}`, + }, + }).catch((err) => logger.warn({ + err, issueId: issue.id, commentId: comment.id, - mutation: "comment", - }, - requestedByActorType: "user", - requestedByActorId: params.actorUserId, - contextSnapshot: { - issueId: issue.id, - taskId: issue.id, - sourceCommentId: comment.id, - wakeReason: "issue_commented", - source: `plugin:${pluginKey}`, - }, - }).catch((err) => logger.warn({ - err, - issueId: issue.id, - commentId: comment.id, - agentId: issue.assigneeAgentId, - }, "failed to wake assignee on plugin-relayed human comment")); + agentId: postCommentIssue.assigneeAgentId, + }, "failed to wake assignee on plugin-relayed human comment")); + } } return comment;