From a2e7ffdc346797ed7f7805d98f37e03a2237a327 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 14 Sep 2026 22:13:57 -0500 Subject: [PATCH] fix(runtime): validate sandbox paths and preserve live controller leases (#13432) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents can run in remote sandboxes. > - Connection checks must use the selected execution target. > - Recovery must respect the controller that owns an active run. > - A host path or PID does not describe a remote sandbox. > - This pull request checks sandbox paths on the target and preserves live controller leases. ## Linked Issues or Issue Description **What happened?** Selecting an AI account for a sandbox agent could fail because the Claude ACP environment check tried to create the sandbox directory on the Paperclip host. The recovery sweep could also interrupt a sandbox run while its controller lease was still valid. It treated a PID absent from the local host as proof that the run had stopped. **Expected behavior** ACP checks directories on the selected execution target. Recovery leaves a run with a live controller lease alone. Its final database write rejects a stale snapshot after renewal, a claim, a controller change, or a runtime change. **Steps to reproduce** 1. Test a Claude ACP sandbox agent with a directory that cannot be created on the host. The check fails before this fix. 2. Give a running sandbox task a valid controller lease and a PID absent from the host. Run the stale-lock sweep without an in-memory handle. The sweep interrupts the run before this fix. 3. Renew or replace the controller between the sweep's read and write. The old snapshot must not end that controller's run. **Paperclip version or commit** Rebased onto `origin/master` at `0e9b24c8216171c26c8358ba387d77858e02c7a9`. All seven regression cases still fail against this base. Refs #13438, which supplies the managed hiring and task-connection behavior, and #13433, which preserves non-assignee subscription comment wakes. This PR preserves both upstream changes and addresses the two remaining sandbox failures. ## What Changed - Resolve and create Claude ACP test directories through the execution-target helpers. - Preserve active legacy controller leases during stale-lock recovery, including finalization after a task becomes terminal. - Recheck the controller, lease, runtime mode, and native ownership in the terminal database write. - Add two sandbox-directory cases and five database-backed controller-lease cases. - Document the target used for ACP directory checks. ## Verification - Red: all seven new cases fail against `0e9b24c82` without these two implementation changes. - Before the final upstream sync, 192 focused tests passed. Full `pnpm test:run` coverage completed using the repository's group/shard runner: all general server and workspace groups passed, and all 147 serialized server suites passed across the initial run and isolated continuations. Five cold-import timeout suites passed with `--experimental.fsModuleCache`; their assertions and deadlines were unchanged. - The hiring routes, default-selection service, and upstream hiring tests match `origin/master` exactly. The two remaining fixes are unchanged by the final rebase. - On final head `bd28d5cefbdf7084acc3759199cd9661907e7a26`, all 372 focused tests pass across 16 suites covering both upstream changes and these fixes. Two timeouts in the combined run (database setup and an existing ACP case) pass in isolated reruns with fresh test homes and temporary directories. `pnpm -r typecheck` and `pnpm build` also pass on this head. - All 32 active checks pass on final head `bd28d5cef`, including the full test matrix and browser shards, in [CI run 34904204849](https://github.com/paperclipai/paperclip/actions/runs/34904204849). Two Storybook checks are skipped by path filters. - Greptile reviewed final head `bd28d5cef` at 5/5 with no findings or unresolved review threads. ## Risks - A failed remote directory check still blocks connection adoption. - A live controller retains finalization authority after its task becomes terminal. Cleanup waits for ownership to expire and must pass the final ownership check. - No schema or credential-storage changes. ## Model Used OpenAI GPT-6 in Codex, with reasoning, repository inspection, code execution, and API tools. The exact serving model ID and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [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/connections/AI-CONNECTIONS.md | 3 ++ .../claude-local/src/server/acp.test.ts | 22 +++++++++ .../adapters/claude-local/src/server/acp.ts | 9 +++- .../recovery-stale-issue-lock-sweep.test.ts | 48 +++++++++++++++++++ server/src/services/recovery/service.ts | 26 +++++++++- 5 files changed, 105 insertions(+), 3 deletions(-) diff --git a/doc/connections/AI-CONNECTIONS.md b/doc/connections/AI-CONNECTIONS.md index 980b07463f..baf936fe6b 100644 --- a/doc/connections/AI-CONNECTIONS.md +++ b/doc/connections/AI-CONNECTIONS.md @@ -93,6 +93,9 @@ subsequent agent creation fails or is cancelled. ## Runtime isolation `prepareManagedAiRuntime` is shared by runs, environment tests, and adoption. +Claude ACP validates working directories on the selected execution target. A +sandbox directory does not need to exist on the Paperclip server. When the agent +has no configured directory, the test uses the remote target's working directory. It checks responsible identity, membership, compatibility, connection health, human audience and agent installation before reading credentials. Missing credentials produce an actionable configuration failure; responsible-user diff --git a/packages/adapters/claude-local/src/server/acp.test.ts b/packages/adapters/claude-local/src/server/acp.test.ts index a0441be380..ffb0747364 100644 --- a/packages/adapters/claude-local/src/server/acp.test.ts +++ b/packages/adapters/claude-local/src/server/acp.test.ts @@ -449,6 +449,28 @@ describe("claude_local ACP lane", () => { }); }); + it.each([undefined, "/sandbox/configured-workspace"])("checks sandbox directories on the sandbox (configured cwd=%s)", async (configuredCwd) => { + const remoteCwd = "/sandbox/workspace"; + const mkdir = vi.spyOn(fs, "mkdir").mockRejectedValue(new Error("Host filesystem must not be used")); + const execute = vi.fn(async () => ({ + exitCode: 0, signal: null, timedOut: false, stdout: "", stderr: "", + pid: null, startedAt: new Date().toISOString(), + })); + try { + const result = await testClaudeAcpEnvironment({ + companyId: "company-1", adapterType: "claude_local", + config: { cwd: configuredCwd, agentCommand: "claude-agent-acp", env: { ANTHROPIC_API_KEY: "fixture" } }, + executionTarget: { kind: "remote", transport: "sandbox", remoteCwd, runner: { execute } }, + }); + expect(result.status, JSON.stringify(result.checks)).toBe("pass"); + expect(result.checks).toContainEqual(expect.objectContaining({ + code: "claude_acp_cwd_valid", message: `Working directory is valid: ${configuredCwd ?? remoteCwd}`, + })); + expect(mkdir).not.toHaveBeenCalled(); + expect(JSON.stringify(execute.mock.calls)).toContain(`mkdir -p '${configuredCwd ?? remoteCwd}'`); + } finally { mkdir.mockRestore(); } + }); + it("reports ACP prerequisites for the ACP lane", async () => { const root = await makeTempRoot("paperclip-claude-acp-env-"); const commandPath = path.join(root, "bin", "claude-agent-acp"); diff --git a/packages/adapters/claude-local/src/server/acp.ts b/packages/adapters/claude-local/src/server/acp.ts index cc7b4b207d..27b253694d 100644 --- a/packages/adapters/claude-local/src/server/acp.ts +++ b/packages/adapters/claude-local/src/server/acp.ts @@ -15,6 +15,7 @@ import { } from "@paperclipai/adapter-utils/local-process-sandbox"; import { ensureAdapterExecutionTargetCommandResolvable, + ensureAdapterExecutionTargetDirectory, readAdapterExecutionTarget, resolveAdapterExecutionTargetCwd, runAdapterExecutionTargetProcess, @@ -718,9 +719,13 @@ export async function testClaudeAcpEnvironment( }); } - const cwd = asString(config.cwd, process.cwd()); + const cwd = resolveAdapterExecutionTargetCwd(target, asString(config.cwd, ""), process.cwd()); try { - await fs.mkdir(cwd, { recursive: true }); + await ensureAdapterExecutionTargetDirectory(`claude-acp-envtest-${Date.now()}`, target, cwd, { + cwd, + env: {}, + createIfMissing: true, + }); checks.push({ code: "claude_acp_cwd_valid", level: "info", diff --git a/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts b/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts index 2cc53380fb..aef5c24ae6 100644 --- a/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts +++ b/server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts @@ -333,6 +333,54 @@ describeEmbeddedPostgres("recovery sweepStaleIssueLocks", () => { ); }); + it.each(["in_progress", "done"])("preserves a live legacy controller lease when the issue is %s", async (status) => { + const { companyId, agentId, runningRunId } = await seed(); + await db.update(heartbeatRuns).set({ + runtimeMode: "legacy", processPid: 2_000_000_000, + controllerBootId: randomUUID(), + controllerLeaseExpiresAt: new Date(Date.now() + 60_000), + }).where(eq(heartbeatRuns.id, runningRunId)); + const issueId = randomUUID(); + await db.insert(issues).values({ + id: issueId, companyId, title: "Remote controller still owns the run", status, + assigneeAgentId: agentId, executionRunId: runningRunId, checkoutRunId: runningRunId, + }); + const result = await recoveryService(db, { enqueueWakeup: vi.fn() }).sweepStaleIssueLocks(); + expect(result).toEqual({ cleared: 0, issueIds: [], terminalizedRunIds: [] }); + expect(await db.select({ status: heartbeatRuns.status }).from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runningRunId))).toEqual([{ status: "running" }]); + expect(await db.select({ executionRunId: issues.executionRunId }).from(issues) + .where(eq(issues.id, issueId))).toEqual([{ executionRunId: runningRunId }]); + }); + + it.each(["renew", "replace", "claim"])("fences a legacy controller %s between the orphan check and terminal write", async (change) => { + const { companyId, agentId, runningRunId } = await seed(); + const bootId = randomUUID(); + await db.update(heartbeatRuns).set({ + runtimeMode: "legacy", processPid: 2_000_000_000, + controllerBootId: change === "claim" ? null : bootId, + controllerLeaseExpiresAt: new Date(Date.now() - 60_000), + }).where(eq(heartbeatRuns.id, runningRunId)); + await db.insert(issues).values({ + id: randomUUID(), companyId, title: "Controller changed during sweep", status: "in_progress", + assigneeAgentId: agentId, executionRunId: runningRunId, checkoutRunId: runningRunId, + }); + const result = await recoveryService(db, { + enqueueWakeup: vi.fn(), + beforeOrphanedRunTerminalWrite: async () => { + await db.update(heartbeatRuns).set({ + controllerBootId: change === "renew" ? bootId : randomUUID(), + // A replacement invalidates the old snapshot even if its lease expires. + controllerLeaseExpiresAt: new Date(Date.now() + (change === "replace" ? -30_000 : 60_000)), + }).where(eq(heartbeatRuns.id, runningRunId)); + }, + }).sweepStaleIssueLocks(); + expect(result).toEqual({ cleared: 0, issueIds: [], terminalizedRunIds: [] }); + expect(await db.select({ status: heartbeatRuns.status }).from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runningRunId))).toEqual([{ status: "running" }]); + expect(mockTelemetryClient.track).not.toHaveBeenCalled(); + }); + it("preserves a process-less native run while same-run resumption owns its retry", async () => { const { companyId, agentId, runningRunId } = await seed(); const issueId = randomUUID(); diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index 648a96761c..f799023e2e 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -1,3 +1,4 @@ +import { hasLiveLegacyController } from "../legacy-controller-lease.js"; import { instanceSettingsService } from "../instance-settings.js"; import { isWaitingConversation, settleConversationTurn, deliverConversationComments } from "../agent-conversations.js"; import { @@ -5474,7 +5475,9 @@ export function recoveryService( // state is auditable. It never overwrites a status that another path already // made terminal. // - // Two independent authorities terminalize the run. Either one is enough: + // A live controller lease owns execution and finalization across server + // processes. Only after that ownership ends can either authority below + // terminalize the run: // // - Issue-terminal authority: the run's issue already reached a terminal // status (done or cancelled), but the run row is still "running". A healthy @@ -5513,6 +5516,12 @@ export function recoveryService( if (isNativeRunnerOwnershipHeld(run)) return { terminalized: false, status: run.status }; + // Another controller may own a sandbox run whose PID has no meaning on + // this host. Its live lease owns both execution and finalization, even if + // the issue is already terminal or this process has no in-memory handle. + if (await hasLiveLegacyController(db, run)) + return { terminalized: false, status: run.status }; + const pid = run.processPid ?? null; const processGroupId = run.processGroupId ?? null; @@ -5624,7 +5633,22 @@ export function recoveryService( and( eq(heartbeatRuns.id, run.id), eq(heartbeatRuns.status, "running"), + eq(heartbeatRuns.runtimeMode, run.runtimeMode), nativeRunnerOwnershipNotHeldCondition(), + // Recheck ownership in the write: a controller can renew or claim + // the run after the liveness read. An old snapshot cannot end a new + // controller's run, even if that controller's lease later expires. + run.runtimeMode === "legacy" + ? and( + run.controllerBootId + ? eq(heartbeatRuns.controllerBootId, run.controllerBootId) + : isNull(heartbeatRuns.controllerBootId), + or( + isNull(heartbeatRuns.controllerBootId), + sql`${heartbeatRuns.controllerLeaseExpiresAt} <= clock_timestamp()`, + ), + ) + : undefined, ), ) .returning()