From a35bd0b1901b29c4fe6140384b04f1426af8a0c6 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 17 Sep 2026 09:21:23 +0200 Subject: [PATCH] fix: refuse chat attachments sent without text and shorten comments Co-Authored-By: Claude Opus 5 (1M context) --- chat-sdk/README.md | 4 ++-- chat-sdk/src/attachments.ts | 16 +++++----------- chat-sdk/src/chat.ts | 6 +++++- chat-sdk/src/types.ts | 2 +- chat-sdk/test/attachments.test.ts | 12 ++++++++++++ .../components/copilot/chat/AIChatInput.svelte | 17 ++++++----------- .../lib/components/copilot/chat/blobUtils.ts | 11 +++-------- .../flows/conversations/agentAttachmentInput.ts | 9 +++------ .../lib/components/inputTransformEnv.svelte.ts | 9 +++------ 9 files changed, 40 insertions(+), 46 deletions(-) diff --git a/chat-sdk/README.md b/chat-sdk/README.md index 0d747f8f20..43e3c1ea1a 100644 --- a/chat-sdk/README.md +++ b/chat-sdk/README.md @@ -248,8 +248,8 @@ objects (the object for a single-file input). Once the uploads return, the pendi message lists them in `attachments`, as `{ input, s3, filename }` references. The name's extension is corrected to the file's media type for PNG, JPEG and PDF, because the worker reads the type off the key. -A failed upload rejects `sendMessage` before any run starts, and `stop()` during the -upload aborts it; both leave the transcript as it was. The workspace needs object +Files need message text to go with them. A failed upload rejects `sendMessage` before any +run starts, and `stop()` during the upload aborts it; both leave the transcript as it was. The workspace needs object storage set up. With Enterprise advanced storage permissions, the user needs read, write and delete on `windmill_uploads/*`, which the default rules grant. The upload goes through `job_helpers`, so a restricted token needs `job_helpers:write`; a sandboxed raw app cannot diff --git a/chat-sdk/src/attachments.ts b/chat-sdk/src/attachments.ts index 140cc310cb..328c9076fa 100644 --- a/chat-sdk/src/attachments.ts +++ b/chat-sdk/src/attachments.ts @@ -17,12 +17,9 @@ export interface UploadedAttachment { } /** - * The extension each type a chat composer sends must be stored under. - * - * The worker reads an attachment's media type from the object key and nothing else — - * `mime_guess::from_path` in `windmill-ai/src/image_handler.rs`, falling back to - * `image/png` when it can read no extension — and never from the content type stored - * beside it. So the key's extension is a claim about the bytes, and it has to be true. + * The extension each type must be stored under. The worker reads an attachment's media type + * from the key's extension only (`mime_guess` in `windmill-ai/src/image_handler.rs`, falling + * back to `image/png`), never from the stored content type, so the extension must be true. */ const EXTENSION_BY_MEDIA_TYPE: Record = { 'image/png': 'png', @@ -31,11 +28,8 @@ const EXTENSION_BY_MEDIA_TYPE: Record = { } /** - * The name an attachment is stored under. A composer commonly re-encodes every image to PNG - * or JPEG, so keeping the picked `photo.webp` would hand the provider PNG bytes labelled webp, - * which Anthropic rejects outright; and a PDF picked without an extension would be read back - * as the `image/png` fallback. A type not listed is left as picked — other files upload byte - * for byte, so their name is already true. + * The name an attachment is stored under: the picked name with the extension its media type + * needs, e.g. a `photo.webp` re-encoded to PNG becomes `photo.png`. Other types keep their name. */ export function storedAttachmentName(filename: string, mediaType: string): string { const extension = EXTENSION_BY_MEDIA_TYPE[mediaType] diff --git a/chat-sdk/src/chat.ts b/chat-sdk/src/chat.ts index e42bae0505..12d7cdf802 100644 --- a/chat-sdk/src/chat.ts +++ b/chat-sdk/src/chat.ts @@ -107,7 +107,11 @@ class ChatImpl implements Chat { sendMessage = async (text: string, options: SendMessageOptions = {}): Promise => { const content = text.trim() - if (!content) return + if (!content) { + // A run needs a message; files alone would otherwise be dropped without a word. + if (options.attachments?.length) throw new Error('windmill-chat: attachments need a message to go with them') + return + } if (this.#turn) { throw new Error('windmill-chat: a message is already being answered; call stop() first') } diff --git a/chat-sdk/src/types.ts b/chat-sdk/src/types.ts index 666b0aa623..aaf72e68aa 100644 --- a/chat-sdk/src/types.ts +++ b/chat-sdk/src/types.ts @@ -147,7 +147,7 @@ export interface SendMessageOptions { * and the run never starts; `stop()` during the upload does the same with an `AbortError`. */ attachments?: AttachmentUpload[] - /** Required with `attachments`. With `multiple: false`, more than one attachment is refused before anything uploads. */ + /** Required with `attachments`, which also need message text. With `multiple: false`, more than one attachment is refused before anything uploads. */ attachmentsInput?: AttachmentsInput } diff --git a/chat-sdk/test/attachments.test.ts b/chat-sdk/test/attachments.test.ts index 84f40dd14c..e93a10f4be 100644 --- a/chat-sdk/test/attachments.test.ts +++ b/chat-sdk/test/attachments.test.ts @@ -152,6 +152,18 @@ describe('sendMessage with attachments', () => { expect(chat.getState().messages).toEqual([]) }) + test('refuses attachments without message text, before uploading', async () => { + const { fetch, calls } = fetchMock(upload, run, answer) + const chat = createChat(options(fetch)) + await expect( + chat.sendMessage(' ', { + attachments: [{ name: 'a.pdf', data: pdf }], + attachmentsInput: { name: 'files', multiple: true } + }) + ).rejects.toThrow('need a message') + expect(calls).toHaveLength(0) + }) + test('refuses attachments without an input to put them in', async () => { const { fetch, calls } = fetchMock(upload, run, answer) const chat = createChat(options(fetch)) diff --git a/frontend/src/lib/components/copilot/chat/AIChatInput.svelte b/frontend/src/lib/components/copilot/chat/AIChatInput.svelte index 8682a81e2d..d3cc2c8bd9 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatInput.svelte +++ b/frontend/src/lib/components/copilot/chat/AIChatInput.svelte @@ -485,11 +485,9 @@ let pendingBlobs = $state(0) /** - * Attach non-image files through the lane the host actually reads: a host that decodes - * them takes text, one that forwards them verbatim (to object storage) takes blobs, and - * its narrower `accept` is re-applied because a drop and a paste both bypass the picker's - * own filtering. Every way of attaching goes through here, so no route can take the lane - * the host ignores and drop the file at send. + * Attach non-image files through the lane the host reads: text for a host that decodes + * them, blobs for one that forwards them verbatim. The picker, a drop and both pastes all + * route here, and `accept` is re-applied since drops and pastes bypass the picker's filter. */ export async function addNonImageFiles(files: File[]) { if (files.length === 0) return @@ -1170,12 +1168,9 @@ }) /** - * Clipboard files on the plain composer. ContextTextarea does this for the rich one; a - * host that attaches but renders the plain field would otherwise take files from the `+` - * and from a drop and silently ignore the same file pasted. - * - * Only when the clipboard carries no text, as there: a spreadsheet or browser copy puts a - * bitmap alongside the text, and pasting a cell range must paste the cells. + * Clipboard files on the plain composer, as ContextTextarea does for the rich one. Only + * when the clipboard has no text: a spreadsheet copy carries a bitmap next to the text, + * and pasting a cell range must paste the cells. */ function handlePlainPaste(e: ClipboardEvent) { if (!chatHost.supportsMessageAttachments) return diff --git a/frontend/src/lib/components/copilot/chat/blobUtils.ts b/frontend/src/lib/components/copilot/chat/blobUtils.ts index 50240b5fa5..41b5c76183 100644 --- a/frontend/src/lib/components/copilot/chat/blobUtils.ts +++ b/frontend/src/lib/components/copilot/chat/blobUtils.ts @@ -1,12 +1,7 @@ /** - * Message-scoped attachments that are neither an image nor readable text — a PDF - * being the case that matters. They ride the composer next to images and files, - * as chips cleared on send, and reach the host through `ChatSendRequestOptions`. - * - * The bytes are kept verbatim, unlike an image (which normalises to a bounded - * PNG/JPEG for the model) and unlike a text file (which is decoded to a string): - * a host that forwards these to object storage has to upload what the user - * picked, not a re-encoding of it. + * Message attachments kept as their original bytes, such as a PDF, for a host that + * forwards them to object storage. Images are re-encoded and text files decoded instead, + * so neither lane can carry what the user picked unchanged. */ /** Blobs one message may carry — the same slot cap images and text files use. */ diff --git a/frontend/src/lib/components/flows/conversations/agentAttachmentInput.ts b/frontend/src/lib/components/flows/conversations/agentAttachmentInput.ts index 42aaad844c..805b6fea1a 100644 --- a/frontend/src/lib/components/flows/conversations/agentAttachmentInput.ts +++ b/frontend/src/lib/components/flows/conversations/agentAttachmentInput.ts @@ -52,12 +52,9 @@ export type AgentChatInput = { const FLOW_INPUT_REF = /flow_input\??\.([A-Za-z_$][\w$]*)/g /** - * The flow input a transform is fed by, when exactly one feeds it. - * - * The expression need not be a bare pass-through — a step commonly reshapes what it - * reads, e.g. `(flow_input.files || []).map(f => ({ bucket: f.storage, key: f.s3 }))`. - * Writing that input is still right, because the expression consumes it. Two or more - * inputs are ambiguous: the composer would have no way to say which one it is editing. + * The flow input a transform reads, when it reads exactly one. The expression may reshape + * it (`(flow_input.files || []).map(...)`) and still counts; two inputs name none, since the + * composer could not tell which one it edits. */ export function flowInputRef(transform: InputTransform | undefined): string | undefined { if (transform?.type !== 'javascript') return undefined diff --git a/frontend/src/lib/components/inputTransformEnv.svelte.ts b/frontend/src/lib/components/inputTransformEnv.svelte.ts index c8d34dc391..9fe42c166e 100644 --- a/frontend/src/lib/components/inputTransformEnv.svelte.ts +++ b/frontend/src/lib/components/inputTransformEnv.svelte.ts @@ -18,12 +18,9 @@ function storageConfigured(storage: LargeFileStorage | undefined): boolean { } /** - * Whether the workspace can store uploaded files. Call during component initialisation and - * read `.current` where the answer is used. - * - * Assumed configured until this workspace's own answer lands, so a surface that says "no - * storage configured" never flashes that on navigation, nor claims it merely because the - * fetch failed. + * Whether the workspace can store uploaded files; read `.current`. Assumed configured until + * this workspace's own answer lands, so "no storage" never flashes on navigation or shows + * merely because the fetch failed. */ export function useWorkspaceStorageConfigured(ws: () => string | undefined): { readonly current: boolean