From 0748b9006273dae7ef8c303905ca0ffa0221a708 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 2 Nov 2026 10:41:44 -0600 Subject: [PATCH] fix(server): stop goal-less native runs before board task closure (#18616) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A task owns its active run and its tool access. > - A board user can close a task while that run is still working. > - The task update route stopped native runs only when they had an active session goal. > - Providers without goals kept running after the route cleared task ownership. > - This pull request stops those native runs before a board closure releases ownership. > - The benefit is an orderly stop instead of tool denials and failed finalization. ## Linked Issues or Issue Description **What happened?** A board user marked a task Done while its native OpenCode run was active. The route cleared its execution lock without stopping the provider. Later tool calls failed with `paperclip_runner_tool_binding_not_authorized`. The final wake fence then failed with `native_current_wake_comments_binding_changed`, leaving workspace finalization in retry. Cancelled also released ownership before its later Stop call. **Expected behavior** A board closure must confirm native Stop before it releases task ownership. A failed Stop must leave the task open. The provider does not need to support session goals. **Steps to reproduce** 1. Start a native task with a provider that has no active session goal. 2. Mark the task Done while its run is active. 3. Observe that the old route updates the task without cancelling the run. ## What Changed - Reuse the existing audited cancellation path before a board user marks an active native task Done or Cancelled. - Keep the existing 409 response when Stop is unconfirmed. - Record the board user in the Stop metadata. The existing acknowledged-Stop policy then leaves queued input and recovery quiet until a later explicit wake. - Inspect additional runs only when a task has an execution owner; tasks without a run lock keep their existing completion path. - Keep native semantic completion, legacy completion, and goal control behavior. - Add route regressions for stop ordering, failed Stop, and legacy completion. - Update the execution contract and task maps. ## Verification - Red before the fix: three regressions failed. Done had no Stop; Cancelled stopped after the update; an unconfirmed Stop allowed Done. - A second red proved that a Stop receipt without the route's operator metadata was not recognized by the real recovery policy. Both cases pass after the metadata repair. - Final local result on macOS arm64 / Node.js 26.4.0: **348 tests pass across eight suites**. These cover the route, native self-completion authority, cancellation, acknowledged Stop, recovery queues, dependency wakeups, and completion telemetry. - CI found seven sibling route failures from an unnecessary active-run lookup on tasks without execution ownership. Those cases failed locally before the correction and pass afterward. - Earlier concurrent local runs hit a module-load hook timeout and one watchdog socket hang-up. The isolated watchdog case and the complete final eight-suite run pass with a 90-second startup budget. No assertion was removed. - `node scripts/check-maps.mjs` and `git diff --check`: pass. - `pnpm build` and `pnpm -r typecheck`: pass. Server build and typecheck also pass after the Stop metadata change; the final server build includes the lookup correction. - `pnpm test:run`: attempted; the unchanged chat shard selector failed before executing its selected tests (`No test found ... in lines ...`). This is separate from the PostgreSQL startup failure. CI supplies full-suite verification. - PostgreSQL integration and live-provider acceptance did not run locally. Embedded PostgreSQL failed before app startup with `could not create semaphores: No space left on device`. No usable isolated local database runtime was available. CI supplies the PostgreSQL regression environment. The full live OpenRouter workflow remains unverified. - The first final-head CI pass hit a 15-second timeout in the unchanged `attention-retention-work.test.ts` blocker-history fixture. Cleanup then deadlocked and later inserts reused IDs. This service test does not call the changed route. The original log is [retained here](https://github.com/paperclipai/paperclip/actions/runs/41476191754/job/125384289220). The one allowed rerun also failed on the same commit: the `running` case exceeded 15 seconds again ([rerun log](https://github.com/paperclipai/paperclip/actions/runs/41476191754/job/125393915025)). The rerun otherwise passed 2,346 tests. No retention source or assertion was changed. This repeatable failure is the remaining CI gate; the PR is **not ready to merge**. The next step is to isolate that retention fixture/query in a usable PostgreSQL environment and repair its cause before rerunning CI. - Greptile reviewed final commit `b3cb41af2d911e9ced219e846cada3ecec835dbf` at **5/5**, with no unresolved review threads. Greptile is green. CI is blocked by the repeated retention test timeout described above. - CI native cancellation evidence: all 16 tests in `server/src/__tests__/native-cancellation-request.integration.test.ts` pass ([job log](https://github.com/paperclipai/paperclip/actions/runs/41476191754/job/125384289222)). - CI PostgreSQL wake-queue evidence: all 96 tests in `server/src/modules/wake-queue/adapters/postgres.test.ts` pass ([job log](https://github.com/paperclipai/paperclip/actions/runs/41476191754/job/125384289470)). ## Risks Board closure now waits for native Stop even when the provider has no goal. A failed or uncertain Stop keeps the task open. The change uses the existing cancellation and workspace-preservation path. It adds no retry loop, credential access, schema change, or publication authority. Direct service updates and agent semantic completion retain their existing paths. ## Model Used OpenAI GPT-6 through Codex, with reasoning, repository tools, shell execution, and browser inspection. The exact serving snapshot ID and context window are not exposed in this session. ## 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 - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] 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 | 10 +++ maps/agent-access-map/tasks.md | 6 ++ maps/feature-map/tasks.md | 3 + ...issue-update-comment-wakeup-routes.test.ts | 88 +++++++++++++++++++ server/src/routes/issues.ts | 18 +++- 5 files changed, 122 insertions(+), 3 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 1c2ebb86dec..dd0378688f5 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -239,6 +239,16 @@ The work is complete and terminal. The work will not continue and is terminal. +When a board user marks an active native task `done` or `cancelled` through +the task update API, Paperclip must confirm the run's cancellation before +clearing its task execution lock. This also applies to providers without +session goals. An unconfirmed stop leaves the task open. Native agents still +report their own completion through `paperclip_finish` or `paperclip_block`, +so normal result and workspace finalization retain their existing ordering. +The cancellation records the board user's identity. Its acknowledged operator +Stop receipt keeps recovery from promoting old queued input or creating a retry +while the task closure commits. + ## 4. Agent-Owned vs User-Owned Execution The execution model differs depending on assignee type. diff --git a/maps/agent-access-map/tasks.md b/maps/agent-access-map/tasks.md index 681376b7a38..7f80a1314c0 100644 --- a/maps/agent-access-map/tasks.md +++ b/maps/agent-access-map/tasks.md @@ -349,6 +349,12 @@ Agent access: `partial` — authorized REST/call_api and CLI status updates; nat Policy: Task mutation/run/assignment/recovery/resolver gates. Blocked requires unresolved blocker/pending input/approval or unblockDescriptor; agent can name only SELF unblock owner. In_review requires actionable path. Native bound completion contract/criterion revision, deliverable evidence and verification must agree; done rejects unfinished work; needs_review names valid different reviewer. Pending input/approval/review/blocker can keep task open. Final state commits after provider turn/workspace finalization; report never overwrites successor/already terminal state. +Board PATCH to Done or Cancelled stops an active native run before releasing +execution ownership, even when it has no session goal. An unconfirmed stop +returns 409 without updating the task. Native self-completion still uses its +semantic finish tools. Route regression: +[issue-update-comment-wakeup-routes.test.ts](../../server/src/__tests__/issue-update-comment-wakeup-routes.test.ts). + Evidence: `source-only` — reviewed at `3bbc2104b65caf9f7763938a01260a5673e12f7a`; [issues.ts](../../server/src/routes/issues.ts), [paperclip-runner-tool-authority.ts](../../server/src/services/native-runtime/paperclip-runner-tool-authority.ts), [native-completion-feedback.ts](../../server/src/services/native-runtime/native-completion-feedback.ts), [native-completion-feedback.test.ts](../../server/src/services/native-runtime/native-completion-feedback.test.ts), [native-deliverable-feedback.test.ts](../../server/src/services/native-runtime/native-deliverable-feedback.test.ts), [paperclip-runner-tool-authority.test.ts](../../server/src/services/native-runtime/paperclip-runner-tool-authority.test.ts), [issue-stalled-review-decision-routes.test.ts](../../server/src/__tests__/issue-stalled-review-decision-routes.test.ts). No live execution claimed. Verification: `not-run` — Actual native accepted report→provider stop→workspace finalization→persisted status/output; stale/failed report correction without repeating work. Test blocked/review/done, pending question/blocker, invalid/self/foreign reviewer, missing output and duplicate report. diff --git a/maps/feature-map/tasks.md b/maps/feature-map/tasks.md index 28dfc336689..a9afa296295 100644 --- a/maps/feature-map/tasks.md +++ b/maps/feature-map/tasks.md @@ -20,6 +20,9 @@ Agent access: [companion](../agent-access-map/tasks.md). - `discussion`: `tasks/discussion` — persist comments, references, and files on the task. - `completion`: `tasks/completion` — distinguish backlog, active work, review, blocked, done, and cancelled outcomes. + A board user closing an active native task must stop its run before the task + releases execution ownership, including providers without session goals. + Verify Done and Cancelled, an unconfirmed stop, and ordinary agent completion. - `privacy`: `tasks/privacy` — create private tasks, share or revoke access, and inspect parent/project inheritance before changing visibility. The [Product task-properties journey](../../tests/runner-e2e/PRODUCT-JOURNEYS.md) diff --git a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts index 81108a04abf..5ff825d9c02 100644 --- a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts +++ b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts @@ -1,5 +1,6 @@ import express from "express"; import request from "supertest"; +import { isAcknowledgedNativeStop } from "../services/acknowledged-native-stop.js"; import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("../services/activity-log.js", () => ({ @@ -335,6 +336,93 @@ describe("issue update comment wakeups", () => { }, ); + it.each(["done", "cancelled"])("stops a goal-less native run before a board marks its task %s", async (status) => { + const existing = makeIssue({ status: "in_progress", assigneeAgentId: ASSIGNEE_AGENT_ID, + assigneeUserId: null, executionRunId: SOURCE_RUN_ID }); + mockIssueService.getById.mockResolvedValue(existing); + mockHeartbeatService.getRun.mockResolvedValue({ id: SOURCE_RUN_ID, companyId: existing.companyId, + agentId: ASSIGNEE_AGENT_ID, runtimeMode: "native", status: "running", + nativeIssueId: existing.id, contextSnapshot: { issueId: existing.id } }); + const effects: string[] = []; + mockHeartbeatService.cancelRun.mockImplementation(async () => { + effects.push("stop"); + return { id: SOURCE_RUN_ID, companyId: existing.companyId, status: "cancelled" }; + }); + mockIssueService.update.mockImplementation(async () => { + effects.push("update"); + return { ...existing, status, executionRunId: null }; + }); + + const res = await request(await createApp()).patch(`/api/issues/${existing.id}`).send({ status }); + + expect(res.status).toBe(200); + expect(effects).toEqual(["stop", "update"]); + expect(mockHeartbeatService.cancelRun).toHaveBeenCalledOnce(); + expect(mockHeartbeatService.cancelRun).toHaveBeenCalledWith(SOURCE_RUN_ID, + "Cancelled before issue terminalization", expect.objectContaining({ + errorCode: "issue_terminalized", + resultJson: expect.objectContaining({ terminalizationStopConfirmed: true }), + eventPayload: { issueId: existing.id, status }, + })); + const stopMetadata = mockHeartbeatService.cancelRun.mock.calls[0]![2].resultJson; + // Exercise the recovery consumer with the route's actual producer fields. + // A Stop receipt alone must not authorize this intentional-stop exception. + expect(isAcknowledgedNativeStop({ id: SOURCE_RUN_ID, companyId: existing.companyId, + status: "cancelled", nativeIssueId: existing.id, resultJson: { ...stopMetadata, + nativeCancellation: { schema: "paperclip.native-cancellation.v1", runId: SOURCE_RUN_ID, + companyId: existing.companyId, issueId: existing.id, scope: "run", + reasonCode: "cancellation_run_only", dispatchState: "acknowledged", dispatched: true, + intentAuditId: "stop-intent", acknowledgementAuditId: "stop-acknowledgement" }, + } })).toBe(true); + expect(res.body).toMatchObject({ status, executionRunId: null }); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("keeps the task open when a goal-less native run cannot confirm its stop", async () => { + const existing = makeIssue({ status: "in_progress", assigneeAgentId: ASSIGNEE_AGENT_ID, + assigneeUserId: null, executionRunId: SOURCE_RUN_ID }); + mockIssueService.getById.mockResolvedValue(existing); + mockHeartbeatService.getRun.mockResolvedValue({ id: SOURCE_RUN_ID, companyId: existing.companyId, + agentId: ASSIGNEE_AGENT_ID, runtimeMode: "native", status: "running", + nativeIssueId: existing.id, contextSnapshot: { issueId: existing.id } }); + mockHeartbeatService.cancelRun.mockResolvedValue(null); + mockIssueService.update.mockResolvedValue({ ...existing, status: "done", executionRunId: null }); + + const res = await request(await createApp()).patch(`/api/issues/${existing.id}`).send({ status: "done" }); + + expect(res.status).toBe(409); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + + it("does not inspect agent runs when closing a task without an execution owner", async () => { + const existing = makeIssue({ status: "in_progress", assigneeAgentId: ASSIGNEE_AGENT_ID, + assigneeUserId: null, executionRunId: null }); + mockIssueService.getById.mockResolvedValue(existing); + mockIssueService.update.mockResolvedValue({ ...existing, status: "done" }); + + const res = await request(await createApp()).patch(`/api/issues/${existing.id}`).send({ status: "done" }); + + expect(res.status).toBe(200); + expect(mockHeartbeatService.getRun).not.toHaveBeenCalled(); + expect(mockHeartbeatService.getActiveRunForAgent).not.toHaveBeenCalled(); + expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled(); + }); + + it("preserves legacy completion without a native goal", async () => { + const existing = makeIssue({ status: "in_progress", assigneeAgentId: ASSIGNEE_AGENT_ID, + assigneeUserId: null, executionRunId: SOURCE_RUN_ID }); + mockIssueService.getById.mockResolvedValue(existing); + mockHeartbeatService.getRun.mockResolvedValue({ id: SOURCE_RUN_ID, companyId: existing.companyId, + agentId: ASSIGNEE_AGENT_ID, runtimeMode: "legacy", status: "running", contextSnapshot: { issueId: existing.id } }); + mockIssueService.update.mockResolvedValue({ ...existing, status: "done", executionRunId: null }); + + const res = await request(await createApp()).patch(`/api/issues/${existing.id}`).send({ status: "done" }); + + expect(res.status).toBe(200); + expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled(); + }); + it.each([false, true])("still wakes explicitly reopened work (reassigned: %s)", async (reassigned) => { const existing = makeIssue({ status: "done", assigneeAgentId: PREVIOUS_AGENT_ID, assigneeUserId: null }); const agentId = reassigned ? ASSIGNEE_AGENT_ID : PREVIOUS_AGENT_ID; diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 1326833bd9a..3d6af2376f9 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -14708,16 +14708,28 @@ export function issueRoutes( issueId: existing.id, agentId: existing.assigneeAgentId, }); - const runToStopForTerminalization = goalStopAction + const runToStopForTerminalization = goalStopAction || + (req.actor.type === "board" && existing.executionRunId) ? await resolveActiveIssueRun(existing) : null; - if (goalStopAction && runToStopForTerminalization) { + // Closing a native task revokes its tool binding. Retire the board's + // active executor before releasing that binding, even without a goal. + const mustStopNativeRun = req.actor.type === "board" && + runToStopForTerminalization?.runtimeMode === "native"; + if (runToStopForTerminalization && (goalStopAction || mustStopNativeRun)) { const cancelled = await heartbeat.cancelRun( runToStopForTerminalization.id, "Cancelled before issue terminalization", { errorCode: "issue_terminalized", - resultJson: { terminalizationStopConfirmed: true, issueMutationStopId }, + resultJson: { + terminalizationStopConfirmed: true, + issueMutationStopId, + ...(actor.actorType === "user" ? { + cancelledByActorType: "user", + cancelledByUserId: actor.actorId, + } : {}), + }, eventMessage: "run cancelled before issue terminalization", eventPayload: { issueId: existing.id,