From 51ea62434413df7a0485e8b0c5f6700671b698a5 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sat, 31 Oct 2026 08:44:44 -0700 Subject: [PATCH] fix(server): reject a native run's own task status change (#18411) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Native runs (the Paperclip Runner) report their outcome with `paperclip_finish` or `paperclip_block`, and the status arbiter then sets the task status > - Every runner tool, `paperclip_finish` included, requires that the task's `executionRunId` is the current run > - When the run sends its own status change through `PATCH /api/issues/{id}` (for example with `call_api`), the issue update clears `executionRunId` and `checkoutRunId`, so the run removes its own execution lock > - After that, every tool call fails, the run cannot report, same-run recovery spends its retries, and the task goes to `blocked` > - This pull request rejects that status change with a 409 before it commits, and the message tells the run to report with `paperclip_finish` or `paperclip_block` > - The benefit is that the run corrects itself in the same turn, and the task does not get stuck ## Linked Issues or Issue Description No public issue exists. This is the bug description. **What happened?** An agent on the Paperclip Runner followed task instructions that said "set the task to `in_review` and keep it assigned to yourself" while it waited on a scheduled monitor. It sent `call_api` with `PATCH /api/issues/{id}` and `{ "status": "in_review" }`. The server applied the change and cleared the execution lock. Then: 1. `get_task`, `set_task_monitor`, and `paperclip_finish` all failed the run binding check. The provider saw only "Paperclip semantic operation failed", or the monitor-wait message from `paperclip_finish`. 2. The `call_api` receipt for the PATCH showed `status: null, outcome: unknown`, because the authority check after the response failed when the lock was gone. 3. The session ended with no semantic result. Exact-checkpoint resume ran out of retries (`native_session_retry_exhausted`), and recovery would not replay the run because the mutation outcome was unknown. The task went to `blocked`. This occurred on each check where the agent tried the status change. Runs that did not try it completed normally. **Expected behavior** A native run that holds the task's execution lock cannot remove its own lock. It gets a clear, actionable error and reports its outcome through `paperclip_finish` or `paperclip_block`. **Steps to reproduce** 1. Assign a task to an agent that uses the Paperclip Runner (native runtime). 2. While the run executes, have it send `call_api` with `PATCH /api/issues/{id}` and `{ "status": "in_review" }` for its own task. 3. Look at the next tool calls of the run. They fail, `paperclip_finish` cannot complete, and recovery blocks the task. **Paperclip version or commit** `master` at 493e229d93b. ## What Changed - `server/src/routes/issues.ts`: add `assertNativeRunKeepsOwnExecutionLock` to `PATCH /issues/:id`. The guard returns 409 `native_run_status_reported_by_finish` when all of these are true: - the actor is the assigned agent, and its run is the task's execution or checkout run - the run is `native` and `running` - the request changes the status and keeps the assignment - the task is not a conversation - The 409 message names `paperclip_finish` (`needs_review`, `done`, or `yielded` with a `monitor` continuation) and `paperclip_block`. `call_api` returns HTTP denials as data, so the provider can read the message. - Conversation tasks are out of scope. A conversation turn settles from its final reply (`nativeConversationReplyResult`), not from a runner tool. So a reply that moves its conversation to `in_review` still settles, and that contract stays. - When the request also carries blockers, the guard first runs `assertOnboardingFirstTaskDependenciesAllowed`. A blocker that the dependency rules reject keeps its specific 400. Every other case gets the 409, including `{ "status": "todo", "blockedByIssueIds": [] }`. - `server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts`: add 14 cases. They cover rejection for `in_review`, `done`, and `blocked` (also with an explicit self-assignment, an empty blocker list, and a real blocker), and the onboarding 400 that wins over the 409. Pass-through cases assert a 200, the patched status, and one update for: other field edits, legacy runs, native runs without the lock, stopped runs, handoffs to a human (after a confirmed stop), conversation replies, and board users. ## Verification - `npx vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts` passes (193 tests). - With the guard disabled, the 4 rejection cases fail. With the `native` runtime condition removed, the legacy pass-through case fails. The tests pin both sides. - `chat-project-tools` and `onboarding-first-task-admission` pass (74 tests). These are the integration suites that caught the first version of the guard in CI. - `npx vitest run` on `issue-queued-comments-routes`, `task-workspace-binding`, and `issue-recovery-actions` together with the ownership suite passes (455 tests). - `npx tsc --noEmit -p server/tsconfig.json` is clean after `pnpm --filter @paperclipai/paperclip-runner build`. ## Risks - Behavior changes only for native runs that change the status of the task whose execution lock they hold. That change always stranded the run before this PR, so no working flow depends on it. - Legacy adapters, board users, agent handoffs or reassignments, and conversation tasks do not change. - Other validation errors that run later (in the service update) are replaced by the 409 for these requests. Each request is still rejected, and the 409 points the run to `paperclip_finish` or `paperclip_block`. The onboarding parent-blocker error keeps its 400, because the guard runs that check first. - If a request names the current assignee with a reference that is not its id, the guard does not apply, and the request follows the earlier rules. - No migration. Related gaps this PR does not change: - The authority check after the response turns a committed mutation into `outcome: unknown`. - The runner hides tool binding errors as "Paperclip semantic operation failed". - The `paperclip_finish` monitor-wait error does not say that the run lost its lock. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, in Claude Code with tool use (shell, file edits, test runs). It did the root-cause analysis from run events and the activity log, wrote the code and tests, and ran local verification. ## 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 - [ ] 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: Paperclip --- ...ue-agent-mutation-ownership-routes.test.ts | 117 ++++++++++++++++++ server/src/routes/issues.ts | 47 +++++++ 2 files changed, 164 insertions(+) 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 68f1c5eda6f..bd038411f98 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -165,6 +165,7 @@ const mockObserveCrossIssueInfluence = vi.hoisted(() => vi.fn(async () => null)) const mockRetryWorkspaceExport = vi.hoisted(() => vi.fn()); const mockRequestWorkspaceRecovery = vi.hoisted(() => vi.fn()); const mockRequestTaskRetry = vi.hoisted(() => vi.fn()); +const mockOnboardingFirstTaskDependenciesAllowed = vi.hoisted(() => vi.fn(async () => undefined)); const mockGetExperimental = vi.hoisted(() => vi.fn()); const mockResumeWorkspaceFinalization = vi.hoisted(() => vi.fn()); @@ -176,6 +177,10 @@ function registerRouteMocks() { }) }; }); vi.doMock("../services/task-retry.js", () => ({ requestTaskRetry: mockRequestTaskRetry })); + vi.doMock("../services/onboarding-first-task-admission.js", async () => ({ + ...await vi.importActual("../services/onboarding-first-task-admission.js"), + assertOnboardingFirstTaskDependenciesAllowed: mockOnboardingFirstTaskDependenciesAllowed, + })); vi.doMock("../services/task-interruption-presentation.js", () => ({ taskInterruptionPresentation: vi.fn(async () => null), taskInterruptionPresentations: vi.fn(async () => new Map()), @@ -2980,6 +2985,118 @@ describe("agent issue mutation checkout ownership", () => { }); }); + describe("native run changing its own task status", () => { + const nativeRun = { + id: ownerRunId, + companyId, + agentId: ownerAgentId, + runtimeMode: "native", + status: "running", + contextSnapshot: { issueId }, + }; + const runActor = () => ({ ...ownerActor(), source: "agent_jwt" }); + + beforeEach(() => { + mockIssueService.getById.mockResolvedValue(makeIssue({ executionRunId: ownerRunId, checkoutRunId: ownerRunId })); + mockHeartbeatService.getRun.mockResolvedValue(nativeRun); + }); + + it.each([ + ["in_review", {}], + ["in_review", { assigneeAgentId: ownerAgentId }], + ["done", {}], + ["blocked", {}], + ["todo", { blockedByIssueIds: [] }], + ["blocked", { blockedByIssueIds: ["bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"] }], + ])("rejects %s before it releases the run's own execution lock (%j)", async (status, extra) => { + const res = await request(createApp(runActor())).patch(`/api/issues/${issueId}`).send({ status, ...extra }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ code: "native_run_status_reported_by_finish", requestedStatus: status }); + expect(res.body.error).toContain("paperclip_finish"); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled(); + }); + + it("still lets the run edit other fields of its task", async () => { + const res = await request(createApp(runActor())).patch(`/api/issues/${issueId}`).send({ title: "Renamed by its run" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["a legacy run", { runtimeMode: "legacy" }, {}], + ["a native run that no longer owns execution", {}, { executionRunId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", checkoutRunId: null }], + ["a native run that is no longer running", { status: "failed" }, {}], + ])("leaves the status change to the existing rules for %s", async (_label, runOverrides, issueOverrides) => { + mockHeartbeatService.getRun.mockResolvedValue({ ...nativeRun, ...runOverrides }); + mockIssueService.getById.mockResolvedValue(makeIssue({ executionRunId: ownerRunId, checkoutRunId: ownerRunId, ...issueOverrides })); + + const res = await request(createApp(runActor())).patch(`/api/issues/${issueId}`).send({ status: "in_review" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject({ status: "in_review" }); + expect(mockIssueService.update).toHaveBeenCalledTimes(1); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "in_review" }); + }); + + it("does not intercept a native run handing its task to a human", async () => { + // A native handoff still stops the outgoing run first; confirm that stop + // so the request reaches the update instead of the stop-unconfirmed 409. + mockHeartbeatService.cancelRun.mockResolvedValue({ ...nativeRun, status: "cancelled" }); + const handoff = { assigneeAgentId: null, assigneeUserId: "board-user", status: "in_review" }; + + const res = await request(createApp(runActor(), createRunContextDb({}, ownerAgentId, ownerRunId, [], true))) + .patch(`/api/issues/${issueId}`).send(handoff); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject(handoff); + expect(mockHeartbeatService.cancelRun).toHaveBeenCalledWith(ownerRunId, expect.anything(), expect.anything()); + expect(mockIssueService.update).toHaveBeenCalledTimes(1); + expect(mockHeartbeatService.cancelRun.mock.invocationCallOrder[0]) + .toBeLessThan(mockIssueService.update.mock.invocationCallOrder[0]!); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject(handoff); + }); + + it("does not intercept a native conversation reply moving to review", async () => { + mockIssueService.getById.mockResolvedValue(makeIssue({ executionRunId: ownerRunId, checkoutRunId: ownerRunId, + conversationAgentId: ownerAgentId, conversationUserId: "board-user" })); + + const res = await request(createApp(runActor())).patch(`/api/issues/${issueId}`).send({ status: "in_review" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject({ status: "in_review" }); + expect(mockIssueService.update).toHaveBeenCalledTimes(1); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "in_review" }); + }); + + it("keeps the dependency rule's error for a blocker it rejects", async () => { + const { HttpError: CurrentHttpError } = await vi.importActual("../errors.js"); + mockOnboardingFirstTaskDependenciesAllowed.mockRejectedValueOnce( + new CurrentHttpError(400, "Remove the onboarding task from blockedByIssueIds.", { code: "onboarding_first_task_parent_blocker" })); + + const res = await request(createApp(runActor())).patch(`/api/issues/${issueId}`) + .send({ status: "blocked", blockedByIssueIds: ["bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"] }); + + expect(res.status, JSON.stringify(res.body)).toBe(400); + expect(res.body.details).toMatchObject({ code: "onboarding_first_task_parent_blocker" }); + expect(mockOnboardingFirstTaskDependenciesAllowed).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + companyId, actorAgentId: ownerAgentId, actorRunId: ownerRunId, blockedByIssueIds: ["bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"], + })); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("does not intercept a board user changing the status", async () => { + const res = await request(createApp(boardActor())).patch(`/api/issues/${issueId}`).send({ status: "in_review" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject({ status: "in_review" }); + expect(mockIssueService.update).toHaveBeenCalledTimes(1); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "in_review" }); + }); + }); + describe("task watchdog scope grants", () => { const watchdogRunId = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaab"; const watchdogReportIssueId = "cccccccc-cccc-4ccc-8ccc-cccccccccccd"; diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 9be40978e26..030d64a2b1a 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -36,6 +36,7 @@ import { reconstructNativeWorkspace, unreconciledNativeExportPredicate } from ". import { reconstructWorkspaceSchema } from "@paperclipai/shared"; import { queuedInteractionId, readQueuedInteractionResponse, hasQueuedInteractionResponse } from "../services/queued-interaction-response.js"; import { deliverConversationComments, isConversation } from "../services/agent-conversations.js"; +import { assertOnboardingFirstTaskDependenciesAllowed } from "../services/onboarding-first-task-admission.js"; import { resolveChatTaskAssignment } from "../services/chat-task-assignment.js"; import { issueRecoveryActionReadModel } from "../services/issue-recovery-actions.js"; import { inspectWorkspaceSource } from "../services/workspace-source-inspection.js"; @@ -13857,6 +13858,51 @@ export function issueRoutes( if (active) throw conflict("An agent currently owns execution. Wait for it to finish, or manage execution in Paperclip.", { code: "MCP_ACTIVE_EXECUTION" }); } + /** + * A native run reports its outcome through paperclip_finish/paperclip_block, + * and the status arbiter applies the task status. A status change sent here + * by that same run (for example through call_api) releases the run's own + * execution lock. Every runner tool after that, paperclip_finish included, + * is unauthorized, so the run can never settle and recovery blocks the task. + * Reject the change before it commits, so the run can correct it in the same + * turn. Handoffs and reassignments are a different path and are unaffected. + * + * Conversation tasks are excluded: a conversation turn settles from its final + * reply (nativeConversationReplyResult), not from a runner tool, so a reply + * that moves the conversation to review still settles without the lock. + */ + 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 }, + ) { + 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 (isConversation(issue)) return; + const keepsAssignment = + (req.body.assigneeAgentId === undefined || req.body.assigneeAgentId === issue.assigneeAgentId) && + (req.body.assigneeUserId === undefined || req.body.assigneeUserId === issue.assigneeUserId); + if (!keepsAssignment) return; + const run = await heartbeat.getRun(runId); + if (!run || run.runtimeMode !== "native" || run.status !== "running" + || run.agentId !== req.actor.agentId || run.companyId !== issue.companyId) return; + // A blocker that the dependency rules reject anyway keeps its specific + // error; otherwise the run reports the block with paperclip_block. + if (Array.isArray(req.body.blockedByIssueIds) && req.body.blockedByIssueIds.length > 0) { + await assertOnboardingFirstTaskDependenciesAllowed(db, { + companyId: issue.companyId, actorAgentId: req.actor.agentId, actorRunId: runId, + blockedByIssueIds: req.body.blockedByIssueIds, parentIssueId: issue.parentId, + }); + } + throw conflict( + "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 }, + ); + } + router.patch( "/issues/:id", (req, _res, next) => { @@ -13887,6 +13933,7 @@ export function issueRoutes( ); if (!existing) return; if (!(await assertIssueReadAllowed(req, res, existing))) return; + await assertNativeRunKeepsOwnExecutionLock(req, existing); if (req.body.expectedStatusVersion !== undefined && ( existing.statusVersion !== req.body.expectedStatusVersion || existing.status !== "backlog" )) {