feat(composer): non-image file attachments — materialized to disk, path-noted in the prompt - #899
Damian-Szczepanski wants to merge 1 commit into
Conversation
…th-noted in the prompt (#file-attachments) The composer accepted only image/* — a CSV or log dropped on it was silently ignored, with no way to hand the agent a data file from the cockpit. Any file type is now accepted (same 4x5MB caps, shared with images). Images keep their inline base64 path; non-image files extend the #357 mechanism instead: the server materializes them under .ai/cezar/runs/<runId>-images/ (original filename, sanitized; collisions suffixed) and appends the absolute-path note to the message text. Riding in the TEXT means the delivery ladder (live session, queued fold, starting-state buffer), restart recovery and the folded task bound all inherit the attachment without learning a new field. Task-start files travel as StartRunInput.files and are recorded on the run as taskFiles - deliberately separate from taskImages, which hydration re-encodes into image blocks at dequeue (a CSV must never become an image block). The opening-prompt note no longer requires image blocks to be present. Known v1 gap: files added while EDITING a stacked message are dropped (the PATCH schema is unchanged); files attached at post time survive edits via the text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review: feat(composer): non-image file attachments — materialized to disk, path-noted in the prompt
🎯 Summary
This PR extends the composer so any file — not just an image — can be handed to an agent. Non-image attachments are base64-uploaded on a new files field of POST /runs, POST /runs/:id/messages and POST /runs/:id/continue, materialized under the run's existing .ai/cezar/runs/<runId>-images/ directory, and referenced by absolute path through the existing #357 path note rather than being inlined as model content. Reviewed scope: the contract (packages/contract/src/runs.ts), the three server routes plus the new record field (packages/cezar/src/server/server.ts, packages/cezar/src/runs/store.ts), the materialization and prompt-assembly changes in packages/cezar/src/workflows/run.ts, and the composer/web intake and wiring under packages/web/src/.
The design choices here are genuinely good and worth calling out. Riding the path note inside the message text means the three-rung delivery ladder, the queued fold, the starting-state buffer and restart recovery all inherit attachments without a single rung learning a new field — that is the cheapest correct place to put it. Keeping taskFiles deliberately separate from taskImages is exactly right, because hydrateQueuedInput re-encodes taskImages into inline image blocks at dequeue and a CSV must never become an image block; the code comment says so and the test asserts it. persistFile is careful in the ways this repo asks for: basename() before anything else, a strict [A-Za-z0-9._-] charset so the serving route's :file param round-trips, leading dots stripped so .env cannot survive as a dotfile, a bounded collision loop, and the same wx exclusive-create guard persistImage uses. Moving the path note out of the if (images.length) block so a file-only task still gets it is the right fix and is covered by a test. The onSubmit two-arg call when no files are attached is a thoughtful piece of backward compatibility.
Two majors keep this from an approval. The first is a disk leak: attachments are written before the request is known to be acceptable, so every rejection path after that point permanently orphans up to 4 × ~5 MB. The second is that a file attached to a new task leaves no trace the user can see anywhere in the cockpit. Neither is architectural — both are contained fixes on top of a sound design.
Verdict
❌ request changes — driven by two majors: POST /runs/:id/messages and POST /runs/:id/continue materialize attachments to disk before the request is accepted, so every subsequent rejection path leaks the files with nothing referencing them (an error response that writes unbounded bytes to disk, which CODE_REVIEW.md classifies as major); and a task-start file attachment is invisible in the cockpit, because taskFiles is a store-only field that never reaches the contract, is rendered nowhere, and is excluded from the "attached to the task" note event. The full validation gate is green, there are no blockers, and no protected surface in BACKWARD_COMPATIBILITY.md is broken.
🧪 Validation Gate
| Command | Status | Notes |
|---|---|---|
npm run typecheck |
✅ PASS | Clean across contract, api-client, server and web. |
npm test |
✅ PASS | 324 test files, 6099 tests, 0 failures — including contract-parity*.test.ts and bc-route-inventory.test.ts. I did not reproduce the WSL-environment failures the PR description mentions; this branch is fully green here. |
npm run test:unit |
✅ PASS | 36 pass, 0 fail. |
npm run build |
✅ PASS | tsc → dist/, vite → packages/cezar/web/dist/, and the check:pack tarball gate all pass. |
npm run test:package |
✅ PASS | 15 pass, 0 fail — the release tarball installs and the dry-run CLI workflow runs. |
Findings
⚠️ Major
1. packages/cezar/src/server/server.ts:3727 (and the same shape at :3869) — attachments are written to disk before the request is accepted, so every rejection path after that point orphans them permanently.
In POST /runs/:id/messages, persistUserFile runs at line 3727, before any of the acceptance decisions below it: too many queued messages (3759), too many queued images (3763), prompt too long (3769) and session closed (3779) all return an error after the bytes are already on disk, with nothing in runs.json or in any queued message referencing them. POST /runs/:id/continue has the same shape — files are persisted at 3869 and continueRun can still answer 409 at 3888.
The concrete failure: post 4 × 5 MB of files to a run whose session has closed. The response is 409 session closed, the user's message never lands, and 20 MB stays under .ai/cezar/runs/<id>-images/ until the run is deleted. Nothing rate-limits or bounds that, so repeating the rejected request is an unbounded disk write through an endpoint that reports failure — the "unbounded input reaching files or processes" case CODE_REVIEW.md names as major. Note that images do not have this problem, which is what makes the asymmetry worth fixing rather than accepting: pasted images are persisted inside the manager only once a message is actually accepted, so a rejected image message writes nothing.
Fix: add a best-effort discard helper next to persistUserFile (an rmSync(path, { force: true }) per entry, swallowing failures the way dropOrphanImages already does) and call it on every post-persistence rejection in both routes, so a request that does not land leaves no bytes behind. The existing dropOrphanImages at packages/cezar/src/workflows/run.ts:1813 is the precedent for both the best-effort tolerance and the "never delete a still-referenced URL" care.
2. packages/cezar/src/workflows/run.ts:2819 and packages/contract/src/runs.ts:148 — a file attached to a new task is invisible in the cockpit.
taskImages reaches the user through three paths: it is on the contract's apiRunSchema (packages/contract/src/runs.ts:148), mainTranscriptSections renders it as the thread's first bubble (packages/web/src/routes/task-thread/session-transcript.tsx:78), and runAgentStep emits a N screenshots attached to the task note event (packages/cezar/src/workflows/run.ts:2819). taskFiles has none of them: it is added to runRecordSchema only (packages/cezar/src/runs/store.ts:142), never reaches the contract, is rendered nowhere, and the note event at 2819 is still gated on images?.length, so a file-only task emits nothing.
The concrete failure: attach bing-export.csv to a new task and submit. The agent does receive the path note in its opening prompt — that part works — but the thread shows only the typed text. The user gets no confirmation the file arrived, no filename, and no way to find or download it afterwards. That is the primary entry point for this feature, and it is the one place where the same action with an image gives full feedback.
Fix: at minimum, widen the note event at 2819 so a file-only or mixed attachment set also emits one, naming the files (1 file attached to the task: bing-export.csv) — that is the cheapest change, matches what images already do, and lands the record in the transcript the user reads. Ideally, also surface taskFiles on the contract's apiRunSchema and render a named chip beside the task bubble's thumbnails, reusing the chip the composer already draws for a pending non-image attachment; the serving route at server.ts:4058 already answers with application/octet-stream, so the file is downloadable as soon as its URL is exposed. If the chip rendering is out of scope for v1, say so in the PR description alongside the existing "Known v1 gap" entry, but the note event should not wait.
🔹 Minor
3. packages/cezar/src/server/server.ts:611 and :803 — the file-input shape is declared three times.
fileInputSchema now exists in packages/contract/src/runs.ts:612, again as a module-local const at server.ts:803, and a third time inlined into startRunSchema at server.ts:611 (which cannot reference the local one, since that is declared 190 lines later). AGENTS.md § The HTTP API is explicit: "Every request and response shape is a zod schema in packages/contract … never declare one in server.ts or in the api-client." Three copies of the same bounds is exactly the drift risk that rule exists to prevent — widen data in one place and the other two silently keep the old cap.
Fix: import fileInputSchema from @open-mercato/cezar-contract (server.ts already imports runHistoryQuerySchema and runIdParamSchema from there) and use that single value in startRunSchema, messageSchema and continueSchema, dropping the local declaration. The pre-existing local imageInputSchema is not a precedent to extend — it is the thing this rule was written about.
4. packages/cezar/src/server/server.ts:4065 — the served content-type is now chosen from a user-controlled filename, with no nosniff.
Before this PR the only files in <id>-images/ were written by persistImage, whose extension is derived from a validated image media_type, so the IMAGE_TYPES lookup at 4065 could only ever see an extension the server itself minted. Now the extension comes from the uploader's filename, so a caller who posts a file named payload.gif containing HTML gets it served back from the cockpit's own origin as image/gif, with no X-Content-Type-Options: nosniff and no Content-Disposition. The IMAGE_TYPES map is narrow (png/jpg/webp/gif only, no svg), so this is hardening rather than a live XSS, and the same-origin cockpit is the only consumer — but the input class changed and the response headers did not.
Fix: add 'x-content-type-options': 'nosniff' to the response headers at server.ts:4066, and consider content-disposition: attachment for anything that falls through to application/octet-stream. One line, no behavior cost for the existing thumbnail rendering.
5. packages/cezar/src/workflows/run.ts:771 — startRun silently drops files whose persistence fails, while the other two routes answer 500.
POST /runs/:id/messages (server.ts:3730) and POST /runs/:id/continue (server.ts:3872) both return 500 attachments could not be saved to disk when every persistUserFile call returns null. startRun filters the same nulls at line 774 and, when nothing survives, simply does not write taskFiles — the run starts as though no file had ever been attached, with no error and no note. The same failure therefore produces a loud error on two paths and silence on the third, which is the path the user is most likely to use.
Fix: at least emit a note event or record the failure so the user learns their attachment did not land; if startRun cannot signal failure to the route without a wider change, say so in a comment at 771 so the asymmetry reads as deliberate rather than missed.
6. BACKWARD_COMPATIBILITY.md § 3 — the new persisted RunRecord field is undocumented.
taskFiles (packages/cezar/src/runs/store.ts:142) is a new field on runs.json, a file § 3 describes as "written by one version, read by the next, and hand-editable by design". That section carries a bullet for every other additive record field — inputTokens/outputTokens for #737, diffStat.repointed for #751, queuedMessages for #472 — and CODE_REVIEW.md routes persisted-field changes through that document. The change itself is correctly additive and optional, so nothing is broken; the record of it is just missing.
Fix: add a § 3 bullet for runs.json → taskFiles, stating that it is optional, that absence reads as no attachments, and — the part worth writing down — why it is separate from taskImages (hydration re-encodes taskImages into inline image blocks at dequeue, so a non-image URL must never be added to that list).
7. packages/cezar/src/server/server.ts:3869 — the /continue files path has no test.
The PR covers the task-start path (pasted-attachments.test.ts) and the queued/messages path (queued-messages.test.ts) well, but POST /runs/:id/continue gained the same up-front materialization and note-building at 3869–3876 with nothing exercising it. It is also the path with the subtly different text assembly (parsed.data.text?.trim() on an optional field, versus .trim() on a defaulted one at 3733), which is precisely the kind of near-duplicate AGENTS.md § "Find every construction site" warns drifts apart.
Fix: add a case alongside the existing continue tests asserting that a file passed to /continue is materialized and that the resulting continuation text carries the path note, including the text-absent case where the note becomes the whole prompt.
8. packages/cezar/src/workflows/run.ts:1813 — a file attached to a stacked message is never cleaned up when that message is edited or removed.
dropOrphanImages deletes attachments a queued-message edit or deletion orphaned, and the #472 spec asks for exactly that ("the orphaned files under <id>-images/ are deleted best-effort at the same time"). Files cannot participate: they are referenced only from inside the message text, so nothing records which file belongs to which stacked entry, and candidates can never contain their URLs. Editing away a path note therefore leaves the file behind for the life of the run.
This one follows directly from the "no new field on the queued message" design the PR chose deliberately, and a leftover file is harmless and goes with the run, so I am not asking for the field to be added. Fix: note the limitation in a comment at dropOrphanImages so the next reader does not assume files are covered, and mention it beside the existing "Known v1 gap" in the PR description.
💅 Nit
9. packages/web/src/components/composer/composer-images.ts — the module and its identifiers now describe more than images.
MAX_IMAGES, MAX_IMAGE_BYTES, PendingImage, fileToPendingImage and the module name itself now govern all attachments, and the user-facing strings were correctly updated to say "attachment" while the code names were not. The header comment acknowledges this. Renaming is churn across several files and is entirely the author's call; if it stays, the current comment is enough.
10. packages/cezar/src/server/server.ts:3798 — the queued-message PATCH refine message no longer matches its POST sibling.
POST /runs/:id/messages now rejects with message needs text, an image or a file (:3765 via the schema at :815), while the PATCH on a stacked message still says message needs text or at least one image. Since PATCH genuinely does not accept files (the documented v1 gap), the old wording is not wrong — but the two errors now describe different vocabularies for the same concept and a user hitting both will read it as an inconsistency.
💥 Breaking Changes
- No exported/public symbol removed or renamed without a deprecation path — the change is purely additive:
fileInputSchema/FileInputare new exports, and no existing export changed shape. - No function signature changed in a breaking way —
ComposerProps.onSubmitgained an optional third parameter and the composer deliberately calls it with two arguments when no files are attached, so hosts that predate the parameter observe the exact legacy signature;ContinueAction.continueWithandpendingPlanOfgained optional trailing parameters the same way. - No required type field removed or narrowed —
filesis.default([])onmessageInputSchemaand.optional()everywhere else, so every existing request body still parses unchanged. - No HTTP route URL removed or renamed; no method changed —
bc-route-inventory.test.tsandroute-parity.test.tsare green. - No field removed or retyped in an existing response shape — no response schema was touched.
- No event or message name renamed or removed; no payload field removed — the NDJSON event vocabulary is unchanged, and the test asserts the base64 payload never enters the event log.
- No CLI command or flag renamed or removed.
- No database table or column renamed or removed —
taskFilesis a new optional field onRunRecord, so pre-existingruns.jsonfiles still parse, as § 3 requires. See minor 6 for the missing documentation of it. - No config key renamed and no default changed silently.
- Where a contract had to change, the old surface keeps working — the one user-visible wording change is the
messageInputSchemarefine message (message needs text or at least one image→message needs text, an image or a file), which is an error string rather than a protected response shape, and the intake it describes only widened.
One behavior change worth naming explicitly, though it is not a compatibility break: the composer's file input dropped accept="image/*" and screenFiles no longer discards non-images, so dragging a text file onto the composer now attaches it where it was previously ignored. That is the point of the PR, paste correctly stays image-only (composer.tsx:293), and the 4 × 5 MB caps are unchanged — but it is the line QA should exercise.
🧪 Test Coverage
What the tests actually cover is solid. pasted-attachments.test.ts drives a real task start end to end: it asserts the file lands under its sanitized-but-recognizable name (bing export (sierpien).csv → bing-export-sierpien-.csv), that the URL is recorded in taskFiles and that taskImages stays undefined — the assertion that actually protects the "a CSV must never become an image block" invariant — that the on-disk bytes round-trip, that the opening prompt carries the path note with zero image blocks, and that the base64 payload never reaches the NDJSON event log. The persistUserFile unit test covers the three sanitization cases that matter: traversal (../../etc/passwd → passwd), collision suffixing (raport.csv → raport-2.csv), the dotfile case (.env → env) and the all-hostile fallback (??? → attachment). queued-messages.test.ts covers both the stacked path note and the file-only message passing the emptiness refine. On the web side, composer-images.test.ts covers intake of non-images, the shared cap, the no-preview chip encoding, the untyped-file application/octet-stream fallback and both splitAttachments shapes.
The gaps, in the order I would add them:
POST /runs/:id/continuewithfiles— no test at all, per minor 7. Add it next to the existing continue tests: one case asserting materialization plus the path note in the continuation text, and one text-absent case where the note becomes the whole prompt.- The rejection-path cleanup from major 1 — once the discard helper exists, assert that a
409 session closedand a400 prompt too longon/messageseach leave<id>-images/with no new file. Without that assertion the leak can silently come back. - The note event from major 2 — assert that a file-only task emits an "attached to the task" note naming the file, so the user-visible acknowledgement is pinned by a test rather than by review.
queued-messages.test.tsstubspersistUserFileon a fakeRunManager, which is the right call for a route test, but it means no test exercises the route against the real manager. Thepasted-attachments.test.tssuite covers the realpersistFilefor the task-start path, so the risk is small — worth knowing rather than worth blocking on.
One required check is still pending at the time of this review. license/cla is PENDING — the CLA has not been signed for this PR, and only @Damian-Szczepanski can clear it. It is not a CI run, so there is nothing for this automation to wait on or report later; this review covers the code, and the CLA gate holds the merge independently of it. No other check is reported on this head, and branch protection is not readable on main, so every reported check is treated as required.
pat-lewczuk
added
changes-requested
|
🤖
|
|
Thanks @Damian-Szczepanski — review found actionable items, so I'm handing this PR back to you for the next pass. When the updates are pushed, re-request review and the automation can pick it up from the latest head. Two things worth flagging beyond the review itself:
The two majors are both contained fixes on top of a design I think is right — the delivery-ladder-through-text approach and the deliberate |
|
🤖 Full code review posted with the complete validation gate green (typecheck, autofix: skipped (fork PR — carrying the work forward would mean opening a replacement PR in the main repo and closing this one, which is the maintainer's call, not the automation's; the fixes are described precisely enough in the review to apply on the fork branch). |
3 participants
Motivation
The composer accepts only
image/*— a CSV export, a log, or any data file dropped on it is silently ignored (screenFilesskips non-images by design). There is currently no way to hand the agent a data file from the cockpit; the workaround is describing a file path in prose.This PR lets the user attach any file. Images keep their existing inline-base64 path untouched; non-image files extend the #357 pasted-attachments mechanism instead of inventing a new one.
Design
Wire. A new
files: [{ name, mediaType?, data }]field (same 4×5 MB bounds asimages) onPOST /runs,POST /runs/:id/messagesandPOST /runs/:id/continue.mediaTypeis advisory; the (sanitized)nameis the value — abing-export.csvmust stay recognizable on disk and in the path note.Server, messages/continue. Files are materialized up front into the run's existing
.ai/cezar/runs/<runId>-images/dir (original filename, sanitized to a URL-routable charset; collisions suffixed-2,-3… via the samewxexclusive-create guard aspersistImage), and the #357 path note is appended to the message TEXT. Riding in the text means the three-rung delivery ladder (live session → queued fold → starting-state buffer), restart recovery, and the folded-task bound all inherit the attachment without any rung learning a new field.Server, task create. Files travel as
StartRunInput.files;startRunmaterializes them once and records URLs in a newtaskFilesrecord field — deliberately separate fromtaskImages, becausehydrateQueuedInputre-encodestaskImagesinto inline image blocks at dequeue, and a CSV must never become an image block. At execute,taskFilesjoinsstartAttachmentsfor the opening-prompt note; the note no longer requires image blocks to be present (previously gated onimages?.length).Serving. The existing
/runs/:id/images/:fileroute already answers unknown extensions withapplication/octet-stream, so persisted files are downloadable as-is.Web. The composer accepts any file type (paperclip + drag-drop; paste stays image-only). Images and files share one pending array and the legacy 4-attachment cap; non-image files render as a named chip instead of a thumbnail, and
splitAttachmentsseparates the wire fields at submit. TheonSubmitseam stays two-arg when no files are attached, so hosts that predate the third parameter observe the exact legacy signature.Tests
pasted-attachments.test.ts: task-start file materializes under its sanitized name, is recorded intaskFiles(nottaskImages), its path lands in the opening prompt with zero image blocks, and the base64 payload never enters the event log;persistUserFiletraversal/sanitization/collision cases.queued-messages.test.ts: a stacked file rides the queued text as a path note; a file-only message passes the emptiness refine.splitAttachmentsshapes, no-preview chip encoding.mainon my machine (the pre-existing WSL-environment failures are identical on both branches);npm run typecheckandnpm run build(incl.check:pack) pass.Known v1 gap
Files added while EDITING a stacked message are dropped (
queuedMessagePatchSchemais unchanged — zod strips the unknown key). Files attached at post time survive edits, since the path note lives in the message text. Called out in code comments; happy to extend the PATCH path if you want it in scope.Follow-up available
A second, deliberately separate commit (branch
feat/attachments-libraryon my fork) adds a per-project.ai/cezar/attachments/folder collecting a copy of every user upload (content-deduped, best-effort, written-never-required per AGENTS.md). Kept out of this PR to keep the core reviewable — say the word if you'd like it included.🤖 Generated with Claude Code