From 27ec1e0c8b13cd881b6a4c794f17b37b56f1ebda Mon Sep 17 00:00:00 2001 From: dotta Date: Wed, 8 Apr 2026 16:54:43 -0500 Subject: [PATCH] Fix execution policy edits on in-review issues --- .../issue-execution-policy-routes.test.ts | 140 ++++++++++++++++++ .../__tests__/issue-execution-policy.test.ts | 115 ++++++++++++++ server/src/services/issue-execution-policy.ts | 60 +++++++- 3 files changed, 313 insertions(+), 2 deletions(-) create mode 100644 server/src/__tests__/issue-execution-policy-routes.test.ts diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts new file mode 100644 index 00000000..4f8d9fc6 --- /dev/null +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -0,0 +1,140 @@ +import express from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { errorHandler } from "../middleware/index.js"; +import { issueRoutes } from "../routes/issues.js"; +import { normalizeIssueExecutionPolicy } from "../services/issue-execution-policy.ts"; + +const mockIssueService = vi.hoisted(() => ({ + getById: vi.fn(), + assertCheckoutOwner: vi.fn(), + update: vi.fn(), + addComment: vi.fn(), + findMentionedAgents: vi.fn(), + getRelationSummaries: vi.fn(), + listWakeableBlockedDependents: vi.fn(), + getWakeableParentAfterChildCompletion: vi.fn(), +})); + +const mockHeartbeatService = vi.hoisted(() => ({ + wakeup: vi.fn(async () => undefined), + reportRunActivity: vi.fn(async () => undefined), + getRun: vi.fn(async () => null), + getActiveRunForAgent: vi.fn(async () => null), + cancelRun: vi.fn(async () => null), +})); + +vi.mock("../services/index.js", () => ({ + accessService: () => ({ + canUser: vi.fn(async () => false), + hasPermission: vi.fn(async () => false), + }), + agentService: () => ({ + getById: vi.fn(async () => null), + }), + documentService: () => ({}), + executionWorkspaceService: () => ({}), + feedbackService: () => ({ + listIssueVotesForUser: vi.fn(async () => []), + saveIssueVote: vi.fn(async () => ({ vote: null, consentEnabledNow: false, sharingEnabled: false })), + }), + goalService: () => ({}), + heartbeatService: () => mockHeartbeatService, + instanceSettingsService: () => ({ + get: vi.fn(async () => ({ + id: "instance-settings-1", + general: { + censorUsernameInLogs: false, + feedbackDataSharingPreference: "prompt", + }, + })), + listCompanyIds: vi.fn(async () => ["company-1"]), + }), + issueApprovalService: () => ({}), + issueService: () => mockIssueService, + logActivity: vi.fn(async () => undefined), + projectService: () => ({}), + routineService: () => ({ + syncRunStatusForIssue: vi.fn(async () => undefined), + }), + workProductService: () => ({}), +})); + +function createApp() { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = { + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + }; + next(); + }); + app.use("/api", issueRoutes({} as any, {} as any)); + app.use(errorHandler); + return app; +} + +describe("issue execution policy routes", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null }); + mockIssueService.findMentionedAgents.mockResolvedValue([]); + mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] }); + mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); + mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); + }); + + it("does not auto-start execution review when reviewers are added to an already in_review issue", async () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [ + { + id: "11111111-1111-4111-8111-111111111111", + type: "review", + participants: [{ type: "agent", agentId: "33333333-3333-4333-8333-333333333333" }], + }, + ], + })!; + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "in_review", + assigneeAgentId: null, + assigneeUserId: "local-board", + createdByUserId: "local-board", + identifier: "PAP-999", + title: "Execution policy edit", + executionPolicy: null, + executionState: null, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + updatedAt: new Date(), + })); + + const res = await request(createApp()) + .patch("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa") + .send({ executionPolicy: policy }); + + expect(res.status).toBe(200); + expect(mockIssueService.update).toHaveBeenCalledWith( + "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + expect.objectContaining({ + executionPolicy: policy, + actorAgentId: null, + actorUserId: "local-board", + }), + ); + const updatePatch = mockIssueService.update.mock.calls[0]?.[1] as Record; + expect(updatePatch.status).toBeUndefined(); + expect(updatePatch.assigneeAgentId).toBeUndefined(); + expect(updatePatch.assigneeUserId).toBeUndefined(); + expect(updatePatch.executionState).toBeUndefined(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index 9f5fb80b..7271b499 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -778,6 +778,25 @@ describe("issue execution policy transitions", () => { expect(result.patch).toEqual({}); }); + + it("does not auto-start workflow when policy is added to an already in_review issue", () => { + const reviewOnly = reviewOnlyPolicy(); + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: null, + assigneeUserId: boardUserId, + executionPolicy: null, + executionState: null, + }, + policy: reviewOnly, + requestedStatus: undefined, + requestedAssigneePatch: {}, + actor: { userId: boardUserId }, + }); + + expect(result.patch).toEqual({}); + }); }); describe("multi-participant stages", () => { @@ -974,4 +993,100 @@ describe("issue execution policy transitions", () => { expect(result.patch.assigneeUserId).toBe(boardUserId); }); }); + + describe("policy edits while a stage is active", () => { + it("clears the active execution state when its stage is removed from the policy", () => { + const reviewAndApproval = twoStagePolicy(); + const approvalOnly = approvalOnlyPolicy(); + + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + executionPolicy: reviewAndApproval, + executionState: { + status: "pending", + currentStageId: reviewAndApproval.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: approvalOnly, + requestedStatus: undefined, + requestedAssigneePatch: {}, + actor: { userId: boardUserId }, + }); + + expect(result.patch).toMatchObject({ + status: "in_progress", + assigneeAgentId: coderAgentId, + assigneeUserId: null, + executionState: null, + }); + }); + + it("reassigns the active stage when the current participant is removed", () => { + const policy = makePolicy([ + { + type: "review", + participants: [ + { type: "agent", agentId: qaAgentId }, + { type: "agent", agentId: ctoAgentId }, + ], + }, + ]); + const updatedPolicy = makePolicy([ + { + type: "review", + participants: [{ type: "agent", agentId: ctoAgentId }], + }, + ]); + + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: { + ...updatedPolicy, + stages: [{ ...updatedPolicy.stages[0], id: policy.stages[0].id }], + }, + requestedStatus: undefined, + requestedAssigneePatch: {}, + actor: { userId: boardUserId }, + }); + + expect(result.patch).toMatchObject({ + status: "in_review", + assigneeAgentId: ctoAgentId, + assigneeUserId: null, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: ctoAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + }, + }); + }); + }); }); diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index 3bc21c03..6f4ba7b5 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -145,6 +145,11 @@ function selectStageParticipant( return first ? { type: first.type, agentId: first.agentId ?? null, userId: first.userId ?? null } : null; } +function stageHasParticipant(stage: IssueExecutionStage, participant: IssueExecutionStagePrincipal | null): boolean { + if (!participant) return false; + return stage.participants.some((candidate) => principalsEqual(candidate, participant)); +} + function patchForPrincipal(principal: IssueExecutionStagePrincipal | null) { if (!principal) { return { assigneeAgentId: null, assigneeUserId: null }; @@ -218,6 +223,19 @@ function buildPendingStagePatch(input: { }); } +function clearExecutionStatePatch(input: { + patch: Record; + issueStatus: string; + requestedStatus?: string; + returnAssignee: IssueExecutionStagePrincipal | null; +}) { + input.patch.executionState = null; + if (input.requestedStatus === undefined && input.issueStatus === "in_review" && input.returnAssignee) { + input.patch.status = "in_progress"; + Object.assign(input.patch, patchForPrincipal(input.returnAssignee)); + } +} + export function applyIssueExecutionPolicyTransition(input: TransitionInput): TransitionResult { const patch: Record = {}; const existingState = parseIssueExecutionState(input.issue.executionState); @@ -251,6 +269,16 @@ export function applyIssueExecutionPolicyTransition(input: TransitionInput): Tra return { patch }; } + if (existingState?.currentStageId && !currentStage) { + clearExecutionStatePatch({ + patch, + issueStatus: input.issue.status, + requestedStatus, + returnAssignee: existingState.returnAssignee, + }); + return { patch }; + } + if (activeStage) { const currentParticipant = existingState?.currentParticipant ?? @@ -261,6 +289,35 @@ export function applyIssueExecutionPolicyTransition(input: TransitionInput): Tra throw unprocessable(`No eligible ${activeStage.type} participant is configured for this issue`); } + if (!stageHasParticipant(activeStage, currentParticipant)) { + const participant = selectStageParticipant(activeStage, { + preferred: explicitAssignee ?? existingState?.currentParticipant ?? null, + exclude: existingState?.returnAssignee ?? null, + }); + if (!participant) { + clearExecutionStatePatch({ + patch, + issueStatus: input.issue.status, + requestedStatus, + returnAssignee: existingState?.returnAssignee ?? null, + }); + return { patch }; + } + + buildPendingStagePatch({ + patch, + previous: existingState, + policy: input.policy, + stage: activeStage, + participant, + returnAssignee: existingState?.returnAssignee ?? currentAssignee ?? actor, + }); + return { + patch, + workflowControlledAssignment: true, + }; + } + if (principalsEqual(currentParticipant, actor)) { if (requestedStatus === "done") { if (!input.commentBody?.trim()) { @@ -362,8 +419,7 @@ export function applyIssueExecutionPolicyTransition(input: TransitionInput): Tra const shouldStartWorkflow = requestedStatus === "done" || - requestedStatus === "in_review" || - (input.issue.status === "in_review" && existingState == null); + requestedStatus === "in_review"; if (!shouldStartWorkflow) { return { patch };