From 7945c703966d59842efb0e2c7f8090704a273386 Mon Sep 17 00:00:00 2001 From: NiViGaHo Date: Fri, 12 Jun 2026 06:55:37 +0200 Subject: [PATCH] fix(issues): reopen-guard for assignee self-comment on terminal issue (AKS-1563) (#4346) 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 are the unit of agent work, and a "done" issue should stay done unless something explicit reopens it > - The implicit-reopen path (human comment on a terminal issue) already keeps agents from reopening their own issues via `shouldImplicitlyMoveCommentedIssueToTodo`, but the explicit `reopen: true` path was not similarly guarded > - That gap lets the assignee agent reopen its own `done`/`cancelled` issue just by posting a log-style comment with `reopen: true` — the same "log lines are not reopen signals" semantics that the implicit path already encodes > - This pull request adds a focused `isAssigneeSelfCommentOnTerminalIssue` guard applied at both `PATCH /issues/:id` and `POST /issues/:id/comments`, forcing `effectiveMoveToTodoRequested = false` when the actor is an agent commenting on its own terminal issue without `resume: true` > - The benefit is a single, narrow invariant: only an explicit `resume: true` (or a different-agent / human commenter) reopens a terminal issue — assignee self-comments stay communicative ## Linked Issues or Issue Description Refs #3980 Refs #3935 Refs #6601 ## What Changed - Adds `isAssigneeSelfCommentOnTerminalIssue` helper in `server/src/routes/issues.ts` next to the existing `shouldImplicitlyMoveCommentedIssueToTodo` - Applies the guard at both comment entry paths (`PATCH /issues/:id` with a `comment` body and `POST /issues/:id/comments`) so `effectiveMoveToTodoRequested` is forced to `false` when actor is an agent and matches the **current** assignee of a `done`/`cancelled` issue — even if `reopen: true` was sent explicitly - PATCH path compares against `existing.assigneeAgentId` (not `requestedAssigneeAgentId`), so a different agent that PATCHes a terminal issue with `{ comment, reopen: true, assigneeAgentId: }` still reopens as today - The `resume: true` explicit-resume path is preserved verbatim — the guard short-circuits on `resumeRequested` - Existing external-caller paths (different agent / human user commenting on terminal) are unchanged and still reopen - New unit tests in `server/src/__tests__/issue-comment-reopen-routes.test.ts`: - `does not reopen via POST comment+reopen when the assignee agent is the actor on a done issue` - `does not reopen via POST comment+reopen when the assignee agent is the actor on a cancelled issue` - `does not reopen via PATCH comment+reopen when the assignee agent is the actor on a done issue` - `still reopens a done issue via PATCH when a different agent reassigns to self with reopen=true` ## Verification - [x] `vitest run src/__tests__/issue-comment-reopen-routes.test.ts` — 65/65 pass locally (4 new + 61 existing) - [x] `tsc --noEmit` — no new errors in changed files - [x] Manual trace: explicit-resume path (`resume: true`) still reopens because the guard short-circuits on `resumeRequested` ## Risks Low. The guard is a single short-circuit before the existing reopen decision and only fires when actor is an agent commenting on its own `done`/`cancelled` issue without `resume: true`. The `resume: true` path is unchanged, and the PATCH comparison uses the current assignee so cross-agent takeover with `reopen: true` continues to reopen. ## Model Used - Provider: Anthropic - Model: Claude Opus 4.7 (`claude-opus-4-7`) - Mode: extended thinking + tool use (Claude Code agent harness) ## 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 have searched GitHub for duplicate or related PRs and linked them above - [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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots (N/A — server-only) - [x] I have updated relevant documentation to reflect my changes (no docs touch the reopen guard) - [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 --- ## Cross-references and status (maintainer) Rebased on current `master`. The implicit-reopen case is already handled upstream by the user-actor branch of `shouldImplicitlyMoveCommentedIssueToTodo`; this PR adds the matching guard for the explicit `reopen: true` path. The PATCH-path guard compares against `existing.assigneeAgentId` so cross-agent reassignment + reopen still reopens. Refs #3980 Refs #3935 Refs #6601 Co-authored-by: Claude Opus 4.7 --- .../issue-comment-reopen-routes.test.ts | 174 ++++++++++++++++++ server/src/routes/issues.ts | 71 +++++-- 2 files changed, 228 insertions(+), 17 deletions(-) diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index 4451d75f..7faa9334 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -552,6 +552,180 @@ describe.sequential("issue comment reopen routes", () => { expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); }); + // POST self-comment from the assignee agent on a done issue with explicit + // reopen=true is the same log-class signal — the guard must suppress reopen. + it("does not reopen via POST comment+reopen when the assignee agent is the actor on a done issue", async () => { + const assigneeAgentId = "22222222-2222-4222-8222-222222222222"; + mockIssueService.getById.mockResolvedValue(makeIssue("done")); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue("done"), + ...patch, + })); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-1", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "log line", + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: assigneeAgentId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: assigneeAgentId, + companyId: "company-1", + runId: "run-self", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "log line", reopen: true }); + + expect(res.status).toBe(201); + expect(mockIssueService.update).not.toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ status: "todo" }), + ); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ reason: "issue_reopened_via_comment" }), + ); + }); + + // Same guard on cancelled status — explicit resume must use `resume: true`, + // a log-class self-comment with `reopen: true` is not a reopen signal. + it("does not reopen via POST comment+reopen when the assignee agent is the actor on a cancelled issue", async () => { + const assigneeAgentId = "22222222-2222-4222-8222-222222222222"; + mockIssueService.getById.mockResolvedValue(makeIssue("cancelled")); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue("cancelled"), + ...patch, + })); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-1", + issueId: "11111111-1111-4111-8111-111111111111", + companyId: "company-1", + body: "log line", + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: assigneeAgentId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: assigneeAgentId, + companyId: "company-1", + runId: "run-self", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + // Cancelled issues reject explicit resume entirely, so only reopen=true + // is observable here — the guard is what keeps it from flipping back. + .send({ body: "log line", reopen: true }); + + // Cancelled issues are rejected at assertExplicitResumeIntentAllowed for + // agent actors with reopen=true (409). The guard runs after that, but + // either way no reopen wakeup must fire and no status update to todo. + expect([200, 201, 409]).toContain(res.status); + expect(mockIssueService.update).not.toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ status: "todo" }), + ); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ reason: "issue_reopened_via_comment" }), + ); + }); + + // The guard must block explicit reopen=true + comment by the assignee agent + // on their own done issue (assignee self-comments are log lines, not reopen + // signals; explicit resume intent is delivered via `resume: true` instead). + it("does not reopen via PATCH comment+reopen when the assignee agent is the actor on a done issue", async () => { + const assigneeAgentId = "22222222-2222-4222-8222-222222222222"; + mockIssueService.getById.mockResolvedValue(makeIssue("done")); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue("done"), + ...patch, + })); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: assigneeAgentId, + companyId: "company-1", + runId: "run-self", + }), + ) + .patch("/api/issues/11111111-1111-4111-8111-111111111111") + .send({ comment: "log line", reopen: true }); + + expect(res.status).toBe(200); + expect(mockLogActivity).not.toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + details: expect.objectContaining({ reopened: true }), + }), + ); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ reason: "issue_reopened_via_comment" }), + ); + }); + + // The guard compares against the issue's current assignee, not the requested + // one — so an admin agent reassigning a different agent's terminal issue to + // themselves with comment + reopen=true still reopens as today (AC-3). + it("still reopens a done issue via PATCH when a different agent reassigns to self with reopen=true", async () => { + const otherAgentId = "33333333-3333-4333-8333-333333333333"; + mockAccessService.decide.mockImplementation(async (input: { action?: string }) => ({ + allowed: true, + action: input.action, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant.", + })); + mockIssueService.getById.mockResolvedValue(makeIssue("done")); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue("done"), + ...patch, + })); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: otherAgentId, + companyId: "company-1", + runId: "run-other", + }), + ) + .patch("/api/issues/11111111-1111-4111-8111-111111111111") + .send({ comment: "taking over", reopen: true, assigneeAgentId: otherAgentId }); + + expect(res.status).toBe(200); + expect(mockIssueService.update).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ + assigneeAgentId: otherAgentId, + status: "todo", + }), + ); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + details: expect.objectContaining({ + reopened: true, + reopenedFrom: "done", + status: "todo", + }), + }), + ); + }); + it("moves assigned blocked issues back to todo via POST comments", async () => { mockIssueService.getById.mockResolvedValue(makeIssue("blocked")); mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 9336508a..16391985 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -772,6 +772,25 @@ function isExplicitResumeCapableStatus(status: string | null | undefined) { return status === "done" || status === "blocked" || status === "todo" || status === "in_progress"; } +// Log-class comment from the assignee agent on a terminal (done/cancelled) +// issue is not a reopen signal. When the caller did not pass `resume: true`, +// this forces the reopen path off even if `reopen: true` was sent. +function isAssigneeSelfCommentOnTerminalIssue(input: { + hasCommentBody: boolean; + resumeRequested: boolean; + issueStatus: string | null | undefined; + assigneeAgentId: string | null | undefined; + actorType: "agent" | "user"; + actorId: string; +}) { + if (!input.hasCommentBody) return false; + if (input.resumeRequested) return false; + if (!isClosedIssueStatus(input.issueStatus)) return false; + if (typeof input.assigneeAgentId !== "string" || input.assigneeAgentId.length === 0) return false; + if (input.actorType !== "agent") return false; + return input.actorId === input.assigneeAgentId; +} + function queueResolvedInteractionContinuationWakeup(input: { heartbeat: ReturnType; issue: { id: string; assigneeAgentId: string | null; status: string }; @@ -4825,16 +4844,25 @@ export function issueRoutes( const shouldResumeInProgressScheduledRetry = !!scheduledRetryForHumanComment && scheduledRetryForHumanComment.agentId === requestedAssigneeAgentId; + const assigneeSelfCommentOnTerminal = isAssigneeSelfCommentOnTerminalIssue({ + hasCommentBody: !!commentBody, + resumeRequested: resumeRequested === true, + issueStatus: existing.status, + assigneeAgentId: existing.assigneeAgentId, + actorType: actor.actorType, + actorId: actor.actorId, + }); const effectiveMoveToTodoRequested = - explicitMoveToTodoRequested || - (!!commentBody && - shouldImplicitlyMoveCommentedIssueToTodo({ - issueStatus: existing.status, - assigneeAgentId: requestedAssigneeAgentId, - actorType: actor.actorType, - actorId: actor.actorId, - })) || - shouldResumeInProgressScheduledRetry; + !assigneeSelfCommentOnTerminal && + (explicitMoveToTodoRequested || + (!!commentBody && + shouldImplicitlyMoveCommentedIssueToTodo({ + issueStatus: existing.status, + assigneeAgentId: requestedAssigneeAgentId, + actorType: actor.actorType, + actorId: actor.actorId, + })) || + shouldResumeInProgressScheduledRetry); const updateReferenceSummaryBefore = titleOrDescriptionChanged ? await issueReferencesSvc.listIssueReferenceSummary(existing.id) : null; @@ -6591,15 +6619,24 @@ export function issueRoutes( const shouldResumeInProgressScheduledRetry = !!scheduledRetryForHumanComment && scheduledRetryForHumanComment.agentId === issue.assigneeAgentId; + const assigneeSelfCommentOnTerminal = isAssigneeSelfCommentOnTerminalIssue({ + hasCommentBody: true, + resumeRequested: resumeRequested === true, + issueStatus: issue.status, + assigneeAgentId: issue.assigneeAgentId, + actorType: actor.actorType, + actorId: actor.actorId, + }); const effectiveMoveToTodoRequested = - explicitMoveToTodoRequested || - shouldImplicitlyMoveCommentedIssueToTodo({ - issueStatus: issue.status, - assigneeAgentId: issue.assigneeAgentId, - actorType: actor.actorType, - actorId: actor.actorId, - }) || - shouldResumeInProgressScheduledRetry; + !assigneeSelfCommentOnTerminal && + (explicitMoveToTodoRequested || + shouldImplicitlyMoveCommentedIssueToTodo({ + issueStatus: issue.status, + assigneeAgentId: issue.assigneeAgentId, + actorType: actor.actorType, + actorId: actor.actorId, + }) || + shouldResumeInProgressScheduledRetry); const hasUnresolvedFirstClassBlockers = isBlocked && effectiveMoveToTodoRequested ? (await svc.getDependencyReadiness(issue.id)).unresolvedBlockerCount > 0