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