From f2d09cef9a9635f87856f0136a2d32d929653de8 Mon Sep 17 00:00:00 2001 From: Waseem Ilyas <1478353+Waseemilyas@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:36:11 +0000 Subject: [PATCH 1/2] fix(server): refuse status:done when an executionPolicy replace discards a live review cycle 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 > - The issue PATCH route applies executionPolicy edits and status changes through one transition function, so both can arrive in a single request > - A replacement executionPolicy regenerates stage ids, which orphans the stored currentStageId; the orphaned-stage path clears executionState > - When a review cycle is live (pending or changes_requested), that clear lets a caller-supplied status:"done" stand although the review never ran > - This pull request rejects done on that combined write and leaves every other requested status — including board cancellation — on the established abandon-and-clear path > - The benefit is that a review gate cannot be bypassed by attaching a fresh policy object to a completion request ## Linked Issues or Issue Description Fixes #13176 Refs #13001 (same transition, distinct defect) ## What Changed - server/src/services/issue-execution-policy.ts: in the orphaned-stage branch, reject the transition with 422 unexecuted_review_stage only when a live review cycle (pending or changes_requested) meets a requested status of done. All other requested statuses — including cancelled, where abandoning the cycle is the intent — fall through to clearExecutionStatePatch exactly as a bare policy edit does. - server/src/__tests__/issue-execution-policy.test.ts: regression coverage for the rejected done, the preserved board cancellation, a routine non-done status change on the same path, the unchanged-policy done route to the stored reviewer, and the bare policy edit. ## Verification - pnpm --filter @paperclipai/server vitest run src/__tests__/issue-execution-policy.test.ts — 75 tests pass. ## Risks - Low risk. The only newly rejected request is one that previously discarded a live review while claiming completion — a silent-bypass bug, not a supported workflow. Callers intending to cancel or repurpose an issue mid-review are unaffected. ## Model Used - Anthropic Claude (SWE-2 Max agent via Devin CLI); tool use and code execution; the original fix commit was authored by an earlier agent pass on the same task. Co-authored-by: Paperclip --- .../__tests__/issue-execution-policy.test.ts | 166 ++++++++++++++++++ server/src/services/issue-execution-policy.ts | 18 ++ 2 files changed, 184 insertions(+) diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index 8b9d560191..2a318d80b0 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -1629,6 +1629,172 @@ describe("issue execution policy transitions", () => { }); }); + describe("replacing executionPolicy mid-cycle must not let done skip a live review", () => { + it("rejects done when a new executionPolicy regenerates the stage id during changes_requested", () => { + const originalPolicy = twoStagePolicy(); + const replacementPolicy = twoStagePolicy(); + + expect(() => + applyIssueExecutionPolicyTransition({ + issue: { + status: "in_progress", + assigneeAgentId: coderAgentId, + assigneeUserId: null, + executionPolicy: originalPolicy, + executionState: { + status: "changes_requested", + currentStageId: originalPolicy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: "changes_requested", + }, + }, + // A brand-new policy object has freshly generated stage ids, so the + // stored currentStageId from the live review no longer resolves. + policy: replacementPolicy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + }), + ).toThrow(expect.objectContaining({ status: 422, details: { code: "unexecuted_review_stage" } })); + }); + + it("lets a board cancellation through: the live cycle is abandoned, not claimed complete", () => { + const originalPolicy = twoStagePolicy(); + const replacementPolicy = twoStagePolicy(); + + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + executionPolicy: originalPolicy, + executionState: { + status: "pending", + currentStageId: originalPolicy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: replacementPolicy, + requestedStatus: "cancelled", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + }); + + // The orphaned cycle is cleared; the requested `cancelled` itself is + // applied by the caller, so the transition emits no status override. + expect(result.patch).toEqual({ executionState: null }); + }); + + it("lets a routine non-done status change through on the same abandon-and-clear path", () => { + const originalPolicy = twoStagePolicy(); + const replacementPolicy = twoStagePolicy(); + + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + executionPolicy: originalPolicy, + executionState: { + status: "pending", + currentStageId: originalPolicy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: replacementPolicy, + requestedStatus: "blocked", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + }); + + expect(result.patch).toEqual({ executionState: null }); + }); + + it("still allows a bare status: done to route to the stored reviewer when the policy is unchanged", () => { + const policy = twoStagePolicy(); + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_progress", + assigneeAgentId: coderAgentId, + assigneeUserId: null, + executionPolicy: policy, + executionState: { + status: "changes_requested", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: "changes_requested", + }, + }, + policy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + }); + + expect(result.patch).toMatchObject({ + status: "in_review", + assigneeAgentId: qaAgentId, + }); + }); + + it("still allows a policy edit with no requested status to clear the orphaned stage and return to the executor", () => { + const originalPolicy = twoStagePolicy(); + const replacementPolicy = twoStagePolicy(); + + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + executionPolicy: originalPolicy, + executionState: { + status: "pending", + currentStageId: originalPolicy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: replacementPolicy, + requestedStatus: undefined, + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + }); + + expect(result.patch).toMatchObject({ + status: "in_progress", + assigneeAgentId: coderAgentId, + executionState: null, + }); + }); + }); + describe("monitor policy", () => { it("schedules a one-shot monitor on an active agent-owned issue", () => { const policy = normalizeIssueExecutionPolicy({ diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index 859dc5fe1e..ed68d6dbec 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -696,6 +696,24 @@ function applyIssueExecutionStageTransition(input: TransitionInput): TransitionR } if (existingState?.currentStageId && !currentStage) { + // The stored stage id no longer resolves against the (possibly just + // replaced) policy — e.g. the caller PATCHed a brand-new executionPolicy + // in the same request that regenerated stage ids. When a review cycle is + // genuinely live (pending, or awaiting the assignee's response to + // changes requested), letting `done` ride along on that request would + // silently drop the cycle and mark the issue complete through a review + // that never ran — so `done` is rejected. Every other requested status, + // including a board cancellation that deliberately abandons the cycle, + // keeps the established clear/return-to-executor behavior below, exactly + // like a bare policy edit with no requested status. + const hasLiveReviewCycle = + existingState.status === PENDING_STATUS || existingState.status === CHANGES_REQUESTED_STATUS; + if (hasLiveReviewCycle && requestedStatus === "done") { + throw unprocessable( + "This issue has an unexecuted review stage from a live review cycle. Resolve the review before marking it done.", + { code: "unexecuted_review_stage" }, + ); + } clearExecutionStatePatch({ patch, issueStatus: input.issue.status, From 6306795cad993ac5a2bad103037ea98d60978ff2 Mon Sep 17 00:00:00 2001 From: Waseem Ilyas <1478353+Waseemilyas@users.noreply.github.com> Date: Thu, 10 Sep 2026 22:39:48 +0000 Subject: [PATCH 2/2] test: cover the pending-review rejection branch of the policy-replace guard Co-authored-by: Paperclip --- .../__tests__/issue-execution-policy.test.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index 2a318d80b0..7fba06204d 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -1663,6 +1663,37 @@ describe("issue execution policy transitions", () => { ).toThrow(expect.objectContaining({ status: 422, details: { code: "unexecuted_review_stage" } })); }); + it("rejects done when a new executionPolicy regenerates the stage id during a pending review", () => { + const originalPolicy = twoStagePolicy(); + const replacementPolicy = twoStagePolicy(); + + expect(() => + applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + executionPolicy: originalPolicy, + executionState: { + status: "pending", + currentStageId: originalPolicy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: replacementPolicy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + }), + ).toThrow(expect.objectContaining({ status: 422, details: { code: "unexecuted_review_stage" } })); + }); + it("lets a board cancellation through: the live cycle is abandoned, not claimed complete", () => { const originalPolicy = twoStagePolicy(); const replacementPolicy = twoStagePolicy();