mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-08 00:54:38 +02:00
## Thinking Path > - Paperclip is an open-source AI-agent management platform; agents run tasks inside sandboxed environments (Daytona, Kubernetes, E2B, etc.) > - The control-plane ↔ sandbox file-transfer path flows through the `environmentExecute` seam in `protocol.ts` — the only verb available to plugins — which forces a base64-over-exec chunked loop for every file move: workspace files, assets, Codex home sync > - This transport is correct and safe, but it bypasses provider-native bulk/streaming APIs (Daytona `uploadFiles`, K8s `FastUploadInterceptor` / volume mounts), leaving significant throughput on the table for large workspaces > - The right fix is an opt-in seam extension: providers with faster native transfer declare two optional verbs; providers that do not opt in stay on the existing fallback with zero code or behavior change required > - This PR adds the first layer of that extension — two optional verbs (`environmentSyncIn` / `environmentSyncOut`) in the plugin SDK, the runtime plumbing to prefer the native path for the two clean destroy-then-replace cases, and a doc for the contract > - The core correctness invariant is byte-identical fallback: if no provider opts in, execution is exactly what ships today; `assertSyncOperationsConfined` enforces host-side path confinement for providers that do opt in > - No provider advertises the verbs yet → zero production behavior change; future PRs wire up Daytona and K8s providers against this contract ## Linked Issues or Issue Description No public GitHub issue exists for this feature. Description follows the `feature_request` issue template: **Subsystem affected:** packages/plugins — plugin system; packages/adapter-utils — adapter runtime; server/ — EnvironmentRuntimeService **Problem or motivation:** Sandbox file transfers currently always use a base64-over-exec chunked loop regardless of what the underlying provider supports. For workspaces larger than a few MB this becomes the dominant wall-clock cost of every sandbox run, and it bypasses bulk/stream APIs that providers like Daytona already expose natively. **Proposed solution:** Add two optional, opt-in plugin hooks — `onEnvironmentSyncIn` / `onEnvironmentSyncOut` — to the plugin SDK. When a provider defines both hooks and both are advertised via the existing `supportedMethods` negotiation, the runtime prefers the native path for the two clean destroy-then-replace transfer cases; all other cases fall back to the existing byte-identical base64 transport. **Alternatives considered:** An unconditional verb would require every provider to implement or stub the verb. The opt-in / `METHOD_NOT_IMPLEMENTED` pattern (already used by `environmentExecute`) preserves backward compatibility with zero provider changes required. **Roadmap alignment:** Consistent with the ✅ "Cloud / Sandbox agents" and ✅ "Plugin system" milestones; extends the plugin seam rather than adding control-plane-level logic. **Additional context:** Searched open pull requests and issues for duplicate sandbox file-sync / native-transfer work; none found. ## What Changed - **`packages/plugins/sdk`** - `protocol.ts`: two new optional `HostToWorkerMethods` — `environmentSyncIn` / `environmentSyncOut` — plus generic `SyncOperation`, `SyncFileMapping`, and `SyncOutcome` types - `define-plugin.ts`: optional `onEnvironmentSyncIn` / `onEnvironmentSyncOut` fields on `PluginDefinition`; worker advertises each verb only when its hook is defined (else `METHOD_NOT_IMPLEMENTED`, mirroring `environmentExecute`) - `worker-rpc-host.ts`: route new verbs to plugin hooks - `index.ts`: re-export new public types - **`packages/adapter-utils`** - `command-managed-runtime.ts`: expose optional `syncIn` / `syncOut` on `CommandManagedRuntimeRunner` (available only when both verbs are advertised); add `assertSyncOperationsConfined` host-side path-confinement guard - `sandbox-managed-runtime.ts`: `SandboxManagedRuntimeClient` gains optional `syncIn` / `syncOut`; orchestrator prefers native path for default-provision asset inbound and workspace-download-into-fresh-dir outbound; all other paths keep the existing base64 fallback - `sandbox-file-sync.test.ts` (new): 234-line characterization suite — native-opt-in branch, fallback branch, `assertSyncOperationsConfined` escape-path rejection, `followSymlinks` → tar `-h` - `command-managed-runtime.test.ts`: negotiation + native-sync + confinement tests - **`server/src/services/environment-runtime.ts`**: `EnvironmentRuntimeService` delegates to `syncIn` / `syncOut`, gated on advertised support - **`server/src/services/environment-execution-target.ts`**: minor typing fix alongside the new verbs - **`doc/plugins/SANDBOX_FILE_SYNC_HOOKS.md`** (new): documents the full contract — opt-in / no-op guarantee, operation ordering, provider-may-tar, atomicity, `followSymlinks`, secret modes (0600, no window), path confinement, `operationId` opacity, resource bounds, shell-quoting ## Verification ```bash # SDK suite pnpm --filter packages/plugins/sdk test # Adapter-utils suite (includes new sandbox-file-sync characterization tests) pnpm --filter packages/adapter-utils test # Expected: 255 pass / 4 skip # Type-check across affected packages pnpm --filter packages/plugins/sdk typecheck pnpm --filter packages/adapter-utils typecheck # Server changed-file spot check: cd server && npx tsc --noEmit --skipLibCheck 2>&1 | grep -E "environment-(runtime|execution-target)" | head -20 ``` Key behavioral invariant to spot-check: with no provider opting in (the current state), run any sandbox task and confirm file-transfer behavior is byte-for-byte identical to what the pre-PR code produces. The characterization tests assert this at the unit level. ## Risks - **Zero production risk today**: no provider advertises `environmentSyncIn` / `environmentSyncOut`, so the new code paths are unreachable in production; all real traffic stays on the existing base64 fallback - **Path confinement**: `assertSyncOperationsConfined` rejects any `targetPath` that escapes the declared root — this is the primary security boundary for future providers. The test suite covers escape-path rejection - **Atomicity**: the contract delegates atomicity to providers; the doc explicitly calls out that directory-level ops are not guaranteed atomic - **Secret transport**: credential assets (e.g., Codex `auth.json`, directory mappings) continue to use the existing tar path — they do not go through the new verbs in any current provider > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used Provider: Anthropic Model: `claude-sonnet-4-6` (Claude Sonnet 4.6) Context window: 200 K tokens Capabilities: extended tool use, multi-file code generation, agentic reasoning via the Paperclip agent framework ## 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: Harold Kim <harold@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
206 lines
9.3 KiB
Markdown
206 lines
9.3 KiB
Markdown
# Sandbox file-sync lifecycle hooks
|
|
|
|
A sandbox environment provider moves workspace and asset files between the host
|
|
and the sandbox around every run. By default the runtime synthesizes that
|
|
transfer over the single `environmentExecute` verb: it base64-encodes bytes and
|
|
pipes them through `base64 -d` shell commands, one bounded round-trip per chunk.
|
|
That works everywhere but is slow for large workspaces because it cannot use a
|
|
provider's native bulk file transport.
|
|
|
|
The two **optional, opt-in** hooks documented here let a provider replace that
|
|
base64-over-exec transfer with its own native mechanism:
|
|
|
|
- **`onEnvironmentSyncIn`** — before execution: place a set of host
|
|
files/directories at target sandbox paths.
|
|
- **`onEnvironmentSyncOut`** — after execution: copy a set of sandbox
|
|
files/directories back to target host paths.
|
|
|
|
They are entirely opt-in. A provider that does not define them keeps the exact
|
|
base64 fallback, byte-for-byte — there is **zero behavior change** for existing
|
|
providers.
|
|
|
|
## Opt-in / no-op semantics
|
|
|
|
A hook is opted into exactly like `onEnvironmentExecute`: defining it on your
|
|
`PluginDefinition` makes the worker advertise the matching verb in
|
|
`InitializeResult.supportedMethods`; leaving it undefined omits the verb and the
|
|
guarded handler throws `METHOD_NOT_IMPLEMENTED` if it is ever called.
|
|
|
|
**Both hooks are advertised and consumed as a pair.** The host runtime uses the
|
|
native path only when the worker advertises **both** `environmentSyncIn` and
|
|
`environmentSyncOut`; if a provider advertises only one, the orchestrator keeps
|
|
the base64 fallback for both directions. Define both or neither.
|
|
|
|
```ts
|
|
export default definePlugin({
|
|
async setup() { /* ... */ },
|
|
async onEnvironmentSyncIn(params) {
|
|
return { operations: await transferInbound(params) };
|
|
},
|
|
async onEnvironmentSyncOut(params) {
|
|
return { operations: await transferOutbound(params) };
|
|
},
|
|
});
|
|
```
|
|
|
|
## The operation / file-mapping contract
|
|
|
|
Each hook receives an ordered list of **operations**. Each operation carries an
|
|
opaque id and a list of source→target **file mappings**:
|
|
|
|
```ts
|
|
interface PluginSyncOperation {
|
|
operationId: string; // opaque, non-sensitive; do NOT interpret it
|
|
files: PluginSyncFileMapping[];
|
|
}
|
|
|
|
interface PluginSyncFileMapping {
|
|
sourcePath: string; // absolute
|
|
targetPath: string; // absolute
|
|
kind: "file" | "directory";
|
|
mode?: number; // POSIX mode to apply at the target
|
|
exclude?: string[]; // glob excludes for a directory mapping
|
|
followSymlinks?: boolean; // directory symlink handling; see below
|
|
}
|
|
|
|
interface PluginEnvironmentSyncResult {
|
|
operations: { operationId: string; filesTransferred: number; bytesTransferred: number }[];
|
|
}
|
|
```
|
|
|
|
For `onEnvironmentSyncIn`, `sourcePath` is a **host** path and `targetPath` a
|
|
**sandbox** path. For `onEnvironmentSyncOut` the direction is reversed. All
|
|
sandbox paths are POSIX. Return per-operation `filesTransferred` /
|
|
`bytesTransferred` for observability.
|
|
|
|
### Ordering
|
|
|
|
Operations are applied strictly in array order, and the orchestrator invokes the
|
|
hooks in a fixed lifecycle order (inbound before execution, outbound after). The
|
|
orchestrator owns *what* and *when*; a provider only executes the opaque
|
|
transfers it is handed and must not reorder them.
|
|
|
|
### A provider may tar internally
|
|
|
|
The contract only describes the observable source→target result. How you move
|
|
the bytes is yours: bulk upload API, an internal `tar` stream, per-file
|
|
enumeration — all are fine. Whatever you do, the materialized target must be
|
|
observationally identical to the mapping (same files, same contents, same modes,
|
|
same symlink treatment) so the native and fallback paths are interchangeable.
|
|
|
|
### `operationId` is opaque
|
|
|
|
`operationId` is an opaque, non-sensitive token authored by the orchestrator. It
|
|
is **not** derived from any secret or user data, it is safe to log and safe to
|
|
expose to the sandbox, and a provider **must not** parse or depend on its value.
|
|
Do not echo it into a path or a place where it could collide with real data.
|
|
|
|
## Symlink contract (`followSymlinks`)
|
|
|
|
`followSymlinks` applies to `kind: "directory"` mappings and has exactly the
|
|
meaning of `tar`'s `-h` flag:
|
|
|
|
- **falsy (default)** — archive and recreate symlinks **as links** (preserve).
|
|
- **`true`** — **dereference** each symlink to its target bytes.
|
|
|
|
A provider honoring a directory mapping MUST reproduce this: preserve links when
|
|
falsy, dereference to bytes when `true`. The orchestrator passes the same value
|
|
it passes to its own tar create step, so native and fallback are observationally
|
|
identical. There is no separate extract-side symlink flag and no execution-time
|
|
special case — symlink handling lives entirely in this one flag.
|
|
|
|
## Atomicity contract
|
|
|
|
The required guarantee level is deliberately equal to the base64 fallback's
|
|
floor, so opting in never weakens integrity and never over-promises.
|
|
|
|
- **Single-file mappings (`kind: "file"`) MUST be atomic-replace (REQUIRED).**
|
|
Stage the bytes to a provider-chosen temporary path, then atomically rename
|
|
onto `targetPath`, so an interrupted transfer never leaves a truncated file at
|
|
`targetPath`. This mirrors the fallback, which stages to
|
|
`<path>.paperclip-upload` and then `mv -f`.
|
|
- The temp file MUST live in the **same directory (same filesystem)** as
|
|
`targetPath`. A cross-device rename degrades to copy-then-unlink and
|
|
reintroduces the truncation window it is meant to close.
|
|
- Reserve the `.paperclip-upload*` scratch names: a provider-chosen temp must
|
|
not collide with the fallback scratch name or with a real target.
|
|
|
|
- **Directory mappings and the sync as a whole are NOT atomic / NOT
|
|
transactional.** A directory transfer is destroy-then-replace: a crash
|
|
mid-transfer can leave a partial tree, and the runtime does not roll back
|
|
already-moved bytes across operations. This matches today's behavior; do not
|
|
assume a directory operation is atomic. Where an individual file must be
|
|
integrity-protected, deliver it as its own `kind: "file"` mapping so it inherits
|
|
the single-file atomic-replace guarantee.
|
|
|
|
- **Every operation is fail-loud.** An operation either completes or raises to
|
|
the orchestrator; never report partial success silently. The orchestrator may
|
|
then retry or fall back.
|
|
|
|
## Secret material and file modes
|
|
|
|
This seam can carry credential-bearing files (for example an auth directory).
|
|
Treat `mode` as mandatory for such mappings:
|
|
|
|
- Apply the requested `mode` (e.g. `0o600`) with **no world-readable window** —
|
|
create the target with the mode, or `chmod` **before** writing any bytes, never
|
|
after.
|
|
- `mode` MUST be honored for files **inside a directory mapping** too, not only
|
|
for `kind: "file"` mappings. If you tar internally, preserve permissions; if
|
|
you enumerate, set the mode as each file lands. A credential that rides a
|
|
directory mapping otherwise silently loses its `0o600` guarantee.
|
|
- A directory mapping is not atomic (see above). If a directory carries an
|
|
individually-sensitive secret whose integrity matters, prefer delivering that
|
|
secret as a `kind: "file"` mapping so it gets atomic-replace, or protect its
|
|
integrity out of band.
|
|
|
|
## Host-side path confinement (required of the orchestrator)
|
|
|
|
The sandbox is untrusted relative to the host, so **the orchestrator — not the
|
|
provider — owns and confines every path**. Before an operation is handed to a
|
|
provider, the runtime canonicalizes each mapping's `sourcePath`/`targetPath` and
|
|
confines it to an orchestrator-owned root (the workspace directory or a specific
|
|
asset directory), rejecting absolute escapes and `..` traversal fail-closed. A
|
|
provider receives only already-confined, orchestrator-authored paths and MUST
|
|
NOT widen them (for example by following a sandbox-planted symlink out of the
|
|
intended root on an outbound write). Confinement is a host-side complete-mediation
|
|
guard and is never delegated below the trust boundary.
|
|
|
|
## Resource bounds
|
|
|
|
The base64 fallback enforces transfer caps so a runaway payload cannot exhaust
|
|
memory. A native provider MUST keep an equivalent bound — stream or chunk large
|
|
transfers rather than buffering unboundedly, and fail closed on an oversized
|
|
inline payload rather than silently removing the cap.
|
|
|
|
## Shell safety (native providers that shell out)
|
|
|
|
If your native transfer builds shell command strings (for example a pod-exec
|
|
`tar`/`base64`/`mv` pipeline), single-quote **every** interpolated path so a path
|
|
containing shell metacharacters is transferred literally, never interpreted.
|
|
Providers whose transport is a non-shell API (a typed bulk-upload call) do not
|
|
need this, but any shell interpolation must quote.
|
|
|
|
## Reference: minimal shape
|
|
|
|
```ts
|
|
async onEnvironmentSyncIn({ operations }) {
|
|
const results = [];
|
|
for (const op of operations) { // apply in order
|
|
let filesTransferred = 0;
|
|
let bytesTransferred = 0;
|
|
for (const f of op.files) {
|
|
if (f.kind === "file") {
|
|
// stage to a same-dir temp, then atomic rename onto f.targetPath,
|
|
// applying f.mode with no world-readable window
|
|
} else {
|
|
// materialize f.sourcePath at f.targetPath (destroy-then-replace),
|
|
// honoring f.exclude and f.followSymlinks, applying per-file modes
|
|
}
|
|
}
|
|
results.push({ operationId: op.operationId, filesTransferred, bytesTransferred });
|
|
}
|
|
return { operations: results };
|
|
}
|
|
```
|