From b06034d762a081d22373393ba76d90a80bdb47dd Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Thu, 27 Aug 2026 10:54:11 -0700 Subject: [PATCH] Write mode-constrained inbound files directly to their target (#12320) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Adapter utilities transfer files between the host and an agent environment > - A sandbox target already provides the security boundary for inbound files > - The generic fallback adds a temporary file and a rename that do not add protection inside that boundary > - This pull request writes a mode-constrained inbound file directly to its target and applies the mode after the write > - The benefit is a simpler transfer path while host targets keep the strict pre-write mode rule ## Linked Issues or Issue Description **What existing behavior does this improve?** The inbound file-sync fallback for a mode-constrained file stages the file under a temporary name, applies the mode, and renames the file into place. **Subsystem affected** `packages/adapter-utils` and `packages/plugins`. **Current behavior** A sandbox target uses a temporary path before it receives the file. The host then changes the mode and renames the file to the target path. **Proposed behavior** A sandbox target receives the file at its target path. The host applies the mode after the write. A host target still applies the mode before the first byte. **Reason and benefit** The sandbox boundary already protects the target. The direct write removes an unnecessary staging path and rename. **Breaking changes** None. The directory path and outbound transfer path keep their existing behavior. ## What Changed - Write a mode-constrained single-file inbound transfer directly to the sandbox target. - Apply the mode after the direct write and keep the confinement check before post-upload commands. - Scope the protocol comment by transfer direction and preserve the strict host-target rule. - Keep directory inbound transfers and outbound transfers unchanged. ## Verification - Run the targeted unit suite for the changed package. - Verify the suite covers direct target writes, post-write mode application, and confinement rejection. - Run `tsc --noEmit` for both changed packages. - Review the full GitHub Actions check set after the PR opens. ## Risks - A sandbox provider that assumes a temporary inbound path could expose a behavior mismatch. - The confinement check remains before post-upload commands, which limits escape risk. - Host targets keep the pre-write mode rule, so host permission behavior does not change. ## Model Used OpenAI GPT-5. This model assisted with Git operations, PR preparation, review coordination, and tool use. Context window size and reasoning mode are not exposed by the runtime. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../src/command-managed-runtime.test.ts | 74 ++++--------------- .../src/command-managed-runtime.ts | 22 ++---- packages/plugins/sdk/src/protocol.ts | 13 +++- 3 files changed, 29 insertions(+), 80 deletions(-) diff --git a/packages/adapter-utils/src/command-managed-runtime.test.ts b/packages/adapter-utils/src/command-managed-runtime.test.ts index f601ce4bce..ff66347bc2 100644 --- a/packages/adapter-utils/src/command-managed-runtime.test.ts +++ b/packages/adapter-utils/src/command-managed-runtime.test.ts @@ -748,7 +748,7 @@ describe("command managed runtime", () => { expect(execTimeouts).toEqual([runTimeoutMs, syncClientTimeoutMs]); }); - it("fallback syncIn stages mode-constrained files before chmod and rename", async () => { + it("fallback syncIn writes a mode-constrained file directly to its target and then applies the mode", async () => { const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-syncin-mode-")); cleanupDirs.push(rootDir); const sourceFile = path.join(rootDir, "source.txt"); @@ -766,67 +766,19 @@ describe("command managed runtime", () => { ]); expect(await readFile(targetFile, "utf8")).toBe("payload\n"); + // The write goes straight to the target path. No staging name and no + // rename step exist between the write and the chmod. const scripts = calls.map((call) => (call.args ?? []).join(" ")); - expect(scripts).toHaveLength(5); - expect(scripts[0]).toContain(targetFile + ".paperclip-syncin."); - expect(scripts[0]).toContain(".paperclip-upload."); - expect(scripts[1]).toContain("rm -rf"); - expect(scripts[1]).toContain(".paperclip-upload."); - expect(scripts[2]).toContain("chmod 640"); - expect(scripts[2]).toContain(targetFile + ".paperclip-syncin."); - expect(scripts[3]).toContain("mv -f"); - expect(scripts[3]).toContain(targetFile + ".paperclip-syncin."); - expect(scripts[3]).toContain(targetFile); - expect(scripts[4]).toContain("rm -rf"); - expect(scripts[4]).toContain(targetFile + ".paperclip-syncin."); + expect(scripts.some((script) => script.includes(".paperclip-syncin."))).toBe(false); + expect(scripts.some((script) => script.includes("mv -f") && script.includes(".paperclip-syncin."))).toBe( + false, + ); + const chmodScript = scripts.find((script) => script.includes("chmod 640")); + expect(chmodScript).toBeDefined(); + expect(chmodScript).toContain(targetFile); }); - it("fallback syncIn cleans up a staged file when chmod fails before rename", async () => { - const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-syncin-cleanup-")); - cleanupDirs.push(rootDir); - const sourceFile = path.join(rootDir, "source.txt"); - const targetFile = path.join(rootDir, "target.txt"); - await writeFile(sourceFile, "payload\n", "utf8"); - - const { runner, calls } = makeSpawnRunner({ supportsSingleStreamStdinProgress: true }); - const delegatedExecute = runner.execute.bind(runner); - runner.execute = async (input) => { - const script = (input.args ?? []).join(" "); - if (script.includes("chmod 600")) { - calls.push({ command: input.command, args: input.args, cwd: input.cwd, stdin: input.stdin }); - return { - exitCode: 1, - signal: null, - timedOut: false, - stdout: "", - stderr: "chmod failed", - pid: null, - startedAt: new Date().toISOString(), - }; - } - return await delegatedExecute(input); - }; - const client = createCommandManagedRuntimeClient({ runner, commandCwd: "/", timeoutMs: 30_000 }); - - await expect( - client.syncIn!([ - { - operationId: "op-cleanup", - files: [{ sourcePath: sourceFile, targetPath: targetFile, kind: "file", mode: 0o600 }], - }, - ]), - ).rejects.toThrow(/chmod failed/); - - const chmodCall = calls.find((call) => (call.args ?? []).join(" ").includes("chmod 600")); - expect(chmodCall).toBeDefined(); - const stagedPath = (chmodCall?.args ?? []).join(" ").match(/chmod 600 '([^']+)'/)?.[1]; - expect(stagedPath).toBeDefined(); - await expect(readFile(stagedPath!, "utf8")).rejects.toMatchObject({ code: "ENOENT" }); - expect(calls.some((call) => (call.args ?? []).join(" ").includes(`rm -rf '${stagedPath}'`))).toBe(true); - await expect(readFile(targetFile, "utf8")).rejects.toMatchObject({ code: "ENOENT" }); - }); - - it("test_post_upload_commands_execute_verbatim_not_rewritten (C1 opaque)", async () => { + it("post-upload commands execute verbatim and are never rewritten", async () => { // The provider/client treats each command as opaque: it is executed VERBATIM, // never concatenated with asset keys / paths or otherwise rewritten. const executed: string[] = []; @@ -850,7 +802,7 @@ describe("command managed runtime", () => { expect(executed).toContain(verbatim); }); - it("test_post_upload_command_cwd_escaping_target_root_is_rejected (C2)", async () => { + it("post-upload command cwd that escapes the target root is rejected", async () => { // A `cwd` that escapes the operation's target root — via `..` or an absolute // path outside the target — is rejected BEFORE any handoff (no execute). let executeCalls = 0; @@ -904,7 +856,7 @@ describe("command managed runtime", () => { expect(executeCalls).toBe(0); }); - it("test_fallback_syncIn_aborts_and_rejects_on_first_nonzero_exit (C4 fail-fast)", async () => { + it("fallback syncIn aborts on the first non-zero exit", async () => { // The first non-zero post-upload command aborts the operation: syncIn rejects, // the remaining commands do NOT run, and there is no silent partial fallback. const executed: string[] = []; diff --git a/packages/adapter-utils/src/command-managed-runtime.ts b/packages/adapter-utils/src/command-managed-runtime.ts index b707529f4d..36500fdc70 100644 --- a/packages/adapter-utils/src/command-managed-runtime.ts +++ b/packages/adapter-utils/src/command-managed-runtime.ts @@ -208,10 +208,6 @@ function buildSyncInExtractDirectoryCommand(input: { remoteTarPath: string; targ function buildSyncInChmodCommand(input: { mode: number; targetPath: string }): string { return `chmod ${(input.mode & 0o7777).toString(8)} ${shellQuote(input.targetPath)}`; } -function buildSyncInRenameCommand(input: { sourcePath: string; targetPath: string }): string { - return "mv -f " + shellQuote(input.sourcePath) + " " + shellQuote(input.targetPath); -} - function buildUniqueStagingPath(input: { targetPath: string; suffix: string }): string { return `${input.targetPath}${input.suffix}.${randomUUID()}`; } @@ -444,18 +440,10 @@ export function createCommandManagedRuntimeClient(input: { bytesTransferred += tarBytes.byteLength; } else { const fileBytes = await fs.readFile(mapping.sourcePath); - const targetPathForWrite = mapping.mode != null - ? buildUniqueStagingPath({ targetPath: mapping.targetPath, suffix: ".paperclip-syncin" }) - : mapping.targetPath; - if (mapping.mode != null) cleanupPaths.push(targetPathForWrite); - await client.writeFile(targetPathForWrite, bufferToArrayBuffer(fileBytes)); + await client.writeFile(mapping.targetPath, bufferToArrayBuffer(fileBytes)); if (mapping.mode != null) { await client.run( - buildSyncInChmodCommand({ mode: mapping.mode, targetPath: targetPathForWrite }), - { timeoutMs: input.timeoutMs }, - ); - await client.run( - buildSyncInRenameCommand({ sourcePath: targetPathForWrite, targetPath: mapping.targetPath }), + buildSyncInChmodCommand({ mode: mapping.mode, targetPath: mapping.targetPath }), { timeoutMs: input.timeoutMs }, ); } @@ -468,9 +456,9 @@ export function createCommandManagedRuntimeClient(input: { } filesTransferred += 1; } - // Ordered, fail-fast post-upload commands (C1 opaque / C4 fail-loud). Each - // command string is executed VERBATIM — never rewritten, concatenated, or - // appended to. First non-zero exit or timeout throws and stops the rest. + // Ordered, fail-fast post-upload commands. Each command string is + // executed VERBATIM — never rewritten, concatenated, or appended to. + // The first non-zero exit or timeout throws and stops the rest. for (const command of operation.postUploadCommands ?? []) { const result = await input.runner.execute({ command: shellCommand, diff --git a/packages/plugins/sdk/src/protocol.ts b/packages/plugins/sdk/src/protocol.ts index 0f7f1933ba..ee8d6d3559 100644 --- a/packages/plugins/sdk/src/protocol.ts +++ b/packages/plugins/sdk/src/protocol.ts @@ -725,8 +725,17 @@ export interface PluginSyncFileMapping { kind: "file" | "directory"; /** * POSIX file mode to apply at the target (e.g. `0o600` for secret material). - * When set, providers MUST create the target with this mode with no - * world-readable window (create-with-mode or chmod-before-bytes, never after). + * The target MUST carry this mode when the transfer completes. + * + * For a transfer to a host target, providers MUST apply the mode with no + * world-readable window: create the target with the mode, or apply the mode + * before the bytes arrive at the target path. A host file sits outside the + * sandbox boundary, so an open window shows the bytes to other host + * processes. + * + * For a transfer to a sandbox target, providers MAY apply the mode after + * they write the bytes. The sandbox is the trust boundary, so a short window + * shows the bytes only to code that already runs in that sandbox. */ mode?: number; /** Glob patterns to exclude when `kind` is `"directory"`. */