mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 07:12:00 +01:00
fix(native): start the configured review stage when a worker reports finished work as needs_review (#21005)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Tasks can have an execution policy with a review stage that names an independent reviewer > - #20882 made a native worker's done report start that configured stage > - A worker can also report finished work as needs_review and list the review itself as the only blocking remaining work > - That report skipped the configured-stage check. The generic "needs_review requires a named reviewer" error made the worker file a separate review card, and the configured stage never started > - This pull request treats that report as finished work and gives the configured-stage guidance first > - The benefit is that the configured reviewer gates Done whichever way the worker phrases its finished report ## Linked Issues or Issue Description Refs #20882 **What happened** A task had a review stage for a human reviewer. The native worker built and verified the deliverable. It reported `needs_review` with `objectiveSatisfied: true`, all criteria satisfied, and one blocking remaining item: "Independent human review of the prepared website.html deliverable." The completion feedback skipped the configured-stage check because that item blocked completion. The generic needs_review error then told the worker to name a reviewer. The worker added a review attention request, and Paperclip made a separate "Review requested" card. `executionState` stayed null, and the configured reviewer was not assigned. **Expected behavior** On a task with a configured review stage, a worker that reports finished, verified work is told to report done. Paperclip then starts the configured stage with its reviewer. **Steps to reproduce** 1. Create a task with `executionPolicy.stages = [{ type: "review", participants: [{ type: "user", userId }] }]` for a native agent. 2. Have the agent report `needs_review` with a satisfied objective and the review as its only blocking remaining work. 3. Before this fix, the feedback asks for a named reviewer. The task gets a separate review card and no stage. ## What Changed - `native-completion-feedback.ts`: on a task whose configured review stage would start, a `needs_review` report with a satisfied objective and satisfied criteria counts as finished work. It gets the configured-stage guidance before the generic named-reviewer error. A review needed before the work can continue (objective not satisfied) keeps its own request path. Approval requests keep their own path. - `native-completion-feedback.ts`: a done report on such a task that still lists the review as outstanding gets the same guidance, not the generic "done report includes unfinished work" error. So the worker is not sent back to needs_review. - The guidance now says to report done without listing the review as remaining work or as an attention request. - Tests: two new cases in `native-execution-policy-review-stage.test.ts` use the recorded report shape. ## Verification - `cd server && npx vitest run src/services/native-runtime/native-execution-policy-review-stage.test.ts`: 15 passed. The new recorded-shape test fails on master (red) and passes with this change (green). - `npx vitest run` for `native-completion-feedback`, `native-current-wake-completion-feedback` and `native-execution-policy-review-stage-race`: 42 passed. - Paid chat eval run on a combined throwaway branch: results are in a PR comment. ## Risks - On a configured-review task, a worker that reports a satisfied objective but has real non-review blocking work now gets the configured-stage guidance. That guidance says to report done only when the work is complete and verified, so the task does not close early. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution in Claude Code. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
601babe322
commit
89ea418e76
2 files changed
+34
-2
No files matched your search
@@ -40,6 +40,8 @@ const repositoryYieldCorrection = 'This task has a saved repository preparation
|
||||
// control plane is reachable now. Ask once per run to re-check with fresh
|
||||
// Paperclip calls instead of resting the task blocked on a past stall.
|
||||
const paperclipServiceRecheckCorrection = "Paperclip answered this report, so its control plane is reachable now. Paperclip tool calls that did not return earlier were most likely delayed by a Paperclip restart or pause, and their results may have arrived since. Do not report this as a blocker. Re-read the current Paperclip state (for example read_document) before writing again; when a write is still needed, repeat it with the same idempotencyKey so it cannot apply twice. Then continue the task and report its real outcome. Report blocked only if a fresh Paperclip call fails now.";
|
||||
const configuredReviewStageCorrection =
|
||||
"This task has a configured review stage with its own reviewer. Do not request a separate review. Report done once the work is complete and verified, without listing that review as remaining work or an attention request; Paperclip then starts the configured review, and the task stays in review until that reviewer decides.";
|
||||
const PAPERCLIP_SERVICE_RECHECK_LIMIT = 1_000;
|
||||
const paperclipServiceRechecked = new Set<string>();
|
||||
|
||||
@@ -184,6 +186,14 @@ export async function nativeCompletionFeedback(
|
||||
signals.verification.some((entry) => entry.status === "failed") && "verification.failed",
|
||||
signals.actionableAttentionRequests.length > 0 && "attentionRequests.actionable",
|
||||
].filter(Boolean).join(", ") : "";
|
||||
if (result.completionClaim.objectiveSatisfied &&
|
||||
result.completionClaim.criteria.every(entry => entry.status === "satisfied") &&
|
||||
!signals.verification.some(entry => entry.status === "failed") &&
|
||||
signals.actionableAttentionRequests.every(request => request.kind === "review") &&
|
||||
issue.executionState?.status !== "pending" && !isConversation(issue) &&
|
||||
completionReviewStageEntry(issue, run.agentId).kind === "starts") {
|
||||
throw new Error(configuredReviewStageCorrection);
|
||||
}
|
||||
throw new Error(
|
||||
"The done report includes unfinished work, failed verification, or an outstanding decision. Finish the work or report the concrete blocker/reviewer request. No human completion approval was created." +
|
||||
(failedActionChat ? ` Rejection fields: ${rejectionReasons}.` + " This chat turn may finish with a truthful report of the failed approved action; that does not mean the external action succeeded. Keep the failure, observed state, and unresolved work explicit in the summary. Verify the observed state factually. Use not_run with a truthful reason only for a check that could not run or complete because its dependency was unavailable. Do not change a failed check to passed or hide incomplete work. Do not automatically replay the action or request approval again to force success; report the outcome and let the user decide whether to retry." : ""),
|
||||
@@ -389,12 +399,12 @@ export async function nativeCompletionFeedback(
|
||||
// Only a review of finished work belongs to the configured stage; a review
|
||||
// needed before the work can continue keeps its own request path.
|
||||
const workFinished = result.completionClaim.objectiveSatisfied &&
|
||||
!result.completionClaim.remainingWork.some(work => work.blocksCompletion);
|
||||
result.completionClaim.criteria.every(entry => entry.status === "satisfied");
|
||||
if (reviewStage && workFinished && result.reportedWorkDisposition === "needs_review" &&
|
||||
(signals.actionableAttentionRequests.length === 0 ||
|
||||
signals.actionableAttentionRequests.some(request => request.kind === "review"))) {
|
||||
throw new Error(
|
||||
"This task has a configured review stage with its own reviewer. Do not request a separate review. Report done once the work is complete and verified; Paperclip then starts the configured review, and the task stays in review until that reviewer decides.",
|
||||
configuredReviewStageCorrection,
|
||||
);
|
||||
}
|
||||
if (
|
||||
|
||||
@@ -146,6 +146,28 @@ describe.skipIf(!support.supported)("native completion with a configured review
|
||||
} });
|
||||
});
|
||||
|
||||
it("routes a finished needs_review report that lists only the review as blocking work to the configured stage", async () => {
|
||||
await finishVerifiedDone({ stage: "user", unprovenEvidence: true, beforeFinish: async (runId, result) => {
|
||||
const reviewOnly = { ...result.completionClaim,
|
||||
remainingWork: [{ description: "Independent human review of the prepared website.html deliverable.", blocksCompletion: true }] };
|
||||
const review = { kind: "review" as const, summary: "Independent human review of website.html by Riley.", ownerClass: "human" as const };
|
||||
await expect(nativeCompletionFeedback(db, runId, { ...result, reportedWorkDisposition: "needs_review",
|
||||
completionClaim: reviewOnly, attentionRequests: [review] })).rejects.toThrow("configured review stage with its own reviewer");
|
||||
await expect(nativeCompletionFeedback(db, runId, { ...result, reportedWorkDisposition: "needs_review", completionClaim: reviewOnly }))
|
||||
.rejects.toThrow("without listing that review as remaining work");
|
||||
await expect(nativeCompletionFeedback(db, runId, { ...result, completionClaim: reviewOnly }))
|
||||
.rejects.toThrow("configured review stage with its own reviewer");
|
||||
} });
|
||||
});
|
||||
|
||||
it("keeps the generic done rejection for unfinished work without a configured review stage", async () => {
|
||||
await finishVerifiedDone({ beforeFinish: async (runId, result) => {
|
||||
await expect(nativeCompletionFeedback(db, runId, { ...result, completionClaim: { ...result.completionClaim,
|
||||
remainingWork: [{ description: "Independent review.", blocksCompletion: true }] } }))
|
||||
.rejects.toThrow("The done report includes unfinished work");
|
||||
} });
|
||||
});
|
||||
|
||||
it("keeps the review request path for a review needed before the work can continue", async () => {
|
||||
await finishVerifiedDone({ stage: "user", beforeFinish: async (runId, result) => {
|
||||
const early = { ...result, reportedWorkDisposition: "needs_review" as const,
|
||||
|
||||
Reference in new issue
Block a user