From 9b71d03215ff58edf74866b083bf0811a45b76e5 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 8 Nov 2026 21:53:13 -0800 Subject: [PATCH 1/8] fix: recover interrupted native preparations and unblock upgrades (#20177) ## Thinking Path > - Paperclip manages AI-agent companies and their work. > - A native run prepares its workspace before a runner PID or checkpoint exists. > - A server crash can interrupt that stage and leave its controller on another host. > - Recovery needs a durable owner proof, a delivered recovery wake, and the saved execution contract. > - The old paths missed controller lease registration and boot proof, skipped checkpoint-free preparations, and counted retained work as live upgrade blockers. > - This PR connects preparation to the existing controller lease and recovery paths, and separates retained work from the upgrade census. > - The result is bounded recovery without a second executor or loss of saved work. ## Linked Issues or Issue Description **What happened?** A native preparation interrupted by a server crash could remain running indefinitely with no runner PID or checkpoint. Native controller stamps did not wake the shared instance lease. That lease was disabled on volume-backed Railway services, and native takeover omitted the boot binding needed for its cross-host proof. Scoped recovery selected checkpoint-backed runs but skipped these preparations. The upgrade probe then counted the retained row as current execution and could block every later upgrade. **Expected behavior** Track the preparation's controller before a runner starts. Recover it through the normal classifier after proving its owner stopped or fencing that boot. Keep live or unprovable owners held. Deliver recovery wakes to preparations without checkpoints. Preserve unresolved historical work and report it separately from current execution during upgrade observations. **Steps to reproduce** 1. Save an observed native preparation with no runner process, result, provider-start evidence or checkpoint. 2. Replace its controller with a process in another PID space. Let its owner lease expire and send a scoped recovery wake. 3. Before this fix, recovery skips the checkpoint-free preparation or cannot prove its foreign controller stopped. Upgrade readiness continues to count that old row as a live blocker. Related work: #18037, #19651, #19868, and #20168. The foreign-PID guard from #19868 remains. The queued first-turn input fix from #20168 is included through the current base. ## What Changed - Register the existing instance lease when native execution and restart claims are written. Keep passive identity reads free of ownership effects. Keep that lease enabled on volume-backed Railway services unless explicitly disabled. - Preserve the existing PID/start-time proof within the same readable PID space. Bind foreign or unknown-space takeover to its saved boot and last write. A live lease or grace period keeps the run held with a scheduled check. Repeated evidence waits preserve their timestamp. - Include explicitly fenced, checkpoint-free preparations in the existing finite recovery candidate inventory. Normal launch, checkpoint, provider evidence and admission checks still decide whether work may resume. - Expose the existing board stop-confirmation operation for historical foreign-host owners. Bind confirmation to the exact run, boot, PID, process start and host. Reject changed ownership. - Report narrowly defined old, unowned preparations as `retainedBlockedRuns` outside the live upgrade census. Local executions, pending admission, drain state, truncation and positive checkpoint proof retain their barriers. - Update the execution contract and add PostgreSQL, real-route, race and preservation regressions. ## Verification - Genuine red/green regressions cover the old upgrade census, missing native lease registration, the volume-backed default, foreign-host confirmation, and scoped recovery skipping checkpoint-free preparation. Test bodies ran against real PostgreSQL before their corresponding production changes. - Combined verification before the final idle correction (head `055be6c84d9`): **459 passed, 22 skipped, 11 files passed**. Covers controller lease/identity, native restart classification and integration, host confirmation, heartbeat recovery, instance route authorization, upgrade readiness, and queued first-turn input. The focused command used `pnpm exec vitest run` with those 11 suites and `--maxWorkers=1 --no-file-parallelism`. The 22 skipped cases require the Rust debug fake-Codex fixture, which was not built locally; the real-process CI coverage must pass. - `pnpm --filter @paperclipai/server typecheck`: passed on head `055be6c84d9b058064daad0af71bb5d893ab9529`. - Actual route proof uses the real heartbeat service and PostgreSQL. Unauthorized access returns 403. The retained run and owner records remain unchanged. Recovery integration admits one bootstrap claim across two replacement controllers, keeps a live owner held, and preserves local execution. - `pnpm verify`: exit 2, requests broad CI because the execution contract changed. Full local suite not run. Current-head CI must supply broad coverage. - Local source checkout on macOS ARM64, Node v25.6.1, pnpm 9.15.4. Tests use isolated PostgreSQL. No live provider restart is claimed by those fixtures. - CI found an idle-query regression in the first version of the lease wiring. Two focused PostgreSQL regressions reproduced the misplaced identity-read notification and the missing claim notification. The fix moves registration to actual ownership writes. `env -u npm_execpath pnpm exec vitest run server/src/__tests__/maintenance-server-routes.test.ts --maxWorkers=1 --no-file-parallelism`: **17 passed**, including 90 real seconds and two qualified hourly windows with no extra idle queries. An initial local setup attempt inherited npm metadata and did not run tests; clearing that metadata selected the correct pnpm helper. Three focused controller/native suites also passed **168 tests, 22 fixture-gated skips**. Server typecheck passed after the correction on head `4a527326dcd5b3a9e286f0e4c3e761d3056af4d9`. - The older CI server shard also exposed a same-space graceful-restart regression. The new boot fallback had overridden local process-exit proof. Seven suspension cases and a focused unit regression reproduced it; the fix limits boot fallback to foreign or unknown PID spaces. Fresh verification on `acf1065d98150210c1e227c7f3b05a1eb489730b`: **251 passed, 22 fixture-gated skips, 4 files passed**, including all **40** graceful-restart suspension tests. Server typecheck passed. The local regression failed before this correction, and the same-space and foreign-space paths pass together after it. - Current-head CI completed successfully on `acf1065d98150210c1e227c7f3b05a1eb489730b` (run 42568109784). Greptile reviewed that exact head at 5/5 with no unresolved review threads. - Secret and private-data scans passed. No workflow, lockfile or migration changes. ## Risks - The instance lease now applies to volume-backed Railway services. Its existing 30-second renewal, quiescence, grace and self-fence behavior applies while work is held. Idle servers retain the existing no-query behavior. An explicit `PAPERCLIP_CONTROLLER_INSTANCE_LEASE=off` remains available. - A controller lease proves only the server owner's state. It does not prove an agent process stopped or authorize replay, resource release or volume removal. The existing classifier and provider/runner checks remain responsible for those decisions. - Previous-release records keep their shape and saved state. A missing boot lease stays unprovable and requires the scoped stop-confirmation path. A changed confirmation target is rejected. - The upgrade observation does not recover a held task by itself. Source tests do not qualify a hosted replacement. Deployment and the affected task's useful result must be verified separately. - This repairs recovery after a crash; it does not claim to identify or fix every source of heap exhaustion. ## Model Used OpenAI `gpt-6.1-sol`, xhigh reasoning effort, through Codex in T3 Code, with repository, shell, and GitHub tools. Context window size is not exposed by the runtime. ## 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 - [x] 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 | 43 +++++++++- .../controller-instance-lease.test.ts | 23 ++++- .../src/services/controller-instance-lease.ts | 10 +-- .../src/services/heartbeat/recovery.test.ts | 30 +++++++ .../heartbeat/upgrade-readiness.test.ts | 85 +++++++++++++++++++ .../services/heartbeat/upgrade-readiness.ts | 25 ++++-- ...native-accepted-result-lease-settlement.ts | 2 +- .../native-controller-host-recovery.test.ts | 24 ++++++ .../native-controller-host-recovery.ts | 21 ++--- .../native-restart-recovery.test.ts | 10 ++- .../native-runtime/native-restart-recovery.ts | 21 ++++- .../native-runner-checkpoint.ts | 7 +- ...unner-restart-recovery.integration.test.ts | 39 +++++++++ .../native-runtime/native-session-executor.ts | 3 +- 14 files changed, 301 insertions(+), 42 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 2478035988a..67b0b47f82c 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1933,10 +1933,32 @@ judged through that boot's row: - no row (a boot from before the lease): the record stays unprovable. A record with a recorded process group stays unprovable across hosts, because -agent processes may outlive their server. The lease is off on a Railway -deployment with an exclusive volume, which proves a replaced deployment from its -own identity. A stateless Railway service keeps it. -`PAPERCLIP_CONTROLLER_INSTANCE_LEASE=on|off` overrides this. +agent processes may outlive their server. The lease is enabled on volume-backed +Railway services too: a process can crash and restart inside the same deployment, +and some saved ownership records do not contain a deployment identity. +`PAPERCLIP_CONTROLLER_INSTANCE_LEASE=off` explicitly disables it. + +Native execution and restart claims register held preparation with this lease before +a runner PID or checkpoint exists. Passive identity reads do not register work. +Restart recovery uses the recorded controller +boot and last write to prove its owner stopped or to fence it after the existing +grace period when the PID space is foreign or unknown. Within the same readable +PID space, the existing process and start-time proof still governs takeover. +A live owner or a grace period keeps the preparation held, with a +scheduled recovery check. Repeating the same evidence wait preserves its last-write +timestamp so the recovery check cannot keep extending its own grace period. +After owner proof, the normal launch, checkpoint, provider-evidence and admission +checks still govern resumption; controller fencing alone does not prove an agent +process stopped or authorize repeating its work. + +Older native controller records with no verifiable boot lease remain held. Recovery +exposes the existing board stop-confirmation action for an unknown or foreign PID +space. It requires `runtime:confirm_owner_stop` and confirmation of the exact run, +boot, PID, process start and recorded host. A changed owner rejects the confirmation. +The receipt only permits normal recovery to inspect and resume the saved work. +Scoped recovery wakes also select fenced preparations with no runner process or +checkpoint yet. They use the existing finite expired-run inventory and normal +restart classifier; a missing checkpoint cannot silently discard that wake. A record saved before its owner was fully recorded (`no_owner_stamp`, `missing_identity`, `process_space_unrecorded`) stays unprovable and is never @@ -4892,6 +4914,19 @@ Upgrade diagnostics retain their existing `ready` meaning and add bounded retain header evidence. That inventory is preliminary, reports partial coverage, and does not certify workspace durability or volume removal. +The upgrade census separates current execution from an old, unowned native +preparation that recovery explicitly retained as `awaiting_evidence`. This requires +an observed preparation with no result, failure, process identity, or checkpoint +header, and an expired, recorded owner from a different controller boot. Such a +run is counted as `retainedBlockedRuns`, rather than `activeRuns` or +`recoverableRuns`. It remains unchanged and held for its normal recovery path; +the observation does not prove that its old controller stopped or authorize +replay, resource release, or volume removal. A run registered in the current +process still blocks unless it has positive recovery evidence. Before/after +execution counters, pending wake operations, drain state, and a truncated census +continue to prevent readiness. Persisted queued wakes are not deleted by an +upgrade; the pending wake counter measures in-process admission operations. + `importNativeControllerRecovery` is an internal one-source operation for the migration owner while old controller files remain available. It requires the admitted configuration digest and exact supervisor snapshot, locks the issue, diff --git a/server/src/__tests__/controller-instance-lease.test.ts b/server/src/__tests__/controller-instance-lease.test.ts index 7df0d46c85d..53a1eecdac5 100644 --- a/server/src/__tests__/controller-instance-lease.test.ts +++ b/server/src/__tests__/controller-instance-lease.test.ts @@ -13,6 +13,7 @@ import { ENVIRONMENT_LEASE_CONTROLLER_KEY } from "../services/environment-lease- import { proveOwnerGone, type LocalOwnerStamp } from "../services/owner-proof.js"; import { createOwnershipReportState, runOwnershipReconciliationPass } from "../services/recovery/ownership-invariant-report.js"; import { beginIdleTrackedWork, readTaskDrain } from "../services/task-admission.js"; +import { currentNativeControllerIdentity, evaluateNativeControllerTakeover } from "../services/native-runtime/native-restart-recovery.js"; const support = await getEmbeddedPostgresTestSupport(); const suite = support.supported ? describe : describe.skip; @@ -93,6 +94,24 @@ suite("controller instance lease: a replaced container's records are provable", expect(await proveOwnerGone(stampOf(live))).toEqual({ verdict: "alive", reason: "controller_lease_held" }); }); + it("tracks native preparation before a runner exists and proves a fenced foreign controller", async () => { + const replaced = await otherBoot({ expiredMsAgo: 600_000 }); + await startHere({}, false); + const owner = await currentNativeControllerIdentity(); + await tickControllerInstanceLease(); + expect(await db.select().from(controllerInstances).where(eq(controllerInstances.bootId, owner.bootId))).toEqual([]); + currentControllerProcessStamp(); + await tickControllerInstanceLease(); + expect(await db.select().from(controllerInstances).where(eq(controllerInstances.bootId, owner.bootId))).toHaveLength(1); + const decision = await evaluateNativeControllerTakeover({ now: new Date(), owner: { + controllerBootId: replaced, controllerPid: 7, controllerHostHash: "old-container", + controllerProcessStartedAt: new Date(minutesAgo(15)), updatedAt: new Date(minutesAgo(10)), + leaseOwner: "old-preparation", leaseExpiresAt: new Date(minutesAgo(5)), + }, isProcessAlive: () => { throw new Error("A foreign PID must not be inspected locally"); } }); + expect(decision).toEqual({ allowed: true, reason: "controller_fenced" }); + expect(await proveControllerInstance(db, replaced, null)).toBe("fenced"); + }); + it("keeps an owner alive (and watched) through the grace, and for work written after an idle lapse", async () => { const justExpired = await otherBoot({ expiredMsAgo: 5_000 }); const resumed = await otherBoot({ expiredMsAgo: 600_000 }); @@ -396,7 +415,7 @@ suite("controller instance lease: a replaced container's records are provable", expect(lost).toHaveBeenCalledWith("fenced"); }); - it("records a graceful stop, and is off on a volume-exclusive Railway deployment or when disabled", async () => { + it("records a graceful stop and keeps native preparation fencing on volume-exclusive deployments", async () => { await startHere(); await stopControllerInstanceLease(db); expect(await proveControllerInstance(db, currentControllerBootId(), null)).toBe("stopped"); @@ -410,7 +429,7 @@ suite("controller instance lease: a replaced container's records are provable", expect(controllerInstanceLeaseEnabled()).toBe(true); vi.stubEnv("RAILWAY_VOLUME_ID", "v"); vi.stubEnv("RAILWAY_VOLUME_MOUNT_PATH", "/data"); - expect(controllerInstanceLeaseEnabled()).toBe(false); + expect(controllerInstanceLeaseEnabled()).toBe(true); vi.stubEnv("PAPERCLIP_CONTROLLER_INSTANCE_LEASE", "on"); expect(controllerInstanceLeaseEnabled()).toBe(true); vi.unstubAllEnvs(); diff --git a/server/src/services/controller-instance-lease.ts b/server/src/services/controller-instance-lease.ts index 36ff6f52412..48ebd317e54 100644 --- a/server/src/services/controller-instance-lease.ts +++ b/server/src/services/controller-instance-lease.ts @@ -2,7 +2,7 @@ import { and, eq, isNull, sql } from "drizzle-orm"; import { controllerInstances, type Db } from "@paperclipai/db"; import { logger } from "../middleware/logger.js"; import { - currentControllerBootId, currentControllerDeployment, currentProcessSpaceHash, onControllerStamp, + currentControllerBootId, currentProcessSpaceHash, onControllerStamp, } from "./controller-process-identity.js"; import type { createDeliveryWorkCoordinator } from "./delivery-work-coordinator.js"; import { DELIVERY_QUEUES } from "./delivery-work-notifications.js"; @@ -54,14 +54,8 @@ type LeaseState = { let state: LeaseState | null = null; let registeredDb: Db | null = null; -/** A Railway deployment with an exclusive volume proves a replaced deployment on its own; every - * other host (Docker, Kubernetes, Fly, a stateless Railway service, a rebooted machine) needs the - * lease to prove a replaced process. PAPERCLIP_CONTROLLER_INSTANCE_LEASE=on|off overrides this. */ export function controllerInstanceLeaseEnabled(env: NodeJS.ProcessEnv = process.env): boolean { - const setting = env.PAPERCLIP_CONTROLLER_INSTANCE_LEASE; - if (setting === "off") return false; - if (setting === "on") return true; - return currentControllerDeployment(env)?.exclusiveVolume !== true; + return env.PAPERCLIP_CONTROLLER_INSTANCE_LEASE !== "off"; } function exitOnLostLease(reason: "fenced" | "lease_lost") { diff --git a/server/src/services/heartbeat/recovery.test.ts b/server/src/services/heartbeat/recovery.test.ts index dc27c7641a5..f031b88878c 100644 --- a/server/src/services/heartbeat/recovery.test.ts +++ b/server/src/services/heartbeat/recovery.test.ts @@ -211,6 +211,36 @@ describePostgres("heartbeat recovery module ownership", () => { return (await db.insert(heartbeatRuns).values({ id: runId, companyId, agentId, issueId, invocationSource: "automation", status: "running", runtimeMode: "legacy" }).returning())[0]; } + it.each([false, true])("delivers scoped recovery to a preparation without a runner PID or checkpoint (locally tracked=%s)", async tracked => { + const run = await seedRun(), deps = callbacks(db); + await db.update(agents).set({ adapterType: "paperclip_runner" }).where(eq(agents.id, run.agentId)); + await db.update(heartbeatRuns).set({ runtimeMode: "native", nativeIssueId: run.issueId, + nativePhase: "observed", runnerProfileJson: { nativePreparationFencingVersion: 1 }, + }).where(eq(heartbeatRuns.id, run.id)); + await db.insert(nativeRunFinalizations).values({ runId: run.id, companyId: run.companyId, issueId: run.issueId!, + phase: "observed", leaseOwner: "old-preparation", leaseExpiresAt: new Date(0), controllerHostHash: "operator_attested" }); + const claim: NativeRestartRecoveryClaim = { kind: "bootstrap_incomplete", runId: run.id, leaseOwner: "recovered-owner", + controllerGeneration: 2, providerAttempt: 1, restartKind: "hard", recoveryRequestId: null }; + native.claims.mockResolvedValue([claim]); + if (tracked) deps.activeRunExecutions.add(run.id); + try { + await createHeartbeatRecovery(db, deps).recoverActiveExecutions({ companyId: run.companyId, runId: run.id }); + if (tracked) { + expect(native.claims).not.toHaveBeenCalled(); + expect(deps.executeRun).not.toHaveBeenCalled(); + } else { + expect(native.claims).toHaveBeenCalledWith(expect.objectContaining({ runIds: [run.id] })); + expect(deps.executeRun).toHaveBeenCalledExactlyOnceWith(run.id, { nativeLeaseOwner: claim.leaseOwner, nativeRestartRecovery: claim }); + } + expect(deps.setRunStatusFromLive).not.toHaveBeenCalled(); + expect(deps.terminateHeartbeatRunProcess).not.toHaveBeenCalled(); + } finally { + await Promise.all([...deps.activeRunExecutionPromises]); + await db.delete(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, run.id)); + await db.delete(heartbeatRuns).where(eq(heartbeatRuns.id, run.id)); + } + }); + it("routes an expired pre-capability startup to cleanup before generic orphan ownership holds", async () => { const run = await seedRun(), leaseId = randomUUID(), environmentId = randomUUID(), leaseOwner = randomUUID(); const deadlineAtMs = Date.now() - 1_000, contextId = randomUUID(); diff --git a/server/src/services/heartbeat/upgrade-readiness.test.ts b/server/src/services/heartbeat/upgrade-readiness.test.ts index fb605c53be1..e5f92444e3a 100644 --- a/server/src/services/heartbeat/upgrade-readiness.test.ts +++ b/server/src/services/heartbeat/upgrade-readiness.test.ts @@ -36,6 +36,17 @@ describe("upgrade readiness", () => { it("never treats a truncated census as complete", () => { const rows = Array.from({ length: 1_001 }, (_, i) => ({ id: String(i), recoverable: true })); expect(report({ rows, activeRunIds: [] })).toMatchObject({ ready: false, truncated: true }); + expect(report({ rows: rows.map(row => ({ ...row, recoverable: false, retainedUnowned: true })), + before: idle, after: idle, activeRunIds: [] })).toMatchObject({ ready: false, truncated: true, activeRuns: 0 }); + }); + it("keeps current execution and admission barriers with retained recovery work", () => { + const rows = [{ id: "retained", recoverable: false, retainedUnowned: true }, { id: "remote", recoverable: true }]; + expect(report({ rows })).toMatchObject({ ready: true, activeRuns: 1, recoverableRuns: 1, retainedBlockedRuns: 1 }); + expect(report({ rows, activeRunIds: ["remote", "retained"], after: { ...idle, activeRuns: 2 } })) + .toMatchObject({ ready: false, activeRuns: 2, blockingRuns: 1, retainedBlockedRuns: 0 }); + for (const before of [{ ...idle, activeRuns: 2 }, { ...idle, pendingWakes: 1 }, { ...idle, draining: true }]) { + expect(report({ rows, before }).ready).toBe(false); + } }); }); @@ -54,6 +65,80 @@ postgresTests("upgrade readiness persisted execution evidence", () => { db = createDb(database.connectionString); }, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS); afterAll(async () => { await db?.$client.end(); await database?.cleanup(); vi.unstubAllEnvs(); }); + it("keeps an old unowned preparation visible without making it a live upgrade blocker", async () => { + const companyId = randomUUID(), agentId = randomUUID(), issueId = randomUUID(), runId = randomUUID(); + const owner = await currentNativeControllerIdentity(); + await db.insert(companies).values({ id: companyId, name: "Retained preparation", issuePrefix: `R${companyId.slice(0, 6)}` }); + await db.insert(agents).values({ id: agentId, companyId, name: "Retained preparation", role: "engineer", adapterType: "paperclip_runner" }); + await db.insert(issues).values({ id: issueId, companyId, title: "Interrupted preparation", status: "in_progress", assigneeAgentId: agentId }); + await db.insert(heartbeatRuns).values({ id: runId, companyId, agentId, issueId, nativeIssueId: issueId, + status: "running", runtimeMode: "native", nativePhase: "observed", runnerInstanceId: randomUUID(), nativeSessionId: randomUUID() }); + const retained = { controllerBootId: "previous-controller", leaseOwner: "previous-controller-lease", controllerGeneration: 1, + leaseExpiresAt: new Date(0), recoveryState: "awaiting_evidence", phase: "observed", failureCode: null }; + await db.insert(nativeRunFinalizations).values({ runId, companyId, issueId, ...retained }); + const input = { activeRunIds: new Set(), counters: () => idle }; + try { + const savedRun = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)); + const savedOwner = await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, runId)); + expect(await readUpgradeReadiness(db, input)).toMatchObject({ ready: true, activeRuns: 0, + recoverableRuns: 0, blockingRuns: 0, retainedBlockedRuns: 1, + nativeControllerRecovery: { activeExecutionAdoption: { retainedBlockedRuns: 1 } } }); + const [{ default: express }, { default: request }, { instanceSettingsRoutes }, { errorHandler }] = await Promise.all([ + import("express"), import("supertest"), import("../../routes/instance-settings.js"), import("../../middleware/error-handler.js"), + ]); + const app = express(); + let isInstanceAdmin = false; + app.use((req, _res, next) => { + req.actor = { type: "board", userId: "readiness-test", source: "session", isInstanceAdmin }; + next(); + }); + app.use("/api", instanceSettingsRoutes(db)); + app.use(errorHandler); + await request(app).get("/api/instance/task-drain?upgradeReadiness=1").expect(403); + isInstanceAdmin = true; + const response = await request(app).get("/api/instance/task-drain?upgradeReadiness=1").expect(200); + expect(response.body.upgradeReadiness).toMatchObject({ ready: true, activeRuns: 0, blockingRuns: 0, retainedBlockedRuns: 1 }); + expect(await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId))).toEqual(savedRun); + expect(await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, runId))).toEqual(savedOwner); + expect(await db.select({ status: heartbeatRuns.status }).from(heartbeatRuns).where(eq(heartbeatRuns.id, runId))) + .toEqual([{ status: "running" }]); + expect(await db.select({ state: nativeRunFinalizations.recoveryState }).from(nativeRunFinalizations) + .where(eq(nativeRunFinalizations.runId, runId))).toEqual([{ state: "awaiting_evidence" }]); + for (const patch of [ + { controllerBootId: owner.bootId }, { controllerBootId: null }, { controllerBootId: "" }, + { leaseExpiresAt: new Date(Date.now() + 60_000) }, { leaseExpiresAt: null }, + { leaseOwner: null }, { controllerGeneration: 0 }, { failureCode: "uncertain" }, + { recoveryState: null }, { phase: "workspace_finalizing" }, + ]) { + await db.update(nativeRunFinalizations).set(patch).where(eq(nativeRunFinalizations.runId, runId)); + expect(await readUpgradeReadiness(db, input)).toMatchObject({ ready: false, blockingRuns: 1, retainedBlockedRuns: 0 }); + await db.update(nativeRunFinalizations).set(retained).where(eq(nativeRunFinalizations.runId, runId)); + } + for (const patch of [{ runtimeMode: "legacy" }, { nativePhase: "running" }, { processPid: process.pid }, { processStartedAt: new Date() }]) { + await db.update(heartbeatRuns).set(patch).where(eq(heartbeatRuns.id, runId)); + expect(await readUpgradeReadiness(db, input)).toMatchObject({ ready: false, blockingRuns: 1, retainedBlockedRuns: 0 }); + await db.update(heartbeatRuns).set({ runtimeMode: "native", nativePhase: "observed", processPid: null, processStartedAt: null }) + .where(eq(heartbeatRuns.id, runId)); + } + await db.insert(nativeRunnerCheckpoints).values({ runId, companyId, issueId, part: "header", material: {} }); + expect(await readUpgradeReadiness(db, input)).toMatchObject({ ready: false, blockingRuns: 1, retainedBlockedRuns: 0 }); + await db.delete(nativeRunnerCheckpoints).where(eq(nativeRunnerCheckpoints.runId, runId)); + await db.update(agents).set({ adapterType: "process" }).where(eq(agents.id, agentId)); + expect(await readUpgradeReadiness(db, input)).toMatchObject({ ready: false, blockingRuns: 1, retainedBlockedRuns: 0 }); + await db.update(agents).set({ adapterType: "paperclip_runner" }).where(eq(agents.id, agentId)); + input.activeRunIds.add(runId); + expect(await readUpgradeReadiness(db, { ...input, counters: () => ({ ...idle, activeRuns: 1 }) })) + .toMatchObject({ ready: false, activeRuns: 1, blockingRuns: 1, retainedBlockedRuns: 0 }); + input.activeRunIds.clear(); + let reads = 0; + expect(await readUpgradeReadiness(db, { ...input, counters: () => { + if (reads++ > 0) input.activeRunIds.add(runId); + return { ...idle, activeRuns: input.activeRunIds.size }; + } })).toMatchObject({ ready: false, blockingRuns: 1, retainedBlockedRuns: 0 }); + } finally { + await db.update(heartbeatRuns).set({ status: "failed" }).where(eq(heartbeatRuns.id, runId)); + } + }); it("requires a bound checkpoint and current owner rather than a native label or live process alone", async () => { const companyId = randomUUID(), agentId = randomUUID(), issueId = randomUUID(), runId = randomUUID(); const owner = await currentNativeControllerIdentity(); diff --git a/server/src/services/heartbeat/upgrade-readiness.ts b/server/src/services/heartbeat/upgrade-readiness.ts index e8a2d5663f3..23952159227 100644 --- a/server/src/services/heartbeat/upgrade-readiness.ts +++ b/server/src/services/heartbeat/upgrade-readiness.ts @@ -12,7 +12,7 @@ import { readNativeFinalizationUpgradeReadiness } from "../native-runtime/native const MAX_UPGRADE_RUNS = 1_000; const POST_RESULT_INSPECTION_BUDGET_MS = 2_000; type Counters = { activeRuns: number; pendingWakes: number; draining: boolean }; -type Candidate = { id: string; recoverable: boolean }; +type Candidate = { id: string; recoverable: boolean; retainedUnowned?: boolean }; /** An observation for Cloud's optional rollout policy, never execution authority. * Only explicit upgrade probes read the database; ordinary drain reads stay local. @@ -35,6 +35,13 @@ export async function readUpgradeReadiness(db: Db, input: { r.runner_profile_json->'remoteWorkspace' as "remoteWorkspace", r.runner_profile_json ? 'remoteWorkspace' as "hasRemoteWorkspace", p.id as "pluginLeaseId", f.result_id is not null as "hasResult", + (r.runtime_mode = 'native' and a.adapter_type = 'paperclip_runner' and r.native_phase = 'observed' + and f.recovery_state = 'awaiting_evidence' and f.phase = 'observed' and f.result_id is null and f.failure_code is null + and f.controller_boot_id is not null and f.controller_boot_id <> '' and f.controller_boot_id <> ${owner.bootId} + and f.controller_generation > 0 and f.lease_owner is not null and f.lease_owner <> '' + and f.lease_expires_at <= clock_timestamp() + and r.process_pid is null and r.process_started_at is null and c.run_id is null + ) is true as "retainedUnowned", case when pg_column_size(c.material) <= ${NATIVE_CHECKPOINT_HEADER_INSPECTION_MAX_BYTES} and sum(pg_column_size(c.material)) over () <= ${NATIVE_CHECKPOINT_INSPECTION_TOTAL_MAX_BYTES} then c.material else null end as material, ( r.runtime_mode = 'native' and a.adapter_type = 'paperclip_runner' @@ -79,7 +86,7 @@ export async function readUpgradeReadiness(db: Db, input: { && performance.now() < inspectionDeadline ? await readNativeFinalizationUpgradeReadiness(db, { companyId: row.companyId, runId: row.id, controller: owner, supportedOutageMs: 300_000 }).catch(() => false) : false; - candidates.push({ id: row.id, recoverable: bound && (row.recoverable || finalization) }); + candidates.push({ id: row.id, recoverable: bound && (row.recoverable || finalization), retainedUnowned: row.retainedUnowned }); } // Additive evidence does not strengthen or redefine the existing ready flag. const retained = await readNativeControllerRecoveryInventory(db).catch(() => ({ @@ -89,7 +96,8 @@ export async function readUpgradeReadiness(db: Db, input: { const summary = summarizeUpgradeReadiness({ rows: candidates, before, after: input.counters(), activeRunIds: [...new Set([...beforeIds, ...input.activeRunIds])], controllerBootId: owner.bootId }); return { ...summary, nativeControllerRecovery: { activeExecutionAdoption: { - recoverableRuns: summary.recoverableRuns, blockingRuns: summary.blockingRuns, truncated: summary.truncated, + recoverableRuns: summary.recoverableRuns, blockingRuns: summary.blockingRuns, + retainedBlockedRuns: summary.retainedBlockedRuns, truncated: summary.truncated, }, retained } }; } @@ -97,10 +105,13 @@ export function summarizeUpgradeReadiness(input: { rows: readonly Candidate[]; before: Counters; after: Counters; activeRunIds: readonly string[]; controllerBootId: string; }) { - const eligible = new Set(input.rows.filter(row => row.recoverable).map(row => row.id)); - const observedIds = new Set(input.rows.map(row => row.id)); + const activeIds = new Set(input.activeRunIds); + const retained = input.rows.filter(row => row.retainedUnowned === true && !activeIds.has(row.id)); + const executions = input.rows.filter(row => row.retainedUnowned !== true || activeIds.has(row.id)); + const eligible = new Set(executions.filter(row => row.recoverable).map(row => row.id)); + const observedIds = new Set(executions.map(row => row.id)); const unknown = input.activeRunIds.filter(id => !observedIds.has(id)).length; - const activeRuns = Math.max(input.rows.length + unknown, input.before.activeRuns, input.after.activeRuns); + const activeRuns = Math.max(executions.length + unknown, input.before.activeRuns, input.after.activeRuns); const recoverableRuns = eligible.size; const blockingRuns = activeRuns - recoverableRuns; const pendingWakes = Math.max(input.before.pendingWakes, input.after.pendingWakes); @@ -108,6 +119,6 @@ export function summarizeUpgradeReadiness(input: { const truncated = input.rows.length > MAX_UPGRADE_RUNS; return { version: 1 as const, observedAt: new Date().toISOString(), controllerBootId: input.controllerBootId, ready: !draining && !truncated && blockingRuns === 0 && pendingWakes === 0, - activeRuns, recoverableRuns, blockingRuns, pendingWakes, draining, truncated, + activeRuns, recoverableRuns, blockingRuns, retainedBlockedRuns: retained.length, pendingWakes, draining, truncated, supportedOutageMs: 300_000, recoveryGraceMs: REMOTE_RUNNER_RECONNECT_GRACE_MS }; } diff --git a/server/src/services/native-runtime/native-accepted-result-lease-settlement.ts b/server/src/services/native-runtime/native-accepted-result-lease-settlement.ts index 928fc9dcc5c..fad566bd33b 100644 --- a/server/src/services/native-runtime/native-accepted-result-lease-settlement.ts +++ b/server/src/services/native-runtime/native-accepted-result-lease-settlement.ts @@ -103,7 +103,7 @@ export async function requestNativeAcceptedResultLeaseOperatorAction(db: Db, rec if (!held) return; await requestNativeControllerStopConfirmation(db, { runId: record.recordId, companyId: record.companyId, issueId: record.issueId, - controllerBootId: stamp.bootId, controllerPid: stamp.pid, + controllerBootId: stamp.bootId, controllerPid: stamp.pid, controllerHostHash: null, controllerProcessStartedAt: stamp.processStartedAt ? new Date(stamp.processStartedAt) : null, }); } diff --git a/server/src/services/native-runtime/native-controller-host-recovery.test.ts b/server/src/services/native-runtime/native-controller-host-recovery.test.ts index 1d3c4c61e52..dec4b4ffdcf 100644 --- a/server/src/services/native-runtime/native-controller-host-recovery.test.ts +++ b/server/src/services/native-runtime/native-controller-host-recovery.test.ts @@ -77,4 +77,28 @@ describe("native controller rows saved before their host was recorded", () => { await expect(confirmNativeControllerStopped({ db, companyId, issueId, actionId: actions[0]!.id, runId, stopEvidence, actorId: "board-user" })).rejects.toMatchObject({ status: 409 }); }); + + it("binds stop confirmation to an exact foreign-host controller and rejects changed ownership", async () => { + const { companyId, issueId, runId, coordinator } = await seed(); + const owner = { ...coordinator, controllerHostHash: "previous-pid-space" }; + await db.update(nativeRunFinalizations).set({ controllerHostHash: owner.controllerHostHash }) + .where(eq(nativeRunFinalizations.runId, runId)); + await requestNativeControllerStopConfirmation(db, owner); + const [action] = await db.select().from(issueRecoveryActions).where(eq(issueRecoveryActions.sourceIssueId, issueId)); + const input = { db, companyId, issueId, runId, actionId: action!.id, actorId: "board-user", + stopEvidence: "The hosting provider confirms the exact previous container stopped." }; + for (const patch of [{ controllerHostHash: "changed-space" }, { controllerBootId: randomUUID() }, + { controllerPid: 1_999_999_999 }, { controllerProcessStartedAt: new Date(owner.controllerProcessStartedAt!.getTime() + 60_000) }]) { + await db.update(nativeRunFinalizations).set(patch).where(eq(nativeRunFinalizations.runId, runId)); + await expect(confirmNativeControllerStopped(input)).rejects.toMatchObject({ status: 409 }); + await db.update(nativeRunFinalizations).set({ controllerHostHash: owner.controllerHostHash, + controllerBootId: owner.controllerBootId, controllerPid: owner.controllerPid, + controllerProcessStartedAt: owner.controllerProcessStartedAt }).where(eq(nativeRunFinalizations.runId, runId)); + } + await expect(confirmNativeControllerStopped(input)).resolves.toEqual({ runId, status: "queued" }); + const [attested] = await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, runId)); + expect(await evaluateNativeControllerTakeover({ owner: attested!, now: new Date() })) + .toEqual({ allowed: true, reason: "operator_attested_controller_stopped" }); + expect((await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, runId)))[0]?.status).toBe("running"); + }); }); diff --git a/server/src/services/native-runtime/native-controller-host-recovery.ts b/server/src/services/native-runtime/native-controller-host-recovery.ts index 84b482dcc83..ae424e1741e 100644 --- a/server/src/services/native-runtime/native-controller-host-recovery.ts +++ b/server/src/services/native-runtime/native-controller-host-recovery.ts @@ -5,12 +5,6 @@ import { conflict } from "../../errors.js"; import { persistActivity, publishActivity } from "../activity-log.js"; import { notifyLiveRunRecovery } from "../live-run-recovery-notifications.js"; -/** - * An ephemeral (multi-host) server cannot judge a native controller row saved - * before controller_host_hash existed: its PID may name a process on another - * host. Restart recovery leaves such a row waiting and asks the board, through - * this recovery action, to confirm that the previous server stopped. - */ export const NATIVE_CONTROLLER_HOST_UNVERIFIED = "native_controller_host_unverified"; /** controller_host_hash value written once the board confirms the stop. */ export const OPERATOR_ATTESTED_CONTROLLER_HOST = "operator_attested"; @@ -20,7 +14,7 @@ const changed = () => conflict("The recorded controller is no longer current. Re { code: "native_controller_host_recovery_stale" }); type Coordinator = Pick; + "runId" | "companyId" | "issueId" | "controllerBootId" | "controllerPid" | "controllerProcessStartedAt" | "controllerHostHash">; /** Idempotent. Another active recovery action on the task keeps precedence. */ export async function requestNativeControllerStopConfirmation(tx: Db, coordinator: Coordinator) { @@ -29,9 +23,10 @@ export async function requestNativeControllerStopConfirmation(tx: Db, coordinato status: "active", ownerType: "board", cause: NATIVE_CONTROLLER_HOST_UNVERIFIED, fingerprint: fingerprint(coordinator.runId), attemptCount: 1, evidence: { runId: coordinator.runId, controller: { bootId: coordinator.controllerBootId, pid: coordinator.controllerPid, + hostHash: coordinator.controllerHostHash, processStartedAt: coordinator.controllerProcessStartedAt?.toISOString() ?? null } }, - nextAction: "This run's previous server was recorded before Paperclip stored which host it ran on, so Paperclip " - + "cannot verify that it stopped. Confirm that the previous server process has stopped (for example, its " + nextAction: "Paperclip cannot verify that this run's previous server stopped from the current host. " + + "Confirm that the previous server process has stopped (for example, its " + "container was replaced), and the run resumes from its saved state.", }).onConflictDoNothing(); } @@ -57,13 +52,15 @@ export async function confirmNativeControllerStopped(input: { const recorded = (action?.evidence?.controller ?? {}) as Record; if (!coordinator || !action || !run || action.cause !== NATIVE_CONTROLLER_HOST_UNVERIFIED || action.ownerType !== "board" || !["active", "escalated"].includes(action.status) || action.evidence?.runId !== input.runId - || coordinator.controllerHostHash !== null || coordinator.controllerPid !== recorded.pid - || coordinator.controllerBootId !== recorded.bootId) throw changed(); + || coordinator.controllerHostHash !== (recorded.hostHash ?? null) || coordinator.controllerPid !== recorded.pid + || coordinator.controllerBootId !== recorded.bootId + || (coordinator.controllerProcessStartedAt?.toISOString() ?? null) !== recorded.processStartedAt) throw changed(); // Work intent before the write it announces (sleep-aware background work). await notifyLiveRunRecovery(tx, { companyId: input.companyId, runId: input.runId, agentId: run.agentId }); const now = new Date(); const updated = await tx.update(nativeRunFinalizations).set({ controllerHostHash: OPERATOR_ATTESTED_CONTROLLER_HOST, updatedAt: now }) - .where(and(eq(nativeRunFinalizations.runId, input.runId), isNull(nativeRunFinalizations.controllerHostHash), + .where(and(eq(nativeRunFinalizations.runId, input.runId), coordinator.controllerHostHash === null + ? isNull(nativeRunFinalizations.controllerHostHash) : eq(nativeRunFinalizations.controllerHostHash, coordinator.controllerHostHash), eq(nativeRunFinalizations.controllerPid, coordinator.controllerPid!), eq(nativeRunFinalizations.controllerBootId, coordinator.controllerBootId!))) .returning({ runId: nativeRunFinalizations.runId }); diff --git a/server/src/services/native-runtime/native-restart-recovery.test.ts b/server/src/services/native-runtime/native-restart-recovery.test.ts index 3f88219608a..71406202ae5 100644 --- a/server/src/services/native-runtime/native-restart-recovery.test.ts +++ b/server/src/services/native-runtime/native-restart-recovery.test.ts @@ -1,4 +1,4 @@ -import { currentProcessSpaceHash } from "../controller-process-identity.js"; +import { currentControllerBootId, currentProcessSpaceHash } from "../controller-process-identity.js"; import { afterEach, describe, expect, it, vi } from "vitest"; import { @@ -297,6 +297,14 @@ describe("native controller takeover fencing", () => { now, isProcessAlive: () => true })).resolves.toEqual({ allowed: true, reason: "operator_attested_controller_stopped" }); }); + it("preserves same-space process proof when boot metadata is present", async () => { + const local = { ...owner(), controllerBootId: currentControllerBootId(), controllerHostHash: currentProcessSpaceHash() }; + expect(await evaluateNativeControllerTakeover({ owner: local, now, isProcessAlive: () => false })) + .toEqual({ allowed: true, reason: "controller_process_dead" }); + expect(await evaluateNativeControllerTakeover({ owner: local, now, isProcessAlive: () => true, + readProcessStartedAt: async () => recordedStart })).toEqual({ allowed: false, reason: "controller_still_alive" }); + }); + it.each([ ["another host", "another-host-pid-space"], ["an unreadable PID space", "unknown"], diff --git a/server/src/services/native-runtime/native-restart-recovery.ts b/server/src/services/native-runtime/native-restart-recovery.ts index 23f9ba7c2e6..14d3954a745 100644 --- a/server/src/services/native-runtime/native-restart-recovery.ts +++ b/server/src/services/native-runtime/native-restart-recovery.ts @@ -1,4 +1,6 @@ import { readPendingNativeRunStop } from "./native-cancellation-request.js"; +import { CONTROLLER_LEASE_RENEW_MS } from "../controller-instance-lease.js"; +import { currentControllerProcessStamp, UNKNOWN_PROCESS_SPACE } from "../controller-process-identity.js"; import { readNativeSessionResumptionAgent, restoreNativeSessionExecution } from "./native-session-resumption-admission.js"; import { notifyLiveRunRecovery } from "../live-run-recovery-notifications.js"; import { NATIVE_RETAINED_OWNER_WAIT } from "./native-controller-recovery.js"; @@ -401,6 +403,8 @@ export async function evaluateNativeControllerTakeover(input: { * existed: those keep the previous local proof for one release, outside * ephemeral mode only (see LEGACY_HOST_FALLBACK_EVENT). */ controllerHostHash?: string | null; + controllerBootId?: string | null; + updatedAt?: Date; }; now: Date; coordinatedPreviousController?: { @@ -454,15 +458,19 @@ export async function evaluateNativeControllerTakeover(input: { pidExists: input.isProcessAlive ?? base.pidExists, readStartedAt: input.readProcessStartedAt ?? base.readStartedAt, }; + const needsBootProof = owner.controllerHostHash != null + && (owner.controllerHostHash !== probe.processSpace || owner.controllerHostHash === UNKNOWN_PROCESS_SPACE); const proof = await proveOwnerGone({ kind: "local", scope: "native_controller", generation: null, host: null, processSpace: recordsHost ? owner.controllerHostHash ?? null : probe.processSpace, processSpaceUnrecorded: recordsHost && owner.controllerHostHash == null, - bootId: null, pid: priorPid, processStartedAt: owner.controllerProcessStartedAt?.toISOString() ?? null, + bootId: needsBootProof ? owner.controllerBootId ?? null : null, touchedAt: owner.updatedAt?.toISOString() ?? null, + pid: priorPid, processStartedAt: owner.controllerProcessStartedAt?.toISOString() ?? null, processGroupId: null, }, probe); const fallback = proof.legacyHostFallback ? { legacyHostFallback: true as const } : {}; if (proof.verdict === "gone") { + if (proof.reason === "controller_fenced" || proof.reason === "controller_stopped") return { allowed: true, reason: proof.reason }; if (proof.reason === "pid_reused") return { allowed: true, reason: "controller_pid_recycled", ...fallback }; return { allowed: true, @@ -473,6 +481,7 @@ export async function evaluateNativeControllerTakeover(input: { ...fallback, }; } + if (proof.reason === "controller_lease_held" || proof.reason === "controller_lease_grace") return { allowed: false, reason: proof.reason }; if (proof.reason === "process_space_unrecorded") return { allowed: false, reason: "controller_host_unrecorded" }; if (proof.reason === "different_process_space" || proof.reason === "unknown_process_space" || proof.reason === "different_host") { @@ -1187,7 +1196,7 @@ export async function claimNativeRestartRecoveries(input: { input.coordinatedPreviousController ?? null, }); await recordLegacyHostFallbackProof(tx as unknown as Db, row.run, takeover, "native_restart_recovery"); - if (!takeover.allowed && takeover.reason === "controller_host_unrecorded") { + if (!takeover.allowed && ["controller_host_unrecorded", "controller_host_unprovable"].includes(takeover.reason)) { await requestNativeControllerStopConfirmation(tx as unknown as Db, row.coordinator); } if (!takeover.allowed) { @@ -1217,7 +1226,8 @@ export async function claimNativeRestartRecoveries(input: { // Repeated scans of one ownership wait record it once, so the // bounded history keeps the drain record the next claim reads. ...(waitAlreadyRecorded ? {} : { recoveryHistory: appendBoundedRecoveryHistory(event) }), - updatedAt: now, + updatedAt: waitAlreadyRecorded && row.coordinator.recoveryState === "awaiting_evidence" + && row.coordinator.recoveryRequestId === (input.recoveryRequestId ?? null) ? row.coordinator.updatedAt : now, }) .where( and( @@ -1230,7 +1240,9 @@ export async function claimNativeRestartRecoveries(input: { kind: "awaiting_evidence", runId: row.run.id, reason: takeover.reason, - ...(durableAuthority && row.coordinator.leaseExpiresAt ? { retryAt: row.coordinator.leaseExpiresAt } : {}), + ...(durableAuthority && row.coordinator.leaseExpiresAt ? { retryAt: row.coordinator.leaseExpiresAt } + : ["controller_lease_held", "controller_lease_grace"].includes(takeover.reason) + ? { retryAt: new Date(now.getTime() + CONTROLLER_LEASE_RENEW_MS) } : {}), } as const; } @@ -1648,6 +1660,7 @@ export async function claimNativeRestartRecoveries(input: { // The attempt the executor runs this resume as (see its execution claim). const progressAttempt = nextNativeProviderAttempt(providerAttempt, claimKind, !resumeKeepsAttempt); await notifyLiveRunRecovery(tx, { companyId: row.run.companyId, runId: row.run.id, agentId: row.run.agentId }); + currentControllerProcessStamp(); const claimed = await tx .update(nativeRunFinalizations) .set({ diff --git a/server/src/services/native-runtime/native-runner-checkpoint.ts b/server/src/services/native-runtime/native-runner-checkpoint.ts index f4b47b94e9c..feb6e6832ac 100644 --- a/server/src/services/native-runtime/native-runner-checkpoint.ts +++ b/server/src/services/native-runtime/native-runner-checkpoint.ts @@ -1,4 +1,5 @@ import { readNativeCheckpointRetirement } from "./native-runner-checkpoint-retirement.js"; +import { NATIVE_PREPARATION_FENCING_VERSION } from "./native-active-runs.js"; import { notifyLiveRunRecovery } from "../live-run-recovery-notifications.js"; import { and, eq, exists, gt, isNull, lte, inArray, notExists, or, sql } from "drizzle-orm"; import { checkpointDb, heartbeatRuns, issues, nativeRunFinalizations, nativeRunnerCheckpoints, type Db } from "@paperclipai/db"; @@ -349,10 +350,12 @@ export async function listExpiredNativeRunnerAuthorities(db: Db, now: Date, runI eq(heartbeatRuns.status, "running"), eq(nativeRunFinalizations.phase, "observed"), isNull(nativeRunFinalizations.resultId), or(isNull(nativeRunFinalizations.leaseExpiresAt), lte(nativeRunFinalizations.leaseExpiresAt, now)), - exists(db.select({ runId: nativeRunnerCheckpoints.runId }).from(nativeRunnerCheckpoints).where(and( + or(exists(db.select({ runId: nativeRunnerCheckpoints.runId }).from(nativeRunnerCheckpoints).where(and( eq(nativeRunnerCheckpoints.runId, nativeRunFinalizations.runId), eq(nativeRunnerCheckpoints.issueId, nativeRunFinalizations.issueId), - eq(nativeRunnerCheckpoints.companyId, nativeRunFinalizations.companyId)))))); + eq(nativeRunnerCheckpoints.companyId, nativeRunFinalizations.companyId)))), + and(sql`${heartbeatRuns.runnerProfileJson}->'nativePreparationFencingVersion' = ${JSON.stringify(NATIVE_PREPARATION_FENCING_VERSION)}::jsonb`, + isNull(heartbeatRuns.processPid), isNull(heartbeatRuns.processGroupId), isNull(heartbeatRuns.processStartedAt))))); return rows.map(row => row.runId); } diff --git a/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts b/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts index 26b9a82da58..8b8ebc17c71 100644 --- a/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts +++ b/server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts @@ -18,6 +18,7 @@ import { agents, authUsers, companies, + controllerInstances, companyMemberships, completionContracts, environmentLeases, @@ -80,6 +81,8 @@ import { PaperclipRunnerToolAuthority } from "./paperclip-runner-tool-authority. import { RUNNER_BRIDGE_READ_OPERATIONS, RUNNER_BRIDGE_SCHEMAS } from "./runner-bridge-tools.js"; import { HttpError } from "../../errors.js"; import { legacyControllerBootId } from "../legacy-controller-lease.js"; +import { onControllerStamp } from "../controller-process-identity.js"; +import { registerControllerInstanceDb, resetControllerInstanceLeaseForTests } from "../controller-instance-lease.js"; const mockCaptureRunFailure = vi.hoisted(() => vi.fn()); vi.mock("../../sentry.js", async () => { @@ -2277,6 +2280,42 @@ process.on("SIGTERM", () => child.kill("SIGTERM")); ]); }); + it("recovers a preparation with no runner PID once its foreign controller stops", async () => { + const fixture = await seedRun("REMOTE-PREPARATION-OWNER"); + const oldBoot = randomUUID(); + await fixture.db.update(heartbeatRuns).set({ + runnerProfileJson: { ...fixture.preparedProfile, nativeWorkspaceSync: remotePreparedWorkspace() }, + }).where(eq(heartbeatRuns.id, fixture.runId)); + await fixture.db.update(nativeRunFinalizations).set({ controllerBootId: oldBoot, controllerPid: 7, + controllerProcessStartedAt: new Date(Date.now() - 600_000), controllerHostHash: "previous-pid-space", + leaseOwner: "previous-preparation", leaseExpiresAt: new Date(0), attempt: 1, controllerGeneration: 1, + }).where(eq(nativeRunFinalizations.runId, fixture.runId)); + await fixture.db.insert(controllerInstances).values({ bootId: oldBoot, processSpace: "previous-pid-space", + pid: 7, leaseExpiresAt: new Date(Date.now() + 60_000) }); + registerControllerInstanceDb(fixture.db); + const stamped = vi.fn(), unsubscribe = onControllerStamp(stamped); + try { + const [waiting] = await claimNativeRestartRecoveries({ db: fixture.db, controller: successor, + restartKind: "hard", runIds: [fixture.runId] }); + expect(waiting).toMatchObject({ kind: "awaiting_evidence", reason: "controller_lease_held", retryAt: expect.any(Date) }); + expect(stamped).not.toHaveBeenCalled(); + const [before] = await fixture.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, fixture.runId)); + await claimNativeRestartRecoveries({ db: fixture.db, controller: successor, restartKind: "hard", runIds: [fixture.runId] }); + const [after] = await fixture.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, fixture.runId)); + expect(after!.updatedAt).toEqual(before!.updatedAt); + await fixture.db.update(controllerInstances).set({ stoppedAt: new Date() }).where(eq(controllerInstances.bootId, oldBoot)); + const dispositions = (await Promise.all([successor, { ...successor, bootId: randomUUID() }].map(controller => + claimNativeRestartRecoveries({ db: fixture.db, controller, restartKind: "hard", runIds: [fixture.runId] })))).flat(); + expect(dispositions.filter(row => row.kind === "bootstrap_incomplete")).toHaveLength(1); + expect(dispositions.filter(row => row.kind === "awaiting_evidence")).toHaveLength(1); + expect(stamped).toHaveBeenCalledOnce(); + const [run] = await fixture.db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, fixture.runId)); + expect(run).toMatchObject({ status: "running", processPid: null, nativeSessionId: fixture.native.normalizedSessionId }); + const [owner] = await fixture.db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, fixture.runId)); + expect(owner).toMatchObject({ attempt: 1, controllerGeneration: 2 }); + } finally { unsubscribe(); resetControllerInstanceLeaseForTests(); } + }); + it("recovers an unclaimed remote startup once without requiring a provider checkpoint", async () => { const fixture = await seedRun("REMOTE-UNCLAIMED"); await fixture.db.update(heartbeatRuns).set({ diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index a7b8873c3a0..ed4ffb7cae8 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -303,7 +303,7 @@ import { noteNativeRunnerEvidence } from "./native-runner-evidence-signal.js"; import { removeNativeHarnessBackup } from "./native-harness-backup-cleanup.js"; import { registerLiveRunnerGoalController } from "../runner-goal-control-broker.js"; import { applyRunnerGoalPrpEvent } from "../runner-goals.js"; -import { currentProcessSpaceHash } from "../controller-process-identity.js"; +import { currentControllerProcessStamp, currentProcessSpaceHash } from "../controller-process-identity.js"; import { proveOwnerGone } from "../owner-proof.js"; import { NativeRunnerOwnershipUnverifiedError, @@ -9996,6 +9996,7 @@ async function executePaperclipNativeSessionWithinScope( } const leaseNow = new Date(); confirmedLeaseDeadline = performance.now() + executionLeaseTtlMs; + currentControllerProcessStamp(); const claimed = await tx .update(nativeRunFinalizations) .set({ From 2fc02935869d2eff0688cd6c9cae1beccc0605cb Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:08:31 -0600 Subject: [PATCH 2/8] fix(api): tell agents only the person can revoke assistant connections, and where (#20170) 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 > - People connect external assistants to Paperclip, and each person manages their own assistant connections in the browser > - Chat agents find API routes through `search_api` and call them with `call_api` > - The catalog entries for `/api/mcp/connections` did not say who can use them or which page holds the control > - `call_api` refuses these routes as protocol endpoints, and its message did not say the refusal is final > - A chat agent asked to "disconnect my old assistant" searched for an agent route for its whole turn and did not reply > - This pull request names the person-only boundary and the page in the catalog, and makes the refusal final > - The benefit is that the agent replies at once with a link to the page where the person revokes the connection ## Linked Issues or Issue Description No public issue exists. This is a bug report. **What happened** A person asked the chat agent: "Disconnect my old assistant, but leave the website task running." The agent found `DELETE /api/mcp/connections/{id}`. `call_api` refused the list route with "This endpoint requires its existing protocol client; call_api supports REST only". The agent then searched the API for other routes (dot bindings, agents, "mcp", "assistant") for two minutes. The turn timed out with no reply. The agent wrote in its reasoning that there "must be a supported path". **Expected behavior** The agent tells the person that only they can revoke the connection, links the Assistant connections page, and says the delegated task keeps running. **Steps to reproduce** 1. Connect two assistants to a company as one person. Let the first assistant create a task for an agent. 2. In chat, ask the CEO: "Disconnect my old assistant, but leave the website task running." 3. See that the agent searches the API until the turn ends and does not reply. ## What Changed - `server/src/routes/openapi.ts`: the `GET /api/mcp/connections` and `DELETE /api/mcp/connections/{id}` entries now have a description. It says only the signed-in person can list or revoke their assistant connections, links `[Assistant connections](//apps/assistant-connection)`, notes that the connected assistant itself can revoke its own token through OAuth token revocation, says not to search for another route, and says that tasks the assistant already created keep running. - `server/src/services/native-runtime/runner-api-client.ts`: the `call_api` protocol refusal now says no other API route substitutes for the endpoint. It points to the catalog description only when that operation has one, so undocumented protocol routes such as `POST /api/mcp/project-tools` do not send the agent to an empty description. - `server/src/__tests__/assistant-connections-guidance.test.ts`: new regression test. It uses the exact search queries from the failed run and covers the refusal with and without a description. - `server/src/__tests__/public-mcp.test.ts`: agent and non-session board actors get 401 on listing and revoking assistant connections. ## Verification - `cd server && pnpm exec vitest run src/__tests__/assistant-connections-guidance.test.ts src/__tests__/primary-agent-guidance.test.ts src/__tests__/openapi-routes.test.ts src/services/native-runtime/runner-api.test.ts` passes (2111 tests). After the review fixes, `src/__tests__/assistant-connections-guidance.test.ts src/__tests__/public-mcp.test.ts src/services/native-runtime/runner-api.test.ts` passes (2157 tests), and the server `tsc --noEmit` passes. - The new test fails on `master` without the change (2 failures) and passes with it. - The end-to-end check is the `assistant-connections.revoke-personal-grant` chat eval with this commit cherry-picked. ## Risks - Low risk. The change is catalog text and one error message. No route, permission or data behavior changes. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), with tool use and code execution in Claude Code. ## 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 --- .../assistant-connections-guidance.test.ts | 34 +++++++++++++++++++ server/src/__tests__/public-mcp.test.ts | 2 ++ server/src/routes/openapi.ts | 7 ++++ .../native-runtime/runner-api-client.ts | 2 +- 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 server/src/__tests__/assistant-connections-guidance.test.ts diff --git a/server/src/__tests__/assistant-connections-guidance.test.ts b/server/src/__tests__/assistant-connections-guidance.test.ts new file mode 100644 index 00000000000..d03719da11a --- /dev/null +++ b/server/src/__tests__/assistant-connections-guidance.test.ts @@ -0,0 +1,34 @@ +import { expect, it } from "vitest"; +import { searchRunnerApi } from "../services/native-runtime/runner-api-catalog.js"; +import { validateRunnerApiCall } from "../services/native-runtime/runner-api-client.js"; + +const context = { companyId: "00000000-0000-4000-8000-000000000121", issueId: "00000000-0000-4000-8000-000000000122", runId: "00000000-0000-4000-8000-000000000123", workMode: "standard" }; + +it("tells agents searching for assistant disconnection that only the person can do it, and where", () => { + for (const query of ["chat client assistant disconnect connection", "assistant connections revoke list", "mcp connection revoke REST"]) { + const found = searchRunnerApi({ query, limit: 10 }).results; + const revoke = found.find(row => row.operationId === "DELETE /api/mcp/connections/{id}"); + expect(revoke, query).toBeDefined(); + expect(revoke!.description).toContain("[Assistant connections](//apps/assistant-connection)"); + expect(revoke!.description).toContain("only the signed-in person"); + expect(revoke!.description).toContain("Do not search for another route"); + expect(revoke!.description).toContain("OAuth token revocation sent by the connected assistant itself"); + expect(revoke!.description).toMatch(/already created keep their assignees and running work/); + } + const list = searchRunnerApi({ query: "GET /api/mcp/connections" }).results[0]!; + expect(list.description).toContain("[Assistant connections](//apps/assistant-connection)"); +}); + +it("says the call_api protocol rejection is final instead of inviting another search", () => { + expect(() => validateRunnerApiCall({ operationId: "GET /api/mcp/connections" }, context)) + .toThrow(/no other API route substitutes for it\. Its catalog description says who can use it\./); +}); + +it("points at the catalog description only when the protocol endpoint has one", () => { + const undocumented = searchRunnerApi({ query: "POST /api/mcp/project-tools" }).results.find(row => row.operationId === "POST /api/mcp/project-tools"); + expect(undocumented?.description).toBe(""); + let message = ""; + try { validateRunnerApiCall({ operationId: "POST /api/mcp/project-tools" }, context); } catch (error) { message = (error as Error).message; } + expect(message).toContain("no other API route substitutes for it."); + expect(message).not.toContain("catalog description"); +}); diff --git a/server/src/__tests__/public-mcp.test.ts b/server/src/__tests__/public-mcp.test.ts index 808bb56847c..c5e406b360a 100644 --- a/server/src/__tests__/public-mcp.test.ts +++ b/server/src/__tests__/public-mcp.test.ts @@ -466,6 +466,8 @@ describe.skipIf(!support.supported)("public MCP OAuth and tool boundary", () => ] as Request["actor"][]) { actor = candidate; expect((await request(app).get("/api/mcp/setup")).status).toBe(401); + expect((await request(app).get("/api/mcp/connections")).status).toBe(401); + expect((await request(app).delete(`/api/mcp/connections/${randomUUID()}`).set("Origin", config.origin)).status).toBe(401); } for (const source of ["session", "cloud_tenant"] as const) { actor = { type: "board", source, userId: randomUUID() }; diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index b3a9bb92797..5efd8ba619b 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -12957,14 +12957,21 @@ registerCurrentRoute({ body: mcpConsentSchema.extend({ userCode: z.string().min(8).max(12) }), responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden }, }); +const ASSISTANT_CONNECTIONS_PERSON_ONLY = "Personal and person-only: only the signed-in person can list or revoke their own assistant connections, in their browser on " + + "[Assistant connections](//apps/assistant-connection), with this conversation's company prefix as . " + + "An agent cannot do it for them: these routes accept only the person's own signed-in browser session, and the only other revocation is OAuth token revocation sent by the connected assistant itself with its own token. " + + "Do not search for another route; link that page and tell the person which connection to revoke. " + + "Revoking stops that assistant's further calls; tasks it already created keep their assignees and running work."; registerCurrentRoute({ method: "get", path: "/api/mcp/connections", tags: ["tool-gateway"], summary: "List the signed-in person's assistant connections", + description: ASSISTANT_CONNECTIONS_PERSON_ONLY, responses: { 200: r.ok(), 401: r.unauthorized }, }); registerCurrentRoute({ method: "delete", path: "/api/mcp/connections/{id}", tags: ["tool-gateway"], summary: "Revoke an assistant connection using a same-origin browser session", + description: ASSISTANT_CONNECTIONS_PERSON_ONLY, responses: { 204: { description: "Connection revoked" }, 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden }, }); diff --git a/server/src/services/native-runtime/runner-api-client.ts b/server/src/services/native-runtime/runner-api-client.ts index 2f051b7309b..42a1af9ad19 100644 --- a/server/src/services/native-runtime/runner-api-client.ts +++ b/server/src/services/native-runtime/runner-api-client.ts @@ -54,7 +54,7 @@ export function validateRunnerApiCall(value: unknown, context: RunnerApiContext) if (!parsed.success) throw badRequest("Invalid call_api arguments", { issues: parsed.error.issues }); const input = parsed.data; const operation = runnerApiOperation(input.operationId); - if (operation.transport !== "rest") throw unprocessable("This endpoint requires its existing protocol client; call_api supports REST only"); + if (operation.transport !== "rest") throw unprocessable(`This endpoint requires its existing protocol client; call_api supports REST only and no other API route substitutes for it.${operation.description.trim() ? " Its catalog description says who can use it." : ""}`); if (input.responseText && operation.method !== "GET") throw badRequest("responseText requires a GET operation; read the saved artifact instead of repeating a mutation"); if (!operation.allowedModes.includes(context.workMode)) throw forbidden("call_api permits only reads in Ask and Plan modes; use the permitted dedicated tools"); if (input.pathParams?.companyId && input.pathParams.companyId !== context.companyId) throw forbidden("API call belongs to another company"); From b07399c241f5abe7817340d4ba4554198f840d70 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:11:08 -0600 Subject: [PATCH 3/8] fix(authz): link the project's access settings when an agent cannot share a project (#20176) 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 > - Some actions are board-only. When an agent calls one, the 403 denial tells it that the denial is final and, when known, links the board page that holds the control > - Project sharing (`/api/projects/:id/access-members`) had no known page, so the denial told agents to link a page only if they already knew its route > - An agent asked to "Let Riley see the acquisition project" got this denial. It told the person to look for "access/sharing settings" and gave no link, so the person had to search for the control > - This pull request maps project access-member routes to the project's Configuration page, where the Manage access control is > - The benefit is that agents give people a direct link to the control that shares a private project ## Linked Issues or Issue Description Bug report (no public issue exists). **What happened** An agent tried to add a person to a private project. `POST /api/projects/:id/access-members` returned 403 "Board access required" without a page link. The agent told the person to find the project's access or sharing settings in the web app and gave no link. **Expected behavior** The denial links the project's Configuration page (`//projects//configuration`), where Manage access is. The agent gives that link to the person. **Steps to reproduce** 1. Create a private project and give an agent access to it. 2. In chat, ask the agent to let another company member see the project. 3. The agent's access-member request is denied, and its reply has no link to the sharing control. Refs #20151 ## What Changed - `server/src/routes/authz.ts`: board-only denials for `/projects/:id/access-members` (and its member subpaths) link "the project's access settings" at `//projects//configuration`. The link uses the resolved route project ID, so short references such as `/api/projects/acquisition/access-members` also get the link. Without a resolved UUID, the denial falls back to the generic instruction. - `server/src/__tests__/board-access-denial.test.ts`: cases for add and remove access-member denials, plus a short-reference case that goes through a route param resolver. ## Verification - `cd server && npx vitest run src/__tests__/board-access-denial.test.ts` passes (17 tests). The short-reference test fails on the previous matcher. The server `tsc --noEmit` passes. - The chat eval `projects.share-project` found this gap: in one run the agent linked the project, and in the next run it gave no link. ## Risks - Low risk. This changes only the remediation text in an agent's 403 response. It does not change authorization. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution, used through Claude Code. ## 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 --- .../src/__tests__/board-access-denial.test.ts | 26 +++++++++++++++++++ server/src/routes/authz.ts | 10 +++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/board-access-denial.test.ts b/server/src/__tests__/board-access-denial.test.ts index 582cc11e6c4..827a085e12c 100644 --- a/server/src/__tests__/board-access-denial.test.ts +++ b/server/src/__tests__/board-access-denial.test.ts @@ -39,6 +39,8 @@ describe("board-only denial", () => { ["patch", `/api/agents/${AGENT_ID}/permissions`, `[Open the agent's permissions](//agents/${AGENT_ID}/permissions)`], ["post", `/api/agents/${AGENT_ID}/keys`, `[Open the agent's API keys](//agents/${AGENT_ID}/api-keys)`], ["post", `/api/agents/${AGENT_ID}/pause`, `[Open the agent's profile](//agents/${AGENT_ID})`], + ["post", `/api/projects/${AGENT_ID}/access-members`, `[Open the project's access settings](//projects/${AGENT_ID}/configuration)`], + ["delete", `/api/projects/${AGENT_ID}/access-members/m-1`, `[Open the project's access settings](//projects/${AGENT_ID}/configuration)`], ] as const)("tells an agent %s %s is final and names the board page", async (method, path, link) => { const res = await request(createApp(agentActor))[method](path).expect(403); expect(res.body.error).toBe("Board access required"); @@ -48,6 +50,30 @@ describe("board-only denial", () => { expect(res.body.remediation).toContain(link); }); + it("links the resolved project when an agent names the project by its short reference", async () => { + const app = express(); + app.use((req, _res, next) => { + req.actor = agentActor; + next(); + }); + const router = express.Router(); + router.param("id", (req, _res, next, rawId) => { + req.params.id = rawId === "acquisition" ? AGENT_ID : rawId; + next(); + }); + router.post("/projects/:id/access-members", (req, res) => { + assertBoard(req); + res.json({ ok: true }); + }); + app.use("/api", router); + app.use(errorHandler); + const resolved = await request(app).post("/api/projects/acquisition/access-members").expect(403); + expect(resolved.body.remediation).toContain(`[Open the project's access settings](//projects/${AGENT_ID}/configuration)`); + const unresolved = await request(app).post("/api/projects/unknown-project/access-members").expect(403); + expect(unresolved.body.remediation).toContain("Link a page only if you already know its route"); + expect(unresolved.body.remediation).not.toContain("](/"); + }); + it("falls back to a general board instruction when no page is known", async () => { const res = await request(createApp(agentActor)).get(`/api/companies/${COMPANY_ID}/managed-agent-profiles`).expect(403); expect(res.body.code).toBe(BOARD_ACCESS_REQUIRED_CODE); diff --git a/server/src/routes/authz.ts b/server/src/routes/authz.ts index 10d2400fd55..c6c60559a03 100644 --- a/server/src/routes/authz.ts +++ b/server/src/routes/authz.ts @@ -45,13 +45,19 @@ const AGENT_PROFILE_TABS: Record = { // The board page that holds the control behind a board-only API route, when one // is known. Paths keep for the agent to fill from its own task identifier. -const BOARD_CONTROL_PAGES: Array<[RegExp, (match: RegExpExecArray) => [string, string]]> = [ +const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +const BOARD_CONTROL_PAGES: Array<[RegExp, (match: RegExpExecArray, req: Request) => [string, string] | null]> = [ [/^\/(?:companies\/[^/]+\/)?secrets?(?:[-/]|$)/, () => ["company secrets", "//company/settings/secrets"]], [/^\/companies\/[^/]+\/(?:budgets|budget-incidents|costs)(?:\/|$)/, () => ["budgets", "//activity/budgets"]], [/^\/companies\/[^/]+\/(?:prompt-injection-prevention|decision-model)(?:\/|$)/, () => ["connection settings", "//company/settings/connections"]], [/^\/companies\/[^/]+\/(?:members|invites|join-requests)(?:\/|$)/, () => ["company members", "//company/settings/members"]], [/^\/companies\/[^/]+\/(?:tools\/)?gateways(?:\/|$)|^\/tool-gateway\//, () => ["tool gateways", "//apps/gateways"]], [/^\/approvals\/([^/]+)/, match => ["the approval", `//approvals/${match[1]}`]], + [/^\/projects\/([^/]+)\/access-members(?:\/|$)/, (match, req) => { + const projectId = [req.params?.id, match[1]].find((id): id is string => typeof id === "string" && UUID_PATTERN.test(id)); + return projectId ? ["the project's access settings", `//projects/${projectId}/configuration`] : null; + }], [/^\/agents\/([0-9a-f-]{36})(?:\/([^/]+))?(?:\/|$)/i, match => { const tab = match[2] && Object.hasOwn(AGENT_PROFILE_TABS, match[2]) ? AGENT_PROFILE_TABS[match[2]] : undefined; return tab ? [`the agent's ${tab[0]}`, `//agents/${match[1]}/${tab[1]}`] : ["the agent's profile", `//agents/${match[1]}`]; @@ -63,7 +69,7 @@ function boardControlPage(req: Request) { const path = (req.originalUrl ?? "").split("?")[0]!.replace(/^\/api(?=\/)/, ""); for (const [pattern, page] of BOARD_CONTROL_PAGES) { const match = pattern.exec(path); - if (match) return page(match); + if (match) return page(match, req); } return null; } From 7086019d4bc4aca80e01a882c2d6ab8f39c6e0f0 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:11:13 -0600 Subject: [PATCH 4/8] fix(api-catalog): say Agent Chat is always on instead of offering a retired toggle (#20169) 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 > - Agents learn what the API can do from the API catalog at `/api/openapi.json` > - PR #19360 made Agent Chat standard. The `enableAgentChat` key is now retired: it always reads true, and the app ignores written values > - The catalog still lists `enableAgentChat` as an ordinary boolean on the experimental settings operations, with no note that it is retired > - An agent that was asked to "Enable Agent Chat on this instance" read the catalog and told the person to turn on a toggle in Instance Settings → Experimental. That toggle no longer exists > - This pull request adds a note to both experimental settings operations: Agent Chat is always on, the key is retired, and agents must tell people it is already on > - The benefit is that agents give a correct answer and do not send people to a control that is not there ## Linked Issues or Issue Description Bug report (no public issue exists). **What happened** An agent was asked "Enable Agent Chat on this instance." It read `PATCH /api/instance/settings/experimental` in the API catalog and found `enableAgentChat`. It replied that an admin must open Instance Settings → Experimental and turn on Enable Agent Chat. That toggle was removed in #19360, and Agent Chat is already on for everyone. **Expected behavior** The agent says that Agent Chat is already on (the person is using it), and that nothing must be enabled. **Steps to reproduce** 1. Open a chat with an agent on any current instance. 2. Send "Enable Agent Chat on this instance." 3. The agent reads the experimental settings operation in the API catalog and sends you to a settings toggle that does not exist. Refs #19360 ## What Changed - `server/src/routes/openapi.ts`: the GET and PATCH `/api/instance/settings/experimental` operations now say that Agent Chat is always on, that `enableAgentChat` is a retired compatibility key that always reads true and ignores written values, and that agents must tell people it is already on instead of sending them to a toggle. - `server/src/__tests__/openapi-routes.test.ts`: a regression test checks this note on both operations. - The note says Agent Chat "needs no setup" instead of "there is nothing to enable", so it does not match unrelated keyword searches. The runner API no-results test (`nothing-zzzzzzzzzz`) had matched the word "nothing". ## Verification - `cd server && npx vitest run src/__tests__/openapi-routes.test.ts src/services/native-runtime/runner-api.test.ts` passes (2108 tests), and the server `tsc --noEmit` passes. - The chat eval `instance-operations.instance-settings` found this defect. Its run log shows that the agent reasoned from the catalog schema ("The setting is `enableAgentChat` on PATCH /api/instance/settings/experimental"). ## Risks - Low risk. This changes only catalog description text. It does not change behavior, validation or the schema. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution, used through Claude Code. ## 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 --- server/src/__tests__/openapi-routes.test.ts | 10 ++++++++++ server/src/routes/openapi.ts | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index 3500aa68f94..a762b955064 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -576,6 +576,16 @@ describe("openapi routes", () => { expect(dismiss.description).toContain("viewers may dismiss their own"); }); + it("tells agents that Agent Chat is always on instead of advertising a retired toggle", async () => { + const res = await request(createApp()).get("/api/openapi.json"); + const experimental = res.body.paths["/api/instance/settings/experimental"]; + for (const operation of [experimental.get, experimental.patch]) { + expect(operation.description).toContain("Agent Chat is always on"); + expect(operation.description).toContain("enableAgentChat is a retired compatibility key"); + expect(operation.description).toContain("tell them it is already on"); + } + }); + it("tells agents that archiving keeps a status card and where a person restores it", async () => { const res = await request(createApp()).get("/api/openapi.json"); const patch = res.body.paths["/api/status-cards/{id}"].patch; diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 5efd8ba619b..a158743c1e7 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -7120,6 +7120,7 @@ registry.registerPath({ path: "/api/instance/settings/experimental", tags: ["instance"], summary: "Get experimental instance settings", + description: "Agent Chat is always on for every person on every instance and needs no setup. enableAgentChat is a retired compatibility key: it always reads true and written values are ignored. If someone asks to enable Agent Chat, tell them it is already on, since they are using it now; do not send them to a settings toggle.", responses: { 200: r.ok(), 401: r.unauthorized }, }); @@ -7128,7 +7129,7 @@ registry.registerPath({ path: "/api/instance/settings/experimental", tags: ["instance"], summary: "Update experimental instance settings", - description: "Requires instance-admin access or a trusted Cloud stack admin. Operator-hidden settings and Cloud-managed feature restrictions still apply. This does not grant access to other instance administration operations.", + description: "Requires instance-admin access or a trusted Cloud stack admin. Operator-hidden settings and Cloud-managed feature restrictions still apply. This does not grant access to other instance administration operations. Agent Chat is always on for every person on every instance and needs no setup. enableAgentChat is a retired compatibility key: it always reads true and written values are ignored. If someone asks to enable Agent Chat, tell them it is already on, since they are using it now; do not send them to a settings toggle.", request: { body: jsonBody(patchInstanceExperimentalSettingsSchema) }, responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden }, }); From 7d92b6a7cd6274b36ba4c8434fd64bf0daa0470d Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:31:49 -0600 Subject: [PATCH 5/8] fix(api): use a neutral catalog pointer in the call_api protocol refusal (#20191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path - Paperclip agents reach REST routes through the `call_api` tool; protocol endpoints (MCP, OAuth, WebSocket) are refused there with guidance. - #20170 changed that refusal to tell the agent the endpoint's catalog description says who can use it. - Review on #20170 found that some protocol endpoints' descriptions (for example the WebSocket entries) do not say who may call them, so the refusal could send the agent to look for information that is not there. - The fix (e7ff8ba610) was pushed three minutes after #20170 merged, so it never reached master. This PR carries it. ## Linked Issues or Issue Description Refs #20170 **What happened:** after #20170, a `call_api` refusal for a protocol endpoint says the catalog description names who can use it. Some descriptions, such as `GET /api/companies/{companyId}/events/ws` ("WebSocket upgrade. Not callable through call_api."), do not, so an agent following the refusal looks for an answer the catalog does not give. **Expected behavior:** the refusal points the agent at the catalog description neutrally ("Read its catalog description for details.") without promising it names who may call the endpoint. **Steps to reproduce:** 1. As an agent, call `call_api` with `GET /api/companies/{companyId}/events/ws`. 2. Read the refusal text: it claims the description says who can use the endpoint. 3. Read the catalog entry: it does not. ## What Changed - `server/src/services/native-runtime/runner-api-client.ts`: the protocol-endpoint refusal now says "Read its catalog description for details." instead of promising the description names who can call it. - `server/src/__tests__/assistant-connections-guidance.test.ts`: adds a case on the WebSocket events endpoint asserting the neutral wording. ## Verification - `cd server && npx vitest run src/__tests__/assistant-connections-guidance.test.ts` (4 passed). - Commit was reviewed on #20170's branch by Ponytail (the finding that prompted it). ## Risks - Low: wording-only change to one refusal message, plus a test. Agents may read slightly less specific guidance for endpoints whose descriptions do name the caller; the description itself is unchanged. ## Model Used - Provider: Anthropic. Model: Claude Opus 5.5 (`claude-opus-5-5`), via Claude Code, with tool use and code execution. The change was authored by a Claude subagent on #20170 and carried over here by Claude. ## 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 --- .../src/__tests__/assistant-connections-guidance.test.ts | 9 ++++++++- server/src/services/native-runtime/runner-api-client.ts | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/assistant-connections-guidance.test.ts b/server/src/__tests__/assistant-connections-guidance.test.ts index d03719da11a..563b3e04891 100644 --- a/server/src/__tests__/assistant-connections-guidance.test.ts +++ b/server/src/__tests__/assistant-connections-guidance.test.ts @@ -21,7 +21,7 @@ it("tells agents searching for assistant disconnection that only the person can it("says the call_api protocol rejection is final instead of inviting another search", () => { expect(() => validateRunnerApiCall({ operationId: "GET /api/mcp/connections" }, context)) - .toThrow(/no other API route substitutes for it\. Its catalog description says who can use it\./); + .toThrow(/no other API route substitutes for it\. Read its catalog description for details\./); }); it("points at the catalog description only when the protocol endpoint has one", () => { @@ -32,3 +32,10 @@ it("points at the catalog description only when the protocol endpoint has one", expect(message).toContain("no other API route substitutes for it."); expect(message).not.toContain("catalog description"); }); + +it("does not claim a non-guidance protocol description says who can use the endpoint", () => { + let message = ""; + try { validateRunnerApiCall({ operationId: "GET /api/companies/{companyId}/events/ws" }, context); } catch (error) { message = (error as Error).message; } + expect(message).toContain("Read its catalog description for details."); + expect(message).not.toContain("who can use it"); +}); diff --git a/server/src/services/native-runtime/runner-api-client.ts b/server/src/services/native-runtime/runner-api-client.ts index 42a1af9ad19..ecce7755c8f 100644 --- a/server/src/services/native-runtime/runner-api-client.ts +++ b/server/src/services/native-runtime/runner-api-client.ts @@ -54,7 +54,7 @@ export function validateRunnerApiCall(value: unknown, context: RunnerApiContext) if (!parsed.success) throw badRequest("Invalid call_api arguments", { issues: parsed.error.issues }); const input = parsed.data; const operation = runnerApiOperation(input.operationId); - if (operation.transport !== "rest") throw unprocessable(`This endpoint requires its existing protocol client; call_api supports REST only and no other API route substitutes for it.${operation.description.trim() ? " Its catalog description says who can use it." : ""}`); + if (operation.transport !== "rest") throw unprocessable(`This endpoint requires its existing protocol client; call_api supports REST only and no other API route substitutes for it.${operation.description.trim() ? " Read its catalog description for details." : ""}`); if (input.responseText && operation.method !== "GET") throw badRequest("responseText requires a GET operation; read the saved artifact instead of repeating a mutation"); if (!operation.allowedModes.includes(context.workMode)) throw forbidden("call_api permits only reads in Ask and Plan modes; use the permitted dedicated tools"); if (input.pathParams?.companyId && input.pathParams.companyId !== context.companyId) throw forbidden("API call belongs to another company"); From 92a18bb0f3ecd81dbd13599ed0231926e35ba500 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Sun, 8 Nov 2026 22:34:26 -0800 Subject: [PATCH 6/8] perf(runner): restart the ACPX provider when a turn settles (#20190) 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. > - Paperclip Runner drives Claude through the ACPX sidecar. The sidecar's MCP endpoint lasts for the whole session, so it cannot tell which turn sent a late tool callback. > - Because of this, the Runner reaps the sidecar and provider of a generation that ran a turn before it releases call-ID tombstones and rotates receipt epochs. Today this restart runs at the start of the next turn. > - Every follow-up turn in a run pays that restart on its critical path: about 1.5 s with real Claude. #20145 moves the restart to park, but a second turn in the same run, after the first turn that follows an attach, still restarts cold. > - The restart does not have to wait for the next turn. When a turn settles and the host has its terminal result, the Runner is idle. > - This pull request restarts the provider at that point. The next turn, or the next park, then finds a fresh generation. > - The benefit: same-run follow-up turns start without a restart, park does no restart after a settled turn, and the late-callback guarantee is unchanged. ## Linked Issues or Issue Description Refs #20145, which moved the restart to park. This pull request builds on it. No public issue exists, so the problem is described here. **Subsystem affected** Paperclip Runner, ACPX provider (Claude). **Current behavior** `AcpxProviderSession::start_turn_timed` restarts the provider before any turn whose generation already ran a turn. A follow-up turn in the same run waits for `session.suspend`, sidecar shutdown, a new sidecar, `initialize`, `session.open` and `run.attach` before `turn.start`. Since #20145, park does the restart instead, but only for the first turn after an attach. **Proposed behavior** When a turn settles and the controller has ACKed every committed event, the Runner restarts the provider. The next `turn.start` uses the fresh generation and only rotates receipts. Park reuses the fresh generation and does no second restart. **Reason and benefit** The restart leaves the turn's critical path, both for same-run follow-ups and for the turns after a park. The order of reap and rotation stays the same, so a late callback from the reaped generation still cannot reach a later turn. ## What Changed - **Runner loop** (`durable/runner.rs`): new `CommandExecutor::prepare_next_turn` hook, a no-op by default. The runner calls it after the idle provider poll, only when no control frame is waiting and the controller has ACKed every committed event (`acked_source_seq >= highest_source_seq`). So the restart always runs after the terminal result reaches the host. - **ACPX turn-end restart** (`acpx_park_preparation.rs`, `acpx_provider_generation.rs`): - `restart_session_after_turn` restarts a Claude session that has a completed turn, nothing pending, no buffered events, a dormant goal and its whole event tail ACKed. It first settles an abandoned park observation. If that settlement reads more events, it skips this round, and the next idle round restarts after those events are ACKed. - `AcpxProviderSession::restart_after_turn` does the same suspend → reap → start → bootstrap → identity check as the turn-start restart. It records the reaped process id in `restarted_after_turn_from`. This field is the explicit "fresh because of a turn-end restart" marker. - The session does not poll a fresh generation until its next turn or rebind, the same as a prepared one. Its startup output is published with the next turn, as today. Polling it early would add events after the host computed its park cut. - **Next turn** (`acpx_provider_session.rs`): `start_turn_timed` restarts only when the generation ran a turn. When `restarted_after_turn_from` is set, it does the post-restart receipt and ledger rotation without a second restart. `session.reconciled` reports the reaped process as `previousProcessId`. - **Park**: `prepare_next_generation` restarts (with the 20 s park deadline) only if no turn-end restart ran. Otherwise it marks the generation for `run.attach` rebind and returns `false`, so the runner does not ask for a second park proof. The park-time receipt rotation moves to the next turn start, where the turn-end path does it too. - **Restart failure**: the executor sets the same state that a failed turn-start restart leaves (`closed`, no session, provider exit confirmed). The next `turn.start` fails with the same `ACPX turn start failed closed: …` error that the turn-start restart gives today. - **Fake sidecar**: new `pending-input` fault, which opens an input request in a turn that then completes. ### Races | Race | Handling | | --- | --- | | A turn or steer arrives during the turn-end restart | The runner loop is single-threaded. The command waits in the socket until the restart returns, then starts on the fresh generation without a restart. Only one generation is live. | | A turn arrives before the hook runs (for example, in the same read as the ACK) | Control frames go first, so the turn restarts at its start, as today. The hook then restarts after that turn. | | Park arrives during the turn-end restart | Host control is read at the top of the loop, so park waits. Park then reuses the fresh generation. No event is published in between, so the host's park cut still matches. | | Park arrives before the hook runs | Park restarts with the 20 s deadline, as in #20145. The hook then finds a fresh generation and does nothing. | | Eviction, `runner.suspend` or retire after the restart | They reap the fresh generation through the ordinary paths. | | The restart fails | The session is `closed`. The next turn fails with the same error and the same durable state as a failed turn-start restart. Park returns conflict, as it does for a closed session today. | | Cancellation | `run.cancel`, `turn.interrupt` and `turn.stop` after the restart return `already_settled`, as after any settled turn. A cancellation that retires the runtime closes the session, so no restart runs. | | Interrupted, cancelled or failed turn | `completed_turn_observed` is false, so no turn-end restart runs. The next turn restarts at its start, as today. | | Pending permission or input requests at the terminal | The reducer ends them at the terminal, so the restart runs. A later `request.resolve` fails with `no active turn`, with or without the restart. | | Active session goal at the terminal | No restart, because the goal can keep working after its prompt. | ### Removed as redundant, and what stays - **Removed:** the park-time restart and dormant verify when a turn-end restart already made the generation fresh. The runner's `host_park_ready` proves dormancy just before `prepare_for_park`, and nothing runs in between. - **Removed:** the runner's second park proof in that case, because the provider did not change. - **Removed:** the park-time receipt and ledger rotation. The next turn start does it once, for both paths. - **Kept: the turn-start restart.** Without it, a turn queued before the hook, a turn after an interrupted or cancelled turn, a turn after an active goal, and any non-Claude ACPX turn would run in the generation that ran the previous turn. Tests cover the queued-turn and interrupted cases. - **Kept: the park-time restart.** Without it, a park that arrives before the hook would leave the next run to restart at attach. A test covers this. - **Kept: the settlement of an abandoned park observation.** Without it, the restart drops events that the old generation sent before the abandoned response. A test covers this, and it fails with the settlement removed. - **Kept: the goal-dormancy check.** Without it, the restart kills an active goal's autonomous work. A test covers this, and it fails with the check removed. - **Kept: the ACK gate.** Without it, the restart can run before the host has the terminal result. A runner test covers this. - **Kept: deferred polling of the fresh generation.** Without it, its startup event lands after the host computed its park cut, and the park conflicts. - **Kept: the kept failure message.** Without it, the next turn fails with `ACPX session is not open`, and the restart error is lost. ## Verification - `cargo test -p paperclip-runner-core --lib` and every `tests/acpx_*.rs`, `native_provider_backend` and `durable_recovery` suite: all pass (debug). - New tests (`tests/support/acpx_turn_end_restart.rs`, run from `acpx_park_preparation`) prove these points: - A settled turn reaps the old pid before it spawns the new one, and no predecessor is still live. A same-run follow-up then sends only `turn.start` and reports the reaped pid in `session.reconciled`. - Park after a turn-end restart sends no sidecar command and returns `false`. Attach rebinds the same pid. The run's second turn restarts at its end, and the third turn starts without a restart. - The reaped generation's late callback never reaches the next turn. - A turn that arrives before the hook restarts at its start. A park before the hook restarts once. - A failed turn-end restart gives the same next-turn error, retry error, durable state and process count as a failed turn-start restart. - Cancellation after the restart is `already_settled`. `runner.suspend` reaps the fresh generation. - An active goal skips the restart. An input request open at the terminal behaves the same with or without the restart. - `acpx_provider_turns`: a session-level turn-end restart drops the old generation's late `issues.delete` callback, which reuses the previous turn's call ID. The next turn then admits that call ID from the fresh provider. Interrupted and retired turns do not restart at turn end. - `acpx_park_readiness`: a turn-end-restarted generation is park-ready. An abandoned park observation tail is delivered before the restart. - `durable::runner`: the hook runs only after the terminal is ACKed and when control is idle. It never runs during ACK debt, a waiting control frame or a disconnect. - Server integration tests (`plugin-runner-release-resume`, `plugin-runner-eviction` and `plugin-runner-task-warm`) with this branch's Runner: 49 passed. The provider process counts do not change: each released run still adds one process start, now at turn end instead of at park. - `cargo fmt --check`: clean. - Fake-sidecar timing (release build, 7 runs, medians): | | before (`master` with #20145) | this PR | | --- | --- | --- | | same-run follow-up `turn.start` → `turn.started` | 51.5 ms | 22.0 ms | | second turn after an attach, `turn.start` → `turn.started` | 270.3 ms | 23.5 ms | | turn-end restart, after the terminal is ACKed | — | 29.0 ms | | park preparation after a settled turn | 34.1 ms | 0.03 ms | - Real Claude: not run for this pull request. With real Claude, the restart took about 1.5 s on the turn-start path, and this change takes it off that path. That number is not measured on this branch. ## Risks - A run that ends after one turn without a park now starts one extra sidecar that no turn uses. The Claude CLI itself starts only at the first prompt, so the cost is the sidecar's start and bootstrap. - The restart blocks the Runner loop for its length (about 1.5 s with real Claude), as the turn-start restart does today. It has no deadline, as before. A park that arrives during it waits, inside the host's 30 s park wait. - The fresh generation is not polled until its next turn. If it exits while idle, the next turn finds out at `turn.start` and fails closed. Park finds out at its observation. - The turn-end restart is for Claude only, the same as park preparation. Other ACPX agents keep restarting at turn start. - If the Runner crashes during the restart, the durable state still says `session_open`. Recovery is the same as for a crash while idle. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), in Claude Code with tool use (shell, file edits, local test runs). ## 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 - [x] 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- .../runner-core/src/acpx_park_preparation.rs | 58 ++- .../runner-core/src/acpx_provider_backend.rs | 24 +- .../src/acpx_provider_generation.rs | 82 +++-- .../runner-core/src/acpx_provider_session.rs | 11 +- .../runner-core/src/bin/fake-acpx-sidecar.rs | 14 + .../crates/runner-core/src/durable/runner.rs | 102 +++++- .../src/native_provider_backend.rs | 7 + .../tests/acpx_park_preparation.rs | 6 +- .../runner-core/tests/acpx_park_readiness.rs | 50 ++- .../runner-core/tests/acpx_provider_turns.rs | 80 +++++ .../tests/support/acpx_turn_end_restart.rs | 333 ++++++++++++++++++ 11 files changed, 707 insertions(+), 60 deletions(-) create mode 100644 packages/paperclip-runner/runner/crates/runner-core/tests/support/acpx_turn_end_restart.rs diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_park_preparation.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_park_preparation.rs index 1b6f91c5bfa..26d4afb8638 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_park_preparation.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_park_preparation.rs @@ -1,18 +1,15 @@ use std::time::Instant; use super::{ - await_provider_lifetime_fence, AcpxCommandExecutor, AcpxProviderDescriptor, AcpxProviderSession, + await_provider_lifetime_fence, goal_is_dormant, AcpxCommandExecutor, AcpxProviderDescriptor, + AcpxProviderSession, }; use crate::durable::DurableRunnerError; use crate::provider_bridge::AuthorizedToolSet; impl AcpxCommandExecutor { - pub(super) fn prepare_session_for_park( - &mut self, - deadline: Instant, - ) -> Result { - self.ensure_persistence_healthy()?; - let eligible = self.candidate_admission().is_none() + fn generation_restartable(&self, session_ready: fn(&AcpxProviderSession) -> bool) -> bool { + self.candidate_admission().is_none() && self.state.as_ref().is_some_and(|state| { state.descriptor.agent == "claude" && state.lifecycle == "session_open" @@ -22,12 +19,47 @@ impl AcpxCommandExecutor { && !state.provider_exit_unconfirmed && state.pending_events.is_empty() && state.pending_runtime_requests.is_empty() + && (state.goal_projection.is_null() || goal_is_dormant(&state.goal_projection)) }) - && self - .session - .as_ref() - .is_some_and(AcpxProviderSession::can_prepare_next_generation); - if !eligible { + && self.session.as_ref().is_some_and(session_ready) + } + + pub(super) fn restart_session_after_turn(&mut self) -> Result<(), DurableRunnerError> { + self.ensure_persistence_healthy()?; + if !self.generation_restartable(AcpxProviderSession::can_restart_after_turn) { + return Ok(()); + } + let session = self.session.as_mut().expect("checked ACPX session"); + if session.settle_park_observation().is_err() + || !self.generation_restartable(AcpxProviderSession::can_restart_after_turn) + { + return Ok(()); + } + let restarted = self + .session + .as_mut() + .expect("checked ACPX session") + .restart_after_turn(None); + let Err(error) = restarted else { + return Ok(()); + }; + let state = self + .state + .as_mut() + .expect("ACPX state remains available after a turn-end restart"); + state.lifecycle = "closed".to_owned(); + self.context.provider_turn_id = None; + self.session = None; + self.failed_turn_end_restart = Some(error.to_string()); + self.save_state() + } + + pub(super) fn prepare_session_for_park( + &mut self, + deadline: Instant, + ) -> Result { + self.ensure_persistence_healthy()?; + if !self.generation_restartable(AcpxProviderSession::can_prepare_next_generation) { return Ok(false); } let prepared = self @@ -36,7 +68,7 @@ impl AcpxCommandExecutor { .expect("checked ACPX session") .prepare_next_generation(deadline); match prepared { - Ok(()) => Ok(true), + Ok(restarted) => Ok(restarted), Err(error) => { self.recover_failed_park_preparation()?; Err(DurableRunnerError::invalid(format!( diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs index 9063b2da0e1..94c33a1ee44 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_backend.rs @@ -966,6 +966,7 @@ pub struct AcpxCommandExecutor { fail_after_state_rename: bool, launch_profile: Option, recovery_pending_effects: Option, + failed_turn_end_restart: Option, } // The durable command's turn and the live provider callback have separate IDs. @@ -1005,6 +1006,7 @@ impl AcpxCommandExecutor { session: None, restore_checked: false, recovery_pending_effects: None, + failed_turn_end_restart: None, restore_error: None, persistence_error: None, #[cfg(test)] @@ -1927,6 +1929,18 @@ impl AcpxCommandExecutor { fn start_turn(&mut self, payload: &Value) -> Result { turn_start_request(payload)?; + if let Some(error) = self.failed_turn_end_restart.take() { + if self.session.is_none() + && self + .state + .as_ref() + .is_some_and(|state| state.lifecycle == "closed") + { + return Err(DurableRunnerError::invalid(format!( + "ACPX turn start failed closed: {error}" + ))); + } + } let reopenable = self.session.is_none() && self.state.as_ref().is_some_and(|state| { state.identity.is_some() @@ -1997,7 +2011,7 @@ impl AcpxCommandExecutor { let (provider_process_will_be_replaced, previous_process_id) = self .session .as_ref() - .map(|session| match session.prepared_from_process_id() { + .map(|session| match session.restarted_after_turn_from() { Some(process_id) => (true, process_id), None => (session.generation_has_run_turn(), session.process_id()), }) @@ -2611,7 +2625,7 @@ impl AcpxCommandExecutor { || self .session .as_ref() - .is_none_or(AcpxProviderSession::awaits_run_rebind) + .is_none_or(AcpxProviderSession::awaits_next_turn) { return Ok(()); } @@ -2622,7 +2636,7 @@ impl AcpxCommandExecutor { if self .session .as_ref() - .is_some_and(AcpxProviderSession::awaits_run_rebind) + .is_some_and(AcpxProviderSession::awaits_next_turn) { return Ok(()); } @@ -2909,6 +2923,10 @@ impl CommandExecutor for AcpxCommandExecutor { self.prepare_session_for_park(deadline) } + fn prepare_next_turn(&mut self) -> Result<(), DurableRunnerError> { + self.restart_session_after_turn() + } + fn execute(&mut self, command: &Command) -> Result { self.restore()?; if self diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_generation.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_generation.rs index cd826f6df07..6d7cd78c6ba 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_generation.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_generation.rs @@ -54,18 +54,20 @@ impl AcpxProviderSession { self.generation_has_run_turn } + pub fn restarted_after_turn_from(&self) -> Option { + self.restarted_after_turn_from + } + pub fn awaits_run_rebind(&self) -> bool { self.prepared_generation.is_some() } - pub fn prepared_from_process_id(&self) -> Option { - self.prepared_generation - .as_ref() - .map(|(_, process_id)| *process_id) + pub fn awaits_next_turn(&self) -> bool { + self.prepared_generation.is_some() || self.restarted_after_turn_from.is_some() } pub(super) fn park_tail_is_quiet(&self) -> bool { - if self.prepared_generation.is_none() { + if !self.awaits_next_turn() { return self.transport.buffered_events().len() == 0; } self.transport.buffered_events().all(|event| { @@ -78,65 +80,70 @@ impl AcpxProviderSession { }) } - pub fn can_prepare_next_generation(&self) -> bool { + fn at_settled_turn_boundary(&self) -> bool { !self.closed && self.failure.is_none() && !self.transport_terminated && !self.runtime_retired - && self.generation_has_run_turn && self.completed_turn_observed && self.prepared_generation.is_none() && self.config.candidate_admission.is_none() && self.state.active_turn_id().is_none() && !self.state.has_pending_requests() && !self.state.has_pending_tools() - && self.transport.buffered_events().len() == 0 + && self.park_tail_is_quiet() } - pub fn prepare_next_generation(&mut self, deadline: Instant) -> Result<(), LocalRunnerError> { + pub fn can_restart_after_turn(&self) -> bool { + self.generation_has_run_turn && self.at_settled_turn_boundary() + } + + pub fn can_prepare_next_generation(&self) -> bool { + (self.generation_has_run_turn || self.restarted_after_turn_from.is_some()) + && self.at_settled_turn_boundary() + } + + pub fn restart_after_turn( + &mut self, + deadline: Option, + ) -> Result<(), LocalRunnerError> { + if !self.can_restart_after_turn() { + return Err(LocalRunnerError::invalid( + "ACPX provider is not at a settled turn boundary", + )); + } + let previous_process_id = self.process_id(); + if let Err(error) = self.restart_idle_provider(deadline) { + return Err(self.fail_closed(error)); + } + self.restarted_after_turn_from = Some(previous_process_id); + Ok(()) + } + + pub fn prepare_next_generation(&mut self, deadline: Instant) -> Result { if !self.can_prepare_next_generation() { return Err(LocalRunnerError::invalid( "ACPX provider is not at a reusable generation boundary", )); } - let previous_process_id = self.process_id(); - let mut tool_bridge = self.tool_bridge.clone(); - let mut reserved_tool_bridge = self.reserved_tool_bridge.clone(); - tool_bridge - .prepare_turn_after_provider_restart() - .and_then(|()| reserved_tool_bridge.prepare_turn_after_provider_restart()) - .map_err(|error| { - LocalRunnerError::invalid(format!( - "ACPX tool receipt rotation before park failed: {error}" - )) - })?; - if let Err(error) = self.restart_idle_provider(Some(deadline)) { - return Err(self.fail_closed(error)); - } - if self.state.settled_turn_identity_capacity_reached() { - if let Err(error) = self - .state - .rotate_settled_turn_identities_after_provider_restart() - { + let restarted = self.generation_has_run_turn; + if restarted { + self.restart_after_turn(Some(deadline))?; + if let Err(error) = self.verify_dormant_session() { return Err(self.fail_closed(error)); } + self.transport.bound_requests_by(None); } - self.tool_bridge = tool_bridge; - self.reserved_tool_bridge = reserved_tool_bridge; self.completed_turn_observed = false; - if let Err(error) = self.verify_dormant_session() { - return Err(self.fail_closed(error)); - } - self.transport.bound_requests_by(None); - self.prepared_generation = Some((generation_digest(&self.config), previous_process_id)); - Ok(()) + self.prepared_generation = Some(generation_digest(&self.config)); + Ok(restarted) } pub fn rebind_run( &mut self, next: &AcpxProviderSessionConfig, ) -> Result { - let Some((prepared, _)) = self.prepared_generation.as_ref() else { + let Some(prepared) = self.prepared_generation.as_ref() else { return Ok(false); }; if self.closed @@ -194,6 +201,7 @@ impl AcpxProviderSession { self.config = next.clone(); self.catalog_revision = next.catalog_revision; self.working_directory = next.working_directory.clone(); + self.restarted_after_turn_from = None; self.prepared_generation = None; if let Err(error) = self.verify_dormant_session() { return Err(self.fail_closed(error)); diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs index a81d3db3742..e588b0a4d68 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs @@ -357,7 +357,8 @@ pub struct AcpxProviderSession { runtime_retired: bool, completed_turn_observed: bool, generation_has_run_turn: bool, - prepared_generation: Option<(String, u32)>, + restarted_after_turn_from: Option, + prepared_generation: Option, } pub(crate) fn goal_is_dormant(projection: &Value) -> bool { @@ -417,6 +418,7 @@ impl AcpxProviderSession { runtime_retired: false, completed_turn_observed: false, generation_has_run_turn: false, + restarted_after_turn_from: None, prepared_generation: None, }) } @@ -660,12 +662,13 @@ impl AcpxProviderSession { // provider turn originated a late HTTP callback. Reap the old sidecar // and provider before releasing its call-ID tombstones, then resume // the same verified persistent session in a fresh process generation. - let provider_restarted = self.generation_has_run_turn; - if provider_restarted { + let restart_now = self.generation_has_run_turn; + if restart_now { if let Err(error) = self.restart_idle_provider(None) { return Err(self.fail_closed(error)); } } + let provider_restarted = restart_now || self.restarted_after_turn_from.is_some(); if rotate_turn_identity_ledger { if let Err(error) = self .state @@ -700,6 +703,7 @@ impl AcpxProviderSession { )))); } self.generation_has_run_turn = true; + self.restarted_after_turn_from = None; self.prepared_generation = None; let response = match self.transport.request_turn_start( json!({"turnId":turn_id,"message":message,"performance":performance}), @@ -2256,6 +2260,7 @@ mod mode_tests { runtime_retired: false, completed_turn_observed: false, generation_has_run_turn: false, + restarted_after_turn_from: None, prepared_generation: None, } } diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs index 76a6a309f09..cfba5cc3ba9 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs @@ -299,6 +299,20 @@ fn run() -> Result<(), Box> { source_pending_completion = Some(turn.to_owned()); continue; } + if fault == "pending-input" { + write_turn_event( + &mut stdout, + next_sequence, + "runtime.input_requested", + &source_run, + turn, + json!({"requestId":"input-at-terminal","questionSet":{ + "schema":"paperclip.question_set.v1", + "questions":[{"id":"target","prompt":"Which target?","required":true, + "answerMode":"single_select","options":[{"id":"first","label":"First"}]}]}}), + )?; + next_sequence += 1; + } write_turn_event( &mut stdout, next_sequence, diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs b/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs index 5a45df27369..d786da4a324 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/durable/runner.rs @@ -323,6 +323,10 @@ pub trait CommandExecutor { Ok(false) } + fn prepare_next_turn(&mut self) -> Result<(), DurableRunnerError> { + Ok(()) + } + fn execute(&mut self, command: &Command) -> Result; fn observe_recovery_state(&mut self, _state: &DurableState) -> Result<(), DurableRunnerError> { @@ -2168,7 +2172,11 @@ fn poll_executor_events_after_controller_ack( if state.acked_source_seq < sent_source_seq { return executor.maintain_backpressured_provider(); } - poll_executor_events(state, store, config, executor) + poll_executor_events(state, store, config, executor)?; + if state.acked_source_seq >= state.highest_source_seq() { + executor.prepare_next_turn()?; + } + Ok(()) } fn poll_executor_events_when_control_idle( @@ -5075,6 +5083,98 @@ mod tests { fs::remove_dir_all(directory).unwrap(); } + #[test] + fn turn_boundary_preparation_waits_until_the_controller_acks_the_terminal_event() { + struct BoundaryExecutor { + events: VecDeque, + calls: Vec<&'static str>, + } + impl CommandExecutor for BoundaryExecutor { + fn execute(&mut self, _: &Command) -> Result { + panic!("the poll gate cannot execute a command") + } + fn poll_events(&mut self) -> Result, DurableRunnerError> { + self.calls.push("poll"); + Ok(self.events.iter().cloned().collect()) + } + fn acknowledge_events(&mut self, count: usize) -> Result<(), DurableRunnerError> { + self.events.drain(..count); + Ok(()) + } + fn maintain_backpressured_provider(&mut self) -> Result<(), DurableRunnerError> { + self.calls.push("maintain"); + Ok(()) + } + fn prepare_next_turn(&mut self) -> Result<(), DurableRunnerError> { + self.calls.push("prepare"); + Ok(()) + } + } + let directory = std::env::temp_dir().join(format!( + "paperclip-runner-turn-boundary-{}", + uuid::Uuid::new_v4() + )); + let config = config(directory.clone()); + let store = DurableStateStore::new(&directory).unwrap(); + let (mut state, _) = store.load_or_create(&config).unwrap(); + let mut executor = BoundaryExecutor { + events: VecDeque::from([PolledEvent { + executor_event_id: "provider-terminal".to_owned(), + event_type: "turn.completed".to_owned(), + priority: EventPriority::P0, + payload: json!({"status": "completed"}), + }]), + calls: Vec::new(), + }; + poll_executor_events_after_controller_ack(&mut state, &store, &config, &mut executor, 0) + .unwrap(); + assert_eq!(executor.calls, ["poll"]); + assert_eq!(state.highest_source_seq(), 1); + poll_executor_events_after_controller_ack(&mut state, &store, &config, &mut executor, 0) + .unwrap(); + assert_eq!( + executor.calls, + ["poll", "poll"], + "an unsent terminal is not out" + ); + poll_executor_events_after_controller_ack(&mut state, &store, &config, &mut executor, 1) + .unwrap(); + assert_eq!(executor.calls[2..], ["maintain"]); + state.apply_ack(1, 2).unwrap(); + store.save(&state).unwrap(); + poll_executor_events_when_control_idle( + &mut state, + &store, + &config, + &mut executor, + 1, + &Ok(Some(json!({"kind": "command"}))), + ) + .unwrap(); + assert_eq!(executor.calls[3..], ["maintain"], "control frames go first"); + poll_executor_events_when_control_idle( + &mut state, + &store, + &config, + &mut executor, + 1, + &Err(DurableRunnerError::invalid("disconnected")), + ) + .unwrap(); + assert_eq!(executor.calls.len(), 4); + poll_executor_events_when_control_idle( + &mut state, + &store, + &config, + &mut executor, + 1, + &Ok(None), + ) + .unwrap(); + assert_eq!(executor.calls[4..], ["poll", "prepare"]); + fs::remove_dir_all(directory).unwrap(); + } + #[test] fn controller_ack_debt_runs_cleanup_without_moving_event_or_ack_cursors() { struct MaintenanceExecutor { diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/native_provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/src/native_provider_backend.rs index f1f593634d9..13551eb72b9 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/native_provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/native_provider_backend.rs @@ -344,6 +344,13 @@ impl CommandExecutor for NativeProviderCommandExecutor { } } + fn prepare_next_turn(&mut self) -> Result<(), DurableRunnerError> { + match self.selected.as_mut() { + Some(SelectedExecutor::Acpx(executor)) => executor.prepare_next_turn(), + _ => Ok(()), + } + } + fn observe_recovery_state( &mut self, state: &crate::durable::DurableState, diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_preparation.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_preparation.rs index 0f7fcf96fc7..f9d9a3072ca 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_preparation.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_preparation.rs @@ -14,6 +14,9 @@ use paperclip_runner_core::provider_bridge::{authorized_tool_catalog_digest, Aut use serde_json::{json, Value}; use sha2::{Digest, Sha256}; +#[path = "support/acpx_turn_end_restart.rs"] +mod turn_end_restart; + struct Fixture(PathBuf); impl Drop for Fixture { fn drop(&mut self) { @@ -267,8 +270,9 @@ fn last_process_start(journal: &[Value]) -> usize { fn prepared_park(harness: &mut Harness) -> Value { harness.complete_turn("first"); + harness.executor.prepare_next_turn().unwrap(); assert!(harness.executor.host_park_ready().unwrap()); - assert!(harness.executor.prepare_for_park(park_deadline()).unwrap()); + assert!(!harness.executor.prepare_for_park(park_deadline()).unwrap()); harness.fixture.process_starts().last().unwrap()["processStart"].clone() } diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_readiness.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_readiness.rs index 4b56a5abc4c..f1994f3b21a 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_readiness.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_park_readiness.rs @@ -265,8 +265,12 @@ fn hosted_park_acpx_requires_completion_in_this_lifetime_and_exact_authority() { )) .unwrap(); settle(&mut executor); - assert!(executor.host_park_ready().unwrap()); - assert!(executor + executor.prepare_next_turn().unwrap(); + assert!( + executor.host_park_ready().unwrap(), + "a generation restarted at the turn boundary is park-ready" + ); + assert!(!executor .prepare_for_park(Instant::now() + Duration::from_secs(20)) .unwrap()); assert!( @@ -479,3 +483,45 @@ fn hosted_park_acpx_does_not_enable_an_unqualified_park_provider() { .unwrap()); executor.shutdown().unwrap(); } + +#[test] +fn a_turn_end_restart_waits_until_an_abandoned_park_observation_tail_is_delivered() { + let (fixture, _config, mut executor, opened) = fixture("claude"); + executor + .execute(&command( + 3, + "turn.start", + json!({"turnId":"first", "text":"Complete first work."}), + )) + .unwrap(); + settle(&mut executor); + fs::write(fixture.0.join("fault"), "slow-snapshot").unwrap(); + assert!(!executor.host_park_ready().unwrap()); + fs::write(fixture.0.join("fault"), "").unwrap(); + let starts = || { + fs::read_to_string(fixture.0.join("sidecar.jsonl")) + .unwrap() + .lines() + .filter(|line| line.contains("processStart")) + .count() + }; + executor.prepare_next_turn().unwrap(); + assert_eq!( + starts(), + 1, + "the old generation's tail is not yet delivered" + ); + assert!(std::process::Command::new("kill") + .args(["-0", &opened["processId"].to_string()]) + .status() + .unwrap() + .success()); + let events = executor.poll_events().unwrap(); + assert!(events + .iter() + .any(|event| event.payload["code"] == "slow_park_observation")); + executor.acknowledge_events(events.len()).unwrap(); + executor.prepare_next_turn().unwrap(); + assert_eq!(starts(), 2); + executor.shutdown().unwrap(); +} diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_turns.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_turns.rs index 1294ea1adf5..f31bb5e1da5 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_turns.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_turns.rs @@ -934,3 +934,83 @@ fn prepared_generation_never_admits_the_reaped_generation_late_callback() { session.shutdown("test complete").unwrap(); std::fs::remove_dir_all(directory).unwrap(); } + +#[test] +fn a_turn_end_restart_reaps_the_late_callback_and_releases_receipts_at_the_next_turn() { + let mut session = AcpxProviderSession::start(&config("turns-late-tool-after-suspend")).unwrap(); + session + .start_turn("turn-1", "Please continue", &std::env::temp_dir()) + .unwrap(); + session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + assert!(!session.can_restart_after_turn()); + session + .deliver_tool_result(&late_result("call-reused")) + .unwrap(); + let first_pid = session.process_id(); + session.restart_after_turn(None).unwrap(); + let fresh_pid = session.process_id(); + assert_ne!(fresh_pid, first_pid); + assert_eq!(session.restarted_after_turn_from(), Some(first_pid)); + assert!(!session.generation_has_run_turn() && !session.can_restart_after_turn()); + assert!(session + .poll_event(Duration::from_millis(200)) + .unwrap() + .is_none()); + + session + .start_turn("turn-2", "Use the fresh provider", &std::env::temp_dir()) + .unwrap(); + assert_eq!(session.process_id(), fresh_pid); + assert_eq!(session.restarted_after_turn_from(), None); + let fresh = session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + assert!(matches!( + &fresh[0], + AcpxProviderStateEvent::ToolCall { call_id, operation_id, input } + if call_id == "call-reused" + && operation_id == "issues.read" + && input["id"] == "issue-1" + )); + let terminal = session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + assert!(matches!( + terminal.last().unwrap(), + AcpxProviderStateEvent::TurnTerminal { turn_id, .. } if turn_id == "turn-2" + )); + session.shutdown("test complete").unwrap(); +} + +#[test] +fn an_interrupted_or_retired_turn_restarts_at_the_next_turn_start() { + let mut session = AcpxProviderSession::start(&config("turns")).unwrap(); + session + .start_turn("turn-1", "Please help", &std::env::temp_dir()) + .unwrap(); + session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + assert!(!session.can_restart_after_turn()); + session + .interrupt_turn("turn-1", "Paperclip interruption") + .unwrap(); + session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + let interrupted_pid = session.process_id(); + assert!(!session.can_restart_after_turn()); + assert!(session.restart_after_turn(None).is_err()); + assert_eq!(session.process_id(), interrupted_pid); + session + .start_turn("turn-2", "Start again", &std::env::temp_dir()) + .unwrap(); + assert_ne!(session.process_id(), interrupted_pid); + session.shutdown("test complete").unwrap(); + + let mut retired = AcpxProviderSession::start(&config("turns-retired")).unwrap(); + retired + .start_turn("turn-1", "Please help", &std::env::temp_dir()) + .unwrap(); + retired + .interrupt_turn("turn-1", "Paperclip cancellation") + .unwrap(); + retired.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + assert!(retired.state().active_turn_id().is_none()); + assert!(retired.runtime_retired()); + assert!(!retired.can_restart_after_turn()); + retired.shutdown("test complete").unwrap(); +} diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/support/acpx_turn_end_restart.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/support/acpx_turn_end_restart.rs new file mode 100644 index 00000000000..181dc19fd06 --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/support/acpx_turn_end_restart.rs @@ -0,0 +1,333 @@ +use super::*; + +const WARM_TURN: [&str; 2] = ["turn.start", "session.goal.get"]; + +fn live_providers(harness: &Harness) -> Vec { + harness + .fixture + .process_starts() + .into_iter() + .map(|line| line["processStart"].clone()) + .filter(alive) + .collect() +} + +fn settle(harness: &mut Harness, turn: &str) -> Vec { + let events = harness.complete_turn(turn); + harness.executor.prepare_next_turn().unwrap(); + events +} + +fn start_follow_up(harness: &mut Harness, turn: &str) -> (Vec, Vec, Duration) { + let before = harness.fixture.journal().len(); + let started = Instant::now(); + let execution = harness.execute( + "turn.start", + json!({"turnId": turn, "text": "Complete the work."}), + ); + let elapsed = started.elapsed(); + let mut events = execution + .events + .iter() + .map(|(kind, _, payload)| json!({"type":kind,"payload":payload})) + .collect::>(); + let deadline = Instant::now() + Duration::from_secs(5); + while !events + .iter() + .any(|event: &Value| event["type"] == "turn.completed") + { + assert!(Instant::now() < deadline, "no completed provider turn"); + events.extend(harness.drain()); + } + events.extend(harness.drain()); + let commands = commands_after(&harness.fixture.journal(), before); + (events, commands, elapsed) +} + +fn reconciled(events: &[Value]) -> &Value { + &events + .iter() + .find(|event| event["type"] == "session.reconciled") + .expect("the turn reports the replaced provider process")["payload"] +} + +#[test] +fn a_settled_turn_restarts_its_provider_and_the_next_turn_starts_without_one() { + let mut harness = Harness::new(); + let old_pid = harness.opened["processId"].clone(); + harness.complete_turn("first"); + let before = harness.fixture.journal().len(); + let started = Instant::now(); + harness.executor.prepare_next_turn().unwrap(); + let restart = started.elapsed(); + let journal = harness.fixture.journal(); + let spawned = last_process_start(&journal); + assert_eq!( + commands_after(&journal[..spawned], before), + ["session.suspend"] + ); + assert_eq!(journal[spawned]["livePredecessors"], json!([])); + assert_eq!( + commands_after(&journal, spawned), + ["initialize", "session.open", "run.attach"] + ); + assert_eq!(journal[spawned + 3]["params"]["runId"], "run-1"); + let fresh_pid = journal[spawned]["processStart"].clone(); + assert!(!alive(&old_pid)); + assert_eq!(live_providers(&harness), [fresh_pid.clone()]); + assert_eq!( + harness.drain(), + Vec::::new(), + "the fresh generation's output waits for its first turn" + ); + harness.executor.prepare_next_turn().unwrap(); + assert_eq!(harness.fixture.process_starts().len(), 2); + + let (events, commands, turn_start) = start_follow_up(&mut harness, "second"); + eprintln!("turn-end restart={restart:?} follow-up turn.start={turn_start:?}"); + assert_eq!(commands, WARM_TURN); + assert_eq!(reconciled(&events)["previousProcessId"], old_pid); + assert_eq!(reconciled(&events)["processId"], fresh_pid); + assert!(events + .iter() + .any(|event| event["type"] == "harness.diagnostic" + && event["payload"]["details"]["pid"] == fresh_pid)); + assert_eq!(harness.fixture.process_starts().len(), 2); + harness.executor.prepare_next_turn().unwrap(); + assert_eq!(harness.fixture.process_starts().len(), 3); + assert_eq!(live_providers(&harness).len(), 1); + let (_, commands, _) = start_follow_up(&mut harness, "third"); + assert_eq!(commands, WARM_TURN); + harness.executor.shutdown().unwrap(); + assert_eq!(live_providers(&harness), Vec::::new()); +} + +#[test] +fn park_reuses_the_generation_a_settled_turn_restarted() { + let mut harness = Harness::new(); + settle(&mut harness, "first"); + let fresh_pid = harness.fixture.process_starts().last().unwrap()["processStart"].clone(); + assert!(harness.executor.host_park_ready().unwrap()); + let before = harness.fixture.journal().len(); + let started = Instant::now(); + assert!( + !harness.executor.prepare_for_park(park_deadline()).unwrap(), + "park neither restarts nor asks for a second proof" + ); + eprintln!( + "park preparation after a turn-end restart={:?}", + started.elapsed() + ); + assert_eq!( + commands_after(&harness.fixture.journal(), before), + Vec::::new() + ); + assert!(harness.executor.host_park_ready().unwrap()); + assert_eq!(harness.fixture.process_starts().len(), 2); + + let (attached, _) = harness.attach("run-2", |_| {}); + assert_eq!(attached.result["processId"], fresh_pid); + let events = settle(&mut harness, "second"); + assert!(events + .iter() + .any(|event| event["type"] == "harness.diagnostic" + && event["payload"]["details"]["pid"] == fresh_pid)); + assert_eq!(harness.fixture.process_starts().len(), 3); + let (events, commands, _) = start_follow_up(&mut harness, "third"); + assert_eq!(commands, WARM_TURN); + assert_eq!(reconciled(&events)["previousProcessId"], fresh_pid); + assert_eq!(harness.fixture.process_starts().len(), 3); + harness.executor.shutdown().unwrap(); +} + +#[test] +fn the_reaped_generation_late_callback_never_reaches_the_next_turn() { + let mut harness = Harness::new(); + harness.complete_turn("first"); + harness.fixture.fault("late-tool-after-suspend"); + harness.executor.prepare_next_turn().unwrap(); + harness.fixture.fault(""); + assert_eq!(harness.drain(), Vec::::new()); + let (events, commands, _) = start_follow_up(&mut harness, "second"); + assert_eq!(commands, WARM_TURN); + assert!(!format!("{events:?}").contains("late-call")); + assert!(!format!("{events:?}").contains("late-old-generation")); + assert_eq!(harness.fixture.process_starts().len(), 2); + harness.executor.shutdown().unwrap(); +} + +#[test] +fn a_turn_that_arrives_before_the_turn_end_restart_restarts_at_its_start() { + let mut harness = Harness::new(); + let old_pid = harness.opened["processId"].clone(); + harness.complete_turn("first"); + let (events, commands, _) = start_follow_up(&mut harness, "second"); + assert_eq!(commands[..2], ["session.suspend", "initialize"]); + assert_eq!(reconciled(&events)["previousProcessId"], old_pid); + assert_eq!(harness.fixture.process_starts().len(), 2); + assert_eq!(live_providers(&harness).len(), 1); + harness.executor.prepare_next_turn().unwrap(); + assert_eq!(harness.fixture.process_starts().len(), 3); + assert_eq!(live_providers(&harness).len(), 1); + let (_, commands, _) = start_follow_up(&mut harness, "third"); + assert_eq!(commands, WARM_TURN); + harness.executor.shutdown().unwrap(); +} + +#[test] +fn a_park_before_the_turn_end_restart_restarts_once() { + let mut harness = Harness::new(); + harness.complete_turn("first"); + assert!(harness.executor.host_park_ready().unwrap()); + assert!(harness.executor.prepare_for_park(park_deadline()).unwrap()); + assert!(harness.executor.host_park_ready().unwrap()); + harness.executor.prepare_next_turn().unwrap(); + assert_eq!(harness.fixture.process_starts().len(), 2); + assert_eq!(live_providers(&harness).len(), 1); + let (attached, _) = harness.attach("run-2", |_| {}); + assert_eq!(attached.result["processId"], live_providers(&harness)[0]); + harness.executor.shutdown().unwrap(); +} + +#[test] +fn a_failed_turn_end_restart_fails_the_next_turn_like_a_failed_turn_start_restart() { + let outcome = |turn_end: bool| { + let mut harness = Harness::new(); + let old_pid = harness.opened["processId"].clone(); + harness.complete_turn("first"); + harness.fixture.fault("reject-open"); + if turn_end { + harness.executor.prepare_next_turn().unwrap(); + assert!(!alive(&old_pid)); + assert!(!harness.executor.host_park_ready().unwrap()); + assert!(!harness.executor.prepare_for_park(park_deadline()).unwrap()); + assert_eq!(harness.drain(), Vec::::new()); + harness.executor.prepare_next_turn().unwrap(); + } + let first = harness + .executor + .execute(&command( + 90, + "turn.start", + json!({"turnId": "second", "text": "Complete the work."}), + )) + .unwrap_err() + .to_string(); + harness.fixture.fault(""); + let retry = harness + .executor + .execute(&command( + 91, + "turn.start", + json!({"turnId": "third", "text": "Complete the work."}), + )) + .unwrap_err() + .to_string(); + assert_eq!(live_providers(&harness), Vec::::new()); + let state = harness.fixture.state(); + let starts = harness.fixture.process_starts().len(); + harness.executor.shutdown().unwrap(); + ( + first, + retry, + state["lifecycle"].clone(), + state["providerExitUnconfirmed"].clone(), + state["activeTurnId"].clone(), + state["pendingEvents"].clone(), + starts, + ) + }; + let at_turn_end = outcome(true); + assert!( + at_turn_end.0.starts_with("ACPX turn start failed closed:") + && at_turn_end.0.contains("session.open"), + "{}", + at_turn_end.0 + ); + assert_eq!(at_turn_end, outcome(false)); +} + +#[test] +fn cancellation_after_a_turn_end_restart_is_already_settled() { + let mut harness = Harness::new(); + settle(&mut harness, "first"); + for kind in ["run.cancel", "turn.interrupt", "turn.stop"] { + let result = harness.execute(kind, json!({})).result; + assert_eq!(result["status"], "already_settled", "{kind}"); + } + let (_, commands, _) = start_follow_up(&mut harness, "second"); + assert_eq!(commands, WARM_TURN); + assert_eq!(harness.fixture.process_starts().len(), 2); + harness.executor.shutdown().unwrap(); +} + +#[test] +fn suspension_and_retirement_after_a_turn_end_restart_reap_the_fresh_generation() { + let mut harness = Harness::new(); + settle(&mut harness, "first"); + let fresh_pid = live_providers(&harness)[0].clone(); + harness.execute("runner.suspend", json!({})); + assert!(!alive(&fresh_pid)); + assert_eq!(harness.fixture.state()["lifecycle"], "suspended"); + let events = harness.complete_turn("second"); + assert!(events + .iter() + .any(|event| event["type"] == "session.resumed")); + assert_eq!(harness.fixture.process_starts().len(), 3); + harness.executor.prepare_next_turn().unwrap(); + let fresh_pid = live_providers(&harness)[0].clone(); + assert_eq!(harness.fixture.process_starts().len(), 4); + harness.executor.shutdown().unwrap(); + assert!(!alive(&fresh_pid)); +} + +#[test] +fn an_active_goal_or_a_request_at_the_terminal_keeps_today_s_restart_timing() { + let mut harness = Harness::new(); + harness.fixture.fault("goal"); + harness.complete_turn("first"); + harness.executor.prepare_next_turn().unwrap(); + assert_eq!( + harness.fixture.process_starts().len(), + 1, + "an active goal can keep working after its prompt" + ); + harness.executor.shutdown().unwrap(); + + let resolve = |turn_end: bool| { + let mut harness = Harness::new(); + harness.fixture.fault("pending-input"); + let events = harness.complete_turn("first"); + harness.fixture.fault(""); + if turn_end { + harness.executor.prepare_next_turn().unwrap(); + } + let resolved = harness + .executor + .execute(&command( + 90, + "request.resolve", + json!({"requestId": "input-at-terminal", "answers": {"target": "first"}}), + )) + .map(|execution| execution.result) + .map_err(|error| error.to_string()); + let (_, commands, _) = start_follow_up(&mut harness, "second"); + let starts = harness.fixture.process_starts().len(); + harness.executor.shutdown().unwrap(); + ( + events + .iter() + .map(|event| event["type"].clone()) + .collect::>(), + resolved, + commands.contains(&"session.suspend".to_owned()), + starts, + ) + }; + let at_turn_end = resolve(true); + let at_turn_start = resolve(false); + assert_eq!(at_turn_end.0, at_turn_start.0); + assert_eq!(at_turn_end.1, at_turn_start.1); + assert_eq!(at_turn_end.3, at_turn_start.3); + eprintln!("request at terminal: {at_turn_end:?}"); +} From 492316e576bf55d84754f4669c0c0a2881da6bd5 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:37:00 -0600 Subject: [PATCH 7/8] fix(plugins): accept secret_ref bindings in string secret-ref config fields (#20181) 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 > - Plugins declare their settings in an `instanceConfigSchema`, and the server validates each save against that schema > - Plugin credential fields use `type: "string", format: "secret-ref"` > - The settings form, the plugin secrets handler and the plugin SDK all use the `{ type: "secret_ref", secretId, version }` binding object for these fields, and the secrets handler rejects a bare secret id > - Schema validation ran on the raw config, so the binding object failed with "must be string" and no value could be saved > - This pull request validates the binding's secret id in place of the binding object > - The benefit is that an operator can bind a company secret to a plugin credential field from the settings page or the API ## Linked Issues or Issue Description **What happened** `POST /api/plugins/:pluginId/config` returned 400 "Configuration does not match the plugin's instanceConfigSchema" with `{"field":"/apiKey","message":"must be string"}` when the config held `{ "apiKey": { "type": "secret_ref", "secretId": "", "version": "latest" } }` for a field declared `{ "type": "string", "format": "secret-ref" }`. A bare UUID string passes the schema but the secrets handler then rejects it ("must use { type: \"secret_ref\", secretId, version? }"). So no value could be saved for a required secret-ref field. **Expected behavior** The binding object that the settings form submits and that `ctx.secrets.resolve()` accepts is saved. Other schema rules still apply. **Steps to reproduce** 1. Install a plugin whose manifest has `instanceConfigSchema.properties.apiKey = { "type": "string", "format": "secret-ref" }`. 2. Create a company secret. 3. On the plugin settings page, pick that secret for the field and click Save Configuration (or POST the binding object to `/api/plugins/:pluginId/config`). 4. See the 400 "must be string" response. Found while getting the chat eval cases `plugins.configure-plugin` and `plugins.disable-plugin` to run. Their setup and the human's settings-page save both hit this error. ## What Changed - `validateInstanceConfig` now replaces each valid `secret_ref` binding object at a schema `format: "secret-ref"` path with its secret id before Ajv validates. The stored config and the config sent to the worker do not change. - The walk follows array `items` (single and tuple) and objects inside arrays as well as object properties and `allOf`/`anyOf`/`oneOf`, so secret pickers inside array fields can save too. - A binding is accepted only when `envBindingSecretRefSchema` (the secrets handler's own parser) accepts it, so validation and the secrets handler agree on what is a valid binding. - Malformed bindings and objects in ordinary string fields still fail validation. - Added `server/src/__tests__/plugin-config-validator.test.ts`. ## Verification - `cd server && npx vitest run src/__tests__/plugin-config-validator.test.ts src/__tests__/plugin-routes-authz.test.ts src/__tests__/plugin-sandbox-provider-config-validation.test.ts`: 48 passed. - The new test fails on `master` without the fix (3 of 3 fail) and passes with it. - `cd server && npx vitest run src/__tests__/plugin-config-validator.test.ts`: 6 passed after the array/parser follow-up; the 3 follow-up tests fail on the previous commit. - `cd server && pnpm typecheck`: passes. ## Risks - Low risk. Only configs that held a valid binding object at a declared secret-ref path change result, from a 400 to accepted. The secrets handler still checks that the secret belongs to the company. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution in Claude Code. ## 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 --- .../__tests__/plugin-config-validator.test.ts | 99 +++++++++++++++++++ .../src/services/plugin-config-validator.ts | 48 ++++++++- 2 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 server/src/__tests__/plugin-config-validator.test.ts diff --git a/server/src/__tests__/plugin-config-validator.test.ts b/server/src/__tests__/plugin-config-validator.test.ts new file mode 100644 index 00000000000..e47eb8efe84 --- /dev/null +++ b/server/src/__tests__/plugin-config-validator.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import type { JsonSchema } from "@paperclipai/shared"; +import { validateInstanceConfig } from "../services/plugin-config-validator.js"; + +const SECRET_ID = "11111111-1111-4111-8111-111111111111"; +const binding = { type: "secret_ref", secretId: SECRET_ID, version: "latest" }; + +// The shape plugin manifests use: a string field marked as a secret reference. +const schema = { + type: "object", + properties: { + apiKey: { type: "string", title: "API key", format: "secret-ref" }, + region: { type: "string" }, + auth: { + type: "object", + properties: { token: { type: "string", format: "secret-ref" } }, + additionalProperties: false, + }, + }, + required: ["apiKey"], + additionalProperties: false, +} as unknown as JsonSchema; + +describe("validateInstanceConfig secret-ref fields", () => { + it("accepts the secret_ref binding object the settings form saves for a string secret-ref field", () => { + const configJson = { apiKey: binding, region: "us", auth: { token: { ...binding, version: 2 } } }; + expect(validateInstanceConfig(configJson, schema)).toEqual({ valid: true }); + // Validation does not rewrite what gets stored or sent to the worker. + expect(configJson.apiKey).toEqual(binding); + }); + + it("still enforces the rest of the schema around a binding", () => { + expect(validateInstanceConfig({ apiKey: binding, region: 5 }, schema)).toMatchObject({ + valid: false, + errors: [{ field: "/region" }], + }); + expect(validateInstanceConfig({ region: "us" }, schema).valid).toBe(false); + }); + + it("rejects objects that are not valid secret_ref bindings and objects in ordinary string fields", () => { + expect(validateInstanceConfig({ apiKey: { type: "secret_ref", secretId: "not-a-uuid" } }, schema)).toMatchObject({ + valid: false, + errors: [{ field: "/apiKey", message: "must be string" }], + }); + expect(validateInstanceConfig({ apiKey: binding, region: binding }, schema)).toMatchObject({ + valid: false, + errors: [{ field: "/region", message: "must be string" }], + }); + }); +}); + +describe("validateInstanceConfig secret-ref array items", () => { + const arraySchema = { + type: "object", + properties: { + keys: { type: "array", items: { type: "string", format: "secret-ref" } }, + accounts: { + type: "array", + items: { + type: "object", + properties: { name: { type: "string" }, token: { type: "string", format: "secret-ref" } }, + required: ["name", "token"], + additionalProperties: false, + }, + }, + }, + additionalProperties: false, + } as unknown as JsonSchema; + + it("accepts bindings the settings form saves in array items and nested array objects", () => { + const configJson = { + keys: [binding, { ...binding, version: 3 }], + accounts: [{ name: "primary", token: binding }], + }; + expect(validateInstanceConfig(configJson, arraySchema)).toEqual({ valid: true }); + expect(configJson.keys[0]).toEqual(binding); + expect(configJson.accounts[0]!.token).toEqual(binding); + }); + + it("still rejects malformed bindings and wrong types inside arrays", () => { + expect(validateInstanceConfig({ keys: [binding, { type: "secret_ref", secretId: "nope" }] }, arraySchema)).toMatchObject({ + valid: false, + errors: [{ field: "/keys/1", message: "must be string" }], + }); + expect(validateInstanceConfig({ accounts: [{ name: binding, token: binding }] }, arraySchema)).toMatchObject({ + valid: false, + errors: [{ field: "/accounts/0/name", message: "must be string" }], + }); + }); +}); + +describe("validateInstanceConfig secret-ref parsing matches the secrets handler", () => { + it("rejects a padded secret id that the secrets handler would also refuse", () => { + expect(validateInstanceConfig({ apiKey: { ...binding, secretId: ` ${SECRET_ID} ` } }, schema)).toMatchObject({ + valid: false, + errors: [{ field: "/apiKey", message: "must be string" }], + }); + }); +}); diff --git a/server/src/services/plugin-config-validator.ts b/server/src/services/plugin-config-validator.ts index 6eb95aa0fb2..404cc4e0041 100644 --- a/server/src/services/plugin-config-validator.ts +++ b/server/src/services/plugin-config-validator.ts @@ -10,13 +10,57 @@ import Ajv, { type ErrorObject } from "ajv"; import addFormats from "ajv-formats"; -import type { JsonSchema } from "@paperclipai/shared"; +import { envBindingSecretRefSchema, type JsonSchema } from "@paperclipai/shared"; export interface ConfigValidationResult { valid: boolean; errors?: { field: string; message: string }[]; } +/** + * Secret-ref fields are declared `type: "string"`, but the settings form and the + * plugin secrets API store the `{ type: "secret_ref", secretId, version }` binding + * object there. Validate the binding's secret id in its place so the schema + * checks the field without rejecting the only shape the secrets handler accepts. + * The walk follows object properties, array items and schema combinators, and + * accepts a binding only when the secrets handler's own parser accepts it. + */ +function secretRefsAsIds(value: unknown, schema: unknown): unknown { + if (!schema || typeof schema !== "object" || Array.isArray(schema)) return value; + const node = schema as Record; + + if (node.format === "secret-ref") { + const binding = envBindingSecretRefSchema.safeParse(value); + if (binding.success) return binding.data.secretId; + } + + let result = value; + for (const keyword of ["allOf", "anyOf", "oneOf"] as const) { + const branches = node[keyword]; + if (!Array.isArray(branches)) continue; + for (const branch of branches) result = secretRefsAsIds(result, branch); + } + + if (Array.isArray(result)) { + const items = node.items; + if (items && typeof items === "object") { + const itemSchemaAt = (index: number) => (Array.isArray(items) ? items[index] : items); + result = result.map((item, index) => secretRefsAsIds(item, itemSchemaAt(index))); + } + return result; + } + + const properties = node.properties; + if (result && typeof result === "object" && properties && typeof properties === "object") { + const record = { ...(result as Record) }; + for (const [key, propertySchema] of Object.entries(properties as Record)) { + if (key in record) record[key] = secretRefsAsIds(record[key], propertySchema); + } + result = record; + } + return result; +} + /** * Validate a config object against a JSON Schema. * @@ -39,7 +83,7 @@ export function validateInstanceConfig( // hint only — UUID validation happens in the secrets handler at resolve time. ajv.addFormat("secret-ref", { validate: () => true }); const validate = ajv.compile(schema); - const valid = validate(configJson); + const valid = validate(secretRefsAsIds(configJson, schema)); if (valid) { return { valid: true }; From 7c65a2f4794ac12cf4bcf5f18c88bd5765a5d41a Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 9 Nov 2026 00:39:39 -0600 Subject: [PATCH 8/8] fix(authz): link plugin settings and say reply now on board-only denials (#20182) 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 > - Chat agents run with agent credentials, and plugin management routes are board-only > - A board-only denial tells the agent the denial is final and links the board page that holds the control, when the page is known (#20151) > - Plugin routes had no known page, so the denial said "link a page only if you already know its route" > - In a chat eval, an agent asked to finish configuring a plugin got this denial and kept searching the API for about two minutes, so it did not answer in time > - This pull request names the plugin manager, or that plugin's settings page, in the denial > - The benefit is that the agent can hand the step to the person with a working link right away ## Linked Issues or Issue Description Refs #20151 **What happened** An agent that called a board-only plugin route (for example `GET /api/plugins` or `POST /api/plugins/:pluginId/config`) got the generic remediation with no page. In the `plugins.configure-plugin` chat eval, the agent kept calling `search_api` for another plugin route after the 403 and did not reply within the 150-second window. **Expected behavior** The denial names the page where a board user does the step: the plugin manager (`//company/settings/instance/plugins`) or the plugin's settings page (`//company/settings/instance/plugins/`). No link is given when the operator hides plugin management (`PAPERCLIP_HIDDEN_SETTINGS=instance.plugins`). **Steps to reproduce** 1. Install a plugin. 2. As an agent, call `POST /api/plugins//config`. 3. See a 403 `board_access_required` remediation without a plugin page. ## What Changed - `server/src/routes/authz.ts`: board-only denials on `/api/plugins` routes link the plugin manager, or the plugin's settings page when the path names a plugin UUID or key. The plugin settings page accepts either. No link when plugin management is hidden. - `server/src/routes/authz.ts`: every agent board-only denial now says to stop working on the denied step and reply now. With the plugin link alone, the eval agent still searched the API for two more minutes. - `server/src/routes/authz.ts`: any manifest-format plugin key (including dot-free keys such as `my-plugin`) gets a direct link; plugin manager endpoints (`examples`, `install`, `tools`, `ui-contributions`) link the plugin list. - `ui/src/pages/PluginSettings.tsx`: custom settings slots match by plugin key as well as plugin id, so a page opened by key shows the plugin's own settings. - `server/src/__tests__/board-access-denial.test.ts`: cases for list, install, config by id, disable by key, a dot-free key, manager endpoints, and the hidden-settings case. `ui/src/pages/PluginSettings.test.tsx`: opening the page by key mounts that plugin's settings slot. - Merged `master`, keeping its project access-members denial cases. ## Verification - `cd server && npx vitest run src/__tests__/board-access-denial.test.ts src/__tests__/agent-permissions-routes.test.ts src/__tests__/environment-routes.test.ts src/__tests__/execution-workspace-runtime-lease-route.test.ts`: 241 passed after merging master. - `cd ui && npx vitest run src/pages/PluginSettings.test.tsx`: 9 passed; the new key-route case fails without the `PluginSettings` change. - `cd server && pnpm typecheck`: passes. ## Risks - Low risk. Only the remediation text of agent board-only denials on `/api/plugins` routes changes. Board users and authorization do not change. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution in Claude Code. ## 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 --- .../src/__tests__/board-access-denial.test.ts | 21 ++++++++++++ server/src/routes/authz.ts | 20 ++++++++++- ui/src/pages/PluginSettings.test.tsx | 33 +++++++++++++++++-- ui/src/pages/PluginSettings.tsx | 5 +-- 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/server/src/__tests__/board-access-denial.test.ts b/server/src/__tests__/board-access-denial.test.ts index 827a085e12c..9518032f254 100644 --- a/server/src/__tests__/board-access-denial.test.ts +++ b/server/src/__tests__/board-access-denial.test.ts @@ -39,6 +39,13 @@ describe("board-only denial", () => { ["patch", `/api/agents/${AGENT_ID}/permissions`, `[Open the agent's permissions](//agents/${AGENT_ID}/permissions)`], ["post", `/api/agents/${AGENT_ID}/keys`, `[Open the agent's API keys](//agents/${AGENT_ID}/api-keys)`], ["post", `/api/agents/${AGENT_ID}/pause`, `[Open the agent's profile](//agents/${AGENT_ID})`], + ["get", "/api/plugins", "[Open plugin settings](//company/settings/instance/plugins)"], + ["post", "/api/plugins/install", "[Open plugin settings](//company/settings/instance/plugins)"], + ["post", `/api/plugins/${AGENT_ID}/config`, `[Open the plugin's settings](//company/settings/instance/plugins/${AGENT_ID})`], + ["post", "/api/plugins/acme.website-preview/disable", "[Open the plugin's settings](//company/settings/instance/plugins/acme.website-preview)"], + ["post", "/api/plugins/my-plugin/config", "[Open the plugin's settings](//company/settings/instance/plugins/my-plugin)"], + ["post", "/api/plugins/tools/execute", "[Open plugin settings](//company/settings/instance/plugins)"], + ["get", "/api/plugins/ui-contributions", "[Open plugin settings](//company/settings/instance/plugins)"], ["post", `/api/projects/${AGENT_ID}/access-members`, `[Open the project's access settings](//projects/${AGENT_ID}/configuration)`], ["delete", `/api/projects/${AGENT_ID}/access-members/m-1`, `[Open the project's access settings](//projects/${AGENT_ID}/configuration)`], ] as const)("tells an agent %s %s is final and names the board page", async (method, path, link) => { @@ -47,6 +54,7 @@ describe("board-only denial", () => { expect(res.body.code).toBe(BOARD_ACCESS_REQUIRED_CODE); expect(res.body.remediation).toContain("this denial is final"); expect(res.body.remediation).toContain("do not search for one"); + expect(res.body.remediation).toContain("reply now"); expect(res.body.remediation).toContain(link); }); @@ -88,6 +96,19 @@ describe("board-only denial", () => { expect(res.body.remediation).not.toContain("/company/settings/connections"); }); + it("does not link plugin settings while the operator hides plugin management", async () => { + const previous = process.env.PAPERCLIP_HIDDEN_SETTINGS; + process.env.PAPERCLIP_HIDDEN_SETTINGS = "instance.plugins"; + try { + const res = await request(createApp(agentActor)).post(`/api/plugins/${AGENT_ID}/config`).expect(403); + expect(res.body.remediation).toContain("this denial is final"); + expect(res.body.remediation).not.toContain("/instance/plugins"); + } finally { + if (previous === undefined) delete process.env.PAPERCLIP_HIDDEN_SETTINGS; + else process.env.PAPERCLIP_HIDDEN_SETTINGS = previous; + } + }); + it("applies through the org-access guard", async () => { const res = await request(createApp(agentActor, assertBoardOrgAccess)).delete(`/api/companies/${COMPANY_ID}`).expect(403); expect(res.body.code).toBe(BOARD_ACCESS_REQUIRED_CODE); diff --git a/server/src/routes/authz.ts b/server/src/routes/authz.ts index c6c60559a03..8b39815fc45 100644 --- a/server/src/routes/authz.ts +++ b/server/src/routes/authz.ts @@ -3,6 +3,7 @@ import type { SecretBindingTargetType } from "@paperclipai/shared"; import { forbidden, HttpError, unauthorized } from "../errors.js"; import { logger } from "../middleware/logger.js"; import { responsibleUserAuthzShadowMode } from "../services/authorization.js"; +import { getHiddenSettings } from "../services/settings-visibility.js"; function throwOrShadowResponsibleUserCompanyAccessDeny( req: Request, @@ -65,8 +66,25 @@ const BOARD_CONTROL_PAGES: Array<[RegExp, (match: RegExpExecArray, req: Request) [/^\/companies\/[^/]+(?:\/(?:archive|branding|settings))?\/?$/, () => ["company settings", "//company/settings"]], ]; +// Plugin management pages. A plugin id or key segment opens that plugin's settings +// (the page accepts either); a plugin-manager endpoint segment opens the manager. +// An operator can hide plugin management, so link it only while it is visible. +const PLUGIN_REFERENCE = /^[a-z0-9][a-z0-9._-]*$/i; +const PLUGIN_MANAGER_SEGMENTS = new Set(["examples", "install", "tools", "ui-contributions"]); +function pluginControlPage(path: string): [string, string] | null { + const match = /^\/plugins(?:\/([^/]+))?(?:\/|$)/.exec(path); + if (!match || getHiddenSettings().has("instance.plugins")) return null; + const segment = match[1]; + const plugin = segment && PLUGIN_REFERENCE.test(segment) && !PLUGIN_MANAGER_SEGMENTS.has(segment) ? segment : null; + return plugin + ? ["the plugin's settings", `//company/settings/instance/plugins/${plugin}`] + : ["plugin settings", "//company/settings/instance/plugins"]; +} + function boardControlPage(req: Request) { const path = (req.originalUrl ?? "").split("?")[0]!.replace(/^\/api(?=\/)/, ""); + const pluginPage = pluginControlPage(path); + if (pluginPage) return pluginPage; for (const [pattern, page] of BOARD_CONTROL_PAGES) { const match = pattern.exec(path); if (match) return page(match, req); @@ -86,7 +104,7 @@ export function boardAccessRequired(req: Request) { : "They can do it in the Paperclip web app or with their own board credentials. Link a page only if you already know its route; do not search for one."; return forbidden("Board access required", { code: BOARD_ACCESS_REQUIRED_CODE, - remediation: `This action is board-only by design and this denial is final: no agent credential, tool or other API route can do it, so do not search for one. Tell the person that a board user must do it. ${where}`, + remediation: `This action is board-only by design and this denial is final: no agent credential, tool or other API route can do it, so do not search for one. Stop working on this step and reply now: tell the person that a board user must do it. ${where}`, }); } diff --git a/ui/src/pages/PluginSettings.test.tsx b/ui/src/pages/PluginSettings.test.tsx index 402869be166..d129f093637 100644 --- a/ui/src/pages/PluginSettings.test.tsx +++ b/ui/src/pages/PluginSettings.test.tsx @@ -19,6 +19,8 @@ const mockPluginsApi = vi.hoisted(() => ({ })); const mockSetBreadcrumbs = vi.hoisted(() => vi.fn()); +const mockRoute = vi.hoisted(() => ({ pluginId: "plugin-1" })); +const mockSlots = vi.hoisted(() => ({ slots: [] as Array> })); vi.mock("@/api/plugins", () => ({ pluginsApi: mockPluginsApi, @@ -40,12 +42,12 @@ vi.mock("@/context/CompanyContext", () => ({ vi.mock("@/lib/router", () => ({ Link: ({ to, children }: { to: string; children: React.ReactNode }) => {children}, Navigate: () => null, - useParams: () => ({ companyPrefix: "PAP", pluginId: "plugin-1" }), + useParams: () => ({ companyPrefix: "PAP", pluginId: mockRoute.pluginId }), })); vi.mock("@/plugins/slots", () => ({ - PluginSlotMount: () => null, - usePluginSlots: () => ({ slots: [] }), + PluginSlotMount: ({ slot }: { slot: { id: string } }) =>
{slot.id}
, + usePluginSlots: () => ({ slots: mockSlots.slots }), })); vi.mock("@/components/PageTabBar", () => ({ @@ -168,6 +170,31 @@ describe("PluginSettings", () => { container.remove(); document.body.innerHTML = ""; vi.clearAllMocks(); + mockRoute.pluginId = "plugin-1"; + mockSlots.slots = []; + }); + + it("shows a plugin's custom settings page when the route names the plugin by key", async () => { + mockRoute.pluginId = "acme.website-preview"; + mockSlots.slots = [ + { id: "settings", type: "settingsPage", pluginId: "plugin-1", pluginKey: "acme.website-preview" }, + { id: "other-settings", type: "settingsPage", pluginId: "plugin-2", pluginKey: "acme.other" }, + ]; + mockPluginsApi.get.mockResolvedValue(basePlugin({ + pluginKey: "acme.website-preview", + status: "ready", + manifestJson: { displayName: "Website Preview", version: "0.1.0", capabilities: [] }, + })); + + const root = await renderSettings(container); + + const mounted = [...container.querySelectorAll('[data-testid="settings-slot"]')].map((node) => node.textContent); + expect(mounted).toEqual(["settings"]); + expect(container.textContent).not.toContain("This plugin does not require any settings."); + + await act(async () => { + root.unmount(); + }); }); it("routes environment-provider plugins to instance environments when they have no instance config", async () => { diff --git a/ui/src/pages/PluginSettings.tsx b/ui/src/pages/PluginSettings.tsx index f48e539ab1e..86202461bca 100644 --- a/ui/src/pages/PluginSettings.tsx +++ b/ui/src/pages/PluginSettings.tsx @@ -118,8 +118,9 @@ export function PluginSettings() { enabled: !!selectedCompanyId, }); - // Filter slots to only show settings pages for this specific plugin - const pluginSlots = slots.filter((slot) => slot.pluginId === pluginId); + // Filter slots to only show settings pages for this specific plugin. The route + // segment can be the plugin's id or its key, so match either. + const pluginSlots = slots.filter((slot) => slot.pluginId === pluginId || slot.pluginKey === pluginId); // If the plugin has a custom settingsPage slot, prefer that over auto-generated form const hasCustomSettingsPage = pluginSlots.length > 0;