fix(issues): base comment-wake decisions on post-insert issue state (#10068)
## 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 <annica@Michaels-Mac-Studio.local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Michael Nguyen
GitHub
parent
7ef75f5636
commit
7a4767017e
|
|
@ -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"));
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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, {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in New Issue