mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 06:42:06 +01:00
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 493e229d93.
## 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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
3ee4c5b613
commit
51ea624344
2 files changed
+164
No files matched your search
@@ -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<typeof import("../services/onboarding-first-task-admission.js")>("../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<typeof import("../errors.js")>("../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";
|
||||
|
||||
@@ -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"
|
||||
)) {
|
||||
|
||||
Reference in new issue
Block a user