fix(server): clarify execution policy decision comments (#9105)
## 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 <sr@samirusani>
This commit is contained in:
parent
fcf66f3a91
commit
39666aa906
|
|
@ -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`.
|
||||
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
Loading…
Reference in New Issue