fix: refuse chat attachments sent without text and shorten comments

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Guilhem Lemouel
2026-09-17 09:21:23 +02:00
co-authored by Claude Opus 5
parent 93aa7d5fdd
commit a35bd0b190
9 changed files with 40 additions and 46 deletions
+2 -2
View File
@@ -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
+5 -11
View File
@@ -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<string, string> = {
'image/png': 'png',
@@ -31,11 +28,8 @@ const EXTENSION_BY_MEDIA_TYPE: Record<string, string> = {
}
/**
* 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]
+5 -1
View File
@@ -107,7 +107,11 @@ class ChatImpl implements Chat {
sendMessage = async (text: string, options: SendMessageOptions = {}): Promise<void> => {
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')
}
+1 -1
View File
@@ -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
}
+12
View File
@@ -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))
@@ -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
@@ -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. */
@@ -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
@@ -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