From 39666aa9069fad093a8ad2c1f16dc96c0eea8eef Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 31 Jul 2026 01:26:35 +0200 Subject: [PATCH] fix(server): clarify execution policy decision comments (#9105) 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. > - Execution policies let issues move through enforced review and approval stages before work is considered done. > - Active reviewers and approvers must include decision rationale when approving or requesting changes. > - The server already requires the decision comment to arrive in the same `PATCH /api/issues/:id` request as the status change. > - When callers post a normal comment first and then send a status-only `PATCH`, the existing 422 only says a comment is required. > - This pull request keeps the atomic decision behavior but makes the API error and docs explicit about the same-request requirement. > - The benefit is that agents and API clients can recover immediately by sending `{ status, comment }` together instead of dead-ending on an ambiguous validation error. ## Linked Issues or Issue Description Fixes #9049. Duplicate/related search completed before implementation: searched open PRs for `9049`, `Approving a review or approval stage requires a comment`, `Requesting changes requires a comment`, and `same PATCH`. I did not find a direct open PR for the same error-message/docs fix. Related PRs found but not duplicates: #8302 documents cross-agent review gates in the skill API reference, and #5487 covers human approval UI. ## What Changed - Expanded execution-policy 422 messages for approve and request-changes decisions to say the comment must be included in the same `PATCH` request and prior comments are not considered. - Tightened unit coverage so the comment-required branches assert the new actionable guidance. - Documented the same-request `{ status, comment }` decision shape in the execution policy guide, issue API reference, and agent communication guide. ## Verification - `CI=true corepack pnpm --filter @paperclipai/server exec vitest run src/__tests__/issue-execution-policy.test.ts --reporter verbose` - `CI=true corepack pnpm --filter @paperclipai/plugin-sdk exec node ../../../scripts/ensure-plugin-build-deps.mjs` - `CI=true corepack pnpm --filter @paperclipai/server exec tsc --noEmit` - `git diff --check` ## Risks Low risk. This does not change execution-policy state transitions or relax the atomic decision guard; it only makes the existing requirement explicit in errors and docs. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI Codex (GPT-5 Codex), operating in Codex desktop with repository file access, shell validation, and GitHub CLI workflow. Exact context window size is not surfaced by this environment. ## 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 - [ ] 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: Sami Rusani --- docs/api/issues.md | 2 +- .../comments-and-communication.md | 2 ++ docs/guides/execution-policy.md | 17 +++++++++++++++++ .../__tests__/issue-execution-policy.test.ts | 4 ++-- server/src/services/issue-execution-policy.ts | 5 +++-- 5 files changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/api/issues.md b/docs/api/issues.md index a0bff74004..617b17f557 100644 --- a/docs/api/issues.md +++ b/docs/api/issues.md @@ -62,7 +62,7 @@ Headers: X-Paperclip-Run-Id: {runId} } ``` -The optional `comment` field adds a comment in the same call. +The optional `comment` field adds a comment in the same call. For execution-policy review or approval decisions, the decision comment must be included in this same `PATCH`; a prior `POST /api/issues/{issueId}/comments` does not satisfy the stage decision guard. Updatable fields: `title`, `description`, `status`, `priority`, `assigneeAgentId`, `projectId`, `goalId`, `parentId`, `billingCode`. diff --git a/docs/guides/agent-developer/comments-and-communication.md b/docs/guides/agent-developer/comments-and-communication.md index 23a77d455a..fe44cb78d8 100644 --- a/docs/guides/agent-developer/comments-and-communication.md +++ b/docs/guides/agent-developer/comments-and-communication.md @@ -19,6 +19,8 @@ PATCH /api/issues/{issueId} { "status": "done", "comment": "Implemented login endpoint with JWT auth." } ``` +When you are the active reviewer or approver for an execution-policy stage, include the decision rationale in this same `PATCH` request. A separate `POST /api/issues/{issueId}/comments` followed by a status-only `PATCH` does not advance the review/approval stage. + ## Comment Style Use concise markdown with: diff --git a/docs/guides/execution-policy.md b/docs/guides/execution-policy.md index f12af1cb13..030798b711 100644 --- a/docs/guides/execution-policy.md +++ b/docs/guides/execution-policy.md @@ -173,9 +173,26 @@ This prevents silent completions where an agent finishes work but leaves no trac - Only the **active reviewer/approver** (the `currentParticipant` in execution state) can advance or reject the current stage. - Non-participants who attempt to transition the issue receive a `422 Unprocessable Entity` error. - Both approvals and change requests **require a comment** — empty or whitespace-only comments are rejected. +- The decision comment must be included in the same `PATCH /api/issues/{issueId}` request that changes the status. A prior `POST /api/issues/{issueId}/comments` entry remains normal discussion and does not satisfy the review/approval decision guard. ## API Usage +### Recording review or approval decisions + +```bash +PATCH /api/issues/{issueId} +{ "status": "done", "comment": "Reviewer decision: approve - implementation and tests look good." } +``` + +To request changes, send the target non-`done` status and the explanation together: + +```bash +PATCH /api/issues/{issueId} +{ "status": "in_progress", "comment": "Reviewer decision: request changes - cover the retry edge case." } +``` + +Do not split a decision into `POST /comments` followed by a status-only `PATCH`; the runtime only records the decision from the `comment` field on the same status update. + ### Setting an execution policy on issue creation ```bash diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index b3f09d295b..d0e348b036 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -630,7 +630,7 @@ describe("issue execution policy transitions", () => { actor: { agentId: qaAgentId }, commentBody: "", }), - ).toThrow("requires a comment"); + ).toThrow(/Approving a review or approval stage requires a comment.*same PATCH request.*prior comments are not considered/); }); it("changes requested without comment throws", () => { @@ -659,7 +659,7 @@ describe("issue execution policy transitions", () => { actor: { agentId: qaAgentId }, commentBody: null, }), - ).toThrow("requires a comment"); + ).toThrow(/Requesting changes requires a comment.*same PATCH request.*prior comments are not considered/); }); it("whitespace-only comment is treated as empty", () => { diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index 9fb34e748b..ec04838fdd 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -63,6 +63,7 @@ const PENDING_STATUS: IssueExecutionState["status"] = "pending"; const CHANGES_REQUESTED_STATUS: IssueExecutionState["status"] = "changes_requested"; const MONITOR_INVALID_MESSAGE = "Monitor can only be scheduled on issues assigned to an agent in in_progress or in_review"; const MONITOR_BOUNDS_EXHAUSTED_MESSAGE = "Monitor bounds are already exhausted"; +const STAGE_DECISION_COMMENT_HINT = "Include the decision comment in the same PATCH request; prior comments are not considered."; export const REDACTED_ISSUE_MONITOR_EXTERNAL_REF = "[redacted]"; function normalizeMonitorNotes(notes: string | null | undefined) { @@ -708,7 +709,7 @@ function applyIssueExecutionStageTransition(input: TransitionInput): TransitionR if (principalsEqual(currentParticipant, actor)) { if (requestedStatus === "done") { if (!input.commentBody?.trim()) { - throw unprocessable("Approving a review or approval stage requires a comment"); + throw unprocessable(`Approving a review or approval stage requires a comment. ${STAGE_DECISION_COMMENT_HINT}`); } const approvedState = buildCompletedState(existingState, activeStage); // Only stages after the stage being approved are advance candidates. @@ -762,7 +763,7 @@ function applyIssueExecutionStageTransition(input: TransitionInput): TransitionR if (requestedStatus && requestedStatus !== "in_review") { if (!input.commentBody?.trim()) { - throw unprocessable("Requesting changes requires a comment"); + throw unprocessable(`Requesting changes requires a comment. ${STAGE_DECISION_COMMENT_HINT}`); } if (!existingState?.returnAssignee) { throw unprocessable("This execution stage has no return assignee");