mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
fix: reject done when an executionPolicy replace discards a live review cycle (#13178)
## 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 function, distinct defect) Supersedes #13164 — same fix, resubmitted on a correctly named branch with the review findings addressed: the guard no longer rejects board cancellation, and the internal ticket reference is gone. ## 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 locally. - `pnpm --filter @paperclipai/server typecheck` passes. - Manual reasoning check: PATCH `{executionPolicy: <new object>, status: "done"}` on an issue whose `executionState.status` is `pending` or `changes_requested` now returns 422; the same request with `status: "cancelled"` or with no status clears the orphaned stage as before. ## 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. ## 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 not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] 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 --------- Co-authored-by: Waseem Ilyas <1478353+Waseemilyas@users.noreply.github.com> Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
2 files changed
+215
No files matched your search
@@ -697,6 +697,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,
|
||||
|
||||
Reference in new issue
Block a user