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 <noreply@paperclip.ing>
This commit is contained in:
DottaandPaperclip authored and GitHub committed 2026-10-31 22:06:08 -05:00
1 parent c38844eafa
commit e09f40888a
5 files changed
+53 -3

No files matched your search

+7
View File
@@ -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.
+4
View File
@@ -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`
+1 -1
View File
@@ -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. |
@@ -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 }],
+4 -2
View File
@@ -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 },
);
}