diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index a304e28a..4451d75f 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -1447,6 +1447,937 @@ describe.sequential("issue comment reopen routes", () => { ); }); + it("auto-approves a reviewer comment with the APPROVED review marker", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nLooks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-1", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record, tx?: unknown) => ({ + ...issue, + ...patch, + executionState: patch.executionState, + status: "done", + completedAt: new Date(), + updatedAt: new Date(), + _tx: tx, + })); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-1", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(res.body).toMatchObject({ + id: "comment-review-1", + issueId: issue.id, + body: reviewBody, + }); + expect(mockDb.transaction).toHaveBeenCalledTimes(1); + expect(mockIssueService.update).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ + status: "done", + actorAgentId: reviewerAgentId, + actorUserId: null, + executionState: expect.objectContaining({ + status: "completed", + lastDecisionId: expect.any(String), + lastDecisionOutcome: "approved", + }), + }), + mockTx, + ); + }); + + it("auto-approves a reviewer comment with structured review metadata", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "kind: review\ndecision: approved\nsummary: ship it"; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-2", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record, tx?: unknown) => ({ + ...issue, + ...patch, + executionState: patch.executionState, + status: "done", + completedAt: new Date(), + updatedAt: new Date(), + _tx: tx, + })); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-2", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(res.body).toMatchObject({ + id: "comment-review-2", + issueId: issue.id, + body: reviewBody, + }); + expect(mockDb.transaction).toHaveBeenCalledTimes(1); + expect(mockIssueService.update).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ + status: "done", + actorAgentId: reviewerAgentId, + actorUserId: null, + executionState: expect.objectContaining({ + status: "completed", + lastDecisionId: expect.any(String), + lastDecisionOutcome: "approved", + }), + }), + mockTx, + ); + }); + + it("auto-approves a reviewer comment and wakes dependents when the final blocker resolves", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const dependentAgentId = "44444444-4444-4444-8444-444444444444"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + parentId: null, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nLooks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-3", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record, tx?: unknown) => ({ + ...issue, + ...patch, + executionState: patch.executionState, + status: "done", + completedAt: new Date(), + updatedAt: new Date(), + _tx: tx, + })); + mockIssueService.listWakeableBlockedDependents.mockResolvedValue([ + { + id: "dependent-1", + assigneeAgentId: dependentAgentId, + blockerIssueIds: [issue.id], + }, + ]); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-3", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(mockIssueService.listWakeableBlockedDependents).toHaveBeenCalledWith(issue.id); + await waitForWakeup(() => { + expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith( + dependentAgentId, + expect.objectContaining({ + reason: "issue_blockers_resolved", + payload: expect.objectContaining({ + issueId: "dependent-1", + resolvedBlockerIssueId: issue.id, + blockerIssueIds: [issue.id], + }), + }), + ); + }); + }); + + it("does not wake the returnAssignee with issue_commented when auto-approval reassigns the issue", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const returnAssigneeAgentId = "22222222-2222-4222-8222-222222222222"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: returnAssigneeAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: APPROVED"; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-5", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + // Simulate the policy transition reassigning the now-done issue back to the + // returnAssignee so the post-mutation assignee differs from the reviewer. + mockIssueService.update.mockImplementation(async (_id: string, patch: Record, tx?: unknown) => ({ + ...issue, + ...patch, + executionState: patch.executionState, + assigneeAgentId: returnAssigneeAgentId, + status: "done", + completedAt: new Date(), + updatedAt: new Date(), + _tx: tx, + })); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-stale-isclosed", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + // Allow any deferred wakeup task to flush before asserting it never fired. + await new Promise((resolve) => setImmediate(resolve)); + const issueCommentedWakeCalls = mockHeartbeatService.wakeup.mock.calls.filter( + ([, wakeup]: [string, { reason?: string }]) => wakeup?.reason === "issue_commented", + ); + expect(issueCommentedWakeCalls).toEqual([]); + }); + + it("does not auto-approve APPROVED comments from a non-review participant", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nLooks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-4", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: null, + authorUserId: "local-board", + }); + + const res = await request(await installActor(createApp())) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ reason: "issue_blockers_resolved" }), + ); + }); + + it("does not auto-approve when actor kind disagrees with the participant kind", async () => { + // The reviewer participant is a USER, but the request actor is an AGENT whose id collides + // with that user's id. The auto-approval gate must dispatch on actor kind, not just id, so + // this comment must follow the normal non-approval insert path. + const sharedId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "user", userId: sharedId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: null, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "user", userId: sharedId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nShipping."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-mismatched-kind", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: sharedId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: sharedId, + companyId: "company-1", + source: "agent_key", + runId: "run-kind-mismatch", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("does not auto-approve structured metadata separated by a blank line", async () => { + // Even though both `kind: review` and `decision: approved` appear, a blank line between + // them means they are not on truly consecutive lines, so the strict regex must reject it. + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "kind: review\n\ndecision: approved"; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-blank-line-metadata", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-blank-line-metadata", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("does not auto-approve reviewer comments without an approval marker", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "Looks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-5", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-5", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("does not auto-approve approval comments outside the in_review status", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_progress", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nLooks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-6", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-6", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + describe.each([ + { name: "uppercase negation", body: "## Review: NOT APPROVED" }, + { name: "uppercase negation with trailing period", body: "## Review: NOT APPROVED." }, + { name: "mixed-case negation", body: "## Review: Not approved." }, + { name: "do-not phrasing", body: "## Review: Do not approve" }, + { name: "present-progressive negation", body: "## Review: Not approving" }, + { name: "structured rejection", body: "kind: review\ndecision: rejected\nsummary: ship it" }, + { name: "structured changes_requested", body: "kind: review\ndecision: changes_requested\nsummary: ship it" }, + { + name: "disjoint structured metadata across prose", + body: "kind: review\n\nThe previous sprint decision: approved by stakeholders, but this round still needs work.", + }, + { + name: "disjoint structured metadata with summary line between", + body: "kind: review\nsummary: needs more work\ndecision: approved", + }, + ])("does not auto-approve negated approval phrasings ($name)", ({ body }) => { + it("rejects the auto-approval transition and keeps the comment as a regular comment", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-negated", + issueId: issue.id, + companyId: issue.companyId, + body, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-negated", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + }); + + describe.each([ + { name: "uppercase approval", body: "## Review: APPROVED" }, + { name: "trailing punctuation", body: "## Review: APPROVED!" }, + { name: "ticketed approval", body: "## Review: PAP-580 - APPROVED" }, + { name: "lowercase approval", body: "## Review: LGTM, approved" }, + { name: "approval with body context", body: "## Review: APPROVED\n\nReady to ship." }, + ])("auto-approves positive approval phrasings ($name)", ({ body }) => { + it("triggers the auto-approval transition", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-positive", + issueId: issue.id, + companyId: issue.companyId, + body, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record, tx?: unknown) => ({ + ...issue, + ...patch, + executionState: patch.executionState, + status: "done", + completedAt: new Date(), + updatedAt: new Date(), + _tx: tx, + })); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-positive", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body }); + + expect(res.status).toBe(201); + expect(mockDb.transaction).toHaveBeenCalledTimes(1); + expect(mockIssueService.update).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ status: "done" }), + mockTx, + ); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + details: expect.objectContaining({ + status: "done", + source: "auto_approval_comment", + _previous: { status: "in_review" }, + }), + }), + ); + }); + }); + + it("rolls back the comment when the auto-approval status transition fails", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nLooks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-atomic", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + const { unprocessable } = await import("../errors.js"); + mockIssueService.update.mockRejectedValue(unprocessable("Issue can only have one assignee")); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-atomic", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + // The route must propagate the 422 (no successful 201) and must insert the + // comment inside the same transaction as the status update so the comment + // rolls back when the status update fails. + expect(res.status).toBe(422); + expect(mockDb.transaction).toHaveBeenCalledTimes(1); + expect(mockIssueService.addComment).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + reviewBody, + expect.objectContaining({ agentId: reviewerAgentId }), + expect.any(Object), + mockTx, + ); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("rolls back the auto-approval comment when the issue is concurrently deleted", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: "22222222-2222-4222-8222-222222222222" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const reviewBody = "## Review: PAP-580 - APPROVED\n\nLooks good."; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.addComment.mockResolvedValue({ + id: "comment-review-missing", + issueId: issue.id, + companyId: issue.companyId, + body: reviewBody, + createdAt: new Date(), + updatedAt: new Date(), + authorAgentId: reviewerAgentId, + authorUserId: null, + }); + // Simulate the concurrent-delete race: svc.update resolves to null instead of throwing. + mockIssueService.update.mockResolvedValue(null); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-missing", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: reviewBody }); + + // The route must surface a 404 AND keep the transaction rollback path intact: + // the addComment INSERT must run inside the same transaction that the throw aborts, + // so the comment cannot survive when the status update finds no issue. + expect(res.status).toBe(404); + expect(mockDb.transaction).toHaveBeenCalledTimes(1); + expect(mockIssueService.addComment).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + reviewBody, + expect.objectContaining({ agentId: reviewerAgentId }), + expect.any(Object), + mockTx, + ); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + it("coerces executor handoff patches into workflow-controlled review wakes", async () => { const policy = await normalizePolicy({ stages: [ diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 48df2ac7..81c91469 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -558,6 +558,44 @@ function executionPrincipalsEqual( return left.type === "agent" ? left.agentId === right.agentId : left.userId === right.userId; } +function actorMatchesExecutionParticipant( + actor: { actorType: "user" | "agent"; actorId: string }, + participant: ParsedExecutionState["currentParticipant"] | null, +) { + if (!participant) return false; + // Require the actor kind to match the participant kind before comparing ids. Without this + // an agent and a user that happen to share an id value would falsely satisfy participant + // gating on the auto-approval path. + if (participant.type !== actor.actorType) return false; + return participant.type === "agent" ? participant.agentId === actor.actorId : participant.userId === actor.actorId; +} + +// Negation/rejection markers that invalidate an otherwise approval-looking heading. +// Match common phrasings ("NOT APPROVED", "Do not approve", "Not approving", "Changes requested", +// "Rejected", "Denied", "Blocked") so a reviewer comment intending to reject cannot auto-complete +// the issue. We rely on the heading being a single line, so testing the heading text alone is safe. +const APPROVAL_NEGATION_REGEX = + /\b(?:NOT|REJECT(?:ED|ING|S)?|DENY|DENIED|DENYING|BLOCK(?:ED|ING|S)?|CHANGES?\s+REQUESTED)\b/i; + +function isApprovalReviewComment(body: string) { + const normalized = body.replace(/\r\n?/g, "\n"); + const headingMatch = normalized.match(/(?:^|\n)##\s*Review:\s*([^\n]*)/i); + if (headingMatch) { + const headingText = headingMatch[1]; + if (/\bAPPROVED\b/i.test(headingText) && !APPROVAL_NEGATION_REGEX.test(headingText)) { + return true; + } + } + // Require the `kind: review` and `decision: approved` lines to appear on truly consecutive + // lines (no blank-line separation) so prose like "the previous sprint decision: approved" + // can't combine with an unrelated `kind: review` line elsewhere in the body to trigger + // auto-approval. Use `[ \t]*` between the lines so `\s*` does not silently swallow a newline. + return ( + /^[ \t]*kind[ \t]*:[ \t]*review[ \t]*\n[ \t]*decision[ \t]*:[ \t]*approved[ \t]*$/im.test(normalized) + || /^[ \t]*decision[ \t]*:[ \t]*approved[ \t]*\n[ \t]*kind[ \t]*:[ \t]*review[ \t]*$/im.test(normalized) + ); +} + function buildExecutionStageWakeContext(input: { state: ParsedExecutionState; wakeRole: ExecutionStageWakeContext["wakeRole"]; @@ -919,6 +957,13 @@ function buildExecutionStageWakeup(input: { return null; } +class AutoApprovalIssueMissingError extends Error { + constructor() { + super("Issue not found during auto-approval transaction"); + this.name = "AutoApprovalIssueMissingError"; + } +} + export function issueRoutes( db: Db, storage: StorageService, @@ -6567,6 +6612,8 @@ export function issueRoutes( let reopenFromStatus: string | null = null; let interruptedRunId: string | null = null; let currentIssue = issue; + let issueBeforeCommentDecision = issue; + let commentDecisionStageWakeup: ReturnType | null = null; const commentReferenceSummaryBefore = await issueReferencesSvc.listIssueReferenceSummary(issue.id); let scheduledRetrySupersededByComment = false; @@ -6654,16 +6701,143 @@ export function issueRoutes( } } - const comment = await svc.addComment(id, req.body.body, { - agentId: actor.agentId ?? undefined, - userId: actor.actorType === "user" ? actor.actorId : undefined, - runId: actor.runId, - }, { - authorType: req.body.authorType ?? (actor.actorType === "agent" ? "agent" : "user"), - presentation: req.body.presentation ?? null, - metadata: req.body.metadata ?? null, - sourceTrust: await sourceTrustForActorWrite(currentIssue, actor), - }); + const currentExecutionState = parseIssueExecutionState(currentIssue.executionState); + const currentExecutionPolicy = normalizeIssueExecutionPolicy(currentIssue.executionPolicy ?? null); + const shouldAutoApproveReviewComment = + currentIssue.status === "in_review" && + currentExecutionState?.status === "pending" && + actorMatchesExecutionParticipant(actor, currentExecutionState.currentParticipant ?? null) && + isApprovalReviewComment(req.body.body); + + // Persist the comment and the auto-approval state transition atomically when both apply. + // Without a single transaction, a 422 (or any error) thrown by the status update after the + // comment is inserted would leave an orphan comment without the corresponding state change. + let comment: Awaited>; + if (shouldAutoApproveReviewComment) { + const transition = applyIssueExecutionPolicyTransition({ + issue: currentIssue, + policy: currentExecutionPolicy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { + agentId: actor.agentId ?? null, + userId: actor.actorType === "user" ? actor.actorId : null, + }, + commentBody: req.body.body, + }); + const decisionId = transition.decision ? randomUUID() : null; + if (decisionId) { + const nextExecutionState = transition.patch.executionState; + if (!nextExecutionState || typeof nextExecutionState !== "object") { + throw new Error("Execution policy decision patch is missing executionState"); + } + transition.patch.executionState = { + ...nextExecutionState, + lastDecisionId: decisionId, + }; + } + + issueBeforeCommentDecision = currentIssue; + const updatePatch = { + ...transition.patch, + status: typeof transition.patch.status === "string" ? transition.patch.status : "done", + actorAgentId: actor.agentId ?? null, + actorUserId: actor.actorType === "user" ? actor.actorId : null, + }; + + const sourceTrust = await sourceTrustForActorWrite(currentIssue, actor); + const commentOptions = { + authorType: req.body.authorType ?? (actor.actorType === "agent" ? "agent" : "user"), + presentation: req.body.presentation ?? null, + metadata: req.body.metadata ?? null, + sourceTrust, + }; + let txResult: { comment: Awaited>; issue: NonNullable>> }; + try { + txResult = await db.transaction(async (tx) => { + const insertedComment = await svc.addComment( + id, + req.body.body, + { + agentId: actor.agentId ?? undefined, + userId: actor.actorType === "user" ? actor.actorId : undefined, + runId: actor.runId, + }, + commentOptions, + tx, + ); + const updated = await svc.update(id, updatePatch, tx); + // Throw (not return null) so drizzle rolls back the inserted comment when the issue + // has been concurrently deleted between the initial fetch and the in-transaction update. + if (!updated) throw new AutoApprovalIssueMissingError(); + + if (transition.decision && decisionId) { + await tx.insert(issueExecutionDecisions).values({ + id: decisionId, + companyId: updated.companyId, + issueId: updated.id, + stageId: transition.decision.stageId, + stageType: transition.decision.stageType, + actorAgentId: actor.agentId ?? null, + actorUserId: actor.actorType === "user" ? actor.actorId : null, + outcome: transition.decision.outcome, + body: transition.decision.body, + createdByRunId: actor.runId ?? null, + }); + } + + return { comment: insertedComment, issue: updated }; + }); + } catch (err) { + if (err instanceof AutoApprovalIssueMissingError) { + res.status(404).json({ error: "Issue not found" }); + return; + } + throw err; + } + comment = txResult.comment; + currentIssue = txResult.issue; + // Mirror the normal status-change audit trail: every other in_review -> done path + // emits an `issue.updated` activity, so emit one here too for the auto-approval path. + if (issueBeforeCommentDecision.status !== currentIssue.status) { + await logActivity(db, { + companyId: currentIssue.companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + runId: actor.runId, + action: "issue.updated", + entityType: "issue", + entityId: currentIssue.id, + details: { + status: currentIssue.status, + identifier: currentIssue.identifier, + source: "auto_approval_comment", + _previous: { status: issueBeforeCommentDecision.status }, + }, + }); + } + commentDecisionStageWakeup = buildExecutionStageWakeup({ + issueId: currentIssue.id, + previousState: currentExecutionState, + nextState: parseIssueExecutionState(currentIssue.executionState), + interruptedRunId, + requestedByActorType: actor.actorType, + requestedByActorId: actor.actorId, + }); + } else { + comment = await svc.addComment(id, req.body.body, { + agentId: actor.agentId ?? undefined, + userId: actor.actorType === "user" ? actor.actorId : undefined, + runId: actor.runId, + }, { + authorType: req.body.authorType ?? (actor.actorType === "agent" ? "agent" : "user"), + presentation: req.body.presentation ?? null, + metadata: req.body.metadata ?? null, + sourceTrust: await sourceTrustForActorWrite(currentIssue, actor), + }); + } + await issueReferencesSvc.syncComment(comment.id); const commentReferenceSummaryAfter = await issueReferencesSvc.listIssueReferenceSummary(currentIssue.id); const commentReferenceDiff = issueReferencesSvc.diffIssueReferenceSummary( @@ -6735,14 +6909,32 @@ export function issueRoutes( // Merge all wakeups from this comment into one enqueue per agent to avoid duplicate runs. void (async () => { - const wakeups = new Map[1]>(); + type WakeupRequest = NonNullable[1]>; + const wakeups = new Map(); + const addWakeup = (agentId: string, wakeup: WakeupRequest) => { + const wakeIssueId = + wakeup.payload && typeof wakeup.payload === "object" && typeof wakeup.payload.issueId === "string" + ? wakeup.payload.issueId + : currentIssue.id; + const key = `${agentId}:${wakeIssueId}`; + if (wakeups.has(key)) return; + wakeups.set(key, { agentId, wakeup }); + }; + + if (commentDecisionStageWakeup) { + addWakeup(commentDecisionStageWakeup.agentId, commentDecisionStageWakeup.wakeup); + } + const assigneeId = currentIssue.assigneeAgentId; const actorIsAgent = actor.actorType === "agent"; const selfComment = actorIsAgent && actor.actorId === assigneeId; - const skipWake = selfComment || isClosed; + // 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); if (assigneeId && (reopened || !skipWake)) { if (reopened) { - wakeups.set(assigneeId, { + addWakeup(assigneeId, { source: "automation", triggerDetail: "system", reason: "issue_reopened_via_comment", @@ -6769,7 +6961,7 @@ export function issueRoutes( }, }); } else { - wakeups.set(assigneeId, { + addWakeup(assigneeId, { source: "automation", triggerDetail: "system", reason: "issue_commented", @@ -6804,9 +6996,8 @@ export function issueRoutes( } for (const mentionedId of mentionedIds) { - if (wakeups.has(mentionedId)) continue; if (actorIsAgent && actor.actorId === mentionedId) continue; - wakeups.set(mentionedId, { + addWakeup(mentionedId, { source: "automation", triggerDetail: "system", reason: "issue_comment_mentioned", @@ -6824,7 +7015,67 @@ export function issueRoutes( }); } - for (const [agentId, wakeup] of wakeups.entries()) { + const becameDone = issueBeforeCommentDecision.status !== "done" && currentIssue.status === "done"; + if (becameDone) { + const dependents = await svc.listWakeableBlockedDependents(currentIssue.id); + for (const dependent of dependents) { + addWakeup(dependent.assigneeAgentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_blockers_resolved", + payload: { + issueId: dependent.id, + resolvedBlockerIssueId: currentIssue.id, + blockerIssueIds: dependent.blockerIssueIds, + }, + requestedByActorType: actor.actorType, + requestedByActorId: actor.actorId, + contextSnapshot: { + issueId: dependent.id, + taskId: dependent.id, + wakeReason: "issue_blockers_resolved", + source: "issue.blockers_resolved", + resolvedBlockerIssueId: currentIssue.id, + blockerIssueIds: dependent.blockerIssueIds, + }, + }); + } + } + + const becameTerminal = + !["done", "cancelled"].includes(issueBeforeCommentDecision.status) && + ["done", "cancelled"].includes(currentIssue.status); + if (becameTerminal && currentIssue.parentId) { + const parent = await svc.getWakeableParentAfterChildCompletion(currentIssue.parentId); + if (parent) { + addWakeup(parent.assigneeAgentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_children_completed", + payload: { + issueId: parent.id, + completedChildIssueId: currentIssue.id, + childIssueIds: parent.childIssueIds, + childIssueSummaries: parent.childIssueSummaries, + childIssueSummaryTruncated: parent.childIssueSummaryTruncated, + }, + requestedByActorType: actor.actorType, + requestedByActorId: actor.actorId, + contextSnapshot: { + issueId: parent.id, + taskId: parent.id, + wakeReason: "issue_children_completed", + source: "issue.children_completed", + completedChildIssueId: currentIssue.id, + childIssueIds: parent.childIssueIds, + childIssueSummaries: parent.childIssueSummaries, + childIssueSummaryTruncated: parent.childIssueSummaryTruncated, + }, + }); + } + } + + for (const { agentId, wakeup } of wakeups.values()) { heartbeat .wakeup(agentId, wakeup) .catch((err) => logger.warn({ err, issueId: currentIssue.id, agentId }, "failed to wake agent on issue comment")); diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 429205a1..d41f27ce 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -5895,12 +5895,13 @@ export function issueService(db: Db) { sourceTrust?: typeof issueComments.$inferInsert.sourceTrust; createdAt?: Date | string | null; }, + dbOrTx: any = db, ) => { - const issue = await db + const issue = await dbOrTx .select({ companyId: issues.companyId }) .from(issues) .where(eq(issues.id, issueId)) - .then((rows) => rows[0] ?? null); + .then((rows: Array<{ companyId: string }>) => rows[0] ?? null); if (!issue) throw notFound("Issue not found"); @@ -5915,7 +5916,7 @@ export function issueService(db: Db) { const presentation = issueCommentPresentationSchema.nullable().parse(options?.presentation ?? null); const metadata = issueCommentMetadataSchema.nullable().parse(options?.metadata ?? null); const createdAt = options?.createdAt ? new Date(options.createdAt) : null; - const [comment] = await db + const [comment] = await dbOrTx .insert(issueComments) .values({ companyId: issue.companyId, @@ -5933,7 +5934,7 @@ export function issueService(db: Db) { .returning(); // Update issue's updatedAt so comment activity is reflected in recency sorting - await db + await dbOrTx .update(issues) .set({ updatedAt: new Date() }) .where(eq(issues.id, issueId));