From 67c80e941ecf682ca44fcecd5be7d6d85e50b4e9 Mon Sep 17 00:00:00 2001 From: Alex Knight Date: Mon, 15 Jun 2026 21:01:42 +1000 Subject: [PATCH] fix(gateway): fall back to managed path when inbound PDF sandbox staging fails (#90097) --- .../chat.directive-tags.test.ts | 234 ++++++++++++++++-- src/gateway/server-methods/chat.ts | 93 ++++--- 2 files changed, 282 insertions(+), 45 deletions(-) diff --git a/src/gateway/server-methods/chat.directive-tags.test.ts b/src/gateway/server-methods/chat.directive-tags.test.ts index b40d4113510..ee8b35d00da 100644 --- a/src/gateway/server-methods/chat.directive-tags.test.ts +++ b/src/gateway/server-methods/chat.directive-tags.test.ts @@ -5395,7 +5395,10 @@ describe("chat directive tag stripping for non-streaming final payloads", () => expect(mockState.lastDispatchCtx?.MediaStaged).toBe(true); }); - it("wraps stageSandboxMedia infrastructure errors as 5xx UNAVAILABLE and cleans up media-store files", async () => { + it("wraps stageSandboxMedia infrastructure errors as 5xx UNAVAILABLE for non-fallback refs and cleans up media-store files", async () => { + // A non-PDF managed offload cannot fall back to a managed path, so an infra + // staging error stays a retryable 5xx. (Managed PDFs fall back instead — see + // the staging-throw fallback test below.) #90097 createTranscriptFixture("openclaw-chat-send-stage-unavailable-"); mockState.finalText = "ok"; mockState.sessionEntry = { @@ -5411,7 +5414,10 @@ describe("chat directive tag stripping for non-streaming final payloads", () => }, ]; mockState.savedMediaResults = [ - { path: "/home/user/.openclaw/media/inbound/report.pdf", contentType: "application/pdf" }, + { + path: "/home/user/.openclaw/media/inbound/report.bin", + contentType: "application/octet-stream", + }, ]; mockState.sandboxWorkspace = { workspaceDir: "/sandbox/workspace" }; const stageError = Object.assign(new Error("ENOSPC: no space left on device"), { @@ -5422,7 +5428,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () => mockState.stageSandboxMediaError = stageError; const respond = vi.fn(); const context = createChatContext(); - const pdf = Buffer.from("%PDF-1.4\n%µ¶\n1 0 obj\n<<>>\nendobj\n").toString("base64"); + const binPayload = Buffer.from("OPENCLAW-BINARY\n").toString("base64"); await runNonStreamingChatSend({ context, @@ -5433,9 +5439,9 @@ describe("chat directive tag stripping for non-streaming final payloads", () => attachments: [ { type: "file", - mimeType: "application/pdf", - fileName: "report.pdf", - content: pdf, + mimeType: "application/octet-stream", + fileName: "report.bin", + content: binPayload, }, ], }, @@ -5518,7 +5524,9 @@ describe("chat directive tag stripping for non-streaming final payloads", () => // absolute path, so a simple `stagedPaths.length === nonImage.length` // check could not detect when one of the files silently fell out (e.g. a // file between the RPC cap and the staging cap). Prestage must compare - // the returned `staged` map against the input refs. + // the returned `staged` map against the input refs. Non-PDF refs cannot fall + // back to a managed path, so an incomplete stage stays a 5xx. (Managed PDFs + // fall back instead — see the staging-skip fallback test below.) #90097 createTranscriptFixture("openclaw-chat-send-partial-stage-"); mockState.finalText = "ok"; mockState.sessionEntry = { @@ -5534,15 +5542,21 @@ describe("chat directive tag stripping for non-streaming final payloads", () => }, ]; mockState.savedMediaResults = [ - { path: "/home/user/.openclaw/media/inbound/report.pdf", contentType: "application/pdf" }, - { path: "/home/user/.openclaw/media/inbound/oversize.pdf", contentType: "application/pdf" }, + { + path: "/home/user/.openclaw/media/inbound/report.bin", + contentType: "application/octet-stream", + }, + { + path: "/home/user/.openclaw/media/inbound/data.bin", + contentType: "application/octet-stream", + }, ]; mockState.sandboxWorkspace = { workspaceDir: "/sandbox/workspace" }; - mockState.stagedRelativePaths = ["media/inbound/report.pdf", "media/inbound/oversize.pdf"]; - mockState.unstagedSources = ["/home/user/.openclaw/media/inbound/oversize.pdf"]; + mockState.stagedRelativePaths = ["media/inbound/report.bin", "media/inbound/data.bin"]; + mockState.unstagedSources = ["/home/user/.openclaw/media/inbound/data.bin"]; const respond = vi.fn(); const context = createChatContext(); - const pdf = Buffer.from("%PDF-1.4\n").toString("base64"); + const binPayload = Buffer.from("OPENCLAW-BINARY\n").toString("base64"); await runNonStreamingChatSend({ context, @@ -5551,8 +5565,18 @@ describe("chat directive tag stripping for non-streaming final payloads", () => message: "read these", requestParams: { attachments: [ - { type: "file", mimeType: "application/pdf", fileName: "report.pdf", content: pdf }, - { type: "file", mimeType: "application/pdf", fileName: "oversize.pdf", content: pdf }, + { + type: "file", + mimeType: "application/octet-stream", + fileName: "report.bin", + content: binPayload, + }, + { + type: "file", + mimeType: "application/octet-stream", + fileName: "data.bin", + content: binPayload, + }, ], }, expectBroadcast: false, @@ -5634,6 +5658,188 @@ describe("chat directive tag stripping for non-streaming final payloads", () => expect(mockState.deleteMediaBufferCalls).toEqual([]); }); + it("falls back to the managed path when sandbox staging throws for an already-managed PDF", async () => { + // #90097: an already-managed inbound PDF below the staging cap normally + // stages into the sandbox, but if staging throws (e.g. workspace mkdir + // ENOSPC) the PDF must still reach the agent via its managed media path + // instead of failing the send — host-side media-understanding reads it from + // the media-store root. + createTranscriptFixture("openclaw-chat-send-managed-pdf-stage-throw-"); + mockState.finalText = "ok"; + mockState.sessionEntry = { + modelProvider: "test-provider", + model: "vision-model", + }; + mockState.modelCatalog = [ + { + provider: "test-provider", + id: "vision-model", + name: "Vision model", + input: ["text", "image"], + }, + ]; + mockState.savedMediaResults = [ + { path: "/home/user/.openclaw/media/inbound/report.pdf", contentType: "application/pdf" }, + ]; + mockState.sandboxWorkspace = { workspaceDir: "/sandbox/workspace" }; + mockState.stageSandboxMediaError = Object.assign(new Error("ENOSPC: no space left on device"), { + code: "ENOSPC", + }); + const respond = vi.fn(); + const context = createChatContext(); + // Small PDF (below the 5MB staging cap) so it takes the staging path, not the + // oversized pass-through path. + const pdf = Buffer.from("%PDF-1.4\n%µ¶\nendobj\n").toString("base64"); + + await runNonStreamingChatSend({ + context, + respond, + idempotencyKey: "idem-managed-pdf-stage-throw", + message: "read this", + requestParams: { + attachments: [ + { type: "file", mimeType: "application/pdf", fileName: "report.pdf", content: pdf }, + ], + }, + expectBroadcast: false, + }); + + // Falls back to the absolute managed path; nothing staged (so no workspace + // dir) and the media-store entry is preserved for host-side extraction. + expect(mockState.lastDispatchCtx?.MediaPath).toBe( + "/home/user/.openclaw/media/inbound/report.pdf", + ); + expect(mockState.lastDispatchCtx?.MediaPaths).toEqual([ + "/home/user/.openclaw/media/inbound/report.pdf", + ]); + expect(mockState.lastDispatchCtx?.MediaType).toBe("application/pdf"); + expect(mockState.lastDispatchCtx?.MediaWorkspaceDir).toBeUndefined(); + expect(mockState.lastDispatchCtx?.MediaStaged).toBe(true); + expect(mockState.deleteMediaBufferCalls).toEqual([]); + }); + + it("falls back to the managed path when sandbox staging silently skips an already-managed PDF", async () => { + // #90097: stageSandboxMedia can silently skip a file (keeping its absolute + // path) and return it absent from the staged map. An already-managed PDF in + // that state falls back to its managed media path rather than failing the + // send; the staged workspace dir is still carried for any files that landed. + createTranscriptFixture("openclaw-chat-send-managed-pdf-stage-skip-"); + mockState.finalText = "ok"; + mockState.sessionEntry = { + modelProvider: "test-provider", + model: "vision-model", + }; + mockState.modelCatalog = [ + { + provider: "test-provider", + id: "vision-model", + name: "Vision model", + input: ["text", "image"], + }, + ]; + mockState.savedMediaResults = [ + { path: "/home/user/.openclaw/media/inbound/report.pdf", contentType: "application/pdf" }, + ]; + mockState.sandboxWorkspace = { workspaceDir: "/sandbox/workspace" }; + // No stagedRelativePaths → staged map is empty and ctx.MediaPaths keeps the + // absolute path, mirroring stageSandboxMedia silently skipping the file. + const respond = vi.fn(); + const context = createChatContext(); + const pdf = Buffer.from("%PDF-1.4\n%µ¶\nendobj\n").toString("base64"); + + await runNonStreamingChatSend({ + context, + respond, + idempotencyKey: "idem-managed-pdf-stage-skip", + message: "read this", + requestParams: { + attachments: [ + { type: "file", mimeType: "application/pdf", fileName: "report.pdf", content: pdf }, + ], + }, + expectBroadcast: false, + }); + + expect(mockState.lastDispatchCtx?.MediaPath).toBe( + "/home/user/.openclaw/media/inbound/report.pdf", + ); + expect(mockState.lastDispatchCtx?.MediaPaths).toEqual([ + "/home/user/.openclaw/media/inbound/report.pdf", + ]); + expect(mockState.lastDispatchCtx?.MediaType).toBe("application/pdf"); + expect(mockState.lastDispatchCtx?.MediaWorkspaceDir).toBe("/sandbox/workspace"); + expect(mockState.lastDispatchCtx?.MediaStaged).toBe(true); + expect(mockState.deleteMediaBufferCalls).toEqual([]); + }); + + it("still fails the send when staging skips a non-PDF in a mixed managed batch", async () => { + // #90097: the PDF fallback is per-ref. A managed PDF that stages does not + // rescue a sibling non-PDF that silently fell out of staging; that batch must + // still surface a retryable 5xx and clean up every offloaded entry. + createTranscriptFixture("openclaw-chat-send-mixed-stage-skip-"); + mockState.finalText = "ok"; + mockState.sessionEntry = { + modelProvider: "test-provider", + model: "vision-model", + }; + mockState.modelCatalog = [ + { + provider: "test-provider", + id: "vision-model", + name: "Vision model", + input: ["text", "image"], + }, + ]; + mockState.savedMediaResults = [ + { path: "/home/user/.openclaw/media/inbound/report.pdf", contentType: "application/pdf" }, + { + path: "/home/user/.openclaw/media/inbound/data.bin", + contentType: "application/octet-stream", + }, + ]; + mockState.sandboxWorkspace = { workspaceDir: "/sandbox/workspace" }; + mockState.stagedRelativePaths = ["media/inbound/report.pdf", "media/inbound/data.bin"]; + mockState.unstagedSources = ["/home/user/.openclaw/media/inbound/data.bin"]; + const respond = vi.fn(); + const context = createChatContext(); + const pdf = Buffer.from("%PDF-1.4\n").toString("base64"); + const bin = Buffer.from("OPENCLAW-BINARY\n").toString("base64"); + + await runNonStreamingChatSend({ + context, + respond, + idempotencyKey: "idem-mixed-stage-skip", + message: "read these", + requestParams: { + attachments: [ + { type: "file", mimeType: "application/pdf", fileName: "report.pdf", content: pdf }, + { + type: "file", + mimeType: "application/octet-stream", + fileName: "data.bin", + content: bin, + }, + ], + }, + expectBroadcast: false, + waitFor: "none", + }); + + expect(mockState.lastDispatchCtx).toBeUndefined(); + expect(respond).toHaveBeenCalledTimes(1); + const [ok, payload, error] = lastRespondCall(respond) ?? []; + expect(ok).toBe(false); + expect(payload).toBeUndefined(); + expect(error?.code).toBe(ErrorCodes.UNAVAILABLE); + expect(responseErrorMessage(error)).toMatch(/staging incomplete/i); + // The whole batch is cleaned up — including the PDF that would have fallen + // back on its own — because the non-PDF cannot be delivered. + expect(mockState.deleteMediaBufferCalls.map((c) => c.id).toSorted()).toEqual([ + "saved-media", + "saved-media", + ]); + }); + it("rejects sandbox-oversized non-image attachments as 4xx before staging", async () => { // Regression: resolveChatAttachmentMaxBytes defaults to 20MB, but // stageSandboxMedia caps each file at STAGED_MEDIA_MAX_BYTES (5MB) and diff --git a/src/gateway/server-methods/chat.ts b/src/gateway/server-methods/chat.ts index 8a1d24b9ff8..84c725d5316 100644 --- a/src/gateway/server-methods/chat.ts +++ b/src/gateway/server-methods/chat.ts @@ -53,7 +53,10 @@ import { type ReplyPayload, } from "../../auto-reply/reply-payload.js"; import { createReplyDispatcher } from "../../auto-reply/reply/reply-dispatcher.js"; -import { stageSandboxMedia } from "../../auto-reply/reply/stage-sandbox-media.js"; +import { + stageSandboxMedia, + type StageSandboxMediaResult, +} from "../../auto-reply/reply/stage-sandbox-media.js"; import type { MsgContext, TemplateContext } from "../../auto-reply/templating.js"; import { resolveSessionFilePath, updateSessionStoreEntry } from "../../config/sessions.js"; import { resolveMirroredTranscriptText } from "../../config/sessions/transcript-mirror.js"; @@ -1344,14 +1347,14 @@ function isPdfOffloadedRef(ref: OffloadedRef): boolean { return path.extname(ref.path.split(/[?#]/u)[0] ?? "").toLowerCase() === ".pdf"; } -// A managed inbound PDF saved to the media store is safe to hand the agent as -// its media path without sandbox staging: host-side media-understanding extracts -// its text (see resolveFileExtractionLimits), and copying a large PDF into every -// sandbox is wasteful. Files above the 5MB staging cap are otherwise rejected as -// a 4xx (see prestageMediaPathOffloads), so pass managed PDFs through instead so -// locked-down agents still receive the document text. #90097 -function shouldPassThroughManagedInboundPdfOffloadRef(ref: OffloadedRef): boolean { - if (ref.sizeBytes <= MEDIA_MAX_BYTES || !isPdfOffloadedRef(ref)) { +// A managed inbound PDF saved to the media store is safe to hand the agent as its +// media path without sandbox staging: host-side media-understanding extracts its +// text (see resolveFileExtractionLimits) by reading the media-store root, so even +// locked-down agents receive the document. This gates both the up-front bypass for +// oversized PDFs and the fallback to the managed path when sandbox staging fails +// for an already-managed PDF. #90097 +function isManagedInboundPdfOffloadRef(ref: OffloadedRef): boolean { + if (!isPdfOffloadedRef(ref)) { return false; } try { @@ -1361,18 +1364,29 @@ function shouldPassThroughManagedInboundPdfOffloadRef(ref: OffloadedRef): boolea } } +// Oversized managed PDFs skip sandbox staging up front: copying a large PDF into +// every sandbox is wasteful, and files above the 5MB staging cap would otherwise +// be rejected as a 4xx (see prestageMediaPathOffloads). +function shouldPassThroughManagedInboundPdfOffloadRef(ref: OffloadedRef): boolean { + return ref.sizeBytes > MEDIA_MAX_BYTES && isManagedInboundPdfOffloadRef(ref); +} + // Stages media-path offloads into the agent sandbox synchronously so chat.send -// can surface 5xx before respond(). Throws MediaOffloadError on any staging -// failure (ENOSPC / EPERM / partial-stage) so the outer chat.send handler can -// map it to UNAVAILABLE (5xx); plain Error would be misclassified as 4xx. All -// offloaded refs are cleaned up from the media store before rethrow. +// can surface 5xx before respond(). Throws MediaOffloadError when staging fails +// for a ref that cannot fall back (ENOSPC / EPERM / partial-stage of a non-PDF or +// unmanaged ref) so the outer chat.send handler maps it to UNAVAILABLE (5xx); +// plain Error would be misclassified as 4xx. Already-managed inbound PDFs instead +// fall back to their managed media path on staging failure (#90097), since +// host-side media-understanding reads them from the media-store root. Offloaded +// refs are cleaned up from the media store before rethrow. // Callers MUST set ctx.MediaStaged=true when this runs so the dispatch // pipeline skips its own stageSandboxMedia pass. // -// Returned paths are absolute media-store paths when no sandbox is active or for -// oversized managed PDFs that pass through staging (#90097), and sandbox-relative -// paths plus `workspaceDir` for files staged into the sandbox. Host-side -// media-understanding resolves both via MediaWorkspaceDir and the media-store root. +// Returned paths are absolute media-store paths when no sandbox is active, for +// oversized managed PDFs that bypass staging, or for already-managed PDFs that +// fall back when staging fails (#90097); files staged into the sandbox use +// sandbox-relative paths plus `workspaceDir`. Host-side media-understanding +// resolves both via MediaWorkspaceDir and the media-store root. async function prestageMediaPathOffloads(params: { offloadedRefs: OffloadedRef[]; includeImageRefs?: boolean; @@ -1437,25 +1451,41 @@ async function prestageMediaPathOffloads(params: { MediaType: refsToStage[0].mimeType, MediaTypes: refsToStage.map((ref) => ref.mimeType), }; - const stageResult = await stageSandboxMedia({ - ctx: stagingCtx, - sessionCtx: stagingCtx as TemplateContext, - cfg: params.cfg, - sessionKey: params.sessionKey, - workspaceDir, - }); + let stageResult: StageSandboxMediaResult; + try { + stageResult = await stageSandboxMedia({ + ctx: stagingCtx, + sessionCtx: stagingCtx as TemplateContext, + cfg: params.cfg, + sessionKey: params.sessionKey, + workspaceDir, + }); + } catch (stageErr) { + // stageSandboxMedia threw before copying anything (e.g. workspace mkdir + // ENOSPC/EPERM), so nothing reached the sandbox. Already-managed inbound + // PDFs still reach the agent via their managed media path (host-side + // media-understanding reads the media-store root); fail the send only when a + // ref cannot fall back. #90097 + if (refsToStage.some((ref) => !isManagedInboundPdfOffloadRef(ref))) { + throw stageErr; + } + return refsByManagedPath(mediaPathRefs); + } // stageSandboxMedia silently keeps unstaged entries as their original // absolute path, so length parity does not prove every file landed in the // sandbox. The RPC max (20MB via resolveChatAttachmentMaxBytes) admits files // above the staging cap (STAGED_MEDIA_MAX_BYTES = 5MB); check the returned - // `staged` map so any missing source becomes a 5xx MediaOffloadError the - // client can retry. + // `staged` map for missing sources. Already-managed inbound PDFs fall back to + // their absolute managed path (host-side media-understanding reads the + // media-store root); any other missing source is a 5xx MediaOffloadError the + // client can retry. #90097 const stagedSources = stageResult.staged; const missing = refsToStage.filter((ref) => !stagedSources.has(ref.path)); - if (missing.length > 0) { + const unstageable = missing.filter((ref) => !isManagedInboundPdfOffloadRef(ref)); + if (unstageable.length > 0) { throw new Error( - `attachment staging incomplete: ${stagedSources.size}/${refsToStage.length} paths staged into sandbox workspace (missing: ${missing.map((ref) => ref.path).join(", ")})`, + `attachment staging incomplete: ${stagedSources.size}/${refsToStage.length} paths staged into sandbox workspace (missing: ${unstageable.map((ref) => ref.path).join(", ")})`, ); } const stagedPaths = stagingCtx.MediaPaths ?? []; @@ -1463,9 +1493,10 @@ async function prestageMediaPathOffloads(params: { // Map each ref to its post-staging path. Staged files become sandbox-relative // (e.g. `media/inbound/foo.pdf`) so the agent inside the container can read - // them; pass-through PDFs keep their absolute managed path. Host-side - // media-understanding resolves both via ctx.MediaWorkspaceDir plus the - // media-store root. Preserve the original attachment order. + // them; pass-through PDFs and managed PDFs that fell back from staging keep + // their absolute managed path (stagedPaths preserves the absolute path for any + // unstaged entry). Host-side media-understanding resolves both via + // ctx.MediaWorkspaceDir plus the media-store root. Preserve attachment order. const resolvedByRef = new Map(); refsToStage.forEach((ref, index) => { resolvedByRef.set(ref, {