From 358977454ba449f37dee1a2c5244e53be34bd0ce Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:42:00 -0600 Subject: [PATCH] test(native): cover unknown-outcome recovery and hold settlement without a recorded PID space (#20649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - #20628 settles a host-tool callback that a server restart orphaned, and settles the no-replay holds that earlier releases left for a completed turn > - Review of #20628 asked for two more cases. That PR merged before the commit with those cases reached it > - This pull request adds only those tests > - The benefit is that the two edge rules of #20628 stay covered ## Linked Issues or Issue Description Refs #20628 **What happened?** The review of #20628 named two untested rules. First, a recovery authority that throws `semantic_tool_outcome_unknown` must settle only for a callback from an earlier controller, and must rethrow for this controller's own callback. Second, a coordinator row saved before `controller_host_hash` existed must still settle its holds through the stopped controller boot. **Expected behavior** Both rules have regression tests. **Steps to reproduce** 1. Read `recoverToolCallback` and `native_completed_turn_cleanup_hold` on master. 2. Search the tests for a thrown `semantic_tool_outcome_unknown`, or for a row with a null `controllerHostHash`. Neither exists. **Paperclip version or commit** master at 2150820bd8. ## What Changed - `durable-prp-control-plane.test.ts`: an authority that throws `semantic_tool_outcome_unknown` settles as `tool_call_interrupted_by_restart` for `earlier_controller`, and rethrows for `this_controller`. Any other rejection rethrows. - `completed-turn-cleanup-hold-settle.test.ts`: a coordinator row without `controllerHostHash`, whose server boot is recorded stopped in `controller_instances`, settles the run's holds through the controller-instance proof. No attestation is needed. ## Verification - `npx vitest run src/control-plane/durable-prp-control-plane.test.ts -t "orphaned by a server restart"` in `packages/paperclip-runner`: 7 passed. - `npx vitest run src/__tests__/completed-turn-cleanup-hold-settle.test.ts` in `server`: 5 passed. - `tsc --noEmit -p packages/paperclip-runner/tsconfig.test.json` and `tsc --noEmit -p server/tsconfig.json` pass. ## Risks - Low risk. Test-only change. No production code changes. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use and code execution. ## 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 - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- .../durable-prp-control-plane.test.ts | 20 ++++++++++++++++ ...completed-turn-cleanup-hold-settle.test.ts | 24 +++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts b/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts index 6eb19d6b58e..fa77aafb347 100644 --- a/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts +++ b/packages/paperclip-runner/src/control-plane/durable-prp-control-plane.test.ts @@ -5635,6 +5635,26 @@ describe("host-tool callback orphaned by a server restart", () => { } }); + it.each([ + { admission: "earlier_controller" as const, settles: true }, + { admission: "this_controller" as const, settles: false }, + ])("treats an authority's unknown outcome by owner ($admission)", async ({ admission, settles }) => { + // The receipt owner proves the effect may have happened, with no result. + const unknown = Object.assign(new Error("paperclip_runner_instruction_outcome_unknown"), { code: "semantic_tool_outcome_unknown" }); + const call = { runId: identity.runId, admission, call: { tool: "update_agent_instructions", callId: `exec-unknown-${admission}`, arguments: {} }, + recover: async () => { throw unknown; } }; + if (settles) { + await expect(recoverToolCallback(call)).resolves.toMatchObject({ isError: true, + result: { error: { code: TOOL_CALLBACK_INTERRUPTED_CODE, outcome: "unknown" } } }); + } else { + // A callback this controller owns keeps the earlier fail-closed rejection. + await expect(recoverToolCallback(call)).rejects.toBe(unknown); + } + // Any other rejection is never turned into a settlement. + await expect(recoverToolCallback({ ...call, recover: async () => { throw new Error("authority unavailable"); } })) + .rejects.toThrow("authority unavailable"); + }); + it("keeps a callback pending while this process still runs it", async () => { const callId = "exec-still-running"; // This process still owns the dispatch (for example a session recreated in diff --git a/server/src/__tests__/completed-turn-cleanup-hold-settle.test.ts b/server/src/__tests__/completed-turn-cleanup-hold-settle.test.ts index 5b38fb79075..842457491c4 100644 --- a/server/src/__tests__/completed-turn-cleanup-hold-settle.test.ts +++ b/server/src/__tests__/completed-turn-cleanup-hold-settle.test.ts @@ -6,6 +6,7 @@ import { activityLog, agents, companies, + controllerInstances, createDb, heartbeatRuns, issueRecoveryActions, @@ -16,6 +17,7 @@ import { import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; import { currentControllerBootId, currentProcessSpaceHash } from "../services/controller-process-identity.js"; import { getExecutionBlocker } from "../services/execution-blocker.js"; +import { registerControllerInstanceDb, resetControllerInstanceLeaseForTests } from "../services/controller-instance-lease.js"; import { createOwnershipReportState, runOwnershipReconciliationPass } from "../services/recovery/ownership-invariant-report.js"; const support = await getEmbeddedPostgresTestSupport(); @@ -65,7 +67,8 @@ suite("completed-turn cleanup holds on the shared ownership path", () => { afterAll(async () => { await temporary?.cleanup(); }); - async function quarantinedRun(options: { controllerAlive?: boolean; turnCompleted?: boolean; filesSaved?: boolean } = {}) { + async function quarantinedRun(options: { controllerAlive?: boolean; turnCompleted?: boolean; filesSaved?: boolean; + controllerBootId?: string; hostHashUnrecorded?: boolean } = {}) { const issueId = randomUUID(), runId = randomUUID(), leaseId = randomUUID(); const finishedAt = new Date(Date.now() - 60_000); await db.insert(issues).values({ id: issueId, companyId, title: "Set up visual reviews", status: "blocked", @@ -83,7 +86,8 @@ suite("completed-turn cleanup holds on the shared ownership path", () => { ...(options.controllerAlive ? { controllerBootId: currentControllerBootId(), controllerPid: process.pid, controllerHostHash: currentProcessSpaceHash(), controllerProcessStartedAt: new Date(Date.now() - process.uptime() * 1_000) } - : { controllerBootId: randomUUID(), controllerPid: await exitedPid(), controllerHostHash: currentProcessSpaceHash(), + : { controllerBootId: options.controllerBootId ?? randomUUID(), controllerPid: await exitedPid(), + controllerHostHash: options.hostHashUnrecorded ? null : currentProcessSpaceHash(), controllerProcessStartedAt: new Date(finishedAt.getTime() - 20 * 60_000) }) }); const noReplay = { runId, policy: "preserve_without_replay_v1", replay: "blocked", recordedAt: finishedAt.toISOString(), actionOutcome: "unknown" }; @@ -148,6 +152,22 @@ suite("completed-turn cleanup holds on the shared ownership path", () => { expect(await pass()).toMatchObject({ inProgress: 0 }); }, 60_000); + it("proves a controller saved before its PID space was recorded gone through its stopped server boot", async () => { + const bootId = randomUUID(); + await db.insert(controllerInstances).values({ bootId, processSpace: "old-container", pid: 7, + leaseExpiresAt: new Date(Date.now() - 5 * 60_000), stoppedAt: new Date(Date.now() - 4 * 60_000) }); + registerControllerInstanceDb(db); + try { + const f = await quarantinedRun({ controllerBootId: bootId, hostHashUnrecorded: true }); + expect(await pass()).toMatchObject({ inProgress: 1, settled: 1 }); + expect(await hold(f.quarantineHoldId)).toMatchObject({ outcome: "restored" }); + expect(await getExecutionBlocker(db, companyId, f.issueId)).toBeNull(); + } finally { + resetControllerInstanceLeaseForTests(); + await db.delete(controllerInstances); + } + }, 60_000); + it("keeps the holds while the controller that may still hold the gate is alive", async () => { const f = await quarantinedRun({ controllerAlive: true }); expect(await pass()).toMatchObject({ inProgress: 1, settled: 0 });