diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index 1aa79fd366..9ba7ea4dde 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -3019,6 +3019,7 @@ describe("ACPX engine remote managed-home seam (PR 2: per-adapter home seed)", ( stagedRuntime, teardown: async () => { teardownCalls += 1; + return { ok: true }; }, }; }, @@ -3362,6 +3363,7 @@ describe("ACPX engine remote session-lifecycle re-staging (PR 3: stage once / re stagedRuntime, teardown: async () => { teardownCalls += 1; + return { ok: true }; }, disposeStaged: async () => { disposeCalls += 1; @@ -3406,6 +3408,7 @@ describe("ACPX engine remote session-lifecycle re-staging (PR 3: stage once / re stagedRuntime: await input.stage([]), teardown: async () => { teardownCalls += 1; + return { ok: true }; }, disposeStaged: async () => { disposeCalls += 1; @@ -4675,6 +4678,7 @@ describe("ACPX engine sandbox-start spans (opt-in root + child parenting)", () = teardownExecFired = true; issueSandboxExecFromStore(traceContext); } + return { ok: true }; }, }; }, diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index f62c6c6a1d..4442e7f642 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -37,6 +37,11 @@ import { } from "@paperclipai/adapter-utils/execution-target"; import type { DuplexLossReason } from "../duplex-observability.js"; import { DUPLEX_CHANNEL_LOST_ERROR_CODE } from "../bridge-transport-contract.js"; +import type { WorkspaceRestoreFailureCode, WorkspaceRestoreOutcome } from "../workspace-restore-merge.js"; +import { + classifyWorkspaceRestoreFailure, + describeWorkspaceRestoreFailure, +} from "../workspace-restore-merge.js"; import { DEFAULT_PAPERCLIP_AGENT_PROMPT_TEMPLATE, applyPaperclipWorkspaceEnv, @@ -218,7 +223,7 @@ export interface StagedRuntimeCacheEntry { * in-sandbox credential, not a stale snapshot. It never removes the staged * in-sandbox home, so re-running it on each reuse can't invalidate this entry. */ - teardown: (() => Promise) | null; + teardown: (() => Promise) | null; /** * The seam's one-time host-side staged-resource cleanup (e.g. remove the * staged home temp dir), or null. Fired ONLY when this entry is dropped — @@ -315,7 +320,7 @@ export interface AcpxRemoteManagedHomeResult { * cached staged runtime across resumes never destroys resources a later run * still needs. */ - teardown?: () => Promise; + teardown?: () => Promise; /** * One-time cleanup of host-side staged resources (e.g. the curated staged * home temp dir). Split out from {@link teardown} so it fires ONLY when the @@ -420,7 +425,7 @@ interface AcpxPreparedRuntime { // exit path by the settlement `syncBack` step; it never removes staged temp, so // it is safe on every compatible resume. Null for local runs, the runner-less // fallback, and adapters with no seam. - remoteManagedHomeTeardown: (() => Promise) | null; + remoteManagedHomeTeardown: (() => Promise) | null; // One-time host-side staged-resource cleanup from the seam (remove staged temp // dirs). Fired ONLY when the staged runtime is dropped (failed/cancelled/timed // -out turn, incompatible re-stage, idle eviction), not on a clean turn that @@ -2013,7 +2018,7 @@ async function buildRuntime(input: { remoteExecutionIdentity, }); let stagedRuntime: PreparedAdapterExecutionTargetRuntime | null = null; - let remoteManagedHomeTeardown: (() => Promise) | null = null; + let remoteManagedHomeTeardown: (() => Promise) | null = null; let remoteStagingDispose: (() => Promise) | null = null; let remoteStagingEnvDelta: Record | null = null; let sessionStagingLeaseRelease: (() => void) | null = null; @@ -2364,14 +2369,22 @@ async function stopRunTransport(prepared: AcpxPreparedRuntime): Promise { // dirs. The seam logs and swallows its own failures — an unclean-teardown // copy-back miss is the accepted, loud `refresh_token_reused` residual on the // next host Codex use, never silent HOST-credential corruption — so a teardown -// fault never masks or fails the run result here. -async function syncBackManagedHome(prepared: AcpxPreparedRuntime): Promise { - if (prepared.remoteManagedHomeTeardown) { - await prepared.remoteManagedHomeTeardown().catch(() => {}); +// fault never masks or fails the run result here. It still returns the restore +// outcome, so the caller can record a failure on the run record; the run's exit +// code and status stay exactly what the turn produced. +// The per-session staging lease does NOT release here. The settlement releases +// it last, in its own `finally`, so a same-session second run cannot re-stage +// until this run fully settles and the caller observes the result. +async function syncBackManagedHome(prepared: AcpxPreparedRuntime): Promise { + if (!prepared.remoteManagedHomeTeardown) { + return { ok: true }; } - // The per-session staging lease does NOT release here. The settlement releases - // it last, in its own `finally`, so a same-session second run cannot re-stage - // until this run fully settles and the caller observes the result. + // The teardown closure already catches and logs its own error (fail-soft); + // this `.catch` is defense in depth for the case where it rejects anyway, so + // a teardown fault can never propagate out of settlement. + return await prepared + .remoteManagedHomeTeardown() + .catch((): WorkspaceRestoreOutcome => ({ ok: false, code: "restore_failed" })); } /** How the settlement `endSession` step releases the runtime a run acquired. */ @@ -3442,8 +3455,29 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { let referencedProjectStagingFailuresField: | { referencedProjectStagingFailures: Array<{ projectId: string; error: string }> } | Record = {}; + // Set only when the settlement sync-back reports a failed workspace + // restore. `reproduceResult` merges this into the returned result's + // `resultJson`, so a clean run adds no new key. The run's exit code and + // status stay exactly what the turn produced — this is a signal, not an + // outcome change. + let workspaceRestoreFailureField: + | { workspaceRestoreFailure: WorkspaceRestoreFailureCode } + | Record = {}; + // The one settlement step name whose error can be the same workspace- + // restore failure the adapter teardown closure already classifies (a + // caught error from `syncBackManagedHome`). The closure already + // sanitizes its own `onLog` line; this is a second, independent layer, + // so a defect in that closure (or a future call site that forgets to + // sanitize) still cannot put a raw `Error.message` — and the host path + // or process id it can carry — on the run log. + const SYNC_BACK_SETTLEMENT_STEP = "settlement-sync_back"; const recordTeardownError = async (step: string, teardownErr: unknown) => { - const reason = teardownErr instanceof Error ? teardownErr.message : String(teardownErr); + const reason = + step === SYNC_BACK_SETTLEMENT_STEP + ? describeWorkspaceRestoreFailure(classifyWorkspaceRestoreFailure(teardownErr)) + : teardownErr instanceof Error + ? teardownErr.message + : String(teardownErr); await ctx .onLog("stderr", `[paperclip] ACPX teardown step "${step}" failed: ${reason}\n`) .catch(() => {}); @@ -4445,7 +4479,10 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { // per-task restore spans parent to `sandbox.syncBack`. syncBack: () => timedPhase("sync_back", async () => { await runRuntimeSpan("sandbox.syncBack", async () => { - await syncBackManagedHome(prepared); + const restoreOutcome = await syncBackManagedHome(prepared); + if (!restoreOutcome.ok) { + workspaceRestoreFailureField = { workspaceRestoreFailure: restoreOutcome.code }; + } }); }), // The staging lease releases as the run's final act, AFTER the coordinator @@ -4490,7 +4527,20 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { if (!capturedResult) { throw new Error("run coordinator reproduced a result before the run recorded one"); } - return capturedResult; + // The sync-back settlement step runs before this reproduces the result + // (settlement precedes reproduction), so a failed workspace restore is + // already recorded by the time we get here. Merge it into `resultJson` + // only on a failure — a clean restore adds no new key. + if (!("workspaceRestoreFailure" in workspaceRestoreFailureField)) { + return capturedResult; + } + return { + ...capturedResult, + resultJson: { + ...(capturedResult.resultJson ?? {}), + ...workspaceRestoreFailureField, + }, + }; }, }; return await runAttempt(plan); diff --git a/packages/adapter-utils/src/acpx-engine/run-contracts.ts b/packages/adapter-utils/src/acpx-engine/run-contracts.ts index afc56f448a..173cb5db9a 100644 --- a/packages/adapter-utils/src/acpx-engine/run-contracts.ts +++ b/packages/adapter-utils/src/acpx-engine/run-contracts.ts @@ -18,6 +18,7 @@ import type { AdapterExecutionTargetProcessSessionBridgeHandle, PreparedAdapterExecutionTargetRuntime, } from "@paperclipai/adapter-utils/execution-target"; +import type { WorkspaceRestoreOutcome } from "../workspace-restore-merge.js"; // --------------------------------------------------------------------------- // Resource identity @@ -92,8 +93,13 @@ export interface StagedRuntimeResource { * `AcpxPreparedRuntime.remoteManagedHomeTeardown` and `remoteStagingEnvDelta`. */ export interface ManagedHomeResource { - /** Per-run copy-back hook. Runs the auth copy-back on every exit path. */ - readonly teardown: () => Promise; + /** + * Per-run copy-back hook. Runs the auth copy-back and the workspace restore + * on every exit path. Resolves to the workspace-restore outcome — never + * rejects — so the run coordinator can surface a failed restore on the run + * record without changing the run's exit code or status. + */ + readonly teardown: () => Promise; /** The env keys the seam mutated on this run. */ readonly envDelta: Record; } diff --git a/packages/adapter-utils/src/acpx-engine/run-fault-matrix.test.ts b/packages/adapter-utils/src/acpx-engine/run-fault-matrix.test.ts index eccb29c366..e8e7c990a9 100644 --- a/packages/adapter-utils/src/acpx-engine/run-fault-matrix.test.ts +++ b/packages/adapter-utils/src/acpx-engine/run-fault-matrix.test.ts @@ -8,6 +8,7 @@ import { startAdapterExecutionTargetProcessSessionBridge, } from "@paperclipai/adapter-utils/execution-target"; import { runChildProcess } from "../server-utils.js"; +import { classifyWorkspaceRestoreFailure } from "../workspace-restore-merge.js"; // The composed fault matrix. // @@ -213,7 +214,22 @@ function managedHomeSeed(options: { teardownRejects?: boolean } = {}): AcpxEngin ? async () => { throw new Error("copy-back boom"); } - : async () => {}, + : async () => ({ ok: true as const }), + }; + }; +} + +// Same seam, but the teardown catches its own restore error and classifies it +// with the real classifier — the way the three real adapter teardown closures +// do — instead of letting it reject the promise. +function managedHomeSeedWithClassifiedTeardownFailure( + error: NodeJS.ErrnoException, +): AcpxEngineExecutorOptions["prepareRemoteManagedHome"] { + return async (input) => { + const stagedRuntime = await input.stage([]); + return { + stagedRuntime, + teardown: async () => ({ ok: false as const, code: classifyWorkspaceRestoreFailure(error) }), }; }; } @@ -689,7 +705,8 @@ describe("composed ACPX run fault matrix", () => { }); // Case 14 — a sync-back copy-back that rejects during settlement never changes - // the result or the report. + // the run's exit code or status, and it surfaces on the result as one + // allowlisted `workspaceRestoreFailure` code — never the raw error message. it("case_14_sync_back_failure_does_not_change_result", async () => { const { stateDir, localCwd, executionTarget } = await setupRemoteSandbox(); stubBridges(); @@ -710,12 +727,67 @@ describe("composed ACPX run fault matrix", () => { expect(result.exitCode).toBe(0); expect(result.resultJson?.status).toBe("completed"); + expect(result.resultJson?.workspaceRestoreFailure).toBe("restore_failed"); + expect(JSON.stringify(result.resultJson)).not.toContain("copy-back boom"); assertDispositionReport(capture.last(), { acquired: SANDBOX_WITH_MANAGED_HOME, transferred: "staged_runtime", }); }); + // Case 14b — a clean sync-back adds no `workspaceRestoreFailure` key at all. + it("case_14b_clean_sync_back_adds_no_workspace_restore_failure_key", async () => { + const { stateDir, localCwd, executionTarget } = await setupRemoteSandbox(); + stubBridges(); + const execute = createAcpxEngineExecutor({ + warmHandles: new Map(), + stagedRuntimes: new Map(), + stagingLocks: new Map(), + createRuntime: () => completedRuntime(), + prepareRemoteManagedHome: managedHomeSeed(), + }); + + const result = await execute({ + runId: "fault-sync-back-clean", + ...remoteArgs(stateDir, localCwd, executionTarget), + } as never); + + expect(result.exitCode).toBe(0); + expect(result.resultJson).not.toHaveProperty("workspaceRestoreFailure"); + }); + + // Case 14c — an EACCES teardown error (the reported lock-mkdir bug) reaches + // the run result as the allowlisted restore_permission_denied code, with no + // filesystem path and no process id in resultJson, and the run's exit code + // and status stay exactly what the turn produced. + it("case_14c_eacces_teardown_reaches_result_as_restore_permission_denied", async () => { + const { stateDir, localCwd, executionTarget } = await setupRemoteSandbox(); + stubBridges(); + const eaccesError: NodeJS.ErrnoException = new Error( + `EACCES: permission denied, mkdir '/srv/telemetry-backend.paperclip-restore.lock' (pid ${process.pid})`, + ); + eaccesError.code = "EACCES"; + const execute = createAcpxEngineExecutor({ + warmHandles: new Map(), + stagedRuntimes: new Map(), + stagingLocks: new Map(), + createRuntime: () => completedRuntime(), + prepareRemoteManagedHome: managedHomeSeedWithClassifiedTeardownFailure(eaccesError), + }); + + const result = await execute({ + runId: "fault-sync-back-eacces", + ...remoteArgs(stateDir, localCwd, executionTarget), + } as never); + + expect(result.exitCode).toBe(0); + expect(result.resultJson?.status).toBe("completed"); + expect(result.resultJson?.workspaceRestoreFailure).toBe("restore_permission_denied"); + const serializedResult = JSON.stringify(result.resultJson); + expect(serializedResult).not.toContain("/srv/telemetry-backend"); + expect(serializedResult).not.toContain(String(process.pid)); + }); + // Case 15 — a concurrent double settlement is a structural error: the second // claim of the ledger throws a LedgerStateError. it("case_15_concurrent_double_settlement_throws_ledger_state_error", async () => { diff --git a/packages/adapter-utils/src/acpx-engine/run-site-sandbox.test.ts b/packages/adapter-utils/src/acpx-engine/run-site-sandbox.test.ts index 426a334276..ebbab2cb60 100644 --- a/packages/adapter-utils/src/acpx-engine/run-site-sandbox.test.ts +++ b/packages/adapter-utils/src/acpx-engine/run-site-sandbox.test.ts @@ -78,7 +78,11 @@ function makeSite(overrides: Partial = {}) { const stagedRuntime = await stage([]); // Repoint a managed-home env var, so the site captures the delta. env.CODEX_HOME = "/remote/home"; - return { stagedRuntime, teardown: async () => {}, dispose: async () => {} } satisfies ManagedHomeSeamResult; + return { + stagedRuntime, + teardown: async () => ({ ok: true }), + dispose: async () => {}, + } satisfies ManagedHomeSeamResult; }, disposeFreshStagedRuntime: async () => {}, measureStageStep: (run) => run(), @@ -246,7 +250,7 @@ describe("sandbox run site", () => { { stagedRuntime: cachedRuntime, envDelta: { CODEX_HOME: "/remote/home" }, - teardown: async () => {}, + teardown: async () => ({ ok: true }), dispose: async () => {}, lastUsedAt: 500, }, diff --git a/packages/adapter-utils/src/acpx-engine/run-site-sandbox.ts b/packages/adapter-utils/src/acpx-engine/run-site-sandbox.ts index 4f85022607..73f6a86fe5 100644 --- a/packages/adapter-utils/src/acpx-engine/run-site-sandbox.ts +++ b/packages/adapter-utils/src/acpx-engine/run-site-sandbox.ts @@ -41,6 +41,7 @@ import type { SitePlan, } from "./run-contracts.js"; import { createSessionReuseStore } from "./session-reuse-store.js"; +import type { WorkspaceRestoreOutcome } from "../workspace-restore-merge.js"; /** * One staged in-sandbox runtime the store keeps for the next compatible resume. @@ -52,7 +53,7 @@ import { createSessionReuseStore } from "./session-reuse-store.js"; export interface StagedRuntimeStoreEntry { stagedRuntime: PreparedAdapterExecutionTargetRuntime; envDelta: Record; - teardown: (() => Promise) | null; + teardown: (() => Promise) | null; dispose: (() => Promise) | null; lastUsedAt: number; } @@ -63,7 +64,7 @@ export interface StagedRuntimeStoreEntry { */ export interface StagedWorkspace { readonly stagedRuntime: PreparedAdapterExecutionTargetRuntime; - readonly teardown: (() => Promise) | null; + readonly teardown: (() => Promise) | null; readonly dispose: (() => Promise) | null; readonly envDelta: Record; /** True when the run reused an already-staged runtime rather than staging fresh. */ @@ -79,7 +80,7 @@ export type StageWorkspace = (assets: AdapterManagedRuntimeAsset[]) => Promise

Promise) | null; + readonly teardown: (() => Promise) | null; readonly dispose: (() => Promise) | null; } diff --git a/packages/adapter-utils/src/acpx-engine/settlement-characterization.test.ts b/packages/adapter-utils/src/acpx-engine/settlement-characterization.test.ts index d546a956de..593fc644d3 100644 --- a/packages/adapter-utils/src/acpx-engine/settlement-characterization.test.ts +++ b/packages/adapter-utils/src/acpx-engine/settlement-characterization.test.ts @@ -334,6 +334,7 @@ describe("ACP settlement — Layer A: engine teardown orchestration", () => { // The lease is still held while the sync-back runs; it releases only // afterward, in the cleanupRemoteBridges finally. leaseSizeDuringSyncBack = stagingLocks.size; + return { ok: true }; }, }; }, @@ -1046,6 +1047,7 @@ describe("ACP settlement — Layer C: per-adapter sync-back teardown fires once stagedRuntime, teardown: async () => { teardownCalls += 1; + return { ok: true }; }, }; }, diff --git a/packages/adapter-utils/src/workspace-restore-merge.test.ts b/packages/adapter-utils/src/workspace-restore-merge.test.ts index ff0a5a126d..d3a06c08bc 100644 --- a/packages/adapter-utils/src/workspace-restore-merge.test.ts +++ b/packages/adapter-utils/src/workspace-restore-merge.test.ts @@ -1,10 +1,20 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { createHash } from "node:crypto"; +import { promises as fsPromises } from "node:fs"; +import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, realpath, rm, stat, symlink, writeFile } from "node:fs/promises"; import net from "node:net"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; -import { captureDirectorySnapshot, mergeDirectoryWithBaseline } from "./workspace-restore-merge.js"; +import { resolvePaperclipInstanceRootForAdapter } from "./server-utils.js"; +import { + captureDirectorySnapshot, + classifyWorkspaceRestoreFailure, + describeWorkspaceRestoreFailure, + mergeDirectoryWithBaseline, + withDirectoryMergeLock, + WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE, +} from "./workspace-restore-merge.js"; describe("workspace restore merge", () => { const cleanupDirs: string[] = []; @@ -81,4 +91,446 @@ describe("workspace restore merge", () => { await new Promise((resolve) => server.close(() => resolve())); } }); + + describe("classifyWorkspaceRestoreFailure", () => { + it("maps an EACCES error to restore_permission_denied", () => { + const error: NodeJS.ErrnoException = new Error("permission denied"); + error.code = "EACCES"; + expect(classifyWorkspaceRestoreFailure(error)).toBe("restore_permission_denied"); + }); + + it("maps an EPERM error to restore_permission_denied", () => { + const error: NodeJS.ErrnoException = new Error("operation not permitted"); + error.code = "EPERM"; + expect(classifyWorkspaceRestoreFailure(error)).toBe("restore_permission_denied"); + }); + + it("maps the lock-timeout code to restore_lock_timeout", () => { + const error: NodeJS.ErrnoException = new Error("Timed out waiting for workspace restore lock at /some/path"); + error.code = WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE; + expect(classifyWorkspaceRestoreFailure(error)).toBe("restore_lock_timeout"); + }); + + it("maps an unrecognized error, a string, and null to the default restore_failed code", () => { + expect(classifyWorkspaceRestoreFailure(new Error("some other failure"))).toBe("restore_failed"); + expect(classifyWorkspaceRestoreFailure("a plain string")).toBe("restore_failed"); + expect(classifyWorkspaceRestoreFailure(null)).toBe("restore_failed"); + }); + }); + + describe("describeWorkspaceRestoreFailure", () => { + it("returns one fixed diagnostic line per allowlisted code, and no other text", () => { + expect(describeWorkspaceRestoreFailure("restore_permission_denied")).toBe( + "the restore could not write to the workspace (permission denied)", + ); + expect(describeWorkspaceRestoreFailure("restore_lock_timeout")).toBe( + "the restore timed out waiting for the workspace merge lock", + ); + expect(describeWorkspaceRestoreFailure("restore_failed")).toBe("the restore failed"); + }); + + it("never reflects a sentinel host path or process id, however the caught error is classified", () => { + const sentinelPath = "/srv/telemetry-backend"; + const sentinelPid = String(process.pid); + const error: NodeJS.ErrnoException = new Error( + `EACCES: permission denied, mkdir '${sentinelPath}.paperclip-restore.lock' (pid ${sentinelPid})`, + ); + error.code = "EACCES"; + + const line = describeWorkspaceRestoreFailure(classifyWorkspaceRestoreFailure(error)); + + expect(line).not.toContain(sentinelPath); + expect(line).not.toContain(sentinelPid); + expect(line).not.toContain(error.message); + }); + }); + + describe("instance-scoped directory merge lock", () => { + // Points PAPERCLIP_HOME (and, where noted, PAPERCLIP_INSTANCE_ID) at a + // temporary directory so the lock root never touches the real Paperclip + // instance, then restores the previous values. Mirrors the save-and-restore + // pattern in acpx-engine/execute.test.ts. + let previousHome: string | undefined; + let previousInstanceId: string | undefined; + + function useTempPaperclipHome(homeDir: string, instanceId: string): void { + previousHome = process.env.PAPERCLIP_HOME; + previousInstanceId = process.env.PAPERCLIP_INSTANCE_ID; + process.env.PAPERCLIP_HOME = homeDir; + process.env.PAPERCLIP_INSTANCE_ID = instanceId; + } + + afterEach(() => { + if (previousHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = previousHome; + if (previousInstanceId === undefined) delete process.env.PAPERCLIP_INSTANCE_ID; + else process.env.PAPERCLIP_INSTANCE_ID = previousInstanceId; + previousHome = undefined; + previousInstanceId = undefined; + }); + + it.skipIf(process.platform === "win32")( + "restores successfully when the parent directory of the target is not writable", + async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + useTempPaperclipHome(path.join(rootDir, "paperclip-home"), "test-instance"); + + // The old lock sat beside the target, so it needed mkdir rights in the + // target's parent. The new lock root lives under PAPERCLIP_HOME instead, + // so a read-only parent must no longer block a restore. + const readOnlyParent = path.join(rootDir, "read-only-parent"); + const targetDir = path.join(readOnlyParent, "target"); + const sourceDir = path.join(rootDir, "source"); + await mkdir(targetDir, { recursive: true }); + await mkdir(sourceDir, { recursive: true }); + + const baseline = await captureDirectorySnapshot(targetDir, { exclude: [] }); + await writeFile(path.join(sourceDir, "new-file.md"), "new content\n", "utf8"); + + await chmod(readOnlyParent, 0o500); + try { + await mergeDirectoryWithBaseline({ baseline, sourceDir, targetDir }); + } finally { + // Restore write access so the outer afterEach can remove rootDir. + await chmod(readOnlyParent, 0o700).catch(() => undefined); + } + + await expect(readFile(path.join(targetDir, "new-file.md"), "utf8")).resolves.toBe("new content\n"); + }, + ); + + it.skipIf(process.platform === "win32")( + "acquires the same lock for two alias paths that resolve to one canonical target", + async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const paperclipHome = path.join(rootDir, "paperclip-home"); + useTempPaperclipHome(paperclipHome, "test-instance"); + + const targetDir = path.join(rootDir, "target"); + const aliasDir = path.join(rootDir, "target-alias"); + await mkdir(targetDir, { recursive: true }); + await symlink(targetDir, aliasDir); + + const lockRootDir = path.join(paperclipHome, "instances", "test-instance", "locks", "directory-merge"); + + let lockNameViaTarget = ""; + await withDirectoryMergeLock(targetDir, async () => { + const entries = await readdir(lockRootDir); + lockNameViaTarget = entries[0] ?? ""; + }); + + let lockNameViaAlias = ""; + await withDirectoryMergeLock(aliasDir, async () => { + const entries = await readdir(lockRootDir); + lockNameViaAlias = entries[0] ?? ""; + }); + + expect(lockNameViaTarget).not.toBe(""); + expect(lockNameViaAlias).toBe(lockNameViaTarget); + }, + ); + + it("rejects a lock root that already exists as a symlink", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const paperclipHome = path.join(rootDir, "paperclip-home"); + useTempPaperclipHome(paperclipHome, "test-instance"); + + const locksDir = path.join(paperclipHome, "instances", "test-instance", "locks"); + const decoyDir = path.join(rootDir, "decoy"); + await mkdir(locksDir, { recursive: true }); + await mkdir(decoyDir, { recursive: true }); + await symlink(decoyDir, path.join(locksDir, "directory-merge")); + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + await expect(withDirectoryMergeLock(targetDir, async () => undefined)).rejects.toThrow( + /not a plain directory/, + ); + }); + + it("rejects a lock root that already exists as a non-directory", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const paperclipHome = path.join(rootDir, "paperclip-home"); + useTempPaperclipHome(paperclipHome, "test-instance"); + + const locksDir = path.join(paperclipHome, "instances", "test-instance", "locks"); + await mkdir(locksDir, { recursive: true }); + await writeFile(path.join(locksDir, "directory-merge"), "not a directory\n", "utf8"); + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + await expect(withDirectoryMergeLock(targetDir, async () => undefined)).rejects.toThrow( + /not a plain directory/, + ); + }); + + it("closes the create/validate TOCTOU window: rejects a lock root a racing writer swapped for a symlink during creation", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const paperclipHome = path.join(rootDir, "paperclip-home"); + useTempPaperclipHome(paperclipHome, "test-instance"); + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + const decoyDir = path.join(rootDir, "decoy"); + await mkdir(decoyDir, { recursive: true }); + // Pre-create the lock root's parent, so the mock below only has to + // reproduce what `fs.mkdir({ recursive: true })` does to the leaf path. + await mkdir(path.join(paperclipHome, "instances", "test-instance", "locks"), { recursive: true }); + + // Real `fs.mkdir({ recursive: true })` does not fail on a leaf that + // already exists as a symlink to a real directory. This stub reproduces + // exactly that: it plants a symlink to the attacker-controlled decoy + // directory in the window between the resolver's own "does the root + // exist yet" check and its own `mkdir` call, then resolves the way a + // real `mkdir` would (silently) — proving the resolver must validate + // what `mkdir` actually left behind, not trust that the call resolved. + const mkdirSpy = vi.spyOn(fsPromises, "mkdir").mockImplementationOnce(async (dirPath) => { + await symlink(decoyDir, dirPath as string); + return undefined; + }); + + try { + await expect(withDirectoryMergeLock(targetDir, async () => undefined)).rejects.toThrow( + /not a plain directory/, + ); + } finally { + mkdirSpy.mockRestore(); + } + }); + + it("creates the lock root at mode 0o700 and removes the lock directory after release", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const paperclipHome = path.join(rootDir, "paperclip-home"); + useTempPaperclipHome(paperclipHome, "test-instance"); + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + const lockRootDir = path.join(paperclipHome, "instances", "test-instance", "locks", "directory-merge"); + let entriesDuringLock: string[] = []; + await withDirectoryMergeLock(targetDir, async () => { + entriesDuringLock = await readdir(lockRootDir); + }); + + expect((await stat(lockRootDir)).mode & 0o777).toBe(0o700); + expect(entriesDuringLock).toHaveLength(1); + await expect(readdir(lockRootDir)).resolves.toHaveLength(0); + }); + + it("classifies the real lock-timeout error by its stable code, never by the message text", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const paperclipHome = path.join(rootDir, "paperclip-home"); + useTempPaperclipHome(paperclipHome, "test-instance"); + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + // Pre-create the lock directory a live process holds, so `isLockStale` + // never reports it stale and the retry loop can only leave through the + // deadline check. The owner pid is this test process, which stays alive. + const canonicalTargetDir = await realpath(targetDir); + const lockKey = createHash("sha256").update(canonicalTargetDir).digest("hex"); + const lockRootDir = path.join(paperclipHome, "instances", "test-instance", "locks", "directory-merge"); + const heldLockDir = path.join(lockRootDir, `${lockKey}.lock`); + await mkdir(heldLockDir, { recursive: true }); + await writeFile( + path.join(heldLockDir, "owner.json"), + `${JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() })}\n`, + "utf8", + ); + + // Reach the real deadline without a real 30-second wait: the first + // `Date.now()` call computes the deadline (unchanged), and every call + // after reports a time far past it, so the retry loop's own deadline + // check — not a mocked message or a shortened constant — throws. + const realNow = Date.now(); + const dateNowSpy = vi + .spyOn(Date, "now") + .mockImplementationOnce(() => realNow) + .mockImplementation(() => Number.MAX_SAFE_INTEGER); + let caughtError: NodeJS.ErrnoException | undefined; + try { + await withDirectoryMergeLock(targetDir, async () => undefined); + } catch (error) { + caughtError = error as NodeJS.ErrnoException; + } finally { + dateNowSpy.mockRestore(); + } + + expect(caughtError).toBeInstanceOf(Error); + expect(caughtError?.code).toBe(WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE); + // The classifier reads only `code`; prove the message text carries no + // trace of the classified outcome, so a message-text match could not + // have produced this result. + expect(caughtError?.message).not.toContain("restore_lock_timeout"); + expect(classifyWorkspaceRestoreFailure(caughtError)).toBe("restore_lock_timeout"); + }); + + it.skipIf(process.platform === "win32")( + "serializes two concurrent writers that address one target through different aliases", + async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + useTempPaperclipHome(path.join(rootDir, "paperclip-home"), "test-instance"); + + const targetDir = path.join(rootDir, "target"); + const aliasDir = path.join(rootDir, "target-alias"); + await mkdir(targetDir, { recursive: true }); + await symlink(targetDir, aliasDir); + + let active = false; + let overlapCount = 0; + let completedCount = 0; + const runWriter = (dir: string) => + withDirectoryMergeLock(dir, async () => { + if (active) overlapCount += 1; + active = true; + await new Promise((resolve) => setTimeout(resolve, 30)); + active = false; + completedCount += 1; + }); + + await Promise.all([runWriter(targetDir), runWriter(aliasDir)]); + + expect(overlapCount).toBe(0); + expect(completedCount).toBe(2); + }, + ); + }); + + describe("caller-provided env for the lock root", () => { + // These tests never touch `process.env`. They prove `withDirectoryMergeLock` + // resolves the lock root from a caller's own `env` object — the shape every + // environment-parameterized Codex credential call site holds — instead of + // always reading `process.env`. + + it("two callers that pass the same env with a temporary PAPERCLIP_HOME take the same lock under that home", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const explicitHome = path.join(rootDir, "explicit-home"); + const env: NodeJS.ProcessEnv = { PAPERCLIP_HOME: explicitHome, PAPERCLIP_INSTANCE_ID: "test-instance" }; + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + const lockRootDir = path.join(explicitHome, "instances", "test-instance", "locks", "directory-merge"); + + let lockNameFirstCaller = ""; + await withDirectoryMergeLock( + targetDir, + async () => { + const entries = await readdir(lockRootDir); + lockNameFirstCaller = entries[0] ?? ""; + }, + env, + ); + + let lockNameSecondCaller = ""; + await withDirectoryMergeLock( + targetDir, + async () => { + const entries = await readdir(lockRootDir); + lockNameSecondCaller = entries[0] ?? ""; + }, + env, + ); + + expect(lockNameFirstCaller).not.toBe(""); + expect(lockNameSecondCaller).toBe(lockNameFirstCaller); + expect(lockRootDir.startsWith(explicitHome + path.sep)).toBe(true); + }); + + it("does not write a lock entry under process.env.PAPERCLIP_HOME when the caller passes its own env", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const explicitHome = path.join(rootDir, "explicit-home"); + const env: NodeJS.ProcessEnv = { PAPERCLIP_HOME: explicitHome, PAPERCLIP_INSTANCE_ID: "test-instance" }; + + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + const canonicalTargetDir = await realpath(targetDir); + const lockKey = createHash("sha256").update(canonicalTargetDir).digest("hex"); + + // Resolved with no `env` argument, so it reads `process.env` exactly the way + // the real instance root does — unaffected by the explicit `env` above. + const realInstanceRoot = resolvePaperclipInstanceRootForAdapter(); + const realLockPath = path.join(realInstanceRoot, "locks", "directory-merge", `${lockKey}.lock`); + + await withDirectoryMergeLock(targetDir, async () => undefined, env); + + await expect(lstat(realLockPath)).rejects.toThrow(); + + const explicitLockRootDir = path.join(explicitHome, "instances", "test-instance", "locks", "directory-merge"); + await expect(stat(explicitLockRootDir)).resolves.toBeTruthy(); + }); + + it("resolves the lock root under the default instance id when the caller env sets PAPERCLIP_HOME but not PAPERCLIP_INSTANCE_ID, ignoring process.env.PAPERCLIP_INSTANCE_ID", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const explicitHome = path.join(rootDir, "explicit-home"); + const env: NodeJS.ProcessEnv = { PAPERCLIP_HOME: explicitHome }; + + const previousInstanceId = process.env.PAPERCLIP_INSTANCE_ID; + process.env.PAPERCLIP_INSTANCE_ID = "wrong-instance"; + try { + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + // The independent, no-caller-env resolution of "PAPERCLIP_HOME set, + // PAPERCLIP_INSTANCE_ID unset" — the expected default instance id. + const expectedInstanceRoot = resolvePaperclipInstanceRootForAdapter({ homeDir: explicitHome, env: {} }); + const expectedLockRootDir = path.join(expectedInstanceRoot, "locks", "directory-merge"); + const wrongInstanceLockRootDir = path.join(explicitHome, "instances", "wrong-instance", "locks", "directory-merge"); + + await withDirectoryMergeLock(targetDir, async () => undefined, env); + + await expect(stat(expectedLockRootDir)).resolves.toBeTruthy(); + await expect(stat(wrongInstanceLockRootDir)).rejects.toThrow(); + } finally { + if (previousInstanceId === undefined) delete process.env.PAPERCLIP_INSTANCE_ID; + else process.env.PAPERCLIP_INSTANCE_ID = previousInstanceId; + } + }); + + it("does not read process.env.PAPERCLIP_HOME when the caller env sets neither variable", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-restore-merge-")); + cleanupDirs.push(rootDir); + const fakeProcessHome = path.join(rootDir, "process-home"); + const fallbackOsHome = path.join(rootDir, "os-home"); + await mkdir(fallbackOsHome, { recursive: true }); + + const previousHome = process.env.PAPERCLIP_HOME; + process.env.PAPERCLIP_HOME = fakeProcessHome; + // Stand in for the real host home directory, so the "no env at all" + // fallback lands under a temp dir instead of the real ~/.paperclip. + const homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(fallbackOsHome); + try { + const targetDir = path.join(rootDir, "target"); + await mkdir(targetDir, { recursive: true }); + + // The independent, no-caller-env resolution of "neither variable set" — + // the expected fallback root under the mocked home directory. + const expectedInstanceRoot = resolvePaperclipInstanceRootForAdapter({ env: {} }); + const expectedLockRootDir = path.join(expectedInstanceRoot, "locks", "directory-merge"); + + await withDirectoryMergeLock(targetDir, async () => undefined, {}); + + await expect(stat(fakeProcessHome)).rejects.toThrow(); + await expect(stat(expectedLockRootDir)).resolves.toBeTruthy(); + } finally { + homedirSpy.mockRestore(); + if (previousHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = previousHome; + } + }); + }); }); diff --git a/packages/adapter-utils/src/workspace-restore-merge.ts b/packages/adapter-utils/src/workspace-restore-merge.ts index 16782973c3..8d59cd06db 100644 --- a/packages/adapter-utils/src/workspace-restore-merge.ts +++ b/packages/adapter-utils/src/workspace-restore-merge.ts @@ -3,6 +3,7 @@ import { createReadStream } from "node:fs"; import { constants as fsConstants, promises as fs } from "node:fs"; import path from "node:path"; import { shouldExcludePath } from "./exclude-patterns.js"; +import { resolvePaperclipInstanceRootForAdapter } from "./server-utils.js"; type SnapshotEntry = | { kind: "dir" } @@ -108,6 +109,67 @@ function entriesMatch(left: SnapshotEntry | null | undefined, right: SnapshotEnt const LOCK_STALE_MS = 30_000; +/** + * The stable `code` a lock-timeout error carries, so a caller can identify it + * without matching on the error message text (the message embeds the lock + * directory path). + */ +export const WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE = "ERR_WORKSPACE_RESTORE_LOCK_TIMEOUT"; + +/** + * The closed set of codes a failed workspace restore can carry off the + * sandbox. Every code is safe to store on a run record readable by any + * same-company actor: none embeds a filesystem path, a raw error message, or + * a process id. + */ +export type WorkspaceRestoreFailureCode = + | "restore_permission_denied" + | "restore_lock_timeout" + | "restore_failed"; + +/** + * The outcome of one workspace restore. `ok: true` on a clean restore. `ok: + * false` carries one allowlisted {@link WorkspaceRestoreFailureCode} — never a + * raw error, a path, or a process id. + */ +export type WorkspaceRestoreOutcome = + | { readonly ok: true } + | { readonly ok: false; readonly code: WorkspaceRestoreFailureCode }; + +/** + * Classifies a caught workspace-restore error into one allowlisted code. Maps + * `EACCES` and `EPERM` to a permission failure, the merge-lock timeout + * (matched by {@link WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE}, never by the error + * message text) to a lock-timeout failure, and every other error to a generic + * failure. Never reads or returns `Error.message`, a filesystem path, or a + * process id. + */ +export function classifyWorkspaceRestoreFailure(error: unknown): WorkspaceRestoreFailureCode { + const code = error && typeof error === "object" ? (error as NodeJS.ErrnoException).code : undefined; + if (code === "EACCES" || code === "EPERM") return "restore_permission_denied"; + if (code === WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE) return "restore_lock_timeout"; + return "restore_failed"; +} + +/** + * The fixed, allowlisted line an ACP adapter writes to the run log when a + * workspace restore fails. Every call site must pass this to `onLog` instead + * of the caught error's own message: the caught error can carry a host + * filesystem path or the lock owner's process id, and the run log is + * readable by any same-company actor. Never add the code's raw + * `Error.message` to this text. + */ +export function describeWorkspaceRestoreFailure(code: WorkspaceRestoreFailureCode): string { + switch (code) { + case "restore_permission_denied": + return "the restore could not write to the workspace (permission denied)"; + case "restore_lock_timeout": + return "the restore timed out waiting for the workspace merge lock"; + case "restore_failed": + return "the restore failed"; + } +} + async function isLockStale(lockDir: string): Promise { try { const raw = await fs.readFile(path.join(lockDir, "owner.json"), "utf8"); @@ -160,20 +222,97 @@ async function acquireDirectoryMergeLock(lockDir: string): Promise<() => Promise continue; } if (Date.now() >= deadline) { - throw new Error(`Timed out waiting for workspace restore lock at ${lockDir}`); + const timeoutError: NodeJS.ErrnoException = new Error( + `Timed out waiting for workspace restore lock at ${lockDir}`, + ); + timeoutError.code = WORKSPACE_RESTORE_LOCK_TIMEOUT_CODE; + throw timeoutError; } await new Promise((resolve) => setTimeout(resolve, 50)); } } } +const DIRECTORY_MERGE_LOCK_ROOT_MODE = 0o700; + +function nonEmpty(value: string | undefined): string | null { + return typeof value === "string" && value.trim().length > 0 ? value.trim() : null; +} + +/** + * Resolves the private, instance-scoped root for every directory-merge lock: + * `/locks/directory-merge`. Every process that can mutate one + * target directory must resolve to the same `PAPERCLIP_HOME` and + * `PAPERCLIP_INSTANCE_ID`. That shared resolution is what keeps mutual + * exclusion true for all five callers of `withDirectoryMergeLock`, including + * the three Codex credential call sites that never touch a workspace. + * + * This never falls back to `os.tmpdir()` and never places the lock beside the + * target directory: both paths funnel through this one instance-scoped root, + * so a read-only target parent (the workspace-restore bug) cannot block a + * lock acquisition. + * + * The root reads `PAPERCLIP_HOME` and `PAPERCLIP_INSTANCE_ID` from `env`, so an + * environment-parameterized caller (a Codex credential call site that builds + * its own `env` object instead of reading `process.env`) resolves its lock + * root under the same instance root as the directory it protects. This never + * reads `process.env` when the caller passes an `env`: every fallback inside + * the resolver also reads from that same `env` object. A caller that omits + * `env` gets `process.env`, which keeps the resolution unchanged for the + * workspace-restore call site. + * + * The root is validated, not trusted: `lstat` rejects a symlink and rejects + * any non-directory before use (fail closed). `fs.mkdir` does not change the + * mode of a directory that already exists, so an existing valid directory + * keeps whatever mode it already has; only a freshly created root gets mode + * `0o700`. + * + * The existence check and the `mkdir` below are two separate calls, so a + * racing writer can plant a symlink at `lockRoot` in between them. `fs.mkdir` + * with `recursive: true` does not fail on a leaf that already exists as a + * symlink to a real directory, so a successful `mkdir` call alone does not + * prove the path is a plain directory. The `lstat` after `mkdir` closes that + * window: it validates what is actually at `lockRoot` (never a `stat`, which + * would follow the symlink) before any caller treats it as the lock root. + */ +async function resolveDirectoryMergeLockRoot(env: NodeJS.ProcessEnv = process.env): Promise { + const instanceRoot = resolvePaperclipInstanceRootForAdapter({ + homeDir: nonEmpty(env.PAPERCLIP_HOME) ?? undefined, + instanceId: nonEmpty(env.PAPERCLIP_INSTANCE_ID) ?? undefined, + env, + }); + const lockRoot = path.join(instanceRoot, "locks", "directory-merge"); + const existing = await fs.lstat(lockRoot).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT") return null; + throw error; + }); + if (existing) { + if (existing.isSymbolicLink() || !existing.isDirectory()) { + throw new Error(`Directory merge lock root at ${lockRoot} is not a plain directory.`); + } + return lockRoot; + } + await fs.mkdir(lockRoot, { recursive: true, mode: DIRECTORY_MERGE_LOCK_ROOT_MODE }); + const created = await fs.lstat(lockRoot); + if (created.isSymbolicLink() || !created.isDirectory()) { + throw new Error(`Directory merge lock root at ${lockRoot} is not a plain directory.`); + } + return lockRoot; +} + export async function withDirectoryMergeLock( targetDir: string, - fn: () => Promise, + fn: (canonicalTargetDir: string) => Promise, + env: NodeJS.ProcessEnv = process.env, ): Promise { - const releaseLock = await acquireDirectoryMergeLock(`${targetDir}.paperclip-restore.lock`); + // Canonicalize before we hash or lock: a retargeted symlink must not let the + // lock protect one directory while the caller mutates another. + const canonicalTargetDir = await fs.realpath(targetDir); + const lockRoot = await resolveDirectoryMergeLockRoot(env); + const lockKey = createHash("sha256").update(canonicalTargetDir).digest("hex"); + const releaseLock = await acquireDirectoryMergeLock(path.join(lockRoot, `${lockKey}.lock`)); try { - return await fn(); + return await fn(canonicalTargetDir); } finally { await releaseLock(); } @@ -227,16 +366,16 @@ export async function mergeDirectoryWithBaseline(input: { afterApply?: () => Promise; }): Promise { const source = await captureDirectorySnapshot(input.sourceDir, { exclude: input.baseline.exclude }); - await withDirectoryMergeLock(input.targetDir, async () => { + await withDirectoryMergeLock(input.targetDir, async (canonicalTargetDir) => { await input.beforeApply?.(); - const current = await captureDirectorySnapshot(input.targetDir, { exclude: input.baseline.exclude }); + const current = await captureDirectorySnapshot(canonicalTargetDir, { exclude: input.baseline.exclude }); const deletedLeafEntries = [...input.baseline.entries.entries()] .filter(([relative, entry]) => entry.kind !== "dir" && !source.entries.has(relative)) .sort(([left], [right]) => right.length - left.length); for (const [relative, baselineEntry] of deletedLeafEntries) { if (!entriesMatch(current.entries.get(relative), baselineEntry)) continue; - await fs.rm(path.join(input.targetDir, relative), { recursive: true, force: true }).catch(() => undefined); + await fs.rm(path.join(canonicalTargetDir, relative), { recursive: true, force: true }).catch(() => undefined); } const deletedDirs = [...input.baseline.entries.entries()] @@ -244,7 +383,7 @@ export async function mergeDirectoryWithBaseline(input: { .sort(([left], [right]) => right.length - left.length); for (const [relative] of deletedDirs) { - await fs.rmdir(path.join(input.targetDir, relative)).catch(() => undefined); + await fs.rmdir(path.join(canonicalTargetDir, relative)).catch(() => undefined); } const changedSourceEntries = [...source.entries.entries()] @@ -252,7 +391,7 @@ export async function mergeDirectoryWithBaseline(input: { .sort(([left], [right]) => left.localeCompare(right)); for (const [relative, entry] of changedSourceEntries) { - await copySnapshotEntry(input.sourceDir, input.targetDir, relative, entry); + await copySnapshotEntry(input.sourceDir, canonicalTargetDir, relative, entry); } await input.afterApply?.(); diff --git a/packages/adapters/claude-local/src/server/acp.test.ts b/packages/adapters/claude-local/src/server/acp.test.ts index a58fbc66b9..0406d345a5 100644 --- a/packages/adapters/claude-local/src/server/acp.test.ts +++ b/packages/adapters/claude-local/src/server/acp.test.ts @@ -703,6 +703,92 @@ describe("claude_local ACP lane", () => { await expect(fs.readFile(path.join(localCwd, "from-sandbox.txt"), "utf8")).resolves.toBe("synced"); }); + it("test_claude_acp_teardown_restore_failure_sanitizes_the_run_log", async () => { + // Security regression for a workspace-restore write failure: the run log + // is readable by any same-company actor, so the teardown must never write + // the caught error's own message there — that message can carry the host + // workspace path. Force a real EACCES by making the workspace read-only, + // and name it with a sentinel marker so any leak is easy to spot. + const root = await makeTempRoot("paperclip-claude-acp-restore-failure-"); + const localCwd = path.join(root, "SENTINEL-HOST-PATH-marker", "worktree"); + const remoteCwd = path.join(root, "remote-workspace"); + await fs.mkdir(localCwd, { recursive: true }); + await fs.mkdir(remoteCwd, { recursive: true }); + await fs.writeFile(path.join(localCwd, "hello.txt"), "hi", "utf8"); + process.env.PAPERCLIP_HOME = path.join(root, "paperclip-home"); + process.env.PAPERCLIP_INSTANCE_ID = "test"; + + // The runtime writes a new file into the in-sandbox workspace during the + // turn, so the teardown's restore has something to copy back — and a new + // file is exactly what a read-only workspace directory rejects. The + // workspace turns read-only only after the turn's own writes (settings + // seeded at startup, the sandbox-authored file) — the teardown restore + // that runs after the turn is the write this test forces to fail. + const runtime = new FakeRuntime({}); + const startTurn = runtime.startTurn.bind(runtime); + runtime.startTurn = (input) => { + const turn = startTurn(input); + const remoteWorkspaceCwd = input.handle.cwd ?? remoteCwd; + return { + ...turn, + result: (async () => { + await fs.writeFile(path.join(remoteWorkspaceCwd, "from-sandbox.txt"), "synced", "utf8"); + await fs.chmod(localCwd, 0o500); + return await turn.result; + })(), + }; + }; + + const execute = createClaudeAcpExecutor({ + createRuntime: (options: FakeRuntimeOptions) => { + Object.assign(runtime.options, options); + return runtime as never; + }, + }); + + const loggedLines: string[] = []; + try { + const result = await execute( + buildContext(localCwd, { + config: { + engine: "acp", + cwd: localCwd, + agentCommand: "node ./fake-acp.js", + stateDir: path.join(root, "state"), + promptTemplate: "Do the assigned work.", + }, + context: { + issueId: "issue-1", + paperclipWorkspace: { cwd: localCwd, source: "project_workspace", workspaceId: "workspace-1" }, + }, + executionTarget: { + kind: "remote", + transport: "sandbox", + providerKey: "fake-plugin", + remoteCwd, + runner: createLocalSandboxRunner(), + } as never, + authToken: "real-run-jwt", + onLog: async (_stream, chunk) => { + loggedLines.push(chunk); + }, + }), + ); + + // Fail-open: the restore miss never changes the run's exit code or + // status, and it surfaces as one allowlisted code — never the raw error. + expect(result.exitCode).toBe(0); + expect(result.resultJson?.workspaceRestoreFailure).toBe("restore_permission_denied"); + const allLogs = loggedLines.join(""); + expect(allLogs).not.toContain("SENTINEL-HOST-PATH-marker"); + expect(allLogs).not.toContain(localCwd); + expect(allLogs).not.toContain("EACCES"); + expect(allLogs).toContain("permission denied"); + } finally { + await fs.chmod(localCwd, 0o700).catch(() => undefined); + } + }); + it("remaps a workspace-relative explicit CLAUDE_CONFIG_DIR onto the in-sandbox workspace path", async () => { const root = await makeTempRoot("paperclip-claude-acp-explicit-inworkspace-"); const localCwd = path.join(root, "worktree"); diff --git a/packages/adapters/claude-local/src/server/acp.ts b/packages/adapters/claude-local/src/server/acp.ts index 721e2c39ab..d064cfae52 100644 --- a/packages/adapters/claude-local/src/server/acp.ts +++ b/packages/adapters/claude-local/src/server/acp.ts @@ -48,6 +48,10 @@ import { classifyThrownErrorClass, logSandboxProbeDiagnostic, } from "./probe-diagnostics.js"; +import { + classifyWorkspaceRestoreFailure, + describeWorkspaceRestoreFailure, +} from "@paperclipai/adapter-utils/workspace-restore-merge"; import { buildLocalAdapterTestProbeEnv } from "./probe-env.js"; import { detectClaudeLoginRequired, parseClaudeStreamJson } from "./parse.js"; import { buildClaudeProbePermissionArgs } from "./permissions.js"; @@ -219,13 +223,18 @@ async function prepareClaudeRemoteManagedHome( try { await onLog("stdout", "[paperclip] Restoring workspace changes from the sandbox.\n"); await stagedRuntime.restoreWorkspace((line) => onLog("stdout", line)); + return { ok: true }; } catch (err) { + // The run log is readable by any same-company actor, so it must never + // carry the caught error's own message: that message can hold a host + // filesystem path or a process id. Log only the fixed, allowlisted + // diagnostic for the classified code. + const code = classifyWorkspaceRestoreFailure(err); await onLog( "stderr", - `[paperclip] Claude ACP teardown workspace restore failed: ${ - err instanceof Error ? err.message : String(err) - }\n`, + `[paperclip] Claude ACP teardown workspace restore failed: ${describeWorkspaceRestoreFailure(code)}\n`, ); + return { ok: false, code }; } }; const envConfig = parseObject(input.config.env); diff --git a/packages/adapters/codex-local/src/server/acp.test.ts b/packages/adapters/codex-local/src/server/acp.test.ts index 37f547324f..6ce672ae98 100644 --- a/packages/adapters/codex-local/src/server/acp.test.ts +++ b/packages/adapters/codex-local/src/server/acp.test.ts @@ -1135,6 +1135,101 @@ describe("codex_local ACP lane", () => { expect(hostAuth.tokens.refresh_token).toBe("ref-sandbox-newer"); }); + it("test_codex_acp_teardown_restore_failure_sanitizes_the_run_log", async () => { + // Security regression for a workspace-restore write failure: the run log + // is readable by any same-company actor, so the teardown must never write + // the caught error's own message there — that message can carry the host + // workspace path. Force a real EACCES by making the workspace read-only, + // and name it with a sentinel marker so any leak is easy to spot. + const runId = "run-restore-failure"; + const root = await makeTempRoot("paperclip-codex-acp-restore-failure-"); + const localCwd = path.join(root, "SENTINEL-HOST-PATH-marker", "worktree"); + const remoteCwd = path.join(root, "remote-workspace"); + const sourceHome = path.join(root, "codex-home"); + const sharedHostHome = path.join(root, "shared-codex-home"); + await fs.mkdir(localCwd, { recursive: true }); + await fs.mkdir(remoteCwd, { recursive: true }); + await fs.mkdir(sourceHome, { recursive: true }); + await fs.mkdir(sharedHostHome, { recursive: true }); + await fs.writeFile(path.join(localCwd, "hello.txt"), "hi", "utf8"); + process.env.CODEX_HOME = sharedHostHome; + + // The runtime writes a new file into the in-sandbox workspace during the + // turn, so the teardown's restore has something to copy back — and a new + // file is exactly what a read-only workspace directory rejects. The + // workspace turns read-only only after the turn's own writes, so the + // teardown restore that runs after the turn is the write this forces to + // fail. + const runtime = new FakeRuntime({}); + const startTurn = runtime.startTurn.bind(runtime); + runtime.startTurn = (input) => { + const turn = startTurn(input); + const remoteWorkspaceCwd = input.handle.cwd ?? remoteCwd; + return { + ...turn, + result: (async () => { + await fs.writeFile(path.join(remoteWorkspaceCwd, "from-sandbox.txt"), "synced", "utf8"); + await fs.chmod(localCwd, 0o500); + return await turn.result; + })(), + }; + }; + + const stagedRuntimes = new Map(); + const execute = createCodexAcpExecutor({ + createRuntime: (options: FakeRuntimeOptions) => { + Object.assign(runtime.options, options); + return runtime as never; + }, + stagedRuntimes, + stagingLocks: new Map(), + }); + + const loggedLines: string[] = []; + try { + const result = await execute( + buildContext(localCwd, { + runId, + config: { + engine: "acp", + cwd: localCwd, + agentCommand: "node ./fake-acp.js", + stateDir: path.join(root, "state"), + env: { CODEX_HOME: sourceHome }, + promptTemplate: "Do the assigned work.", + }, + context: { + issueId: "issue-1", + paperclipWorkspace: { cwd: localCwd, source: "project_workspace", workspaceId: "workspace-1" }, + }, + executionTarget: { + kind: "remote", + transport: "sandbox", + providerKey: "fake-plugin", + remoteCwd, + runner: createLocalSandboxRunner(), + } as never, + authToken: "real-run-jwt", + onLog: async (_stream, chunk) => { + loggedLines.push(chunk); + }, + }), + ); + + // Fail-open: the restore miss never changes the run's exit code or + // status, and it surfaces as one allowlisted code — never the raw error. + expect(result.exitCode).toBe(0); + expect(result.resultJson?.workspaceRestoreFailure).toBe("restore_permission_denied"); + const allLogs = loggedLines.join(""); + expect(allLogs).not.toContain("SENTINEL-HOST-PATH-marker"); + expect(allLogs).not.toContain(localCwd); + expect(allLogs).not.toContain("EACCES"); + expect(allLogs).toContain("permission denied"); + } finally { + await fs.chmod(localCwd, 0o700).catch(() => undefined); + } + }); + it("falls back to the CLI lane for a runner-less sandbox even when the ACP command is set", async () => { setNodeVersion("v24.11.0"); // Isolate the missing bidirectional runner as the sole fallback cause: diff --git a/packages/adapters/codex-local/src/server/acp.ts b/packages/adapters/codex-local/src/server/acp.ts index 1c58d836e3..9bd45142ee 100644 --- a/packages/adapters/codex-local/src/server/acp.ts +++ b/packages/adapters/codex-local/src/server/acp.ts @@ -35,6 +35,10 @@ import { asString, parseObject, } from "@paperclipai/adapter-utils/server-utils"; +import { + classifyWorkspaceRestoreFailure, + describeWorkspaceRestoreFailure, +} from "@paperclipai/adapter-utils/workspace-restore-merge"; import { normalizeCodexModel } from "../index.js"; import { classifyCodexAuthRefreshFailure } from "./parse.js"; import { copyBackCodexAuth } from "./codex-auth-copyback.js"; @@ -243,17 +247,23 @@ async function prepareCodexRemoteManagedHome( "[paperclip] Restoring workspace changes and Codex auth from the sandbox.\n", ); await stagedRuntime.restoreWorkspace((line) => onLog("stdout", line)); + return { ok: true }; } catch (err) { // Fail-soft: a teardown copy-back miss loses this rotation and surfaces // loudly as refresh_token_reused on the next host Codex use (re-auth // recovers) — never silent host-credential corruption, so it must not // mask the run result. + // + // The run log is readable by any same-company actor, so it must never + // carry the caught error's own message: that message can hold a host + // filesystem path or a process id. Log only the fixed, allowlisted + // diagnostic for the classified code. + const code = classifyWorkspaceRestoreFailure(err); await onLog( "stderr", - `[paperclip] Codex ACP teardown restore/copy-back failed: ${ - err instanceof Error ? err.message : String(err) - }\n`, + `[paperclip] Codex ACP teardown restore/copy-back failed: ${describeWorkspaceRestoreFailure(code)}\n`, ); + return { ok: false, code }; } }, // One-time cleanup of the HOST staged home temp dir. Fired ONLY when the diff --git a/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts b/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts index 1bc6c78384..27d4a012fc 100644 --- a/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts +++ b/packages/adapters/codex-local/src/server/adapter-auth-promotion.ts @@ -224,6 +224,7 @@ export async function promoteDeviceLoginCredential( keptLine: "[paperclip] Codex device-login promotion: kept the company credential home (the login is not a seed or a strictly-newer same-identity credential).", tempPrefix: "auth.json.promotion-home", errorLabel: "codex device-login promotion", + env, }); // A kept home has two very different meanings. The writer keeps the home when @@ -258,7 +259,7 @@ export async function promoteDeviceLoginCredential( if (isCodexAuthCacheEnabled(env)) { try { const cacheEntryPath = await ensureCodexAuthCacheEntryDir(env, accountId, companyId); - await writeCodexAuthCacheEntry({ sandboxAuthBytes: authBytes, cacheEntryPath, log }); + await writeCodexAuthCacheEntry({ sandboxAuthBytes: authBytes, cacheEntryPath, log, env }); } catch { await log( "[paperclip] Codex device-login promotion: the per-identity cache write failed; the company credential home is durable, so the login stays successful.", diff --git a/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts b/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts index f3818ecd8a..014093bd65 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-cache.test.ts @@ -204,7 +204,7 @@ describe("codex auth cache store", () => { "acct-x": subscriptionAuth({ accountId: "acct-x", lastRefresh: NEWER, marker: "cache" }), }, }); - const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined); + const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined, env); expect(outcome).toBe("vended"); const finalHost = await readFile(sharedHomeAuthPath, "utf8"); expect(finalHost).toContain("acct-x"); @@ -221,7 +221,7 @@ describe("codex auth cache store", () => { }, }); const before = await readFile(sharedHomeAuthPath, "utf8"); - const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined); + const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined, env); expect(outcome).toBe("kept-host"); expect(await readFile(sharedHomeAuthPath, "utf8")).toBe(before); } @@ -235,7 +235,7 @@ describe("codex auth cache store", () => { }, }); const before = await readFile(sharedHomeAuthPath, "utf8"); - const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined); + const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined, env); expect(outcome).toBe("kept-host"); expect(await readFile(sharedHomeAuthPath, "utf8")).toBe(before); }); @@ -246,7 +246,7 @@ describe("codex auth cache store", () => { "acct-y": subscriptionAuth({ accountId: "acct-y", lastRefresh: NEWER, marker: "other" }), }, }); - const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined); + const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined, env); expect(outcome).toBe("no-host-identity"); await expect(lstat(sharedHomeAuthPath)).rejects.toThrow(); }); @@ -259,7 +259,7 @@ describe("codex auth cache store", () => { }, }); const before = await readFile(sharedHomeAuthPath, "utf8"); - const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined); + const outcome = await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), () => undefined, env); expect(outcome).toBe("no-host-identity"); expect(await readFile(sharedHomeAuthPath, "utf8")).toBe(before); }); @@ -272,9 +272,14 @@ describe("codex auth cache store", () => { }, }); const logs: string[] = []; - await selectVendCredential(sharedHomeAuthPath, resolveEntry(env), (line) => { - logs.push(line); - }); + await selectVendCredential( + sharedHomeAuthPath, + resolveEntry(env), + (line) => { + logs.push(line); + }, + env, + ); const combined = logs.join("\n"); expect(combined).not.toContain("SENTINEL"); expect(combined).not.toContain("SECRET-ACCT"); diff --git a/packages/adapters/codex-local/src/server/codex-auth-cache.ts b/packages/adapters/codex-local/src/server/codex-auth-cache.ts index cf33c17f87..5d9755a278 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-cache.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-cache.ts @@ -220,6 +220,8 @@ export async function writeCodexAuthCacheEntry(input: { sandboxAuthBytes: Buffer; cacheEntryPath: string; log: (line: string) => void | Promise; + /** Environment for the merge lock root. Defaults to `process.env`. */ + env?: NodeJS.ProcessEnv; }): Promise { const outcome = await writeCredentialSeedOrNewer({ sourceBytes: input.sandboxAuthBytes, @@ -231,6 +233,7 @@ export async function writeCodexAuthCacheEntry(input: { "[paperclip] Codex auth cache: kept the cache slot (source is not a strictly-newer same-identity subscription credential).", tempPrefix: "auth.json.cache-source", errorLabel: "codex auth cache", + env: input.env, }); return outcome === "written" ? "written" : "kept-slot"; } @@ -258,6 +261,7 @@ export async function selectVendCredential( sharedHomeAuthPath: string, resolveCacheEntryPath: (accountId: string) => string, log: (line: string) => void | Promise, + env: NodeJS.ProcessEnv = process.env, ): Promise { const hostBytes = await readFile(sharedHomeAuthPath).catch((error: NodeJS.ErrnoException) => { if (error.code === "ENOENT") return null; @@ -322,7 +326,7 @@ export async function selectVendCredential( await handle.close().catch(() => undefined); await rm(stagedTempPath, { force: true }).catch(() => undefined); } - }); + }, env); } /** @@ -345,9 +349,13 @@ export async function clearCodexAuthCacheEntry( throw error; }); if (!existing) return; - await withDirectoryMergeLock(entryDir, async () => { - await rm(entryDir, { recursive: true, force: true }); - }); + await withDirectoryMergeLock( + entryDir, + async () => { + await rm(entryDir, { recursive: true, force: true }); + }, + env, + ); } /** diff --git a/packages/adapters/codex-local/src/server/codex-auth-copyback.ts b/packages/adapters/codex-local/src/server/codex-auth-copyback.ts index 8206e7d1b2..96d29f3e20 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-copyback.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-copyback.ts @@ -96,43 +96,47 @@ export async function copyBackCodexAuth(input: CopyBackCodexAuthInput): Promise< const hostDir = path.dirname(hostAuthPath); await mkdir(hostDir, { recursive: true }); - const hostOutcome = await withDirectoryMergeLock(hostDir, async () => { - // Stage on the same filesystem as the host target so both the predicate read - // and the final rename stay device-local (rename across devices is not - // atomic and would fail with EXDEV). - const stagedTempPath = path.join(hostDir, `.auth.json.copyback-${process.pid}-${randomUUID()}.tmp`); - // `wx` + explicit mode create the temp private (0600) and fail if it somehow - // already exists, so we never write through a pre-existing symlink. - const handle = await open(stagedTempPath, "wx", 0o600); - try { - await handle.writeFile(sandboxAuthBytes); - await handle.close(); + const hostOutcome = await withDirectoryMergeLock( + hostDir, + async () => { + // Stage on the same filesystem as the host target so both the predicate read + // and the final rename stay device-local (rename across devices is not + // atomic and would fail with EXDEV). + const stagedTempPath = path.join(hostDir, `.auth.json.copyback-${process.pid}-${randomUUID()}.tmp`); + // `wx` + explicit mode create the temp private (0600) and fail if it somehow + // already exists, so we never write through a pre-existing symlink. + const handle = await open(stagedTempPath, "wx", 0o600); + try { + await handle.writeFile(sandboxAuthBytes); + await handle.close(); + + const decision = await decideCodexAuthMerge(stagedTempPath, hostAuthPath, { + errorLabel: "codex auth copy-back", + }); + if (decision === USE_SOURCE_EXIT) { + // Atomic same-directory swap; rename preserves the temp's 0600 mode. + await rename(stagedTempPath, hostAuthPath); + await log( + "[paperclip] Codex auth copy-back: sandbox credential is strictly newer for the same subscription identity; installed to the host at mode 0600.", + ); + return "copied"; + } - const decision = await decideCodexAuthMerge(stagedTempPath, hostAuthPath, { - errorLabel: "codex auth copy-back", - }); - if (decision === USE_SOURCE_EXIT) { - // Atomic same-directory swap; rename preserves the temp's 0600 mode. - await rename(stagedTempPath, hostAuthPath); await log( - "[paperclip] Codex auth copy-back: sandbox credential is strictly newer for the same subscription identity; installed to the host at mode 0600.", + "[paperclip] Codex auth copy-back: host credential kept (sandbox copy is not a strictly-newer same-identity subscription credential).", ); - return "copied"; + return "kept-host"; + } finally { + // Close is idempotent-safe to skip after an explicit close; the temp is the + // thing that must never linger. On the copy path rename already consumed it + // (force makes the removal a no-op); on every other path this deletes the + // staged credential bytes. + await handle.close().catch(() => undefined); + await rm(stagedTempPath, { force: true }).catch(() => undefined); } - - await log( - "[paperclip] Codex auth copy-back: host credential kept (sandbox copy is not a strictly-newer same-identity subscription credential).", - ); - return "kept-host"; - } finally { - // Close is idempotent-safe to skip after an explicit close; the temp is the - // thing that must never linger. On the copy path rename already consumed it - // (force makes the removal a no-op); on every other path this deletes the - // staged credential bytes. - await handle.close().catch(() => undefined); - await rm(stagedTempPath, { force: true }).catch(() => undefined); - } - }); + }, + env, + ); // Additive cache write. Independent of the host default overwrite above: it // runs on its own directory lock, keys the slot by the real sandbox @@ -150,7 +154,7 @@ export async function copyBackCodexAuth(input: CopyBackCodexAuthInput): Promise< const sandboxAccountId = readSubscriptionAccountId(sandboxAuthBytes); if (sandboxAccountId) { const cacheEntryPath = await resolveCacheEntryPath(sandboxAccountId); - await writeCodexAuthCacheEntry({ sandboxAuthBytes, cacheEntryPath, log }); + await writeCodexAuthCacheEntry({ sandboxAuthBytes, cacheEntryPath, log, env }); } } catch (error) { // Log only the errno code, never the error message. The message embeds the diff --git a/packages/adapters/codex-local/src/server/codex-auth-seed-write.ts b/packages/adapters/codex-local/src/server/codex-auth-seed-write.ts index aa675c00ac..a35b0504c0 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-seed-write.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-seed-write.ts @@ -33,6 +33,8 @@ export interface WriteCredentialSeedOrNewerInput { tempPrefix: string; /** The caller name that prefixes a predicate error. */ errorLabel: string; + /** Environment for the merge lock root. Defaults to `process.env`. */ + env?: NodeJS.ProcessEnv; } /** @@ -46,31 +48,35 @@ export async function writeCredentialSeedOrNewer( input: WriteCredentialSeedOrNewerInput, ): Promise { const destinationDir = path.dirname(input.destinationPath); - return withDirectoryMergeLock(destinationDir, async () => { - const stagedTempPath = path.join( - destinationDir, - `.${input.tempPrefix}-${process.pid}-${randomUUID()}.tmp`, - ); - // `wx` + explicit mode create the temp private (0600) and fail if it already - // exists, so the writer never writes through a pre-existing symlink. - const handle = await open(stagedTempPath, "wx", 0o600); - try { - await handle.writeFile(input.sourceBytes); - await handle.close(); - const decision = await decideCodexAuthMerge(stagedTempPath, input.destinationPath, { - seedIfDestAbsent: input.seedIfDestAbsent, - errorLabel: input.errorLabel, - }); - if (decision === USE_SOURCE_EXIT) { - await rename(stagedTempPath, input.destinationPath); - await input.log(input.writtenLine); - return "written"; + return withDirectoryMergeLock( + destinationDir, + async () => { + const stagedTempPath = path.join( + destinationDir, + `.${input.tempPrefix}-${process.pid}-${randomUUID()}.tmp`, + ); + // `wx` + explicit mode create the temp private (0600) and fail if it already + // exists, so the writer never writes through a pre-existing symlink. + const handle = await open(stagedTempPath, "wx", 0o600); + try { + await handle.writeFile(input.sourceBytes); + await handle.close(); + const decision = await decideCodexAuthMerge(stagedTempPath, input.destinationPath, { + seedIfDestAbsent: input.seedIfDestAbsent, + errorLabel: input.errorLabel, + }); + if (decision === USE_SOURCE_EXIT) { + await rename(stagedTempPath, input.destinationPath); + await input.log(input.writtenLine); + return "written"; + } + await input.log(input.keptLine); + return "kept"; + } finally { + await handle.close().catch(() => undefined); + await rm(stagedTempPath, { force: true }).catch(() => undefined); } - await input.log(input.keptLine); - return "kept"; - } finally { - await handle.close().catch(() => undefined); - await rm(stagedTempPath, { force: true }).catch(() => undefined); - } - }); + }, + input.env, + ); } diff --git a/packages/adapters/codex-local/src/server/device-login-export.ts b/packages/adapters/codex-local/src/server/device-login-export.ts index cdf258bba7..c281834f5f 100644 --- a/packages/adapters/codex-local/src/server/device-login-export.ts +++ b/packages/adapters/codex-local/src/server/device-login-export.ts @@ -247,6 +247,7 @@ export async function installDeviceLoginCredential( sandboxAuthBytes, cacheEntryPath: authPath, log, + env, }); return outcome === "written" ? "seeded" : "kept"; } diff --git a/packages/adapters/codex-local/src/server/execute.ts b/packages/adapters/codex-local/src/server/execute.ts index 48fa6705d1..f2f0635ede 100644 --- a/packages/adapters/codex-local/src/server/execute.ts +++ b/packages/adapters/codex-local/src/server/execute.ts @@ -665,6 +665,9 @@ export async function execute(ctx: AdapterExecutionContext): Promise resolveCodexAuthCacheEntryPath(process.env, accountId, agent.companyId), diff --git a/packages/adapters/gemini-local/src/server/acp.test.ts b/packages/adapters/gemini-local/src/server/acp.test.ts index 27ce706f82..27ba27f786 100644 --- a/packages/adapters/gemini-local/src/server/acp.test.ts +++ b/packages/adapters/gemini-local/src/server/acp.test.ts @@ -613,6 +613,90 @@ describe("gemini_local ACP lane", () => { await expect(fs.readFile(path.join(localCwd, "from-sandbox.txt"), "utf8")).resolves.toBe("synced"); }); + it("test_gemini_acp_teardown_restore_failure_sanitizes_the_run_log", async () => { + // Security regression for a workspace-restore write failure: the run log + // is readable by any same-company actor, so the teardown must never write + // the caught error's own message there — that message can carry the host + // workspace path. Force a real EACCES by making the workspace read-only, + // and name it with a sentinel marker so any leak is easy to spot. + const root = await makeTempRoot("paperclip-gemini-acp-restore-failure-"); + const localCwd = path.join(root, "SENTINEL-HOST-PATH-marker", "worktree"); + const remoteCwd = path.join(root, "remote-workspace"); + await fs.mkdir(localCwd, { recursive: true }); + await fs.mkdir(remoteCwd, { recursive: true }); + await fs.writeFile(path.join(localCwd, "hello.txt"), "hi", "utf8"); + + // The runtime writes a new file into the in-sandbox workspace during the + // turn, so the teardown's restore has something to copy back — and a new + // file is exactly what a read-only workspace directory rejects. The + // workspace turns read-only only after the turn's own writes, so the + // teardown restore that runs after the turn is the write this forces to + // fail. + const runtime = new FakeRuntime({}); + const startTurn = runtime.startTurn.bind(runtime); + runtime.startTurn = (input) => { + const turn = startTurn(input); + const remoteWorkspaceCwd = input.handle.cwd ?? remoteCwd; + return { + ...turn, + result: (async () => { + await fs.writeFile(path.join(remoteWorkspaceCwd, "from-sandbox.txt"), "synced", "utf8"); + await fs.chmod(localCwd, 0o500); + return await turn.result; + })(), + }; + }; + + const execute = createGeminiAcpExecutor({ + createRuntime: (options) => { + Object.assign(runtime.options, options); + return runtime as never; + }, + }); + + const loggedLines: string[] = []; + try { + const result = await execute( + buildContext(localCwd, { + config: { + engine: "acp", + cwd: localCwd, + agentCommand: "node ./fake-acp.js", + stateDir: path.join(root, "state"), + promptTemplate: "Do the assigned work.", + }, + context: { + issueId: "issue-1", + paperclipWorkspace: { cwd: localCwd, source: "project_workspace", workspaceId: "workspace-1" }, + }, + executionTarget: { + kind: "remote", + transport: "sandbox", + providerKey: "fake-plugin", + remoteCwd, + runner: createLocalSandboxRunner(), + } as never, + authToken: "real-run-jwt", + onLog: async (_stream, chunk) => { + loggedLines.push(chunk); + }, + }), + ); + + // Fail-open: the restore miss never changes the run's exit code or + // status, and it surfaces as one allowlisted code — never the raw error. + expect(result.exitCode).toBe(0); + expect(result.resultJson?.workspaceRestoreFailure).toBe("restore_permission_denied"); + const allLogs = loggedLines.join(""); + expect(allLogs).not.toContain("SENTINEL-HOST-PATH-marker"); + expect(allLogs).not.toContain(localCwd); + expect(allLogs).not.toContain("EACCES"); + expect(allLogs).toContain("permission denied"); + } finally { + await fs.chmod(localCwd, 0o700).catch(() => undefined); + } + }); + it("does not persist an api-key auth selector from a host-only credential", async () => { const root = await makeTempRoot("paperclip-gemini-acp-hostkey-"); const localCwd = path.join(root, "worktree"); diff --git a/packages/adapters/gemini-local/src/server/acp.ts b/packages/adapters/gemini-local/src/server/acp.ts index f169b45801..95d8db4b69 100644 --- a/packages/adapters/gemini-local/src/server/acp.ts +++ b/packages/adapters/gemini-local/src/server/acp.ts @@ -31,6 +31,10 @@ import { asString, parseObject, } from "@paperclipai/adapter-utils/server-utils"; +import { + classifyWorkspaceRestoreFailure, + describeWorkspaceRestoreFailure, +} from "@paperclipai/adapter-utils/workspace-restore-merge"; import { DEFAULT_GEMINI_LOCAL_MODEL } from "../index.js"; const moduleDir = path.dirname(fileURLToPath(import.meta.url)); @@ -166,13 +170,18 @@ async function prepareGeminiRemoteManagedHome( try { await onLog("stdout", "[paperclip] Restoring workspace changes from the sandbox.\n"); await stagedRuntime.restoreWorkspace((line) => onLog("stdout", line)); + return { ok: true }; } catch (err) { + // The run log is readable by any same-company actor, so it must never + // carry the caught error's own message: that message can hold a host + // filesystem path or a process id. Log only the fixed, allowlisted + // diagnostic for the classified code. + const code = classifyWorkspaceRestoreFailure(err); await onLog( "stderr", - `[paperclip] Gemini ACP teardown workspace restore failed: ${ - err instanceof Error ? err.message : String(err) - }\n`, + `[paperclip] Gemini ACP teardown workspace restore failed: ${describeWorkspaceRestoreFailure(code)}\n`, ); + return { ok: false, code }; } }; const geminiSkillsHome = resolveGeminiSkillsHome(input.config);