fix(issues): reopen-guard for assignee self-comment on terminal issue (AKS-1563) (#4346)
## 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: <self> }` 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 <noreply@anthropic.com>
This commit is contained in:
NiViGaHo
@@ -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<string, unknown>) => ({
|
||||
...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<string, unknown>) => ({
|
||||
...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<string, unknown>) => ({
|
||||
...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<string, unknown>) => ({
|
||||
...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<string, unknown>) => ({
|
||||
|
||||
@@ -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<typeof heartbeatService>;
|
||||
issue: { id: string; assigneeAgentId: string | null; status: string };
|
||||
@@ -4825,8 +4844,17 @@ 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 ||
|
||||
!assigneeSelfCommentOnTerminal &&
|
||||
(explicitMoveToTodoRequested ||
|
||||
(!!commentBody &&
|
||||
shouldImplicitlyMoveCommentedIssueToTodo({
|
||||
issueStatus: existing.status,
|
||||
@@ -4834,7 +4862,7 @@ export function issueRoutes(
|
||||
actorType: actor.actorType,
|
||||
actorId: actor.actorId,
|
||||
})) ||
|
||||
shouldResumeInProgressScheduledRetry;
|
||||
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 ||
|
||||
!assigneeSelfCommentOnTerminal &&
|
||||
(explicitMoveToTodoRequested ||
|
||||
shouldImplicitlyMoveCommentedIssueToTodo({
|
||||
issueStatus: issue.status,
|
||||
assigneeAgentId: issue.assigneeAgentId,
|
||||
actorType: actor.actorType,
|
||||
actorId: actor.actorId,
|
||||
}) ||
|
||||
shouldResumeInProgressScheduledRetry;
|
||||
shouldResumeInProgressScheduledRetry);
|
||||
const hasUnresolvedFirstClassBlockers =
|
||||
isBlocked && effectiveMoveToTodoRequested
|
||||
? (await svc.getDependencyReadiness(issue.id)).unresolvedBlockerCount > 0
|
||||
|
||||
Reference in New Issue
Block a user