From e09f40888ae451213a529c3b9c8e85385a38d39d Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:06:08 -0500 Subject: [PATCH] fix(recovery): preserve native task access during resolution (#18489) ## Thinking Path > - Paperclip controls agent execution and task ownership. > - A native run needs its task binding until it reports its result. > - Direct task updates already reject self-status changes that remove this binding. > - Recovery resolution can also change the source task status. > - That route did not use the same protection. > - This change rejects the unsafe status change before the task or recovery receipt changes. > - The run can then use semantic completion while normal recovery permissions remain effective. ## Linked Issues or Issue Description Refs #18411. **What happened?** A running native owner can resolve an older recovery action with `sourceIssueStatus: "in_review"` or `"done"`. The request succeeds and changes its own task status. That can release the execution binding before the run reports. Later task tools fail with `paperclip_runner_tool_binding_not_authorized`. **Expected behavior** Return a conflict before the task status or action receipt changes. Tell the run to report through `paperclip_finish` or `paperclip_block`. Preserve unchanged-status resolution, Board recovery, legacy-agent behavior, and the existing conversation and handoff rules. **Steps to reproduce** 1. Give a running native owner the task execution lock and an older recovery action. 2. Call `POST /api/issues/{id}/recovery-actions/resolve` as that run. Use `outcome: "restored"` and `sourceIssueStatus: "in_review"`. 3. Before this fix, the route returns 200 and changes the status. After this fix, it returns 409 with `native_run_status_reported_by_finish` and makes no task or action mutation. ## What Changed - Reuse the existing native self-status guard inside recovery resolution's source-task transaction. - Let the guard receive the requested status from either route contract. - Add HTTP route regressions for rejected review/completion transitions and continued task editing after rejection. - Cover unchanged-status native resolution, Board recovery and legacy-agent resolution. - Update execution semantics and both recovery maps. ## Verification - Baseline `91fdf02`: the two new rejection cases fail because the real Express route returns 200. Three safety cases pass. - Fixed source: all 198 tests in `server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts` pass. - `node scripts/check-maps.mjs` passes. This is structural evidence. - Greptile and Paperclip Review both give the integrated head 5/5 with no actionable findings. - Full recursive typecheck, full build, and a repeat of all 198 route tests pass at integrated head `0fa104dfb240500acd01303cd446363ea39c6c75`. All current-head CI gates pass (55 successful checks/statuses; no failed or pending checks). - The full local `pnpm test:run` command fails in chat-test line selection with `No test found`, before assertions. This also occurs with the unchanged master chat test file. It is not a passing full local suite. - The route tests use the real route, validation and guard. They mock service and database boundaries. The observed hosted failure establishes the original mechanism. Fixed-source live provider acceptance remains unverified. No deployment or merge is part of this PR. ## Risks - A native owner now receives a conflict for a previously accepted recovery status change. It must use the existing semantic result path. - This prevents a new binding loss. It does not prove that an old remote executor stopped or recover missing provider state. - Board, legacy, conversation and handoff behavior retains existing authority. The regression includes representative allowed paths. ## Model Used OpenAI GPT-6 through Codex, with reasoning, repository tools and code execution. The runtime does not expose a more specific snapshot ID or context-window size. ## 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 (198 focused route cases; the full local command has the recorded selection failure) - [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 - [x] 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: Paperclip --- doc/execution-semantics.md | 7 ++++ maps/agent-access-map/recovery.md | 4 ++ maps/feature-map/recovery.md | 2 +- ...ue-agent-mutation-ownership-routes.test.ts | 37 +++++++++++++++++++ server/src/routes/issues.ts | 6 ++- 5 files changed, 53 insertions(+), 3 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index c799eb58a39..e70e5de4208 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -79,6 +79,13 @@ another agent still require the outgoing execution to stop before ownership chan Paperclip issue statuses are not just UI labels. They imply different expectations about ownership and execution. +A running native owner reports its own task status through `paperclip_finish` or +`paperclip_block`. Both ordinary task updates and recovery-action resolution reject +direct self-status changes before they can release that run's task-tool access. +Recovery resolution can still record an outcome without changing the status. +Board recovery, legacy agents, and the existing conversation and handoff paths +retain their normal permission and lifecycle checks. + ### `backlog` The issue is not ready for active work. diff --git a/maps/agent-access-map/recovery.md b/maps/agent-access-map/recovery.md index 0624f02976e..d551c6b1824 100644 --- a/maps/agent-access-map/recovery.md +++ b/maps/agent-access-map/recovery.md @@ -156,10 +156,14 @@ Agent access: `partial` — eligible REST/CLI issue recovery:resolve; native cal Policy: Agent needs issue:mutate, authenticated run, low-trust exclusion and cross-issue allowance; source assignee/action owner or admitted active-checkout management override. Action ownership does not itself grant source-task mutation rights. Board and agent route policies remain distinct. Status changes separately require source owner/current execution participant/management grant or guarded exact-owner handback. blocked needs unresolved first-class blocker. Restored todo cannot bypass task/project/subtree/budget/review/approval/competing-run gates. Dedicated workspace causes reject generic retry. +A running native owner cannot change its own task status through recovery resolution. It reports the outcome with `paperclip_finish` or `paperclip_block`. An unchanged-status resolution retains the normal recovery permissions. Board recovery and legacy-agent status changes keep their existing rules. + Evidence: `source-only` — [recovery routes](../../server/src/routes/issues.ts), [resolution schema](../../packages/shared/src/validators/issue.ts); reviewed at `3bbc2104b65caf9f7763938a01260a5673e12f7a`. No live provider proof. Verification: `not-run` — Eligible owner resolves fresh action, reload recorded outcome/state and useful continuation. Peer/recovery-owner source completion, stale target, no blocker, pause/budget/review gates denied without side effects. +The [ownership route regression](../../server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts) also checks rejection before task/action mutation, continued task edits after rejection, unchanged-status native resolution, Board recovery and legacy-agent resolution. These HTTP route fixtures do not establish live provider acceptance. + Gaps: CLI body lacks executionReconciliation/workspaceBaseRef; no generic agent permission for operator source repair. ### `replay-and-deliver-resolution` diff --git a/maps/feature-map/recovery.md b/maps/feature-map/recovery.md index 2a2041d2e1a..a5b8431f6cf 100644 --- a/maps/feature-map/recovery.md +++ b/maps/feature-map/recovery.md @@ -234,7 +234,7 @@ Use these checks alongside the entry-point recipes below and the operation recor | --- | --- | --- | | `recovery/diagnosis` | Open the source task and exact run; compare cause, source action, original owner, recovery owner, attempt bounds and next action after reload. | Test an unrelated/private task and a stale run notice. Read-side retirement of a stale action must not create a new attempt, participant, or permission to replay. After workspace recovery, compare task and run instructions; recovered files must not leave instructions to repair a retained sandbox. The stopped run keeps its replay hold until continuation admission. An obsolete exact-session notice retires only after terminal coordinator proof and a separate exact-run workspace hold; no new execution or file cleanup follows. | | `recovery/bounded-continuation` | Use a deterministic recoverable interruption and inspect the original run/session, attempt receipts, saved result and eventual useful output. | Repeat restart/receipt delivery, race Stop or a new owner, and retain unknown external outcomes. Accepted completion/finalization recovery must not start a new provider turn or repeat its side effect. | -| `recovery/source-action` | Inspect active and retained actions, follow the repair owner, and resolve one eligible source-bound action with its exact ID. | Retry the settled resolution and reject a stale/foreign action. Recovery ownership alone cannot mutate the source task; inspect source status, audit and wake independently. | +| `recovery/source-action` | Inspect active and retained actions, follow the repair owner, and resolve one eligible source-bound action with its exact ID. | Retry the settled resolution and reject a stale/foreign action. Recovery ownership alone cannot mutate the source task; inspect source status, audit and wake independently. A running native owner must receive a conflict when resolution would change its own task status; its task tools must remain usable so it can report through semantic completion. Unchanged-status, Board and legacy recovery retain their normal permissions. | | `recovery/exhaustion` | Exhaust or age a bounded fixture; use the currently offered retry once, then inspect its durable acknowledgement and actual continuation. | Reassign, pause, stop, close, add an unresolved dependency, or impose a budget hold before retry. The old notice must not reopen terminal work or run the wrong owner. | | `recovery/workspace` | Exercise retained-file recovery, export retry, reconstruction, branch repair/reconciliation and exact-owner finalization as distinct authorized operator paths. | Use exact source/action/run/receipt bindings and preserve existing files. Reject stale source, live/uncertain writers, unsafe archives and insufficient authority. Verify actual files and accepted result separately from queued acknowledgement. | | `recovery/unknown-outcome` | Inspect the provider and saved intent before recording a source-specific chat, email or governed-provider outcome. | Use performed, not-performed and unresolved cases. Wrong company/request, stale state and unauthorized resolution must fail. Unknown effects must not replay automatically. An explicit retry must record the operator’s duplicate-risk acceptance and dispatch only the authorized attempt. | diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index bd038411f98..de2b46bc3e7 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -3025,6 +3025,43 @@ describe("agent issue mutation checkout ownership", () => { expect(mockIssueService.update).toHaveBeenCalledTimes(1); }); + it.each(["in_review", "done"])("rejects recovery resolution changing its own task to %s", async status => { + mockIssueRecoveryActionService.getActiveForIssue.mockResolvedValue({ + id: recoveryActionId, + ownerAgentId, + evidence: { runId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" }, + }); + + const app = createApp(runActor()); + const res = await request(app).post(`/api/issues/${issueId}/recovery-actions/resolve`) + .send({ actionId: recoveryActionId, outcome: "restored", sourceIssueStatus: status }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ code: "native_run_status_reported_by_finish", requestedStatus: status }); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockIssueRecoveryActionService.resolveActiveForIssue).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + + const edit = await request(app).patch(`/api/issues/${issueId}`).send({ title: "Continue authorized work" }); + expect(edit.status, JSON.stringify(edit.body)).toBe(200); + }); + + it.each(["native", "legacy", "board"])("preserves permitted %s recovery resolution", async kind => { + mockIssueRecoveryActionService.getActiveForIssue.mockResolvedValue({ id: recoveryActionId, ownerAgentId }); + if (kind === "legacy") mockHeartbeatService.getRun.mockResolvedValue({ ...nativeRun, runtimeMode: "legacy" }); + if (kind === "native") mockIssueService.getById.mockResolvedValue(makeIssue({ + status: "in_review", executionRunId: ownerRunId, checkoutRunId: ownerRunId, + })); + + const res = await request(createApp(kind === "board" ? boardActor() : runActor())) + .post(`/api/issues/${issueId}/recovery-actions/resolve`) + .send({ actionId: recoveryActionId, outcome: "restored", sourceIssueStatus: "in_review" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueRecoveryActionService.resolveActiveForIssue).toHaveBeenCalledTimes(1); + expect(mockIssueService.update).toHaveBeenCalledTimes(kind === "native" ? 0 : 1); + }); + it.each([ ["a legacy run", { runtimeMode: "legacy" }, {}], ["a native run that no longer owns execution", {}, { executionRunId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", checkoutRunId: null }], diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 030d64a2b1a..1326833bd9a 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -10196,6 +10196,7 @@ export function issueRoutes( } if (sourceStatusChanged) { + await assertNativeRunKeepsOwnExecutionLock(req, lockedIssue, sourceIssueStatus); const safeHandBack = outcome === "restored" && sourceIssueStatus === "todo" && @@ -13874,11 +13875,12 @@ export function issueRoutes( async function assertNativeRunKeepsOwnExecutionLock( req: Request, issue: { companyId: string; status: string; parentId: string | null; assigneeAgentId: string | null; assigneeUserId: string | null; checkoutRunId: string | null; executionRunId: string | null; conversationAgentId?: string | null; conversationUserId?: string | null }, + requestedStatus: unknown = req.body.status, ) { const runId = req.actor.type === "agent" ? req.actor.runId : null; if (!runId || !req.actor.agentId || req.actor.agentId !== issue.assigneeAgentId) return; if (runId !== issue.executionRunId && runId !== issue.checkoutRunId) return; - if (req.body.status === undefined || req.body.status === issue.status) return; + if (requestedStatus === undefined || requestedStatus === issue.status) return; if (isConversation(issue)) return; const keepsAssignment = (req.body.assigneeAgentId === undefined || req.body.assigneeAgentId === issue.assigneeAgentId) && @@ -13899,7 +13901,7 @@ export function issueRoutes( "This run owns the task's execution, so it cannot change the task status directly; doing so would end its access to the task before it reports. " + "Report the outcome with paperclip_finish instead: reportedWorkDisposition \"needs_review\" for review, \"done\" when complete, or \"yielded\" with continuation.kind \"monitor\" to wait on a scheduled monitor. " + "Use paperclip_block for a blocker. Paperclip sets the task status from that report.", - { code: "native_run_status_reported_by_finish", requestedStatus: req.body.status }, + { code: "native_run_status_reported_by_finish", requestedStatus }, ); }