From d49f1683810d4d8f34e3e329d6490f6e760ae4fb Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:46:57 -0500 Subject: [PATCH] fix: publish sandbox files on legacy and native runners (#13493) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents must publish generated files so users can inspect their results after a sandbox stops. > - Legacy sandbox bridges blocked attachment listing and could not carry multipart binary uploads through the queue transport. > - The native runner has a separate verified file registration path that needs the same durable result. > - This pull request repairs legacy binary transport, makes native download receipts explicit, and reveals new outputs in the task Artifacts tab. > - Users can open generated files from either runner without a transport flag change. ## Linked Issues or Issue Description Refs #13355 for the existing native file publication path. Related filename fixes: #2615 and #4788. Related sandbox persistence work: #13376. This change repairs attachment delivery through the existing API; it does not add workspace persistence. **What happened?** The upload helper first lists task attachments to avoid duplicates. Both legacy bridge allowlists rejected that GET request with 403. A direct multipart upload also failed: the queue bridge accepted only JSON, excluded attachment uploads, and converted bytes to UTF-8 text. Enabling HTTP/2 alone did not fix the missing listing route. These failures occurred before attachment storage. **Expected behavior** Both runners can publish a workspace file, register its work product, bind it to a response, and return a working download. The file stays accessible after sandbox deletion. A new output opens the task Artifacts tab. The agent receives accurate errors and decides how to retry or report a failure. **Steps to reproduce** 1. Run a legacy agent in Daytona with the duplex bridge disabled. 2. Invoke the bundled upload helper with Bash on a PNG or PDF. 3. Repeat with the duplex bridge enabled. 4. Register the same file through the native runner with generic API tools disabled. 5. Retry registration, delete the sandbox, and compare the downloaded bytes with the original file. **Paperclip version or commit** The failing baseline was `f2c5e54dc`. This branch is rebased onto `6cfe4acff`. **Deployment mode** Source checkout with a local API and real isolated Daytona sandboxes. ## What Changed - Allow authenticated attachment listing, upload, and content download through both legacy bridge transports. - Add optional base64 body encoding to queue envelopes. Preserve the existing UTF-8 contract when the encoding field is absent. Decode binary bodies before forwarding them. - Preserve multipart headers. Bound raw bytes, encoded envelopes, and in-flight reservations. Retain timeout and uncertain-write behavior. - Preserve helper deduplication and return structured uncertain-write failures. Document explicit Bash invocation in live skills. - Add attachment IDs and content/download paths to native registration receipts. Reuse verified local and remote file reads, attachment storage, work-product registration, and response binding. - Preserve Unicode upload filenames and provide a valid Content-Disposition header. - Open the task Artifacts tab when new stored outputs arrive, including a closed desktop panel or mobile drawer. Deduplicate upload and registration events by object ID. Preserve manual selection on refetches, edits, and panel remounts. - Remove task artifact filters, the company Artifacts footer link, and the unassigned group heading and timestamp. ### Screenshot ![Generated images and a document in the task Artifacts tab](https://pages.paperclip.ing/sandbox-file-delivery-2026-09-15/artifacts-tab.png) This is the local display fixture. The image was generated separately and published through the attachment and work-product APIs. ## Verification - Post-rebase `pnpm -r typecheck` and `pnpm build` pass. - The post-rebase local `pnpm test:run` passed 12,369 tests before one existing conversation reset test timed out; all 33 tests in that suite pass when rerun with isolated test configuration. The aggregate command stopped before its remaining groups. GitHub runs the complete suite in separate shards. - All [GitHub verification checks](https://github.com/paperclipai/paperclip/actions/runs/35017893350) pass on `b66ac276dd3d5fc738a22ecea783400106a494d4`: 32 successful checks and two configured skips. The native-session recovery assertion initially raced its fire-and-forget Sentry report; all 13 tests pass locally, and the same-commit CI rerun passes all 170 suites (3,079 tests). - Live post-rebase Daytona: all three file-delivery tests pass. They cover the real Bash helper with the queue bridge, the helper with HTTP/2, and native `register_deliverable` with generic API tools disabled. - Daytona cases cover PNG/PDF bytes, spaced and Unicode names, duplicate registration, response binding, authorization controls, and byte-for-byte downloads after sandbox deletion. - Local focused coverage includes transfer bounds, malformed encoding, interrupted transfers, remote path containment, and native file verification. The attachment route suite passes all 32 tests, including an eight-case filename-header matrix for Unicode and special characters, inline and forced downloads, and full and partial responses. - Browser verification confirms image previews, persisted downloads, automatic Artifacts selection, and preserved manual selection after edits and reloads. Desktop/mobile component coverage passes. The latest UI cleanup passes its 10 affected tests and token gates. - Coverage limit: the Daytona tests call the real helper and native registration path directly. They do not replay a complete model-led image-generation task through the browser. Live command (requires a configured Daytona credential): ```sh PAPERCLIP_FILE_DELIVERY_DAYTONA=1 pnpm exec vitest run server/src/__tests__/file-delivery-bridges.test.ts ``` ## Risks - Binary queue bodies use more memory because base64 adds encoding overhead. Transfer and process limits must remain aligned. - An interrupted write can have an unknown result. The bridge reports this state and preserves stable retry identities. - New artifacts intentionally change the active task tab. Existing history and repeated updates must not take focus again. - Transport flag defaults, server authorization, frozen skill snapshots, and completion policies remain unchanged. No schema migration is required. ## Model Used OpenAI Codex, based on GPT-6, with reasoning, repository tools, code execution, and browser testing. The runtime does not expose a more specific model ID or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/AGENT-ARTIFACTS.md | 59 ++++- .../adapter-utils/src/execution-target.ts | 34 ++- .../src/sandbox-callback-bridge-body.test.ts | 34 +++ .../src/sandbox-callback-bridge-body.ts | 40 ++++ .../src/sandbox-callback-bridge.test.ts | 92 ++++++- .../src/sandbox-callback-bridge.ts | 225 +++++++++++++----- .../__tests__/file-delivery-bridges.test.ts | 176 ++++++++++++++ .../helpers/file-delivery-daytona.ts | 59 +++++ .../__tests__/issue-attachment-routes.test.ts | 40 ++++ .../__tests__/paperclip-skill-utils.test.ts | 8 +- server/src/routes/issues.ts | 7 +- server/src/services/heartbeat.ts | 2 +- .../native-runner-file-handoff.test.ts | 6 + .../native-runner-file-handoff.ts | 9 + .../native-runtime/remote-deliverable-file.ts | 4 +- skills/paperclip/SKILL.md | 4 +- skills/paperclip/references/artifacts.md | 22 +- .../scripts/paperclip-upload-artifact.sh | 14 +- .../IssuePropertiesArtifactsTab.tsx | 92 ++----- ...ssuePropertiesMarkdownWorkProduct.test.tsx | 10 +- .../task-side-panel/TaskSidePanel.test.tsx | 48 ++++ .../task-side-panel/TaskSidePanel.tsx | 15 ++ ui/src/hooks/useTaskArtifactArrival.test.tsx | 114 +++++++++ ui/src/hooks/useTaskArtifactArrival.ts | 62 +++++ ui/src/pages/IssueDetail.test.tsx | 40 ++++ ui/src/pages/IssueDetail.tsx | 36 +++ 26 files changed, 1064 insertions(+), 188 deletions(-) create mode 100644 packages/adapter-utils/src/sandbox-callback-bridge-body.test.ts create mode 100644 packages/adapter-utils/src/sandbox-callback-bridge-body.ts create mode 100644 server/src/__tests__/file-delivery-bridges.test.ts create mode 100644 server/src/__tests__/helpers/file-delivery-daytona.ts create mode 100644 ui/src/hooks/useTaskArtifactArrival.test.tsx create mode 100644 ui/src/hooks/useTaskArtifactArrival.ts diff --git a/doc/AGENT-ARTIFACTS.md b/doc/AGENT-ARTIFACTS.md index 8832a7d526c..4fe76cc7702 100644 --- a/doc/AGENT-ARTIFACTS.md +++ b/doc/AGENT-ARTIFACTS.md @@ -5,10 +5,28 @@ must be attached to the Paperclip issue before the agent chooses a final disposition. A local workspace path is not enough, because cloud users and reviewers often cannot access the agent's disk. -Use the helper bundled with the Paperclip skill from the repo root: +## Native runner + +When `register_deliverable` is available, use it for files in the bound local or +remote workspace. Supply a workspace-relative `contentRef`, basename `filename`, +`contentType`, exact `byteSize` and SHA-256, `title`, and a stable `idempotencyKey`. +The tool verifies the file, stores an attachment and artifact work product, and +binds it to the response. Generic API tools and a legacy API key are unnecessary. + +Wait for the receipt. It includes `attachmentId`, `contentPath`, and +`downloadPath`, along with the existing command, revision, entity references, +and disposition. Reuse the original key after an ambiguous result. A receipt +confirms storage and response binding in Paperclip; it does not confirm delivery +to an external chat provider. If registration fails, use the returned error to +resolve the failure or explain the limitation; do not describe a workspace path +as an uploaded file. + +## Legacy adapters + +Use Bash to run the helper bundled with the Paperclip skill from the repo root; installed skill files may not retain executable permissions: ```sh -skills/paperclip/scripts/paperclip-upload-artifact.sh path/to/output.webm \ +bash skills/paperclip/scripts/paperclip-upload-artifact.sh path/to/output.webm \ --title "Walkthrough render" \ --summary "Rendered walkthrough for review" ``` @@ -27,6 +45,16 @@ It uploads the file to artifact work product on `POST /api/issues/{issueId}/work-products` by default. The command prints issue-safe markdown links for the final task comment. +## Task artifact presentation + +While a task is open, a new agent attachment, work product, or document opens +the task's Artifacts tab and reveals the side panel (or mobile drawer). This +uses stored object IDs, so it works with either runner. Uploading a file and +registering its work product counts as one arrival. Existing history, revisions, +and repeated query refreshes preserve the user's tab selection. Plans retain +their existing Plan-tab behavior; unregistered user input attachments remain in +the conversation. + ## Uploaded Artifacts vs Workspace Files Use uploaded artifacts for deliverables: videos, PDFs, screenshots, archives, @@ -96,7 +124,7 @@ available, not the preferred way to deliver files to users. Upload an `.mp4` render: ```sh -skills/paperclip/scripts/paperclip-upload-artifact.sh dist/demo.mp4 \ +bash skills/paperclip/scripts/paperclip-upload-artifact.sh dist/demo.mp4 \ --title "Demo video render" \ --summary "MP4 render for board review" ``` @@ -104,7 +132,7 @@ skills/paperclip/scripts/paperclip-upload-artifact.sh dist/demo.mp4 \ Upload a `.webm` render: ```sh -skills/paperclip/scripts/paperclip-upload-artifact.sh out/walkthrough.webm \ +bash skills/paperclip/scripts/paperclip-upload-artifact.sh out/walkthrough.webm \ --title "Walkthrough video" \ --summary "WebM walkthrough render" ``` @@ -113,7 +141,7 @@ The helper detects `.mp4`, `.webm`, and `.mov` content types. If a renderer uses an unusual extension, pass the MIME type explicitly: ```sh -skills/paperclip/scripts/paperclip-upload-artifact.sh render.bin \ +bash skills/paperclip/scripts/paperclip-upload-artifact.sh render.bin \ --title "Demo video render" \ --content-type video/mp4 ``` @@ -144,3 +172,24 @@ curl -sS -X POST \ Use `type: "artifact"`, `provider: "paperclip"`, and metadata containing the uploaded `attachmentId`. The server canonicalizes `contentType`, `byteSize`, `contentPath`, `openPath`, `downloadPath`, and `originalFilename`. + +## Verification + +The file-delivery integration suite runs the real helper through queue and +HTTP/2 gateways against a disposable API, database, and storage. It also tests +native registration with generic API tools disabled, duplicate retries, Unicode +filenames, company isolation, and downloads after deleting the workspace. + +```sh +pnpm exec vitest run server/src/__tests__/file-delivery-bridges.test.ts +``` + +To run the same suite on disposable Daytona sandboxes, install the standalone +Daytona plugin's dependencies and set `DAYTONA_API_KEY` in the test process: + +```sh +PAPERCLIP_FILE_DELIVERY_DAYTONA=1 pnpm exec vitest run server/src/__tests__/file-delivery-bridges.test.ts +``` + +The live fixture deletes each sandbox before checking that its attachments +remain downloadable from Paperclip. It does not run unless explicitly enabled. diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index 4ed16aa0de3..eaae713b283 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -4219,10 +4219,16 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { const queueDir = path.posix.join(bridgeRuntimeDir, "queue"); const assetRemoteDir = path.posix.join(bridgeRuntimeDir, "server"); const bridgeToken = createSandboxCallbackBridgeToken(); + const configuredAttachmentBytes = Number(process.env.PAPERCLIP_ATTACHMENT_MAX_BYTES); + // A larger upload limit needs multipart headroom. A smaller attachment limit + // remains enforced by the API and must not shrink unrelated JSON responses. + const defaultBodyBytes = Number.isSafeInteger(configuredAttachmentBytes) && configuredAttachmentBytes > 0 + ? Math.max(DEFAULT_SANDBOX_CALLBACK_BRIDGE_MAX_BODY_BYTES, configuredAttachmentBytes + 64 * 1024) + : DEFAULT_SANDBOX_CALLBACK_BRIDGE_MAX_BODY_BYTES; const maxBodyBytes = typeof input.maxBodyBytes === "number" && Number.isFinite(input.maxBodyBytes) && input.maxBodyBytes > 0 ? Math.trunc(input.maxBodyBytes) - : DEFAULT_SANDBOX_CALLBACK_BRIDGE_MAX_BODY_BYTES; + : defaultBodyBytes; // The bridge worker runs inside the same process that serves the Paperclip // API, so forwarded sandbox calls must target the LOCAL listen origin. The // PAPERCLIP_RUNTIME_API_URL / PAPERCLIP_API_URL exports now prefer a @@ -4282,20 +4288,15 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { path: string; query: string; headers: Record; - /** The file bridge passes the whole request body here as one string. - * The HTTP/2 bridge passes it as the raw `Buffer` it read off the wire. */ + /** Legacy text envelopes remain strings; binary uploads are raw bytes. */ body?: string | Buffer; }, signal?: AbortSignal, options?: { suppressDebugLog?: boolean; /** - * The caller's stream reservation owner, if it has one. The HTTP/2 - * bridge passes the stream's own owner here, so the response body copy - * reserves against the same ceiling the request body copy already - * reserved against. The queue transport passes no owner, so its - * response-body read enforces only the per-request size ceiling, exactly - * as it did before this option existed. + * Both transports pass the request's reservation owner, so response + * buffers count toward the same host process ceiling as request buffers. */ reservation?: BridgeBodyReservation; }, @@ -4324,8 +4325,8 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { const timeoutSignal = AbortSignal.timeout(forwardTimeoutMs); const forwardSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; // Build the request-body init. A GET or a HEAD carries no body. The file - // bridge passes the whole body as one string; the HTTP/2 bridge passes it - // as a raw `Buffer`. Undici accepts a `Buffer` request body directly (a + // bridge passes legacy JSON as a string and binary data as a `Buffer`; + // HTTP/2 passes raw `Buffer` bodies. Undici accepts a `Buffer` request body directly (a // `Buffer` is an `ArrayBufferView`), so neither shape needs a conversion. // The cast below only bridges a `BodyInit` typing gap: the DOM library // type this project's ambient `RequestInit` resolves to excludes a @@ -4736,13 +4737,10 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { maxBodyBytes, getRuntimeParentContext: input.getRuntimeParentContext, runtimeSpan: input.runtimeSpan, - // The queue transport writes the response body to a text file, so this - // is the one place the forward path decodes the response `Buffer` to a - // UTF-8 string. The queue's own on-wire behavior does not change. - handleRequest: async (request, options) => { - const result = await forwardBridgeRequest(request, options?.signal); - return { status: result.status, headers: result.headers, body: result.body.toString("utf8") }; - }, + // The worker encodes binary bodies only at the queue boundary. + handleRequest: (request, options) => forwardBridgeRequest(request, options?.signal, { + reservation: options?.reservation, + }), }); server = await startSandboxCallbackBridgeServer({ runner, diff --git a/packages/adapter-utils/src/sandbox-callback-bridge-body.test.ts b/packages/adapter-utils/src/sandbox-callback-bridge-body.test.ts new file mode 100644 index 00000000000..b9cc3160f27 --- /dev/null +++ b/packages/adapter-utils/src/sandbox-callback-bridge-body.test.ts @@ -0,0 +1,34 @@ +import { runInNewContext } from "node:vm"; +import { describe, expect, it } from "vitest"; +import { decodeSandboxBridgeBody, encodeSandboxBridgeBody, sandboxBridgeBodyCodecSource, sandboxBridgeEnvelopeLimit } from "./sandbox-callback-bridge-body.js"; + +const embedded = runInNewContext(`${sandboxBridgeBodyCodecSource()}; ({ encodeSandboxBridgeBody, decodeSandboxBridgeBody })`, { Buffer }); + +describe.each([ + { name: "host", encode: encodeSandboxBridgeBody, decode: decodeSandboxBridgeBody }, + { name: "generated gateway", encode: embedded.encodeSandboxBridgeBody as typeof encodeSandboxBridgeBody, decode: embedded.decodeSandboxBridgeBody as typeof decodeSandboxBridgeBody }, +])("queue body codec: $name", ({ encode, decode }) => { + it("preserves old text envelopes and arbitrary binary bytes at the limit", () => { + const text = "猫\u0000\n"; + expect(encode(text, 5)).toEqual({ body: text }); + expect(decode({ body: text }, 5)).toEqual(Buffer.from(text)); + expect(decode({ body: text, bodyEncoding: "utf8" }, 5)).toEqual(Buffer.from(text)); + const bytes = Buffer.from(Array.from({ length: 256 }, (_, index) => index)); + expect(decode(encode(bytes, bytes.length), bytes.length)).toEqual(bytes); + expect(encode(bytes, bytes.length).bodyEncoding).toBe("base64"); + expect(() => encode(bytes, bytes.length - 1)).toThrow(/size limit/); + expect(() => decode(encode(bytes, bytes.length), bytes.length - 1)).toThrow(/size limit/); + expect(() => decode({ body: text }, 4)).toThrow(/size limit/); + }); + + it.each(["!AAA", "YQ", "YQ=", "YQ===", "YQ==\n", "YR==", "=AAA", "____", "猫"])('rejects malformed or noncanonical base64 "%s"', body => { + expect(() => decode({ body, bodyEncoding: "base64" }, 1024)).toThrow(/base64/); + }); + + it("rejects unknown encodings and bounds JSON escaping overhead", () => { + expect(() => decode({ body: "abc", bodyEncoding: "hex" as "utf8" }, 1024)).toThrow(/encoding/); + expect(() => decode({ body: null as unknown as string }, 1024)).toThrow(/Invalid/); + const envelope = { id: "request", ...encode("\u0000".repeat(1024), 1024) }; + expect(Buffer.byteLength(JSON.stringify(envelope))).toBeLessThan(sandboxBridgeEnvelopeLimit(1024)); + }); +}); diff --git a/packages/adapter-utils/src/sandbox-callback-bridge-body.ts b/packages/adapter-utils/src/sandbox-callback-bridge-body.ts new file mode 100644 index 00000000000..cbacfd7c520 --- /dev/null +++ b/packages/adapter-utils/src/sandbox-callback-bridge-body.ts @@ -0,0 +1,40 @@ +export interface SandboxCallbackBridgeBody { + body: string; + /** Omitted by older queue peers, whose bodies are UTF-8 text. */ + bodyEncoding?: "utf8" | "base64"; +} + +/** JSON can escape each input byte as six characters. Metadata is bounded too. */ +export function sandboxBridgeEnvelopeLimit(maxBodyBytes: number): number { + return 6 * maxBodyBytes + 64 * 1024; +} + +export function encodeSandboxBridgeBody(body: string | Buffer, maxBodyBytes: number): SandboxCallbackBridgeBody { + if (Buffer.byteLength(body) > maxBodyBytes) throw new Error("Bridge body exceeded the configured size limit."); + return Buffer.isBuffer(body) ? { body: body.toString("base64"), bodyEncoding: "base64" } : { body }; +} + +/** Self-contained so the same decoder can be embedded in the remote gateway. */ +export function decodeSandboxBridgeBody(envelope: SandboxCallbackBridgeBody, maxBodyBytes: number): Buffer { + if (!envelope || typeof envelope.body !== "string") throw new Error("Invalid bridge body."); + if (envelope.bodyEncoding === undefined || envelope.bodyEncoding === "utf8") { + if (Buffer.byteLength(envelope.body, "utf8") > maxBodyBytes) throw new Error("Bridge body exceeded the configured size limit."); + return Buffer.from(envelope.body, "utf8"); + } + if (envelope.bodyEncoding !== "base64") throw new Error("Unsupported bridge body encoding."); + const value = envelope.body; + if (value.length > 4 * Math.ceil(maxBodyBytes / 3)) throw new Error("Bridge body exceeded the configured size limit."); + // Buffer.from is permissive; reject malformed input before allocating bytes. + if (value.length % 4 !== 0 || /[^A-Za-z0-9+/=]/.test(value) || !/^[A-Za-z0-9+/]*={0,2}$/.test(value)) { + throw new Error("Invalid bridge base64 body."); + } + const bytes = Buffer.from(value, "base64"); + if (bytes.length > maxBodyBytes) throw new Error("Bridge body exceeded the configured size limit."); + if (bytes.toString("base64") !== value) throw new Error("Invalid bridge base64 body."); + return bytes; +} + +export function sandboxBridgeBodyCodecSource(): string { + return [sandboxBridgeEnvelopeLimit, encodeSandboxBridgeBody, decodeSandboxBridgeBody] + .map(fn => `const ${fn.name} = ${fn.toString()};`).join("\n"); +} diff --git a/packages/adapter-utils/src/sandbox-callback-bridge.test.ts b/packages/adapter-utils/src/sandbox-callback-bridge.test.ts index 5bac8e218b6..1009b6e4253 100644 --- a/packages/adapter-utils/src/sandbox-callback-bridge.test.ts +++ b/packages/adapter-utils/src/sandbox-callback-bridge.test.ts @@ -164,7 +164,7 @@ describe("sandbox callback bridge", () => { path: string; query: string; headers: Record; - body: string; + body: string | Buffer; }> = []; const worker = await startSandboxCallbackBridgeWorker({ @@ -1495,9 +1495,9 @@ describe("sandbox callback bridge", () => { ); } - // The HTTP/2 route list adds the two binary attachment routes on top of - // the documented heartbeat surface. + // Both transports admit the full attachment workflow. const http2Allowed: Array<{ method: string; path: string }> = [ + { method: "GET", path: "/api/issues/issue-1/attachments" }, { method: "POST", path: "/api/companies/co-1/issues/issue-1/attachments" }, { method: "GET", path: "/api/attachments/att-1/content" }, ]; @@ -1522,15 +1522,14 @@ describe("sandbox callback bridge", () => { } }); - it("denies both attachment routes on the default (queue) route list", () => { + it("admits listing, uploads and downloads on the default queue route list", () => { const attachmentRequests: Array<{ method: string; path: string }> = [ + { method: "GET", path: "/api/issues/issue-1/attachments" }, { method: "POST", path: "/api/companies/co-1/issues/issue-1/attachments" }, { method: "GET", path: "/api/attachments/att-1/content" }, ]; for (const request of attachmentRequests) { - expect(authorizeSandboxCallbackBridgeRequestWithRoutes(request)).toBe( - `Route not allowed: ${request.method} ${request.path}`, - ); + expect(authorizeSandboxCallbackBridgeRequestWithRoutes(request)).toBeNull(); } }); @@ -3576,6 +3575,83 @@ describe("sandbox callback bridge", () => { expect(response.headers.get("x-paperclip-bridge-outcome")).toBeNull(); }, 15_000); + async function startQueueGatewayForFileTest(options: { + maxBodyBytes: number; + client?: SandboxCallbackBridgeQueueClient; + handleRequest: Parameters[0]["handleRequest"]; + }) { + const root = await mkdtemp(path.join(os.tmpdir(), "paperclip-file-codec-")); + cleanupDirs.push(root); + const asset = await createSandboxCallbackBridgeAsset(); + cleanupFns.push(asset.cleanup); + const queueDir = path.join(root, "queue"); + const bridgeToken = createSandboxCallbackBridgeToken(); + const worker = await startSandboxCallbackBridgeWorker({ + client: options.client ?? createFileSystemSandboxCallbackBridgeQueueClient(), queueDir, + maxBodyBytes: options.maxBodyBytes, handleRequest: options.handleRequest, pollIntervalMs: 10, + }); + cleanupFns.push(() => worker.stop()); + const gateway = await startSandboxCallbackBridgeServer({ + runner: createExecRunner(), remoteCwd: root, assetRemoteDir: asset.localDir, + queueDir, bridgeToken, maxBodyBytes: options.maxBodyBytes, pollIntervalMs: 10, + }); + cleanupFns.push(() => gateway.stop()); + return { ...gateway, bridgeToken, queueDir }; + } + + it.each(["queue", "http2"])("preserves multipart bytes at the configured limit on %s and rejects overflow before forwarding", async mode => { + const maxBodyBytes = 1024; + const bytes = Buffer.alloc(maxBodyBytes, 0xff); + const handled = vi.fn(async (request: { body?: string | Buffer }) => ({ + status: 200, headers: { "content-type": "application/octet-stream" }, body: Buffer.from(request.body ?? ""), + })); + const bridgeToken = createSandboxCallbackBridgeToken(); + const gateway = mode === "queue" + ? await startQueueGatewayForFileTest({ maxBodyBytes, handleRequest: handled }) + : { ...await startHttp2GatewayForTest({ bridgeToken, maxBodyBytes, forwardRequest: handled }), bridgeToken }; + const post = (body: Buffer) => fetch(`${gateway.baseUrl}/api/companies/c/issues/i/attachments`, { + method: "POST", headers: { authorization: `Bearer ${gateway.bridgeToken}`, "content-type": "multipart/form-data; boundary=exact-boundary" }, body: new Uint8Array(body), + }); + const response = await post(bytes); + expect(response.status).toBe(200); + expect(Buffer.from(await response.arrayBuffer())).toEqual(bytes); + expect(handled).toHaveBeenCalledTimes(1); + expect(handled.mock.calls[0][0]).toMatchObject({ headers: { "content-type": "multipart/form-data; boundary=exact-boundary" } }); + const overflow = await post(Buffer.alloc(maxBodyBytes + 1)); + expect(overflow.status).toBeGreaterThanOrEqual(400); + expect(handled).toHaveBeenCalledTimes(1); + }); + + it("rejects malformed queue encodings without forwarding a mutation", async () => { + const handled = vi.fn(async () => ({ status: 200, body: "ok" })); + const gateway = await startQueueGatewayForFileTest({ maxBodyBytes: 1024, handleRequest: handled }); + const directories = sandboxCallbackBridgeDirectories(gateway.queueDir); + for (const [id, bodyEncoding, body] of [["bad-base64", "base64", "YR=="], ["bad-encoding", "hex", "00"], ["too-big", "base64", Buffer.alloc(1025).toString("base64")]]) { + await createFileSystemSandboxCallbackBridgeQueueClient().writeTextFile(path.join(directories.requestsDir, `${id}.json`), JSON.stringify({ + id, method: "POST", path: "/api/companies/c/issues/i/attachments", query: "", headers: {}, bodyEncoding, body, + })); + const responsePath = path.join(directories.responsesDir, `${id}.json`); + await vi.waitFor(async () => expect(JSON.parse(await readFile(responsePath, "utf8")).status).toBe(400)); + } + expect(handled).not.toHaveBeenCalled(); + }); + + it("reports a corrupted upload response as indeterminate and never forwards twice", async () => { + const client = createFileSystemSandboxCallbackBridgeQueueClient(); + const writeResponse = client.writeResponseFile!.bind(client); + client.writeResponseFile = (destination, body, options) => writeResponse(destination, + JSON.stringify({ ...JSON.parse(body), bodyEncoding: "base64", body: "invalid" }), options); + const handled = vi.fn(async () => ({ status: 201, body: Buffer.from("saved") })); + const gateway = await startQueueGatewayForFileTest({ client, maxBodyBytes: 1024, handleRequest: handled }); + const response = await fetch(`${gateway.baseUrl}/api/companies/c/issues/i/attachments`, { + method: "POST", headers: { authorization: `Bearer ${gateway.bridgeToken}`, "content-type": "multipart/form-data; boundary=boundary" }, body: Buffer.from([0xff]), + }); + expect(response.status).toBe(409); + expect(response.headers.get("x-paperclip-bridge-outcome")).toBe("indeterminate"); + await expect(response.json()).resolves.toMatchObject({ retryable: false, outcome: "indeterminate" }); + expect(handled).toHaveBeenCalledTimes(1); + }); + it("rejects a request body over maxBodyBytes on the queue path before it writes the queue file", async () => { const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-bridge-queue-maxbody-")); cleanupDirs.push(rootDir); @@ -3656,7 +3732,7 @@ describe("sandbox callback bridge", () => { const bridgeToken = createSandboxCallbackBridgeToken(); const requestBodyText = JSON.stringify({ note: "café" }); - const seenRequests: Array<{ body: string }> = []; + const seenRequests: Array<{ body: string | Buffer }> = []; const worker = await startSandboxCallbackBridgeWorker({ client: createFileSystemSandboxCallbackBridgeQueueClient(), queueDir, diff --git a/packages/adapter-utils/src/sandbox-callback-bridge.ts b/packages/adapter-utils/src/sandbox-callback-bridge.ts index 05f27e3fa96..d9145c9c256 100644 --- a/packages/adapter-utils/src/sandbox-callback-bridge.ts +++ b/packages/adapter-utils/src/sandbox-callback-bridge.ts @@ -4,6 +4,14 @@ import http2 from "node:http2"; import os from "node:os"; import path from "node:path"; import type { Duplex } from "node:stream"; +import { + decodeSandboxBridgeBody, + encodeSandboxBridgeBody, + sandboxBridgeBodyCodecSource, + sandboxBridgeEnvelopeLimit, + type SandboxCallbackBridgeBody, +} from "./sandbox-callback-bridge-body.js"; +import type { BridgeBodyReservation } from "./http2-bridge-server.js"; import { runWithoutActiveStep, @@ -162,6 +170,11 @@ export const DEFAULT_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST: readonly SandboxCa { method: "PATCH", path: /^\/api\/issues\/[^/]+$/ }, { method: "GET", path: /^\/api\/issues\/[^/]+\/approvals$/ }, + // Files: the queue encodes binary bodies; HTTP/2 carries the same bytes directly. + { method: "GET", path: /^\/api\/issues\/[^/]+\/attachments$/ }, + { method: "POST", path: /^\/api\/companies\/[^/]+\/issues\/[^/]+\/attachments$/ }, + { method: "GET", path: /^\/api\/attachments\/[^/]+\/content$/ }, + // Work products: publish branch/commit/artifact metadata for completed work. { method: "GET", path: /^\/api\/issues\/[^/]+\/work-products$/ }, { method: "POST", path: /^\/api\/issues\/[^/]+\/work-products$/ }, @@ -209,15 +222,8 @@ export const DEFAULT_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST: readonly SandboxCa { method: "DELETE", path: /^\/api\/routine-triggers\/[^/]+$/ }, ] as const; -// The HTTP/2 bridge carries the request body as raw bytes, so it can admit -// the two binary attachment routes. The queue transport keeps -// `DEFAULT_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST`, because its envelope -// carries a string body only. -export const HTTP2_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST: readonly SandboxCallbackBridgeRouteRule[] = [ - ...DEFAULT_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST, - { method: "POST", path: /^\/api\/companies\/[^/]+\/issues\/[^/]+\/attachments$/ }, - { method: "GET", path: /^\/api\/attachments\/[^/]+\/content$/ }, -] as const; +// Keep the public alias for callers selecting the HTTP/2 transport. +export const HTTP2_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST = DEFAULT_SANDBOX_CALLBACK_BRIDGE_ROUTE_ALLOWLIST; export const DEFAULT_SANDBOX_CALLBACK_BRIDGE_HEADER_ALLOWLIST = [ "accept", @@ -227,21 +233,16 @@ export const DEFAULT_SANDBOX_CALLBACK_BRIDGE_HEADER_ALLOWLIST = [ "x-paperclip-github-capability", ] as const; -export interface SandboxCallbackBridgeRequest { +export interface SandboxCallbackBridgeRequest extends SandboxCallbackBridgeBody { id: string; method: string; path: string; query: string; headers: Record; - /** - * UTF-8 body contents. The bridge rejects non-JSON request bodies; binary - * payloads are intentionally out of scope for this queue protocol. - */ - body: string; createdAt: string; } -export interface SandboxCallbackBridgeResponse { +export interface SandboxCallbackBridgeResponse extends SandboxCallbackBridgeBody { id: string; status: number; headers: Record; @@ -273,7 +274,8 @@ export interface SandboxCallbackBridgeQueueClient { // external implementation stays compatible without a change. makeDirs?(remotePaths: string[]): Promise; listJsonFiles(remotePath: string): Promise; - readTextFile(remotePath: string): Promise; + fileSize?(remotePath: string): Promise; + readTextFile(remotePath: string, maxBytes?: number): Promise; writeTextFile(remotePath: string, body: string): Promise; writeResponseFile?( responsePath: string, @@ -522,7 +524,24 @@ export function createFileSystemSandboxCallbackBridgeQueueClient(): SandboxCallb .map((entry) => entry.name) .sort((left, right) => left.localeCompare(right)); }, - readTextFile: async (remotePath) => await fs.readFile(remotePath, "utf8"), + fileSize: async (remotePath) => (await fs.stat(remotePath)).size, + readTextFile: async (remotePath, maxBytes) => { + if (maxBytes === undefined) return fs.readFile(remotePath, "utf8"); + const file = await fs.open(remotePath, "r"); + try { + const stat = await file.stat(); + if (stat.size > maxBytes) throw new Error("Bridge envelope exceeded the configured size limit."); + const bytes = Buffer.alloc(Math.min(stat.size, maxBytes) + 1); + let length = 0; + while (length < bytes.length) { + const read = await file.read(bytes, length, bytes.length - length, length); + if (!read.bytesRead) break; + length += read.bytesRead; + } + if (length > stat.size) throw new Error("Bridge envelope changed while reading."); + return bytes.subarray(0, length).toString("utf8"); + } finally { await file.close(); } + }, writeTextFile: async (remotePath, body) => { await fs.mkdir(path.posix.dirname(remotePath), { recursive: true }); // Write to a temporary path that does NOT end in `.json`, then rename it @@ -661,9 +680,18 @@ export function createCommandManagedSandboxCallbackBridgeQueueClient(input: { .filter((line) => line.length > 0) .sort((left, right) => left.localeCompare(right)); }, - readTextFile: async (remotePath) => { - const result = await runChecked(`read ${remotePath}`, `base64 < ${shellQuote(remotePath)}`); - return Buffer.from(result.stdout.replace(/\s+/g, ""), "base64").toString("utf8"); + fileSize: async (remotePath) => { + const result = await runChecked(`size ${remotePath}`, `wc -c < ${shellQuote(remotePath)}`); + return Number(result.stdout.trim()); + }, + readTextFile: async (remotePath, maxBytes) => { + const command = maxBytes === undefined + ? `base64 < ${shellQuote(remotePath)}` + : `head -c ${Math.trunc(maxBytes) + 1} ${shellQuote(remotePath)} | base64`; + const result = await runChecked(`read ${remotePath}`, command); + const bytes = Buffer.from(result.stdout.replace(/\s+/g, ""), "base64"); + if (maxBytes !== undefined && bytes.length > maxBytes) throw new Error("Bridge envelope exceeded the configured size limit."); + return bytes.toString("utf8"); }, writeTextFile: async (remotePath, body) => { const remoteDir = path.posix.dirname(remotePath); @@ -792,12 +820,12 @@ export async function startSandboxCallbackBridgeWorker(input: { // not strand with no response. A handler that ignores the signal keeps its // earlier behavior. handleRequest: ( - request: SandboxCallbackBridgeRequest, - options?: { signal: AbortSignal }, + request: Omit & { body: string | Buffer }, + options?: { signal: AbortSignal; reservation: BridgeBodyReservation }, ) => Promise<{ status: number; headers?: Record; - body?: string; + body?: string | Buffer; }>; maxBodyBytes?: number | null; // Return the current-run parent-context token. The worker reads it per request @@ -821,6 +849,10 @@ export async function startSandboxCallbackBridgeWorker(input: { DEFAULT_BRIDGE_ABORTED_HANDLER_GRACE_MS, ); const maxBodyBytes = normalizeTimeoutMs(input.maxBodyBytes, DEFAULT_BRIDGE_MAX_BODY_BYTES); + const maxEnvelopeBytes = sandboxBridgeEnvelopeLimit(maxBodyBytes); + // Load lazily to avoid the HTTP/2 module's constants depending on this module + // during initialization. Both transports share the host process memory ceiling. + const { createBridgeBodyReservation } = await import("./http2-bridge-server.js"); const directories = sandboxCallbackBridgeDirectories(input.queueDir); const queueDirectories = [ directories.rootDir, @@ -924,6 +956,8 @@ export async function startSandboxCallbackBridgeWorker(input: { finalized: false, }; inFlightRequestGuards.set(fileName, guard); + const reservation = createBridgeBodyReservation(); + const envelopeReadReservation = createBridgeBodyReservation(); // Claim the request for the handler. Return `false` when the recovery path // already claimed it; the caller must then not run the mutation and must not // write a response, because the recovery path writes a 503 and the caller @@ -993,9 +1027,33 @@ export async function startSandboxCallbackBridgeWorker(input: { await writeAbortedHandlerBackstop(fileName, guard, lastWriteError); }; try { + // Bound the read and its encoded/parsed copies before allocating. Built-in + // clients stat first so a high configured file limit does not reserve that + // whole limit for a tiny request. Older clients use the conservative cap. + const envelopeBytes = input.client.fileSize + ? await input.client.fileSize(requestPath).catch(() => maxEnvelopeBytes) + : maxEnvelopeBytes; + if (envelopeBytes > maxEnvelopeBytes) { + await finalize({ + id: fileName.replace(/\.json$/i, ""), status: 413, + headers: { "content-type": "application/json" }, + body: JSON.stringify({ error: "Bridge request envelope exceeded the configured size limit." }), + completedAt: new Date().toISOString(), + }); + return; + } + const readLimit = Number.isSafeInteger(envelopeBytes) && envelopeBytes >= 0 + ? Math.min(envelopeBytes, maxEnvelopeBytes) : maxEnvelopeBytes; + if (!envelopeReadReservation.reserve(6 * readLimit)) { + await finalize({ id: fileName.replace(/\.json$/i, ""), status: 503, + headers: { "content-type": "application/json" }, + body: JSON.stringify({ error: "Bridge host body capacity is busy. Retry later." }), + completedAt: new Date().toISOString() }); + return; + } let raw: string; try { - raw = await input.client.readTextFile(requestPath); + raw = await input.client.readTextFile(requestPath, readLimit); } catch (error) { // The gateway deletes a request file when its caller stops waiting // (client-side timeout cleanup). A read that fails because the file is @@ -1011,7 +1069,9 @@ export async function startSandboxCallbackBridgeWorker(input: { } let request: SandboxCallbackBridgeRequest; try { + if (Buffer.byteLength(raw) > maxEnvelopeBytes) throw new Error("Bridge envelope too large"); request = JSON.parse(raw) as SandboxCallbackBridgeRequest; + decodeSandboxBridgeBody(request, maxBodyBytes); } catch { const requestId = fileName.replace(/\.json$/i, "") || randomUUID(); await finalize({ @@ -1024,6 +1084,16 @@ export async function startSandboxCallbackBridgeWorker(input: { return; } + // Keep only the actual request allocation reserved while forwarding; + // an abandoned small request must not retain a maximum-sized reservation. + envelopeReadReservation.release(); + if (!reservation.reserve(4 * Buffer.byteLength(raw) + 2 * Buffer.byteLength(request.body))) { + await finalize({ id: request.id, status: 503, + headers: { "content-type": "application/json" }, + body: JSON.stringify({ error: "Bridge host body capacity is busy. Retry later." }), + completedAt: new Date().toISOString() }); + return; + } const denialReason = await authorizeRequest(request); if (denialReason) { await finalize({ @@ -1048,17 +1118,24 @@ export async function startSandboxCallbackBridgeWorker(input: { // Build the response, then finalize once. The handler already holds the // claim, so `finalize` writes the real response. let response: SandboxCallbackBridgeResponse; + let handlerReturned = false; try { - const result = await input.handleRequest(request, { signal: guard.controller.signal }); - const responseBody = result.body ?? ""; - if (Buffer.byteLength(responseBody, "utf8") > maxBodyBytes) { - throw new Error(`Bridge response body exceeded the configured size limit of ${maxBodyBytes} bytes.`); + const { bodyEncoding, ...forwardRequest } = request; + const result = await input.handleRequest({ ...forwardRequest, + body: bodyEncoding === "base64" ? decodeSandboxBridgeBody(request, maxBodyBytes) : request.body, + }, { signal: guard.controller.signal, reservation }); + handlerReturned = true; + const responseBytes = Buffer.byteLength(result.body ?? ""); + if (responseBytes > maxBodyBytes) throw new Error("Bridge response body exceeded the configured size limit."); + if (!reservation.reserve(4 * sandboxBridgeEnvelopeLimit(responseBytes))) { + throw new Error("Bridge host response body capacity is busy."); } + const responseBody = encodeSandboxBridgeBody(result.body ?? "", maxBodyBytes); response = { id: request.id, status: result.status, headers: result.headers ?? {}, - body: responseBody, + ...responseBody, completedAt: new Date().toISOString(), }; } catch (error) { @@ -1075,7 +1152,7 @@ export async function startSandboxCallbackBridgeWorker(input: { // the outcome indeterminate. The caller must not retry a 504 from the // bridge, unlike the retry-safe 503 that the recovery path writes only // before the host operation starts. - if (guard.controller.signal.aborted) { + if (guard.controller.signal.aborted || (handlerReturned && !["GET", "HEAD", "OPTIONS", "TRACE"].includes(request.method))) { response = { id: request.id, status: 504, @@ -1104,6 +1181,8 @@ export async function startSandboxCallbackBridgeWorker(input: { } await finalize(response); } finally { + envelopeReadReservation.release(); + reservation.release(); // Drop the guard only when it still points to this attempt. A retry can // register a new attempt under the same file name; that new guard must // stay in the map. Keep the guard when a backstop is still pending: a @@ -2201,8 +2280,9 @@ ${DUPLEX_GATEWAY_CODEC_SOURCE} // use it: readBodyBytes reserves against it, and each mode's request // handler releases what it reserved once the body is no longer needed. ${BRIDGE_PROCESS_BODY_LEDGER_SOURCE} +${sandboxBridgeBodyCodecSource()} -// The multiplier matches HTTP2_BRIDGE_MAX_CONCURRENT_STREAMS (4) in +// HTTP/2's multiplier matches HTTP2_BRIDGE_MAX_CONCURRENT_STREAMS (4) in // http2-bridge-server.ts, doubled because readBodyBytes reserves a body's // bytes twice: once for the retained chunk array, once for the concatenated // copy, since both are live buffers at once. This gives the gateway process @@ -2210,8 +2290,11 @@ ${BRIDGE_PROCESS_BODY_LEDGER_SOURCE} // concurrent requests cannot grow this process's memory without limit, even // though every individual body already passes the maxBodyBytes check below. // This ceiling is independent of, and separate from, the ceiling the host -// enforces on its own side of the bridge connection. -const maxProcessBodyBytes = maxBodyBytes * 8; +// enforces on its own side of the bridge connection. Queue mode also counts +// JSON/base64 envelopes; its aggregate ceiling never exceeds 1 GiB. +const maxProcessBodyBytes = bridgeMode === "${SANDBOX_CALLBACK_BRIDGE_FILE_MODE}" + ? Math.min(1024 * 1024 * 1024, 4 * (4 * sandboxBridgeEnvelopeLimit(maxBodyBytes) + 4 * maxBodyBytes)) + : maxBodyBytes * 8; const processBodyLedger = createBridgeProcessBodyLedger(maxProcessBodyBytes); // A denied process-ledger reservation answers 503: the sandbox client should @@ -2280,11 +2363,6 @@ async function readBodyBytes(req) { } } -async function readBody(req) { - const { body, release } = await readBodyBytes(req); - return { body: body.toString("utf8"), release }; -} - function tokensMatch(received) { const expected = Buffer.from(bridgeToken, "utf8"); const actual = Buffer.from(typeof received === "string" ? received : "", "utf8"); @@ -2327,14 +2405,32 @@ async function runFileGateway() { } } - async function waitForResponse(requestId) { + async function waitForResponse(requestId, reserveResponse) { const responsePath = path.posix.join(responsesDir, \`\${requestId}.json\`); const deadline = Date.now() + responseTimeoutMs; while (Date.now() < deadline) { - const body = await fs.readFile(responsePath, "utf8").catch(() => null); - if (body != null) { - await fs.rm(responsePath, { force: true }).catch(() => undefined); - return JSON.parse(body); + const handle = await fs.open(responsePath, "r").catch(error => { + if (error.code === "ENOENT") return null; + throw error; + }); + if (handle) { + try { + const stat = await handle.stat(); + if (stat.size > sandboxBridgeEnvelopeLimit(maxBodyBytes)) throw new Error("Bridge response envelope exceeded the configured size limit."); + reserveResponse(6 * stat.size + 1); + const bytes = Buffer.alloc(stat.size + 1); + let length = 0; + while (length < bytes.length) { + const read = await handle.read(bytes, length, bytes.length - length, length); + if (!read.bytesRead) break; + length += read.bytesRead; + } + if (length !== stat.size) throw new Error("Bridge response envelope changed while reading."); + return JSON.parse(bytes.subarray(0, length).toString("utf8")); + } finally { + await handle.close(); + await fs.rm(responsePath, { force: true }).catch(() => undefined); + } } await sleep(pollIntervalMs); } @@ -2342,12 +2438,14 @@ async function runFileGateway() { } const server = createServer(async (req, res) => { - // readBody reserves the body's bytes against the process ledger and + // readBodyBytes reserves the body's bytes against the process ledger and // hands back a release function; this holds it so the finally below // releases those bytes exactly once no matter how this handler ends — // its normal completion, a thrown error, a client abort, or a deadline // timeout all reach the same finally. let releaseBodyReservation = null; + let envelopeReservation = 0; + let dispatched = false; try { const auth = req.headers.authorization || ""; const receivedToken = auth.startsWith("Bearer ") ? auth.slice("Bearer ".length) : ""; @@ -2368,30 +2466,42 @@ async function runFileGateway() { const url = new URL(req.url || "/", "http://127.0.0.1"); const contentType = typeof req.headers["content-type"] === "string" ? req.headers["content-type"] : ""; - if (req.method && req.method !== "GET" && req.method !== "HEAD" && !/json/i.test(contentType)) { + const multipartAttachment = req.method === "POST" + && /^\\/api\\/companies\\/[^/]+\\/issues\\/[^/]+\\/attachments$/.test(url.pathname) + && /^multipart\\/form-data(?:;|$)/i.test(contentType); + if (req.method && req.method !== "GET" && req.method !== "HEAD" && !/json/i.test(contentType) && !multipartAttachment) { writeJsonResponse(res, 415, { error: "Bridge only accepts JSON request bodies." }); return; } const requestId = randomUUID(); - const { body: requestBody, release } = await readBody(req); + // Reserve encoded envelopes and response decoding separately from the + // incoming byte buffers. Capacity rejection precedes dispatch, hence 503. + const { body: requestBody, release } = await readBodyBytes(req); releaseBodyReservation = release; + const reserveBytes = 4 * sandboxBridgeEnvelopeLimit(requestBody.length); + if (!processBodyLedger.reserve(reserveBytes)) throw new BridgeProcessCapacityError(); + envelopeReservation = reserveBytes; const payload = { id: requestId, method: req.method || "GET", path: url.pathname, query: url.search, headers: normalizeHeaders(req.headers), - body: requestBody, + ...encodeSandboxBridgeBody(multipartAttachment ? requestBody : requestBody.toString("utf8"), maxBodyBytes), createdAt: new Date().toISOString(), }; const requestPath = path.posix.join(requestsDir, \`\${requestId}.json\`); const tempPath = \`\${requestPath}.tmp\`; await fs.writeFile(tempPath, \`\${JSON.stringify(payload)}\\n\`, "utf8"); + dispatched = true; await fs.rename(tempPath, requestPath); let response; try { - response = await waitForResponse(requestId); + response = await waitForResponse(requestId, bytes => { + if (!processBodyLedger.reserve(bytes)) throw new BridgeProcessCapacityError(); + envelopeReservation += bytes; + }); } catch (error) { // The host never delivered a response inside the deadline. Remove this // request's file so it cannot pile up toward the queue-depth cap. The @@ -2420,15 +2530,20 @@ async function runFileGateway() { if (typeof value !== "string" || key.toLowerCase() === "content-length") continue; res.setHeader(key, value); } - res.end(typeof response.body === "string" ? response.body : ""); + res.end(decodeSandboxBridgeBody(response, maxBodyBytes)); } catch (error) { - // A denied process-ledger reservation is retryable: the caller should - // try again once other in-flight bodies release their bytes. Every - // other body-read or handling fault stays a generic 502. - const status = error instanceof BridgeProcessCapacityError ? 503 : 502; - writeJsonResponse(res, status, { error: error instanceof Error ? error.message : String(error) }); + // Capacity rejection before dispatch is retryable. After a mutation was + // dispatched, a missing or corrupt receipt cannot prove that it failed. + const uncertainWrite = dispatched && !["GET", "HEAD", "OPTIONS", "TRACE"].includes(req.method || "GET"); + const status = uncertainWrite ? 409 : error instanceof BridgeProcessCapacityError ? 503 : 502; + if (uncertainWrite) res.setHeader("x-paperclip-bridge-outcome", "indeterminate"); + writeJsonResponse(res, status, { + error: error instanceof Error ? error.message : String(error), + ...(uncertainWrite ? { outcome: "indeterminate", retryable: false } : {}), + }); } finally { releaseBodyReservation?.(); + processBodyLedger.release(envelopeReservation); } }); diff --git a/server/src/__tests__/file-delivery-bridges.test.ts b/server/src/__tests__/file-delivery-bridges.test.ts new file mode 100644 index 00000000000..16ab0b6ad18 --- /dev/null +++ b/server/src/__tests__/file-delivery-bridges.test.ts @@ -0,0 +1,176 @@ +import { spawn } from "node:child_process"; +import { createHash } from "node:crypto"; +import { readFile, rm } from "node:fs/promises"; +import path from "node:path"; +import { eq } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { agents, heartbeatRuns, issueAttachments, issueComments, issues, issueWorkProducts } from "@paperclipai/db"; +import { startAdapterExecutionTargetPaperclipBridge, type AdapterSandboxExecutionTarget } from "@paperclipai/adapter-utils/execution-target"; +import type { CommandManagedRuntimeRunner } from "@paperclipai/adapter-utils/command-managed-runtime"; +import { runChildProcess } from "@paperclipai/adapter-utils/server-utils"; +import { createLocalAgentJwt } from "../agent-auth-jwt.js"; +import { PaperclipRunnerToolAuthority } from "../services/native-runtime/paperclip-runner-tool-authority.js"; +import { readVerifiedRemoteWorkspaceFile } from "../services/native-runtime/remote-deliverable-file.js"; +import { startFileDeliveryDaytona } from "./helpers/file-delivery-daytona.js"; +import { startRunnerApiTestServer } from "./helpers/runner-api-server.js"; + +const liveDaytona = process.env.PAPERCLIP_FILE_DELIVERY_DAYTONA === "1"; +const helper = path.resolve(import.meta.dirname, "../../../skills/paperclip/scripts/paperclip-upload-artifact.sh"); +// A real PNG and a PDF with binary bytes in its comment exercise UTF-8 corruption. +const files = [ + { name: "猫 picture.png", type: "image/png", body: Buffer.from("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+aK1sAAAAASUVORK5CYII=", "base64") }, + { name: "résumé report.pdf", type: "application/pdf", body: Buffer.concat([Buffer.from("%PDF-1.4\n%"), Buffer.from([255, 0, 128, 254]), Buffer.from("\n1 0 obj <> endobj\n%%EOF\n")]) }, +]; + +function localProcessRunner(): CommandManagedRuntimeRunner { + return { + execute: input => runChildProcess("file-delivery-test", input.command, input.args ?? [], { + cwd: input.cwd ?? process.cwd(), env: input.env ?? {}, stdin: input.stdin, + timeoutSec: (input.timeoutMs ?? 30_000) / 1000, graceSec: 1, onLog: async () => {}, + }), + openDuplexChannel: async ({ command }) => { + const child = spawn(command[0], command.slice(1), { stdio: ["pipe", "pipe", "pipe"] }); + child.stderr.resume(); + return { + write: data => { child.stdin.write(data); }, + onData: listener => { child.stdout.on("data", listener); }, + onExit: listener => { child.once("exit", exitCode => listener({ exitCode })); }, + stop: () => { child.kill(); }, + close: async () => { child.stdin.end(); child.kill(); }, + }; + }, + }; +} + +describe(`durable file delivery (${liveDaytona ? "Daytona" : "local processes"})`, () => { + let server: Awaited>; + const cleanup: Array<() => Promise> = []; + afterEach(async () => { for (const close of cleanup.splice(0).reverse()) await close(); }); + async function execution(localWorkspace: string) { + const live = liveDaytona ? await startFileDeliveryDaytona() : null; + const workspace = live?.workspace ?? localWorkspace; + const runner = live?.runner ?? localProcessRunner(); + let closed = false; + const close = async () => { if (closed) return; closed = true; await live?.close(); }; + cleanup.push(close); + const run = async (command: string, args: string[], env: Record = {}) => { + const result = await runner.execute({ command, args, env, timeoutMs: 90_000 }); + expect(result.exitCode, result.stderr).toBe(0); + return result.stdout; + }; + const write = async (relative: string, body: Buffer) => { + const destination = path.join(workspace, relative); + await run("node", ["-e", "const fs=require('fs');fs.mkdirSync(require('path').dirname(process.argv[1]),{recursive:true});fs.writeFileSync(process.argv[1],Buffer.from(process.argv[2],'base64'))", destination, body.toString("base64")]); + return destination; + }; + const fetchFromWorkspace = async (url: string, headers: Record) => { + const output = await run("node", ["-e", "fetch(process.argv[1],{headers:JSON.parse(process.argv[2])}).then(async r=>console.log(JSON.stringify({status:r.status,body:Buffer.from(await r.arrayBuffer()).toString('base64')})))", url, JSON.stringify(headers)]); + const response = JSON.parse(output); + return new Response(Buffer.from(response.body, "base64"), { status: response.status }); + }; + return { workspace, runner, close, run, write, fetch: fetchFromWorkspace }; + } + beforeAll(async () => { + vi.stubEnv("PAPERCLIP_AGENT_JWT_SECRET", "isolated-file-delivery-test-secret"); + server = await startRunnerApiTestServer(); + }); + afterAll(async () => { await server?.close(); vi.unstubAllEnvs(); }); + + it.each([false, true])("runs the actual Bash helper with HTTP/2 enabled=%s", async duplex => { + const fixture = await server.fixture({ apiToolsEnabled: false }); + await server.db.update(agents).set({ adapterType: "codex_local" }).where(eq(agents.id, fixture.agentId)); + await server.db.update(heartbeatRuns).set({ runtimeMode: "legacy" }).where(eq(heartbeatRuns.id, fixture.runId)); + const token = createLocalAgentJwt(fixture.agentId, fixture.companyId, "codex_local", fixture.runId)!; + const remote = await execution(fixture.workspace); + const installedHelper = await remote.write("paperclip-upload-artifact.sh", await readFile(helper)); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", transport: "sandbox", providerKey: liveDaytona ? "daytona" : "local-test", remoteCwd: remote.workspace, + runner: remote.runner, timeoutMs: 30_000, + effectiveCapabilities: { + reusableLeases: false, nativeSyncIn: false, nativeSyncOut: false, + persistentProcessSessions: false, independentControlCommands: false, + incrementalSessionOutput: false, concurrentSyncOperations: false, + duplexCommandStream: true, runnerWebSocketIngress: false, + }, + }; + const bridge = await startAdapterExecutionTargetPaperclipBridge({ + runId: fixture.runId, target, runtimeRootDir: path.join(remote.workspace, ".runtime"), + adapterKey: "codex", hostApiToken: token, hostApiUrl: server.apiUrl, + enableSandboxDuplexBridge: duplex, + }); + expect(bridge?.env.PAPERCLIP_API_BRIDGE_MODE).toBe(duplex ? "http2_v1" : "queue_v1"); + const env = { ...bridge!.env, PAPERCLIP_RUN_ID: fixture.runId, + PAPERCLIP_COMPANY_ID: fixture.companyId, PAPERCLIP_TASK_ID: fixture.issueId, + PAPERCLIP_HELPER_STATE_DIR: path.join(remote.workspace, ".helper-state") }; + const headers = { authorization: `Bearer ${bridge!.env.PAPERCLIP_API_KEY}` }; + const receipts: Array<{ attachment: { id: string; contentPath: string; downloadPath: string } }> = []; + try { + for (const file of files) { + const local = await remote.write(file.name, file.body); + const args = [installedHelper, local, "--content-type", file.type, "--title", file.name, "--chat-comment", "Requested files", "--output", "json"]; + const receipt = JSON.parse(await remote.run("bash", args, env)); + const duplicate = JSON.parse(await remote.run("bash", args, env)); + expect(duplicate.attachment.id).toBe(receipt.attachment.id); + expect(duplicate.workProduct.id).toBe(receipt.workProduct.id); + expect(duplicate.chatComment.id).toBe(receipt.chatComment.id); + expect(receipt.attachment.originalFilename).toBe(file.name); + expect(receipt.attachment.originatingRunId).toBe(fixture.runId); + const download = await remote.fetch(bridge!.env.PAPERCLIP_API_URL + receipt.attachment.downloadPath, headers); + expect(download.status).toBe(200); + expect(Buffer.from(await download.arrayBuffer())).toEqual(file.body); + receipts.push(receipt); + } + const foreign = await server.fixture(); + const denied = await remote.fetch(`${bridge!.env.PAPERCLIP_API_URL}/api/issues/${foreign.issueId}/attachments`, headers); + expect(denied.status).toBe(404); + const unauthorized = await remote.fetch(`${bridge!.env.PAPERCLIP_API_URL}/api/issues/${fixture.issueId}/attachments`, { authorization: "Bearer wrong-token" }); + expect(unauthorized.status).toBe(401); + expect(await server.db.select().from(issueAttachments).where(eq(issueAttachments.issueId, fixture.issueId))).toHaveLength(2); + expect(await server.db.select().from(issueWorkProducts).where(eq(issueWorkProducts.issueId, fixture.issueId))).toHaveLength(2); + expect(await server.db.select().from(issueComments).where(eq(issueComments.issueId, fixture.issueId))).toHaveLength(1); + } finally { await bridge?.stop(); await remote.close(); } + // The workspace and runner are gone; a fresh request still reads persisted bytes. + await rm(fixture.workspace, { recursive: true, force: true }); + for (const [index, receipt] of receipts.entries()) { + const download = await fetch(server.apiUrl + receipt.attachment.contentPath, { headers: { authorization: `Bearer ${token}` } }); + expect(download.status).toBe(200); + expect(Buffer.from(await download.arrayBuffer())).toEqual(files[index].body); + } + }, liveDaytona ? 240_000 : 60_000); + + it("registers native files with API tools disabled and returns durable download receipts", async () => { + const fixture = await server.fixture({ apiToolsEnabled: false }); + const remote = await execution(fixture.workspace); + const authority = new PaperclipRunnerToolAuthority(server.db, { ...fixture, + workspaceRoot: remote.workspace, executionTargetKind: liveDaytona ? "remote" : "local", + readRemoteWorkspaceFile: liveDaytona ? request => readVerifiedRemoteWorkspaceFile({ ...request, runner: remote.runner, workspaceRoot: remote.workspace }) : undefined, + }); + expect(authority.definitions().some(definition => definition.name === "register_deliverable")).toBe(true); + expect(authority.definitions().some(definition => definition.name === "api_post")).toBe(false); + const token = createLocalAgentJwt(fixture.agentId, fixture.companyId, "paperclip_runner", fixture.runId)!; + const receipts: Array<{ downloadPath: string }> = []; + for (const file of files) { + await remote.write(`out/${file.name}`, file.body); + const call = { tool: "register_deliverable", callId: file.name, arguments: { + contentRef: `out/${file.name}`, filename: file.name, contentType: file.type, byteSize: file.body.length, + sha256: createHash("sha256").update(file.body).digest("hex"), title: file.name, idempotencyKey: file.name, + } }; + const receipt = await authority.execute(call) as { attachmentId: string; contentPath: string; downloadPath: string; entityRefs: string[] }; + expect(receipt.attachmentId).toBe(receipt.entityRefs[0]); + expect(await authority.execute(call)).toEqual(receipt); + receipts.push(receipt); + } + await remote.close(); + await rm(fixture.workspace, { recursive: true, force: true }); + for (const [index, receipt] of receipts.entries()) { + const download = await fetch(server.apiUrl + receipt.downloadPath, { headers: { authorization: `Bearer ${token}` } }); + expect(download.status).toBe(200); + expect(Buffer.from(await download.arrayBuffer())).toEqual(files[index].body); + } + const rows = await server.db.select().from(issueAttachments).where(eq(issueAttachments.issueId, fixture.issueId)); + expect(rows).toHaveLength(2); + expect(rows.every(row => row.originatingRunId === fixture.runId && row.issueCommentId)).toBe(true); + await server.db.update(issues).set({ executionRunId: null }).where(eq(issues.id, fixture.issueId)); + await expect(authority.execute({ tool: "register_deliverable", callId: "stale", arguments: {} })).rejects.toThrow(); + }, liveDaytona ? 240_000 : 15_000); +}); diff --git a/server/src/__tests__/helpers/file-delivery-daytona.ts b/server/src/__tests__/helpers/file-delivery-daytona.ts new file mode 100644 index 00000000000..e505c5ced6b --- /dev/null +++ b/server/src/__tests__/helpers/file-delivery-daytona.ts @@ -0,0 +1,59 @@ +import { createRequire } from "node:module"; +import type { CommandManagedRuntimeRunner } from "@paperclipai/adapter-utils/command-managed-runtime"; +import { shellQuote } from "@paperclipai/adapter-utils/ssh"; +import { openDaytonaDuplexChannelSession, type DaytonaPtyProcess } from "../../../../packages/plugins/sandbox-providers/daytona/src/duplex-command-stream.js"; + +/** Opt-in live fixture; uses an isolated sandbox and never reuses a user's lease. */ +export async function startFileDeliveryDaytona() { + const require = createRequire(new URL("../../../../packages/plugins/sandbox-providers/daytona/package.json", import.meta.url)); + const { Daytona } = require("@daytonaio/sdk"); + const sandbox: { + id: string; + process: DaytonaPtyProcess & { executeCommand(command: string, cwd?: string, env?: Record, timeout?: number): Promise<{ exitCode: number; result: string }> }; + delete(): Promise; + } = await new Daytona({ apiKey: process.env.DAYTONA_API_KEY }).create({ + language: "typescript", autoStopInterval: 10, + labels: { "paperclip-test": "gus-file-delivery" }, + }); + const workspace = "/home/daytona/gus-file-delivery"; + // Capture stdout/stderr separately: SDK result text otherwise combines them. + const executeSource = ` + const {spawn} = require('node:child_process'); + const input = JSON.parse(process.argv[1]); + const child = spawn(input.command, input.args || [], { + cwd: input.cwd, env: {...process.env, ...input.env}, timeout: input.timeoutMs || 30000 + }); + const stdout = [], stderr = []; + child.stdout.on('data', chunk => stdout.push(chunk)); + child.stderr.on('data', chunk => stderr.push(chunk)); + child.on('error', error => stderr.push(Buffer.from(error.message))); + child.on('close', (exitCode, signal) => process.stdout.write(JSON.stringify({ + exitCode, signal, timedOut: signal === 'SIGTERM', pid: null, + startedAt: new Date().toISOString(), + stdout: Buffer.concat(stdout).toString(), stderr: Buffer.concat(stderr).toString() + }))); + child.stdin.on('error', error => { if (error.code !== 'EPIPE') stderr.push(Buffer.from(error.message)); }); + child.stdin.end(input.stdin || ''); + `; + const runner: CommandManagedRuntimeRunner = { + execute: async input => { + const command = `node --input-type=commonjs -e ${shellQuote(executeSource)} ${shellQuote(JSON.stringify(input))}`; + const result = await sandbox.process.executeCommand(command, undefined, undefined, Math.ceil((input.timeoutMs ?? 30000) / 1000) + 10); + if (result.exitCode !== 0) throw new Error(`Daytona test command transport failed (exit ${result.exitCode})`); + return JSON.parse(result.result); + }, + openDuplexChannel: async ({ command }) => { + const session = await openDaytonaDuplexChannelSession(sandbox.process, command); + return { + write: bytes => session.write(bytes), onData: listener => session.onData(listener), + onExit: listener => { void session.wait().then(listener); }, + stop: () => session.kill(), close: () => session.close(), + }; + }, + }; + try { + const setup = await runner.execute({ command: "bash", args: ["-lc", `mkdir -p ${shellQuote(workspace)} && command -v node && command -v curl && command -v jq`], timeoutMs: 30000 }); + if (setup.exitCode !== 0) throw new Error("Daytona test image requires node, curl, and jq"); + return { workspace, runner, close: () => sandbox.delete() }; + } catch (error) { await sandbox.delete(); throw error; } +} diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts index f05470d2d97..d3bb4380016 100644 --- a/server/src/__tests__/issue-attachment-routes.test.ts +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -589,6 +589,46 @@ describe("issue attachment routes", () => { ]).toContain(res.headers["content-disposition"]); }); + it.each([ + { + filename: '猫 "chart"; 100%.png', + contentType: "image/png", + filenameParameters: String.raw`filename="? \"chart\"; 100%.png"; filename*=UTF-8''%E7%8C%AB%20%22chart%22%3B%20100%25.png`, + }, + { + filename: 'report "final"; 100%.pdf', + contentType: "application/pdf", + filenameParameters: String.raw`filename="report \"final\"; 100%.pdf"`, + }, + ].flatMap((file) => [false, true].flatMap((download) => + [false, true].map((range) => ({ ...file, download, range })), + )))("preserves disposition filenames: $filename (download=$download, range=$range)", async ({ + filename, contentType, filenameParameters, download, range, + }) => { + const bytes = Buffer.from([0, 255, 128, 10, 13, 42]); + const storage = createStorageService(bytes); + mockIssueService.getAttachmentById.mockResolvedValue({ + ...makeAttachment(contentType, filename), + byteSize: bytes.length, + }); + const app = await createApp(storage); + const downloadRequest = request(app) + .get(`/api/attachments/attachment-1/content${download ? "?download=1" : ""}`) + .buffer(true) + .parse(parseBinaryResponse); + if (range) downloadRequest.set("Range", "bytes=1-3"); + const res = await downloadRequest; + + expect(res.status).toBe(range ? 206 : 200); + expect(res.headers["content-disposition"]).toBe(`${download ? "attachment" : "inline"}; ${filenameParameters}`); + expect(res.headers["content-type"]).toBe(contentType); + expect(res.headers["content-length"]).toBe(String(range ? 3 : bytes.length)); + expect(res.headers["accept-ranges"]).toBe("bytes"); + expect(res.headers["content-range"]).toBe(range ? "bytes 1-3/6" : undefined); + expect(res.headers["x-content-type-options"]).toBe("nosniff"); + expect(res.body).toEqual(range ? bytes.subarray(1, 4) : bytes); + }); + it("serves video attachments inline with byte-range support", async () => { const storage = createStorageService(Buffer.from("abcdef")); mockIssueService.getAttachmentById.mockResolvedValue({ diff --git a/server/src/__tests__/paperclip-skill-utils.test.ts b/server/src/__tests__/paperclip-skill-utils.test.ts index 10da7042743..c2127acac00 100644 --- a/server/src/__tests__/paperclip-skill-utils.test.ts +++ b/server/src/__tests__/paperclip-skill-utils.test.ts @@ -23,7 +23,7 @@ async function makeArtifactHelperHarness( cleanupDirs: Set, options: { apiUrl?: string; - ambiguousHttpStatusAfterCommit?: "408" | "502"; + ambiguousHttpStatusAfterCommit?: "408" | "409" | "502"; commitAfterDropDelaySeconds?: string; dropFirstUpload?: boolean; dropWithoutCommit?: boolean; @@ -151,7 +151,7 @@ if [[ "$method" == "POST" && "$url" == */attachments ]]; then printf '%s' "$PAPERCLIP_RUN_ID" >"$FAKE_CURL_STATE_DIR/originating-run-id" if [[ -n "\${FAKE_CURL_AMBIGUOUS_STATUS:-}" && ! -f "$FAKE_CURL_STATE_DIR/upload-status-used" ]]; then : >"$FAKE_CURL_STATE_DIR/upload-status-used" - respond '{"error":"ambiguous upstream response"}' "$FAKE_CURL_AMBIGUOUS_STATUS" + respond '{"error":"ambiguous upstream response","outcome":"indeterminate","retryable":false}' "$FAKE_CURL_AMBIGUOUS_STATUS" exit 0 fi if [[ "\${FAKE_CURL_MALFORMED_SUCCESS:-0}" == "1" && ! -f "$FAKE_CURL_STATE_DIR/malformed-success-used" ]]; then @@ -311,7 +311,7 @@ describe("paperclip skill utils", () => { expect(normalizedShortcut).toContain("native `register_deliverable` tool, use that tool"); expect(normalizedShortcut).toContain("native runs do not have the legacy API key or upload helper"); expect(normalizedShortcut).toContain( - "For non-native adapters, invoke `scripts/paperclip-upload-artifact.sh` directly", + "For non-native adapters, invoke `bash scripts/paperclip-upload-artifact.sh`", ); expect(normalizedShortcut).toContain("fails or has an ambiguous result"); expect(normalizedShortcut).toContain("use the full heartbeat procedure below"); @@ -336,7 +336,7 @@ describe("paperclip skill utils", () => { expect(await fs.readFile(path.join(harness.stateDir, "upload-count"), "utf8")).toBe("1"); }); - it.each(["408", "502"] as const)( + it.each(["408", "409", "502"] as const)( "keeps upload ambiguity after an HTTP %s until the immutable attachment is observed", async (status) => { const harness = await makeArtifactHelperHarness(cleanupDirs, { diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 716b334588a..1436aecc304 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -4855,6 +4855,8 @@ export function issueRoutes( ) { const upload = multer({ storage: multer.memoryStorage(), + // Curl and browser FormData send unlabelled filenames as UTF-8. + defParamCharset: "utf8", limits: { fileSize: fileSizeLimit, files: 1 }, }); await new Promise((resolve, reject) => { @@ -18567,6 +18569,8 @@ export function issueRoutes( contentType: responseContentType, originalFilename: attachment.originalFilename, }); + // Express formats filenames with an encoded Unicode parameter when needed. + res.attachment(attachment.originalFilename ?? "attachment"); res.setHeader( "Content-Type", isMarkdownResponse @@ -18581,7 +18585,6 @@ export function issueRoutes( "sandbox; default-src 'none'; img-src 'self' data:; style-src 'unsafe-inline'", ); } - const filename = attachment.originalFilename ?? "attachment"; const disposition = parseBooleanQuery(req.query.download) ? "attachment" : isInlineAttachmentContentType(responseContentType) @@ -18589,7 +18592,7 @@ export function issueRoutes( : "attachment"; res.setHeader( "Content-Disposition", - `${disposition}; filename=\"${filename.replaceAll('"', "")}\"`, + String(res.getHeader("Content-Disposition")).replace(/^attachment;/, `${disposition};`), ); object.stream.on("error", (err) => { diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 27a32e95b23..9321867d358 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -8569,7 +8569,7 @@ export function buildPaperclipTaskMarkdown(input: { lines.push( "", "External chat file delivery:", - "When asked to send an image or file back to this chat, use the bundled Paperclip artifact helper `scripts/paperclip-upload-artifact.sh --chat-comment ` with the local file. Resolve the helper from the installed skill location, not the task workspace. This selects the uploaded file for Paperclip's final-response delivery; an upload or artifact record alone does not. For ordinary file handoffs the helper is the direct path; consult the skill's artifact reference for advanced options, missing tooling, failures, or ambiguous results. Do not search for a separate provider tool connection or fetch a CLI with `npx` to send chat files. Bind only the files the user asked to share, and do not claim provider delivery merely because binding succeeded. GitHub uses task links/notices rather than native file uploads.", + "When asked to send an image or file back to this chat, use the bundled Paperclip artifact helper `bash scripts/paperclip-upload-artifact.sh --chat-comment ` with the local file. Resolve the helper from the installed skill location, not the task workspace. This selects the uploaded file for Paperclip's final-response delivery; an upload or artifact record alone does not. For ordinary file handoffs the helper is the direct path; consult the skill's artifact reference for advanced options, missing tooling, failures, or ambiguous results. Do not search for a separate provider tool connection or fetch a CLI with `npx` to send chat files. Bind only the files the user asked to share, and do not claim provider delivery merely because binding succeeded. GitHub uses task links/notices rather than native file uploads.", "Prepare and validate the requested files together. Batch independent file preparation and one helper command per file into as few tool calls as practical. Use the same caption for files in one reply so their helper calls share one handoff comment. After a helper reports success, its attachment, artifact, and comment binding are already recorded: do not manually bind the same file again, re-list those records, or add a second handoff comment just to confirm success. Complete the required final-response protocol using the successful receipts. Retry or investigate only a failed or ambiguous step; never repeat a successful upload merely to confirm it.", ); } diff --git a/server/src/services/native-runtime/native-runner-file-handoff.test.ts b/server/src/services/native-runtime/native-runner-file-handoff.test.ts index fe5ce7b8341..c6cd2a0845e 100644 --- a/server/src/services/native-runtime/native-runner-file-handoff.test.ts +++ b/server/src/services/native-runtime/native-runner-file-handoff.test.ts @@ -285,11 +285,17 @@ describe("native runner file handoff", () => { stateRevision: 0, entityRefs: [expect.any(String), expect.any(String), expect.any(String)], scheduledWakeIds: [], + attachmentId: first.entityRefs[0], + contentPath: `/api/attachments/${first.entityRefs[0]}/content`, + downloadPath: `/api/attachments/${first.entityRefs[0]}/content?download=1`, }); expect(replay).toEqual(first); expect(replayWithNewKey).toMatchObject({ disposition: "duplicate", entityRefs: first.entityRefs, + attachmentId: first.attachmentId, + contentPath: first.contentPath, + downloadPath: first.downloadPath, }); await expect(nativeCompletionFeedback(db, runId, doneReport([`deliverable:${first.entityRefs[0]}`]))) diff --git a/server/src/services/native-runtime/native-runner-file-handoff.ts b/server/src/services/native-runtime/native-runner-file-handoff.ts index bb91506db01..c5132643f54 100644 --- a/server/src/services/native-runtime/native-runner-file-handoff.ts +++ b/server/src/services/native-runtime/native-runner-file-handoff.ts @@ -63,6 +63,9 @@ export interface NativeRunnerFileHandoffResult { readonly stateRevision: number; readonly entityRefs: readonly string[]; readonly scheduledWakeIds: readonly string[]; + readonly attachmentId: string; + readonly contentPath: string; + readonly downloadPath: string; } export interface PreparedNativeRunnerFileHandoff { @@ -1195,6 +1198,9 @@ export async function prepareNativeRunnerFileHandoff(input: { result: { commandId: `deliverable-prepared:${existing.attachmentId}`, disposition: "duplicate", + attachmentId: existing.attachmentId, + contentPath: `/api/attachments/${existing.attachmentId}/content`, + downloadPath: `/api/attachments/${existing.attachmentId}/content?download=1`, stateRevision: statusVersion, entityRefs: [ existing.attachmentId, @@ -1265,6 +1271,9 @@ export async function prepareNativeRunnerFileHandoff(input: { result: { commandId: `deliverable-prepared:${attachment.id}`, disposition: "applied", + attachmentId: attachment.id, + contentPath: `/api/attachments/${attachment.id}/content`, + downloadPath: `/api/attachments/${attachment.id}/content?download=1`, stateRevision: statusVersion, entityRefs: [ attachment.id, diff --git a/server/src/services/native-runtime/remote-deliverable-file.ts b/server/src/services/native-runtime/remote-deliverable-file.ts index a99c18824f1..db75f7eb7c6 100644 --- a/server/src/services/native-runtime/remote-deliverable-file.ts +++ b/server/src/services/native-runtime/remote-deliverable-file.ts @@ -2,7 +2,9 @@ import { createHash } from "node:crypto"; import { posix } from "node:path"; import type { CommandManagedRuntimeRunner } from "@paperclipai/adapter-utils/command-managed-runtime"; -export const MAX_REMOTE_DELIVERABLE_BYTES = 10 * 1024 * 1024; +import { MAX_ATTACHMENT_BYTES } from "../../attachment-types.js"; + +export const MAX_REMOTE_DELIVERABLE_BYTES = MAX_ATTACHMENT_BYTES; const PREFIX = "paperclip_runner_file_handoff_"; const READ_TIMEOUT_MS = 10_000; diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index 68bb62455b3..c6f0f7cf5e9 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -17,7 +17,7 @@ In Paperclip, **task** and **issue** refer to the same work item. The UI may use ## Authentication -Env vars auto-injected: `PAPERCLIP_AGENT_ID`, `PAPERCLIP_COMPANY_ID`, `PAPERCLIP_API_URL`, `PAPERCLIP_RUN_ID`. Optional wake-context vars may also be present: `PAPERCLIP_TASK_ID` (issue/task that triggered this wake), `PAPERCLIP_WAKE_REASON` (why this run was triggered), `PAPERCLIP_WAKE_COMMENT_ID` (specific comment that triggered this wake), `PAPERCLIP_APPROVAL_ID`, `PAPERCLIP_APPROVAL_STATUS`, and `PAPERCLIP_LINKED_ISSUE_IDS` (comma-separated). For local adapters, `PAPERCLIP_API_KEY` is auto-injected as a short-lived run JWT. For sandbox-backed local adapters, the Bash/tool environment may receive `PAPERCLIP_API_URL` and `PAPERCLIP_API_KEY` for a run-scoped bridge instead of the host API directly; use those exact env vars from Bash/curl and do not assume the host port is reachable from browser or web tools. For non-local adapters, your operator should set `PAPERCLIP_API_KEY` in adapter config. All requests use `Authorization: Bearer $PAPERCLIP_API_KEY`. All endpoints under `/api`, all JSON. Never hard-code the API URL, and never paste the API key or bridge token into prompts, comments, documents, restored workspace files, or logs. +Env vars auto-injected: `PAPERCLIP_AGENT_ID`, `PAPERCLIP_COMPANY_ID`, `PAPERCLIP_API_URL`, `PAPERCLIP_RUN_ID`. Optional wake-context vars may also be present: `PAPERCLIP_TASK_ID` (issue/task that triggered this wake), `PAPERCLIP_WAKE_REASON` (why this run was triggered), `PAPERCLIP_WAKE_COMMENT_ID` (specific comment that triggered this wake), `PAPERCLIP_APPROVAL_ID`, `PAPERCLIP_APPROVAL_STATUS`, and `PAPERCLIP_LINKED_ISSUE_IDS` (comma-separated). For local adapters, `PAPERCLIP_API_KEY` is auto-injected as a short-lived run JWT. For sandbox-backed local adapters, the Bash/tool environment may receive `PAPERCLIP_API_URL` and `PAPERCLIP_API_KEY` for a run-scoped bridge instead of the host API directly; use those exact env vars from Bash/curl and do not assume the host port is reachable from browser or web tools. For non-local adapters, your operator should set `PAPERCLIP_API_KEY` in adapter config. All requests use `Authorization: Bearer $PAPERCLIP_API_KEY`. All endpoints are under `/api`. Use JSON except for multipart attachment uploads and binary content downloads. Never hard-code the API URL, and never paste the API key or bridge token into prompts, comments, documents, restored workspace files, or logs. Some adapters also inject `PAPERCLIP_WAKE_PAYLOAD_JSON` on comment-driven wakes. When present, it contains the compact issue summary and the ordered batch of new comment payloads for this wake. Use it first. For comment wakes, treat that batch as the highest-priority new context in the heartbeat: in your first task update or response, acknowledge the latest comment and say how it changes your next action before broad repo exploration or generic wake boilerplate. Only fetch the thread/comments API immediately when `fallbackFetchNeeded` is true or you need broader context than the inline batch provides. @@ -73,7 +73,7 @@ because a request arrived through chat. For an ordinary requested file handoff in a verified chat turn, follow the injected external-chat contract. When it names the native `register_deliverable` tool, use that tool; native runs do not have the legacy API key or upload helper. -For non-native adapters, invoke `scripts/paperclip-upload-artifact.sh` directly. +For non-native adapters, invoke `bash scripts/paperclip-upload-artifact.sh`. Read `references/artifacts.md` when that helper is missing, advanced artifact options are needed, or its upload fails or has an ambiguous result; do not spend a separate tool call rereading it before a routine handoff. diff --git a/skills/paperclip/references/artifacts.md b/skills/paperclip/references/artifacts.md index 97ce228e197..ad2f8493653 100644 --- a/skills/paperclip/references/artifacts.md +++ b/skills/paperclip/references/artifacts.md @@ -2,10 +2,10 @@ When work produces a user-inspectable file, upload true deliverables to the current issue before final disposition. Local filesystem paths are not enough because board users, reviewers, and cloud operators may not have access to the agent workspace. -Use the helper bundled with this skill. From an installed `paperclip` skill directory, the helper lives at `scripts/paperclip-upload-artifact.sh`: +Use Bash to run the helper bundled with this skill; installed skill files may not retain executable permissions. From an installed `paperclip` skill directory, the helper lives at `scripts/paperclip-upload-artifact.sh`: ```bash -scripts/paperclip-upload-artifact.sh path/to/output.webm \ +bash scripts/paperclip-upload-artifact.sh path/to/output.webm \ --title "Walkthrough render" \ --summary "Rendered walkthrough for review" ``` @@ -102,7 +102,7 @@ part of the response intended for that external conversation, have the upload helper bind that specific file to an explicit response comment: ```bash -scripts/paperclip-upload-artifact.sh path/to/result.png \ +bash scripts/paperclip-upload-artifact.sh path/to/result.png \ --title "Requested image" \ --chat-comment "Here is the requested image." ``` @@ -140,3 +140,19 @@ response comment; never also claim that it appears above or is attached. Do not infer sharing intent from other files on the task or bind every attachment from a run. Only the file passed with `--chat-comment` is eligible for external publication; unbound artifacts remain Paperclip-only. + +**Native runner** + +When `register_deliverable` is available, use it for files in the bound local or +remote workspace. Supply a workspace-relative `contentRef`, basename `filename`, +`contentType`, exact `byteSize` and SHA-256, `title`, and a stable `idempotencyKey`. +The tool verifies the file, stores an attachment and artifact work product, and +binds it to the response. Generic API tools and a legacy API key are unnecessary. + +Wait for the receipt. It includes `attachmentId`, `contentPath`, and +`downloadPath`, along with the existing command, revision, entity references, +and disposition. Reuse the original key after an ambiguous result. A receipt +confirms storage and response binding in Paperclip; it does not confirm delivery +to an external chat provider. If registration fails, use the returned error to +resolve the failure or explain the limitation; do not describe a workspace path +as an uploaded file. diff --git a/skills/paperclip/scripts/paperclip-upload-artifact.sh b/skills/paperclip/scripts/paperclip-upload-artifact.sh index 15f7d41c73a..0dfa111d5e2 100755 --- a/skills/paperclip/scripts/paperclip-upload-artifact.sh +++ b/skills/paperclip/scripts/paperclip-upload-artifact.sh @@ -29,15 +29,15 @@ Options: --help, -h Show this help Examples: - scripts/paperclip-upload-artifact.sh dist/demo.mp4 \ + bash scripts/paperclip-upload-artifact.sh dist/demo.mp4 \ --title "Demo video render" \ --summary "MP4 render for board review" - scripts/paperclip-upload-artifact.sh out/walkthrough.webm \ + bash scripts/paperclip-upload-artifact.sh out/walkthrough.webm \ --title "Walkthrough video" \ --content-type video/webm - scripts/paperclip-upload-artifact.sh out/result.png \ + bash scripts/paperclip-upload-artifact.sh out/result.png \ --title "Generated image" \ --chat-comment "Here is the requested image." EOF @@ -159,6 +159,7 @@ upload_file() { local response_file local status_code local curl_status=0 + local indeterminate=0 escaped_path="${path//\\/\\\\}" escaped_path="${escaped_path//\"/\\\"}" @@ -180,8 +181,13 @@ upload_file() { printf 'Upload failed (%s): %s\n' "$status_code" "$url" >&2 cat "$response_file" >&2 printf '\n' >&2 + # The bridge uses a structured 409 outcome after a possibly committed + # mutation. Keep the marker so the next invocation reconciles by listing. + if [[ "$status_code" == "409" ]] && jq -e '.outcome == "indeterminate"' "$response_file" >/dev/null 2>&1; then + indeterminate=1 + fi rm -f "$response_file" - if [[ "$status_code" == "408" || "$status_code" -ge 500 ]]; then + if [[ "$status_code" == "408" || "$status_code" -ge 500 || "$indeterminate" == "1" ]]; then return 75 fi return 1 diff --git a/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx b/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx index 0873c3da3e9..64e0613ee38 100644 --- a/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesArtifactsTab.tsx @@ -37,7 +37,7 @@ import { MarkdownBody } from "@/components/MarkdownBody"; import { RichWorkProductCard } from "@/components/task-chat/RichWorkProductCard"; import { DocumentAnnotationsCountChip, IssueDocumentAnnotations } from "@/components/IssueDocumentAnnotations"; import { cn, formatDateTime } from "@/lib/utils"; -import { Link, useLocation } from "@/lib/router"; +import { useLocation } from "@/lib/router"; interface IssuePropertiesArtifactsTabProps { issue: Issue; @@ -357,8 +357,6 @@ function DocumentRow({ * thread. */ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDocument }: IssuePropertiesArtifactsTabProps) { - const [typeFilter, setTypeFilter] = useState("all"); - const [runFilter, setRunFilter] = useState("all"); const { data: attachments } = useQuery({ queryKey: queryKeys.issues.attachments(issue.id), queryFn: () => issuesApi.listAttachments(issue.id), @@ -387,9 +385,9 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc const agentsById = useMemo(() => new Map((agents ?? []).map((agent) => [agent.id, agent])), [agents]); type ArtifactRow = - | { kind: "work_product"; id: string; runId: string | null; date: Date; type: string; value: IssueWorkProduct } - | { kind: "document"; id: string; runId: null; date: Date; type: "document"; value: IssueDocument } - | { kind: "attachment"; id: string; runId: null; date: Date; type: "file" | "image"; value: NonNullable[number] }; + | { kind: "work_product"; id: string; runId: string | null; date: Date; value: IssueWorkProduct } + | { kind: "document"; id: string; runId: null; date: Date; value: IssueDocument } + | { kind: "attachment"; id: string; runId: null; date: Date; value: NonNullable[number] }; const allRows = useMemo(() => [ ...workProductRows.map((value): ArtifactRow => ({ @@ -397,9 +395,6 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc id: value.id, runId: value.createdByRunId, date: new Date(value.createdAt), - type: value.type === "artifact" && typeof value.metadata?.contentType === "string" && value.metadata.contentType.startsWith("image/") - ? "image" - : value.type === "artifact" ? "file" : value.type, value, })), ...documentRows.map((value): ArtifactRow => ({ @@ -407,7 +402,6 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc id: value.id, runId: null, date: new Date(value.createdAt), - type: "document", value, })), ...fileRows.map((value): ArtifactRow => ({ @@ -415,16 +409,11 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc id: value.id, runId: null, date: new Date(value.createdAt), - type: value.contentType.startsWith("image/") ? "image" : "file", value, })), ], [documentRows, fileRows, workProductRows]); - const filteredRows = allRows.filter((row) => - (typeFilter === "all" || row.type === typeFilter) && - (runFilter === "all" || (runFilter === "other" ? row.runId === null : row.runId === runFilter)), - ); - const groupedRows = [...filteredRows.reduce((groups, row) => { + const groupedRows = [...allRows.reduce((groups, row) => { const key = row.runId ?? "other"; const group = groups.get(key) ?? []; group.push(row); @@ -439,7 +428,6 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc : new Date(runsById.get(runId)?.startedAt ?? rows[0]?.date ?? 0), })) .sort((a, b) => b.date.getTime() - a.date.getTime()); - const runOptions = [...new Set(allRows.flatMap((row) => row.runId ? [row.runId] : []))]; if (workProductRows.length === 0 && documentRows.length === 0 && fileRows.length === 0) { return ( @@ -451,65 +439,21 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc return (
-
- - -
- - {groupedRows.length === 0 ? ( -

No artifacts match these filters.

- ) : groupedRows.map((group) => { + {groupedRows.map((group) => { const run = group.runId === "other" ? null : runsById.get(group.runId); const agent = run ? agentsById.get(run.agentId) : null; return (
-
-

- {group.runId === "other" ? "Other artifacts" : agent?.name ?? `Run ${group.runId.slice(0, 8)}`} -

- -
+ {group.runId !== "other" ? ( +
+

+ {agent?.name ?? `Run ${group.runId.slice(0, 8)}`} +

+ +
+ ) : null}
    {group.rows.map((row) => { if (row.kind === "work_product") { @@ -563,10 +507,6 @@ export function IssuePropertiesArtifactsTab({ issue, documentDeepLink, onOpenDoc
); })} - - - View all in company Artifacts → -
); } diff --git a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx index 83f02963ece..1cf82b0d404 100644 --- a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx @@ -354,7 +354,7 @@ describe("markdown work product review row", () => { expect(container.querySelector("button[aria-expanded]")).toBeNull(); }); - it("groups compact rows by producing run and filters by type", async () => { + it("groups compact rows by producing run", async () => { const runOne = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa"; const runTwo = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"; const imagePath = `/api/attachments/${ATTACHMENT_ID}/content`; @@ -401,13 +401,5 @@ describe("markdown work product review row", () => { expect(container.querySelector('a[aria-label="Open on GitHub: Artifact grouping PR"]')).not.toBeNull(); expect(container.querySelector('button[aria-label="Open gallery: Artifacts screenshot"]')).not.toBeNull(); }); - - const typeSelect = container.querySelector('select[aria-label="Filter artifacts by type"]') as HTMLSelectElement; - await act(async () => { - typeSelect.value = "pull_request"; - typeSelect.dispatchEvent(new Event("change", { bubbles: true })); - }); - expect(container.textContent).toContain("Artifact grouping PR"); - expect(container.textContent).not.toContain("Artifacts screenshot"); }); }); diff --git a/ui/src/components/task-side-panel/TaskSidePanel.test.tsx b/ui/src/components/task-side-panel/TaskSidePanel.test.tsx index f5b292a3654..0436981e2b1 100644 --- a/ui/src/components/task-side-panel/TaskSidePanel.test.tsx +++ b/ui/src/components/task-side-panel/TaskSidePanel.test.tsx @@ -164,6 +164,54 @@ describe("TaskSidePanel", () => { expect(container.textContent).toContain("Properties content"); }); + it("opens Artifacts on a new arrival, preserving manual selection until the next arrival", async () => { + await render(panel()); + await act(async () => container.querySelector("#side-panel-tab-properties")?.click()); + await render(panel({ artifactsOpenRequestId: 1 })); + expect(container.querySelector('[role="tab"][aria-selected="true"]')?.textContent).toContain("Artifacts"); + + await act(async () => container.querySelector("#side-panel-tab-properties")?.click()); + fixture.documents = [issueDocument("report", "Updated report")]; + await render(panel({ artifactsOpenRequestId: 1 })); + expect(container.querySelector('[role="tab"][aria-selected="true"]')?.textContent).toContain("Properties"); + + await render(panel({ artifactsOpenRequestId: 2 })); + expect(container.querySelector('[role="tab"][aria-selected="true"]')?.textContent).toContain("Artifacts"); + expect(container.querySelectorAll('[data-side-panel-tab-target="artifacts"]')).toHaveLength(1); + }); + + it("reopens a dismissed Artifacts tab only for a new arrival", async () => { + await render(panel({ artifactsOpenRequestId: 1 })); + await act(async () => container.querySelector('button[aria-label="Close Artifacts"]')?.click()); + await render(panel({ artifactsOpenRequestId: 1 })); + expect(container.querySelector('[data-side-panel-tab-target="artifacts"]')).toBeNull(); + await render(panel({ artifactsOpenRequestId: 2 })); + expect(container.querySelector('[role="tab"][aria-selected="true"]')?.textContent).toContain("Artifacts"); + }); + + it("acknowledges an arrival so remounting the panel preserves a later manual selection", async () => { + const onArtifactsOpened = vi.fn(); + await render(panel({ artifactsOpenRequestId: 1, onArtifactsOpened })); + expect(onArtifactsOpened).toHaveBeenCalledExactlyOnceWith(1); + await render(panel({ onArtifactsOpened })); + await act(async () => container.querySelector("#side-panel-tab-properties")?.click()); + await render(null); + await render(panel({ onArtifactsOpened })); + expect(container.querySelector('[role="tab"][aria-selected="true"]')?.textContent).toContain("Properties"); + expect(onArtifactsOpened).toHaveBeenCalledTimes(1); + }); + + it("clears the workspace-file route when an arriving artifact selects Artifacts", async () => { + routeFixture.location.search = "?file=ui%2Fsrc%2FApp.tsx&workspace=project"; + window.history.replaceState(null, "", `${routeFixture.location.pathname}${routeFixture.location.search}`); + await render(panel({ fileTabsEnabled: true })); + await render(panel({ fileTabsEnabled: true, artifactsOpenRequestId: 1 })); + expect(container.querySelector('[role="tab"][aria-selected="true"]')?.textContent).toContain("Artifacts"); + expect(routeFixture.navigate).toHaveBeenCalledWith( + expect.objectContaining({ search: "" }), expect.anything(), + ); + }); + it("uses the approved pre-rebase tab appearance for Streamlined UI", async () => { await render(panel({ streamlinedTabs: true })); const propertiesTab = container.querySelector('[data-side-panel-tab-target="properties"]'); diff --git a/ui/src/components/task-side-panel/TaskSidePanel.tsx b/ui/src/components/task-side-panel/TaskSidePanel.tsx index f352764b424..86fe5b481d8 100644 --- a/ui/src/components/task-side-panel/TaskSidePanel.tsx +++ b/ui/src/components/task-side-panel/TaskSidePanel.tsx @@ -91,6 +91,9 @@ export interface TaskSidePanelProps { checkingMonitorNow?: boolean; fileTabsEnabled: boolean; documentDeepLink?: { requestId: number; documentKey: string } | null; + /** A new durable output asks the host to reveal this tab once. */ + artifactsOpenRequestId?: number; + onArtifactsOpened?: (requestId: number) => void; onRequestClose?: () => void; streamlinedTabs?: boolean; showSubtasksTab?: boolean; @@ -222,6 +225,8 @@ export function TaskSidePanel({ checkingMonitorNow = false, fileTabsEnabled, documentDeepLink, + artifactsOpenRequestId, + onArtifactsOpened, onRequestClose, streamlinedTabs = false, showSubtasksTab = false, @@ -248,6 +253,7 @@ export function TaskSidePanel({ const scrollPositionsRef = useRef(new Map()); const userInteractedRef = useRef(restoredRef.current?.userInteracted ?? false); const autoPlanHandledRef = useRef(restoredRef.current?.autoPlanHandled ?? false); + const handledArtifactsRequestRef = useRef(undefined); const initialState = useMemo(() => { const restored = restoredRef.current?.state; let tabs = restored?.tabs ?? (issue.conversationAgentId ? [taskPanelArtifactsTab()] : [taskPanelPropertiesTab()]); @@ -366,6 +372,15 @@ export function TaskSidePanel({ viewer.state, ]); + useEffect(() => { + if (artifactsOpenRequestId === undefined || handledArtifactsRequestRef.current === artifactsOpenRequestId) return; + handledArtifactsRequestRef.current = artifactsOpenRequestId; + setLauncherOpen(false); + controller.openTab(taskPanelArtifactsTab()); + if (viewer.state || viewer.browse) viewer.close(); + onArtifactsOpened?.(artifactsOpenRequestId); + }, [artifactsOpenRequestId, controller.openTab, viewer.state, viewer.browse, viewer.close, onArtifactsOpened]); + const recentFilesQuery = useQuery({ queryKey: queryKeys.issues.fileResources(issue.id, { workspace: "auto", diff --git a/ui/src/hooks/useTaskArtifactArrival.test.tsx b/ui/src/hooks/useTaskArtifactArrival.test.tsx new file mode 100644 index 00000000000..00e7a16ff84 --- /dev/null +++ b/ui/src/hooks/useTaskArtifactArrival.test.tsx @@ -0,0 +1,114 @@ +// @vitest-environment jsdom +import { act, type ComponentProps } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import type { IssueAttachment, IssueDocumentSummary, IssueWorkProduct } from "@paperclipai/shared"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useTaskArtifactArrival } from "./useTaskArtifactArrival"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const fileId = "00000000-0000-4000-8000-000000000001"; +const attachment = (id = fileId, userUpload = false) => ({ + id, createdByAgentId: userUpload ? null : "agent-1", createdByUserId: userUpload ? "user-1" : null, +} as IssueAttachment); +const document = (key = "report") => ({ id: `doc-${key}`, key } as IssueDocumentSummary); +const product = (attachmentId = fileId) => ({ + id: `product-${attachmentId}`, companyId: "company-1", issueId: "task-1", + projectId: null, executionWorkspaceId: null, runtimeServiceId: null, + externalId: null, title: "Image", url: null, status: "active", reviewState: "none", + isPrimary: false, healthStatus: "unknown", summary: null, createdByRunId: null, + createdAt: new Date(), updatedAt: new Date(), + type: "artifact", provider: "paperclip", metadata: { + attachmentId, contentType: "image/png", byteSize: 123, + contentPath: `/api/attachments/${attachmentId}/content`, + openPath: `/api/attachments/${attachmentId}/content`, + downloadPath: `/api/attachments/${attachmentId}/content?download=1`, + }, +} as IssueWorkProduct); + +function Watch(props: Parameters[0]) { + useTaskArtifactArrival(props); + return null; +} + +describe("new task artifacts", () => { + let root: Root; + let container: HTMLDivElement; + let props: ComponentProps; + const onArrival = vi.fn(); + beforeEach(() => { + container = window.document.createElement("div"); + root = createRoot(container); + onArrival.mockReset(); + props = { issueId: "task-1", attachments: [], workProducts: [], documents: [], onArrival }; + }); + afterEach(() => act(() => root.unmount())); + async function render(update: Partial = {}) { + props = { ...props, ...update }; + await act(async () => root.render()); + } + + it("baselines independently loaded history without opening the panel", async () => { + await render({ attachments: undefined, workProducts: undefined, documents: undefined }); + await render({ attachments: [attachment()] }); + await render({ documents: [document()] }); + await render({ workProducts: [product()] }); + expect(onArrival).not.toHaveBeenCalled(); + }); + + it.each(["attachments", "workProducts", "documents"] as const)("reveals new %s after the initial load", async (source) => { + await render(); + await render({ [source]: { attachments: [attachment()], workProducts: [product()], documents: [document()] }[source] }); + expect(onArrival).toHaveBeenCalledOnce(); + }); + + it("does not reopen for upload promotion, revisions, removals, or retry snapshots", async () => { + await render(); + await render({ attachments: [attachment()] }); + await render({ workProducts: [product()] }); + await render({ workProducts: [{ ...product(), title: "Updated title" }] }); + await render({ attachments: undefined }); + await render({ attachments: [] }); + await render({ attachments: [attachment()] }); + expect(onArrival).toHaveBeenCalledOnce(); + }); + + it("deduplicates a file when its work product arrives before its attachment", async () => { + await render(); + await render({ workProducts: [product()] }); + await render({ attachments: [attachment()] }); + expect(onArrival).toHaveBeenCalledOnce(); + }); + + it("opens again for a later, different artifact", async () => { + await render(); + await render({ attachments: [attachment()] }); + await render({ attachments: [attachment(), attachment("second-file")] }); + expect(onArrival).toHaveBeenCalledTimes(2); + }); + + it("baselines a newly navigated task and detects its subsequent additions", async () => { + await render(); + await render({ issueId: "task-2", attachments: undefined, documents: undefined, workProducts: undefined }); + await render({ attachments: [attachment()], documents: [], workProducts: [] }); + expect(onArrival).not.toHaveBeenCalled(); + await render({ documents: [document()] }); + expect(onArrival).toHaveBeenCalledOnce(); + }); + + it("leaves Plan handling and user input uploads alone, but reveals a published user file", async () => { + await render(); + await render({ attachments: [attachment(fileId, true)], documents: [document("plan"), document(`artifact-review-${fileId}`)] }); + expect(onArrival).not.toHaveBeenCalled(); + await render({ workProducts: [product()] }); + expect(onArrival).toHaveBeenCalledOnce(); + }); + + it("ignores document revision changes and watches non-file work products", async () => { + await render({ documents: [document()] }); + await render({ documents: [{ ...document(), latestRevisionNumber: 2 }] }); + expect(onArrival).not.toHaveBeenCalled(); + await render({ workProducts: [{ id: "pr-1", type: "pull_request", provider: "github" } as IssueWorkProduct] }); + expect(onArrival).toHaveBeenCalledOnce(); + }); +}); diff --git a/ui/src/hooks/useTaskArtifactArrival.ts b/ui/src/hooks/useTaskArtifactArrival.ts new file mode 100644 index 00000000000..88299dea089 --- /dev/null +++ b/ui/src/hooks/useTaskArtifactArrival.ts @@ -0,0 +1,62 @@ +import { useEffect, useRef } from "react"; +import { + getAttachmentArtifactWorkProductMetadata, + isArtifactReviewDocumentKey, + type IssueAttachment, + type IssueDocumentSummary, + type IssueWorkProduct, +} from "@paperclipai/shared"; +import { isAgentAttachment } from "@/lib/issue-artifacts"; + +interface TaskArtifactArrivalOptions { + issueId: string | undefined; + attachments: IssueAttachment[] | undefined; + workProducts: IssueWorkProduct[] | undefined; + documents: IssueDocumentSummary[] | undefined; + onArrival: () => void; +} + +/** Watch the same durable objects as the Artifacts tab, excluding its initial load. */ +export function useTaskArtifactArrival({ + issueId, attachments, workProducts, documents, onArrival, +}: TaskArtifactArrivalOptions) { + const observed = useRef<{ + issueId: string | undefined; + loaded: Set; + ids: Set; + }>({ issueId: undefined, loaded: new Set(), ids: new Set() }); + + useEffect(() => { + if (observed.current.issueId !== issueId) { + observed.current = { issueId, loaded: new Set(), ids: new Set() }; + } + if (!issueId) return; + + const sources = { + attachments: attachments?.filter(isAgentAttachment).map((file) => `attachment:${file.id}`), + workProducts: workProducts?.map((product) => { + const attachment = getAttachmentArtifactWorkProductMetadata(product); + // Uploading a file and then registering it is one arrival, even when + // the two queries settle separately. + return attachment ? `attachment:${attachment.attachmentId}` : `work-product:${product.id}`; + }), + documents: documents + ?.filter((doc) => doc.key !== "plan" && !isArtifactReviewDocumentKey(doc.key)) + .map((doc) => `document:${doc.id}`), + }; + const state = observed.current; + let arrived = false; + for (const [source, ids] of Object.entries(sources)) { + if (!ids) continue; + const loaded = state.loaded.has(source); + for (const id of ids) { + if (loaded && !state.ids.has(id)) arrived = true; + state.ids.add(id); + } + state.loaded.add(source); + } + // Keep observed IDs across removals and failed refetches. A repeated + // snapshot or a revision update must not take the user's tab selection. + if (arrived) onArrival(); + }, [issueId, attachments, workProducts, documents, onArrival]); +} diff --git a/ui/src/pages/IssueDetail.test.tsx b/ui/src/pages/IssueDetail.test.tsx index 6fa0a8f79f5..c5cf9dec795 100644 --- a/ui/src/pages/IssueDetail.test.tsx +++ b/ui/src/pages/IssueDetail.test.tsx @@ -2156,6 +2156,46 @@ describe("IssueDetail", () => { }); }); + it.each([false, true])("reveals new artifacts once in the task panel (mobile: %s)", async (isMobile) => { + mockSidebarState.isMobile = isMobile; + mockPanelState.panelVisible = false; + mockIssuesApi.get.mockResolvedValue(createIssue()); + await act(async () => { + root.render(); + }); + await waitForAssertion(() => { + expect(queryClient.getQueryData(queryKeys.issues.attachments("PAP-1"))).toEqual([]); + }); + await flushReact(); + const panelProps = () => (isMobile + ? mockTaskSidePanelRender.mock.calls.at(-1)?.[0] + : mockOpenPanel.mock.calls.at(-1)?.[0]?.props.children?.props) as { + artifactsOpenRequestId?: number; + onArtifactsOpened: (requestId: number) => void; + }; + const file = createAttachment({ id: "new-output", createdByAgentId: "agent-1" }); + act(() => { queryClient.setQueryData(queryKeys.issues.attachments("PAP-1"), [file]); }); + await waitForAssertion(() => expect(panelProps()?.artifactsOpenRequestId).toBe(1)); + if (isMobile) { + expect(document.querySelector('[data-testid="mobile-task-side-panel"]')).not.toBeNull(); + expect(mockSetPanelVisible).not.toHaveBeenCalled(); + expect(mockOpenPanel.mock.calls.at(-1)?.[0]?.props.children?.props.artifactsOpenRequestId).toBeUndefined(); + } else { + expect(mockSetPanelVisible).toHaveBeenCalledWith(true); + } + + act(() => panelProps().onArtifactsOpened(1)); + await waitForAssertion(() => expect(panelProps().artifactsOpenRequestId).toBeUndefined()); + act(() => { queryClient.setQueryData(queryKeys.issues.attachments("PAP-1"), [{ ...file, originalFilename: "Renamed output" }]); }); + await flushReact(); + expect(panelProps().artifactsOpenRequestId).toBeUndefined(); + + act(() => { queryClient.setQueryData(queryKeys.issues.attachments("PAP-1"), [file, + createAttachment({ id: "next-output", createdByAgentId: "agent-1" }), + ]); }); + await waitForAssertion(() => expect(panelProps().artifactsOpenRequestId).toBe(2)); + }); + it("opens the mobile properties sheet for a document deep link", async () => { mockSidebarState.isMobile = true; mockLocation.hash = "#document-qa-evidence"; diff --git a/ui/src/pages/IssueDetail.tsx b/ui/src/pages/IssueDetail.tsx index 1a2b4979b4a..f4c3bb53bbb 100644 --- a/ui/src/pages/IssueDetail.tsx +++ b/ui/src/pages/IssueDetail.tsx @@ -223,6 +223,7 @@ import { waitForStoppedRuns } from "../lib/wait-for-stopped-runs"; import { useIssueExternalObjects } from "../hooks/useIssueExternalObjects"; import { IssueGalleryContext } from "../context/IssueGalleryContext"; import { useIssuePlanDocument } from "../hooks/useIssuePlanDocument"; +import { useTaskArtifactArrival } from "../hooks/useTaskArtifactArrival"; import { IssueRunLedger } from "../components/IssueRunLedger"; import { IssueWorkspaceCard } from "../components/IssueWorkspaceCard"; import type { MentionOption } from "../components/MarkdownEditor"; @@ -2891,6 +2892,11 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS const [moreOpen, setMoreOpen] = useState(false); const [copied, setCopied] = useState(false); const [mobilePropsOpen, setMobilePropsOpen] = useState(false); + const [artifactsOpenRequest, setArtifactsOpenRequest] = useState<{ + issueId: string; + requestId: number; + handled?: boolean; + } | null>(null); const [documentDeepLink, setDocumentDeepLink] = useState< (IssuePropertiesDocumentDeepLink & { issueId: string }) | null >(null); @@ -3578,6 +3584,27 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS } setPanelVisible(true); }, [issue?.id, setPanelVisible, suppressPanelUntilPlan]); + const revealNewArtifact = useCallback(() => { + if (!issue?.id) return; + setDocumentDeepLink(null); + setArtifactsOpenRequest((previous) => ({ + issueId: issue.id, + requestId: (previous?.requestId ?? 0) + 1, + })); + if (isMobile) setMobilePropsOpen(true); + else openTaskSidePanel(); + }, [issue?.id, isMobile, openTaskSidePanel]); + const handleArtifactsOpened = useCallback((requestId: number) => { + setArtifactsOpenRequest((request) => request?.requestId === requestId + ? { ...request, handled: true } : request); + }, []); + useTaskArtifactArrival({ + issueId: taskChatShellEnabled ? issue?.id : undefined, + attachments, + workProducts, + documents: issue?.documentSummaries, + onArrival: revealNewArtifact, + }); const toggleTaskSidePanel = useCallback(() => { if (!panelVisible || suppressPanelUntilPlan) { openTaskSidePanel(); @@ -5584,6 +5611,9 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS streamlinedTabs={streamlinedTaskDetailEnabled} showSubtasksTab={streamlinedTaskDetailEnabled} tasksTab={resolvedTasksTab} + artifactsOpenRequestId={!isMobile && !artifactsOpenRequest?.handled && artifactsOpenRequest?.issueId === panelIssue.id + ? artifactsOpenRequest.requestId : undefined} + onArtifactsOpened={handleArtifactsOpened} /> , { contentMode: "full-bleed" }, @@ -5621,6 +5651,9 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS currentUserId, fileViewerEnabled, resolvedTasksTab, + artifactsOpenRequest, + handleArtifactsOpened, + isMobile, ]); const goToInboxShortcutArmedRef = useRef(false); @@ -8059,6 +8092,9 @@ export function TaskDetailSurface({ conversation, tasksTab }: { tasksTab?: TaskS streamlinedTabs={streamlinedTaskDetailEnabled} showSubtasksTab={streamlinedTaskDetailEnabled} tasksTab={resolvedTasksTab} + artifactsOpenRequestId={isMobile && !artifactsOpenRequest?.handled && artifactsOpenRequest?.issueId === issue.id + ? artifactsOpenRequest.requestId : undefined} + onArtifactsOpened={handleArtifactsOpened} documentDeepLink={ documentDeepLink?.issueId === issue.id ? documentDeepLink