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" )) {