mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-03 17:51:58 +01:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
912cff9392
commit
0748b90062
5 files changed
+122
-3
No files matched your search
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in new issue
Block a user