feat(composer): non-image file attachments, restored on current main

[Damian-Szczepanski]

Restores the file-attachments feature that never landed: the composer on main still only accepted image/* — a CSV/log/JSON dropped on it was silently ignored, and the picker filtered to images.

This is #899 (6363000) rebased onto current main as a clean cherry-pick, plus its follow-up commit from the same branch family:

• feat(composer): non-image file attachments — any file type via paperclip/paste/drag-drop (same 4×5 MB caps). Images stay inline base64; non-image files are materialized under .ai/cezar/runs/<runId>-images/ and the absolute path is appended to the message text, so the delivery ladder, restart recovery and folded-task bound all inherit attachments for free. Non-image attachments render as a named chip in the composer; the file input no longer has an accept filter.
• feat(attachments): per-project library — every user upload also copied to .ai/cezar/attachments/ (pre-collision names + content dedupe, best-effort).

Supersedes #899 (same feature commit; that branch is 4 commits behind main and lacks the library follow-up).

Verified on this branch: npm run typecheck ✓, npm run build (incl. check:pack) ✓, all 171 attachment-related tests ✓. The 9 remaining suite failures reproduce identically on clean main in the same environment (WSL-specific) and are unrelated.

Known v1 gap (unchanged from #899): files added while editing a stacked message are dropped; files attached at post time survive edits via the text. The in-flight #915 touches the same prompt-note wording and can merge independently.

🤖 Generated with Claude Code

[CLAassistant]

All committers have signed the CLA.

[pat-lewczuk]

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-08-28T21:52:29Z. Other auto-skills will skip this PR until the lock is released.

[pat-lewczuk]

🔍 Code Review: feat(composer): non-image file attachments, restored on current main

🎯 Summary

This PR restores the non-image attachment feature on current main and adds a per-project attachments library on top. The composer's intake (screenFiles) stops filtering to image/*, the <input type="file"> loses its accept filter, and a shared pending array is split at submit time (splitAttachments) into the inline images wire field and a new files field. Server-side, RunManager.persistFile materializes each non-image upload under .ai/cezar/runs/<runId>-images/ with a sanitized version of its original filename, and the absolute path is appended to the message text via the existing pastedAttachmentsText note. Reviewed scope: the 17 changed files across packages/cezar (run manager, server routes, run store), packages/contract, and packages/web (composer, API client, new-task and task-thread routes).

The core design decision is genuinely good and worth calling out: riding the attachment inside the message text rather than adding a new field to every rung means the three-rung delivery ladder, the queued-message fold, the starting-state buffer and restart recovery all inherit attachments with no new plumbing, and the taskFiles / taskImages split correctly prevents a CSV from ever being re-encoded as an inline image block at hydration. The filename sanitization in persistFile is careful — basename() first, then a strict charset, then a leading-[.-] strip — and I verified it defeats both POSIX traversal (../../etc/passwd → passwd) and the Windows-separator variant that basename() alone would miss. Test coverage for the new units is real and specific rather than decorative.

What holds it back is not the feature but its housekeeping. The second commit introduces a brand-new state directory, .ai/cezar/attachments/, that is never registered with the .gitignore the CLI maintains in users' repositories — so every file a user uploads lands in a git-visible folder inside their project. On top of that there are four correctness and hardening gaps, the sharpest being two project-scoped API client helpers that accept the widened input type and silently discard the new files field with no compile error.

Verdict

❌ request changes — one blocker (the new .ai/cezar/attachments/ directory is not added to ensureDataGitignore, which both AGENTS.md and BACKWARD_COMPATIBILITY.md §3 require in the same PR, and which causes user-uploaded files to appear in users' git status) and four majors (silent files drop in the project-scoped client helpers; the image-serving route now handing out arbitrary user bytes without the nosniff/sandbox hardening its sibling raw path applies; an unbounded, write-only attachments library; and silent partial-failure handling when only some attachments materialize). The validation gate is otherwise green and the feature's central design is sound — none of these findings requires rethinking the approach.

🧪 Validation Gate

Command	Status	Notes
npm run typecheck	✅ PASS	All four projects (contract, client, server, web) clean.
npm test	⚠️ PASS (with pre-existing environment failures)	12 suites fail on the PR head. I ran the identical suite on a clean origin/main worktree in the same environment as a baseline: 13 suites fail there — a strict superset of the PR's 12. Every failure is in the git-environment/health/route-parity family (git.test.ts, git-changes.test.ts, health-forge.test.ts, projects-api.test.ts, route-parity.test.ts, the task-git web tests), none touch the changed code, and the count varies run to run. No regression is attributable to this PR, and the author's claim on that point is verified. The PR head also runs 6187 tests vs main's 6177 — the 10 new tests all pass.
npm run test:unit	✅ PASS	The node:test core-module suite is clean.
npm run build	✅ PASS	Includes the check:pack tarball gate.
npm run test:package	✅ PASS	Packaged CLI E2E is clean.

Findings

⛔ Blocker

packages/cezar/src/index.ts:666 — the new .ai/cezar/attachments/ state directory is missing from ensureDataGitignore, so every user upload becomes a tracked file in the user's repository.

copyToAttachmentsLibrary (packages/cezar/src/workflows/run.ts:3450) creates .ai/cezar/attachments/ and copies every user upload into it. The wanted list in ensureDataGitignore — the .ai/cezar/.gitignore that cezar writes and maintains inside each user's project — enumerates runs.json, runs/, worktrees/, tmp/, todos.json, launch-key and the automation files, but not attachments/. That list is a per-entry allowlist rather than a blanket *, and deliberately so: .ai/cezar/workflows/ and .ai/cezar/skills/ are meant to be committable. I verified the maintained file's current contents; attachments/ is absent, so the folder is not covered by anything.

The concrete failure: a user drags q3-revenue.csv, a production log, or a .env-shaped export onto the composer. It is copied into .ai/cezar/attachments/, shows up in their next git status as untracked, and a git add -A commits it — potentially to a public repository. Every pasted screenshot is archived there too (run.ts:3411 calls the copy for the pasted prefix), so this is not a rare path but the default one for anyone who attaches anything. This is the exact hazard CODE_REVIEW.md names under Security ("No secrets in state files: nothing under .ai/cezar/ may contain tokens or credentials") and it violates two explicit, documented rules: AGENTS.md line 128 ("Keep .ai/cezar/.gitignore maintenance (ensureDataGitignore) in sync with any new state file") and BACKWARD_COMPATIBILITY.md §3 ("any new run-data file must be added there in the same PR").

Note that this is invisible when testing inside the cezar repo itself, because cezar's own root .gitignore carries a blanket .ai/cezar/ entry — which is very likely why it was not caught. It only reproduces in a consumer repo.

Fix: add 'attachments/' to the wanted array in ensureDataGitignore. Because ensureDataGitignore appends only missing entries, existing installs pick it up on the next openStore, so no migration is needed. Please add a regression test asserting attachments/ is present in the generated .gitignore, alongside the existing state-file entries.

⚠️ Major

1. packages/web/src/api/client.ts:1482 and packages/web/src/api/client.ts:1268 — the project-scoped twins of sendMessage and continueRun accept the widened input types but silently discard files.

sendMessage (line 1633) was updated to forward json: { text, images, files }, and continueRun (line 1243) gained its ...(opts.files !== undefined ? { files: opts.files } : {}) spread. Their project-scoped siblings — sendProjectRunMessage, which still sends json: { text: message.text ?? '', images: message.images ?? [] }, and continueProjectRun, whose spread list stops at agentProfile — were not. Both take the same widened types (MessageInput now carries files; ContinueOptions now carries files), so a caller can pass attachments, get no type error, receive a 200, and have the files vanish: the server's messageSchema defaults the absent files to [], so nothing anywhere reports a problem.

This is latent rather than live today — the only caller that passes a projectId is useAskAnswer (packages/web/src/routes/task-thread/ask-answer.ts:116-117), which sends { text } only. But the projectId variants exist precisely for the global Tasks page, which spans the registry, and the moment any surface there routes a composer through them uploads disappear silently. A one-line divergence that the type system cannot catch and that fails without an error is worth closing while the context is fresh.

Fix: mirror the two updated helpers — add files: message.files ?? [] to sendProjectRunMessage's json, and the ...(opts.files !== undefined ? { files: opts.files } : {}) spread to continueProjectRun. packages/web/src/api/client.test.ts already asserts the exact request shape for the unscoped sendMessage; adding the scoped twin to that table would pin the parity permanently.

2. packages/cezar/src/server/server.ts:4072-4085 — GET /api/v1/runs/:id/images/:file now serves arbitrary user-supplied bytes without the nosniff and sandbox-CSP hardening its sibling raw-file path applies.

Before this PR, everything in <runId>-images/ was written by persistImage, whose extension is derived from the image media type and can only be one of png, jpg, webp, gif, img (run.ts:3394-3399). The route's response headers reflect that assumption: it sets content-type from the IMAGE_TYPES map with an application/octet-stream fallback, plus a long-lived cache-control, and nothing else. persistFile now writes files with any user-chosen extension into that same directory, so the route's guarantee has silently changed from "only ever image bytes" to "arbitrary bytes the user uploaded."

The repo already documents the correct shape for exactly this situation. The raw branch of GET /api/v1/runs/:id/files sends x-content-type-options: nosniff and content-security-policy: default-src 'none'; style-src 'unsafe-inline'; sandbox (server.ts:4194-4195), and its own doc comment (server.ts:977) describes the protections as "image extensions only, size cap, nosniff, sandbox CSP". The images route now carries none of them while serving a strictly wider set of content.

I want to be precise about severity rather than overstate it: IMAGE_TYPES has no svg entry, so an uploaded evil.svg is served as application/octet-stream, and current Chrome and Firefox will not sniff that into an executable document. So I did not find a working same-origin XSS today. But this is a same-origin route on the process that holds the launch key and executes agents with file access, the hardening its sibling applies costs two headers, and the safety currently rests on a browser behavior rather than on anything the route asserts. CODE_REVIEW.md puts server-security regressions in the blocker/major band on principle, not on demonstrated exploitability.

Fix: add 'x-content-type-options': 'nosniff' and the same sandbox CSP to this route's headers, and consider a content-disposition: attachment for any extension outside IMAGE_TYPES so non-image attachments download rather than render.

3. packages/cezar/src/workflows/run.ts:3429-3454 — the attachments library is write-only and unbounded: nothing reads it, nothing prunes it, and pasted screenshots are archived under provenance-free sequence names.

copyToAttachmentsLibrary's doc comment sells the folder as a place where uploads "survive run cleanup and are browsable in one place." I grepped the whole repository: the only references to .ai/cezar/attachments are the writer itself and its tests. There is no route, no CLI command, no cockpit surface, and no entry in packages/cezar/src/runs/retention.ts, so nothing browses it and nothing ever reclaims it. Every user upload is duplicated there permanently.

The naming compounds it. The copy is also invoked from persistImage for the pasted prefix (run.ts:3411), where the name is a bare sequence number: pasted-1.png, pasted-2.png. Those numbers restart per run, so run A's pasted-1.png and run B's different pasted-1.png collide, and the content-inequality branch archives the second as pasted-1-2.png. After a few months of normal use the folder is a flat pile of pasted-N-M.png files carrying no run id, no date, and no original name — the opposite of browsable, and (per the blocker above) sitting in the user's git working tree.

The dedupe logic itself is well built — matching on the pre-collision name plus byte-identical content is the right call, and the "never let the library break the upload" degradation matches the repo's "written, never required" convention. The problem is scope, not implementation.

Fix, in roughly increasing cost: at minimum, give the archived copies provenance (prefix with the run id, or keep the user's original filename for pasted images rather than the sequence name) and add a size or age bound so the folder cannot grow without limit. Better still, given that nothing consumes the folder yet, consider landing the file-attachments commit on its own and holding the library commit until the surface that browses it exists — that also shrinks this PR back to one reviewable feature.

4. packages/cezar/src/server/server.ts:3733-3739, server.ts:3885-3891 and packages/cezar/src/workflows/run.ts:768-772 — a partial materialization failure is silent, and on the run-start path a total failure is silent too.

All three call sites use the same shape: map every attachment through persistFile, then .filter() out the nulls that a failed write returns. The two route handlers guard only the all-or-nothing case (parsed.data.files.length > 0 && persistedFiles.length === 0 → 500). If a user attaches three files and one write fails — a full disk, a permission problem, a name that exhausts the 100-attempt collision loop — the request answers 200, the path note lists two files, and neither the user nor the agent is told the third is missing. The agent then works from an incomplete set while believing it has everything, which is the kind of quiet wrongness that is expensive to debug later.

startRun is weaker still: if (persistedFiles.length) { … } records taskFiles when anything survived and does nothing at all when nothing did. A user whose attachments all failed to materialize gets a normally-started run with no attachments and no error anywhere.

Fix: compare persistedFiles.length against parsed.data.files.length and fail the request when they differ (the message and continue routes can reuse the existing 500 with a count, e.g. "2 of 3 attachments could not be saved to disk"). For startRun, where failing the run outright is likely too aggressive, emit a note event naming the attachments that did not materialize so the transcript records the loss.

🔹 Minor

1. packages/web/src/components/composer/composer.tsx:295 — onPaste still filters to image/*, so ⌘V of a non-image file is silently ignored.

The PR description states the feature accepts "any file type via paperclip/paste/drag-drop". The paperclip path works (the accept="image/*" attribute was removed) and onDrop passes dataTransfer.files through unfiltered, but onPaste still runs .filter((item) => item.type.startsWith('image/')) before reaching addFiles. Copying a CSV in the OS file manager and pressing ⌘V in the composer therefore does nothing at all — no attachment, no rejection toast, no explanation. The three intake paths the feature advertises as equivalent are not.

Fix: drop the image/* predicate and instead filter event.clipboardData.items to item.kind === 'file', letting screenFiles apply the caps as it does for the other two paths. If keeping paste image-only is deliberate, please say so in the PR body and in the tooltip, since the current text ("Attach a file (or paste a screenshot)") reads as if paste were the narrower path by accident.

2. packages/web/src/components/composer/composer-images.ts:70-80 — a 0-byte file fails the whole send with a raw zod message.

screenFiles checks only the upper bound (file.size > MAX_IMAGE_BYTES). An empty file encodes to data: '', which the server's fileInputSchema rejects with data: z.string().min(1), so the entire message is bounced with a schema error rather than a human sentence. Before this PR the case could not arise, because non-images were dropped client-side and a 0-byte image is not something a browser produces from a paste. CODE_REVIEW.md asks that user-facing errors be "one human-readable line".

The draft is restored on error so nothing the user typed is lost, which is why this is minor rather than major. Fix: reject file.size === 0 in screenFiles with a sentence in the same style as the existing ones, e.g. `${file.name || 'attachment'} is empty`.

3. packages/cezar/src/server/server.ts:3726-3776 — attachments are written to disk before the queue-cap and fold-length checks, and files are not counted against MAX_QUEUED_IMAGES.

The persistUserFile loop runs immediately after the provider gate, ahead of the MAX_QUEUED_IMAGES check and the foldedLength guard that can still answer 400. A message rejected by either check therefore leaves its files on disk with nothing referencing them, and dropOrphanImages cannot reclaim them because it only considers URLs recorded in queuedMessages[].images — attachments live in the message text instead. Relatedly, MAX_QUEUED_IMAGES counts stacked images only, so the stack-wide attachment ceiling is now enforced over one of the two attachment kinds.

Fix: move the materialization below both guards (nothing above it depends on the persisted paths except the text used by foldedLength, which can be computed from a projected note), and include files in the stackedImages tally so the cap covers everything a stack can hold.

4. packages/contract/src/runs.ts:148 — taskFiles is absent from the contract's runRecordSchema, and non-image attachments are consequently invisible everywhere in the cockpit.

packages/cezar/src/runs/store.ts gained taskFiles, and the routes hand the record out, but the contract mirror — the file whose parity test states it "must describe EXACTLY what the runs routes send — no wider, no narrower" — was not updated. The compile-time Mutual<> check does not catch it because an added optional property is assignable in both directions, so this drift passes typecheck silently.

The practical consequence is a visible product gap. session-transcript.tsx:78 builds the first bubble from run.taskImages only, the note event announcing attachments is still emitted solely when images.length is non-zero (run.ts:2847), and the typed client cannot even see taskFiles. So a user attaches a CSV, sees a chip in the composer, presses send — and the thread's opening bubble shows no trace of it. The agent gets the file; the human loses the record of having sent it. This is worth stating in the PR body's "known v1 gaps" section even if the rendering work lands separately.

Fix: add taskFiles: z.array(z.string()).optional() to the contract's runRecordSchema with a doc comment matching the store's, and (separately, if you prefer) extend the transcript's first bubble and the attachment note to cover non-image files.

5. packages/web/src/components/composer/composer-images.ts — naming no longer matches behavior.

The module is composer-images.ts, the type is PendingImage, the encoder is fileToPendingImage, and the composer's state is images/setImages/imagesRef — all of which now routinely hold CSVs and logs. The JSDoc has been updated to say "attachment", which makes the mismatch more conspicuous rather than less. CODE_REVIEW.md lists naming drift under Minor. A rename to PendingAttachment / fileToPendingAttachment / attachments (the module filename can stay if churn is a concern) would make the next reader's job much easier.

💅 Nit

1. packages/web/src/components/composer/composer.tsx:322 — the conditional two-arg call is more machinery than the compatibility it buys.

await (files.length ? onSubmit(body, inlineImages, files) : onSubmit(body, inlineImages)) exists so hosts that predate the third parameter "keep observing the exact legacy signature". In JavaScript, a handler declared with two parameters is unaffected by a third argument, and every in-repo host has already been updated, so the branch protects against nothing observable outside a test that counts arguments. await onSubmit(body, inlineImages, files) with files defaulting to [] would read more plainly. Author's call — if a test does assert the arity, that assertion is the thing worth revisiting.

2. packages/cezar/src/server/server.ts:606-613, 899 — the schema's maximum now exceeds the global body limit.

images and files are each capped at 4 entries of up to 7,000,000 base64 characters, so a request can legitimately satisfy both schemas at roughly 56 MB while GLOBAL_BODY_LIMIT is 32 MiB. The cockpit never hits this because MAX_IMAGES caps the two kinds at 4 combined, but a scripted client that reads the schema bounds gets a bare 413 instead of the schema's friendly message. Either document that the two arrays share an effective ceiling or cap their combined size in the refine.

💥 Breaking Changes

• No exported/public symbol removed or renamed without a deprecation path — everything added (fileInputSchema, FileInput, persistUserFile, splitAttachments, taskFiles) is additive.
• No function signature changed in a breaking way — Composer.onSubmit gains an optional third parameter, pendingPlanOf an optional fourth; both are backward compatible.
• No required type field removed or narrowed — taskFiles and every new files field are optional or defaulted.
• No HTTP route URL removed or renamed; no method changed for an existing operation.
• No field removed or retyped in an existing response shape — taskFiles is an additive record field. (See Minor 4: it is additive on the wire but undocumented in the contract mirror, which is a drift finding rather than a break.)
• No event or message name renamed or removed; no payload field removed.
• No CLI command or flag renamed or removed; no machine-parsed output format changed.
• No database table or column renamed or removed — not applicable; state is file-based.
• No config key renamed and no default changed silently.
• Where a contract had to change: old surface kept working through a deprecation window, with migration notes — the .ai/cezar/ state contract in BACKWARD_COMPATIBILITY.md §3 requires a new run-data file to be registered with ensureDataGitignore in the same PR, and .ai/cezar/attachments/ was not (see the Blocker). No user-facing state becomes unreadable, so this is a rule violation rather than a data break, and the fix is a one-line addition to the wanted list.

Two error-message strings change as a side effect: 'message needs text or at least one image' becomes 'message needs text, an image or a file', and the composer's cap toast changes "images" to "attachments". Both are human-readable prose rather than machine-parsed output, both are updated in their tests, and both are more accurate after the change — I do not consider them breaking, but flagging them since they are user-visible.

🧪 Test Coverage

The new tests are specific and cover the parts most likely to break. pasted-attachments.test.ts exercises the full task-start path end to end — the sanitized on-disk name, taskFiles being set while taskImages stays undefined (the distinction the whole design rests on), the path note landing in the opening prompt with imageCount === 0, and an explicit assertion that the base64 payload never enters the NDJSON event log. The persistUserFile unit test covers traversal (../../etc/passwd → passwd), collision suffixing, the dotfile case (.env → env), and an all-hostile name degrading to attachment. queued-messages.test.ts covers both the path note riding the stacked text and a file-only message passing the emptiness refine. composer-images.test.ts covers splitAttachments in both directions and the shared cap. That is genuinely good coverage of the happy paths and the security-relevant sanitizer.

The gaps map onto the findings above, and each would have caught one of them:

1. No test asserts attachments/ is in the generated .gitignore. The existing ensureDataGitignore coverage should gain attachments/ alongside runs/ and tmp/ — this is the test that turns the blocker into a permanent regression guard.
2. No test covers partial materialization failure. Add a case where persistFile is stubbed to return null for one of three attachments and assert the route reports the loss rather than answering 200 (Major 4).
3. No test covers the project-scoped client helpers' request shape. client.test.ts pins sendMessage's body exactly; adding sendProjectRunMessage and continueProjectRun to that table would have caught the silent files drop at review time (Major 1).
4. No test covers the 0-byte file through screenFiles (Minor 2), nor the paste path with a non-image file (Minor 1) — the composer tests exercise paste only with pngFile(...), which is exactly why the image/* filter survived.
5. copyToAttachmentsLibrary's dedupe is tested, but its unbounded growth is not — there is no test pinning what the library does after many runs, which is the behavior Major 3 is about.

One note on the suite as a whole: I could not use the vitest run as a pass/fail signal on its own because this environment fails 13 suites on clean main. I established that baseline explicitly rather than taking the PR description's word for it, and the PR head fails a strict subset (12 of those same 13). The 10 tests this PR adds all pass.

[pat-lewczuk]

🤖 om-auto-review-pr — 🏷️ label rationale

• ⛔ blocked — progress depends on an external blocker: the required check Unit, build, E2E, and package is red on a test this PR does not touch and cannot fix. packages/cezar/test/e2e/release-snapshot.test.ts:165 leaks the ambient GITHUB_RUN_ATTEMPT into its child process, so the nightly-version assertion fails on any workflow re-run — which, for a fork PR whose first attempt is always action_required, is every run. The code review approved this PR and that verdict stands; blocked replaces merge-queue only because a PR must not sit in the merge queue while a required check is red. I chose it over changes-requested on purpose: changes-requested would say the author owes a change, and here they owe nothing — the one-line fix belongs on main. Full diagnosis and the unblock are in the CI-result comment.
• ✨ feature — unchanged: this PR adds a new user-facing capability (non-image attachments through the composer) plus a per-project attachments library, rather than fixing a defect.
• 🧪 needs-qa — retained, and now more clearly warranted than before: the change alters the composer's paperclip, paste and drag-drop intake (paste was widened again this round to accept any file, not only images), adds a chip affordance for non-image attachments, changes what reaches the agent, and adds three response headers to the route that serves pasted images — one of which may affect how non-PNG/JPEG/WebP/GIF images render. A human should exercise the real UI before it merges; the manual-QA instructions comment lists the routes, and QA can proceed in parallel with the CI unblock.
• 🔺 priority-high — kept. The data-exposure hazard that originally earned this rating is fixed, but the PR still carries that fix plus server hardening on a route that hands out user-supplied bytes, and the inference rule puts security hardening in this band. Downgrading now would understate what is waiting to ship.
• ⚠️ risk-high — kept. The diff still spans 19 files across three packages and touches shared contract surfaces (packages/contract/src/runs.ts, the runs.json record schema), filesystem writes driven by user-supplied filenames, a new pruning sweep that deletes files, and a same-origin file-serving route on the process that holds the launch key. That breadth is unchanged by the fixes and reinforces the case for manual QA.

[pat-lewczuk]

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.

[pat-lewczuk]

🤖 om-auto-review-pr completed: CHANGES REQUESTED. Lock released.

Reviewed the head at e97735d0 in an isolated worktree. The full validation gate is green — npm run typecheck, npm run test:unit, npm run build (incl. check:pack) and npm run test:package all pass, and the 12 vitest suites that fail here also fail on a clean origin/main baseline in the same environment (13 there, a strict superset), so no regression is attributable to this PR and the description's claim on that point is verified. The 10 tests this PR adds all pass.

The verdict is driven by one blocker (.ai/cezar/attachments/ missing from ensureDataGitignore, so user uploads land in consumers' git working trees) and four majors, detailed with fixes in the review body.

autofix: skipped (not my PR — re-run with --autofix to fix it here).

[Damian-Szczepanski]

Thanks — the blocker and all four majors are addressed in 9160590, plus the three minors that were one-line fixes.

Blocker — .ai/cezar/attachments/ in ensureDataGitignore. Added. I also added packages/cezar/src/data-gitignore.test.ts, which guards the class rather than this instance: it reads the wanted list and every join(dataDir, '<literal>') in the service and fails on any name written under .ai/cezar/ that is not ignored, with workflows, skills and config.json declared as the deliberately committable exceptions. I verified it fails with the entry removed and passes with it restored — and you were right that this is invisible from inside this repo, which is exactly why it needed a test rather than care.

Major 1 — scoped client twins. sendProjectRunMessage now sends files, continueProjectRun gained the spread. Both are pinned in client.test.ts beside their unscoped siblings, so the parity is asserted rather than remembered.

Major 2 — the images route. It now sets x-content-type-options: nosniff and the same sandbox CSP the sibling raw-file path uses, and adds content-disposition: attachment for any extension outside IMAGE_TYPES. The filename in that header is re-sanitized rather than trusted: persistFile already restricts what can exist on disk, but a header assembled from a path param should not lean on a guarantee made elsewhere.

Major 3 — the library. Pasted copies now carry the run id (<runId8>-pasted-1.png), so two runs' different first screenshots no longer collide into a provenance-free pasted-1-2.png. The folder is bounded at 512 MB, oldest-first, swept in the same best-effort spirit as the copy itself — a failed sweep never breaks an upload. I also dropped "browsable in one place" from the doc comment, since you are right that nothing browses it yet; it is a durable archive, and calling it more than that was a promise the code does not keep. I kept the library in this PR rather than splitting it, but I would happily split it if you would still prefer that.

Major 4 — partial failures. Both routes now fail on ANY shortfall, not just total loss, with the count in the message (2 of 3 attachments could not be saved to disk). startRun still starts — failing a run over one attachment seemed too aggressive when the prompt is usually still worth running — but it now appends a note naming the files that did not materialize, so the transcript records the loss instead of the run looking normal.

Minors. Paste now filters on item.kind === 'file' instead of image/* (⌘V of a CSV did nothing at all, silently, while the other two intake paths worked — the test stub was passing only because it lacked kind, so that was fixed too and three cases added). A 0-byte file is refused with <name> is empty rather than bouncing the whole message with a zod error. taskFiles is mirrored into the contract with a note about why the Mutual<> check cannot catch that drift. Rejected messages no longer strand their already-written attachments on disk, and files now count against the stack's attachment cap — the residual gap there (attachments already stacked are invisible, because the stack records image URLs only) is named in a comment, since closing it needs the stack to record a file count and that is a record change worth its own PR.

Not addressed: rendering taskFiles in the thread's first bubble and the attachment note. Agreed it is a real product gap and I have noted it as a known v1 gap rather than quietly leaving it out.

Validation on the new head: typecheck, test:unit and build (incl. check:pack) all clean. 8 suites fail in my environment and fail identically on a clean baseline here (git-in-tmpdir, WSL path translation, route-parity timing) — none touch the changed code. Re-requesting review.

[pat-lewczuk]

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-08-30T15:58:27Z. Other auto-skills will skip this PR until the lock is released.

Re-review pass: the previous review's changes-requested handoff left @Damian-Szczepanski as assignee and that was the only claim signal on the PR — no in-progress label, and the prior lock was explicitly released — so this is a normal re-review claim, not an override of anyone's live work.

[pat-lewczuk]

🔍 Re-review: feat(composer): non-image file attachments, restored on current main

🎯 Summary

This is a re-review of head 9160590f, following the changes-requested review of e97735d0 on 2026-08-28. The intervening commit, fix(attachments): close the review's blocker and four majors, is 396 added lines across 11 files, of which 221 are tests.

Every previous finding that blocked the PR is closed, and closed properly rather than minimally:

• The blocker is fixed and, more importantly, made un-repeatable. 'attachments/' is in the wanted list (packages/cezar/src/index.ts:670), and the new packages/cezar/src/data-gitignore.test.ts guards the class of bug rather than this one instance: it statically reads the wanted array out of index.ts, collects every join(dataDir, '<literal>') first segment across packages/cezar/src, and fails on any name that is written under .ai/cezar/ and absent from the ignore list — with workflows, skills and config.json declared as the deliberately committable exceptions, each with its reason. That is a better answer than the one I asked for. I verified the assertion is load-bearing rather than vacuous: wantedEntries() parses line-by-line specifically because an apostrophe in the surrounding prose would otherwise shift every match, and the array's last entry does carry the trailing comma the regex requires.
• Major 1 (silent files drop in the project-scoped client twins) — sendProjectRunMessage now sends files, continueProjectRun gained the spread, and both are pinned in client.test.ts's request-shape table directly beside their unscoped siblings, which is exactly the thing that makes the parity survive the next refactor.
• Major 2 (images route serving arbitrary bytes unhardened) — the route now sends nosniff and the same sandbox CSP as the sibling raw-file branch, plus content-disposition: attachment for anything outside IMAGE_TYPES, with the filename re-sanitized rather than trusted. Declining to lean on persistFile's guarantee for a header assembled from a path param is the right instinct.
• Major 3 (unbounded, write-only, provenance-free library) — pasted copies carry the run id, the folder is bounded at 512 MB oldest-first with the same "never break the upload" degradation as the copy itself, and the doc comment dropped the "browsable in one place" claim that nothing in the repo delivered.
• Major 4 (silent partial materialization failure) — both routes now reject on any shortfall with the count in the message, and startRun appends a note naming the files that did not materialize. I checked the note's shape against the other twenty-odd type: 'note' emitters in run.ts and against thread-state.ts:517; it renders, and NoteLine interpolates note.text as a React child, so the unsanitized user filenames it carries are escaped rather than injected.
• Minors 1–3 are fixed with tests, and the ⌘V-of-a-CSV fix came with the observation that the old test stub was passing only because it lacked kind — which is the honest reading of why that filter survived.
• Minor 4 landed as the contract mirror plus a comment explaining why Mutual<> cannot catch that class of drift; the rendering half is explicitly declined and recorded as a known v1 gap, which is a legitimate way to close a finding.

What remains is five minors and four nits. None of them is a correctness or security hazard, none matches this repository's Major criteria in CODE_REVIEW.md ("incorrect run/step state transitions; SSE replay duplication or event loss; unbounded input reaching files or processes; a schema field added as required when old files carry it as absent"), and all of them are the kind of thing that is cheaper to fix in a follow-up than to hold a finished feature for. Two are new consequences of the fixes themselves (Minor 1 and Minor 2), two are gaps in the fixes' test coverage or resource use (Minor 3 and Minor 5), and one is carried forward unaddressed from the last round (Minor 4).

Verdict

✅ approve — no blockers, no majors. The one blocker and all four majors from the previous review are closed, each with the reasoning stated rather than just the code changed, and the validation gate is green with the vitest failures proven identical to a clean origin/main baseline. The five minors and four nits below are listed so they can be picked up; per CODE_REVIEW.md's severity guidance ("Minor — approve with comments") none of them blocks. Minor 3 in particular was requested last round and did not land, so it is worth not losing a third time.

Note that needs-qa stays on: this changes the composer's paperclip, paste and drag-drop intake, adds a chip affordance, and changes what reaches the agent, so a human should exercise the real UI before it merges. Manual QA instructions are posted separately.

🧪 Validation Gate

Command	Status	Notes
npm run typecheck	✅ PASS	Clean across all projects.
npm test	⚠️ PASS (with pre-existing environment failures)	6 suites fail on the PR head; the identical 6 fail on a clean origin/main (ae9b38b2) in the same environment — src/git-worktree.test.ts, src/server/automations-api.test.ts, src/server/git-changes.test.ts, src/server/git.test.ts, src/server/health-forge.test.ts, src/server/projects-api.test.ts. Not a subset or a superset: the same set, byte for byte. I ran both from the repository root so the scopes are comparable (327 files each; 6196 tests on the head, 6195 on main), and re-ran the head to confirm the result reproduces. The failures are all environmental — health-forge.test.ts:105 and projects-api.test.ts:753 expect repo/forge to be null in a temp dir and get this checkout's real git remote, because TMPDIR resolves inside the repository here. None touches the changed code. No regression is attributable to this PR.
npm run test:unit	✅ PASS	36/36 in the node:test core-module suite.
npm run build	✅ PASS	Includes check:pack — "ok — 475 files, 85 under web/dist".
npm run test:package	✅ PASS	15/15 packaged-CLI E2E.

I also ran the PR's own tests in isolation to confirm they are real rather than merely collected: composer.test.tsx, client.test.ts, composer-images.test.ts → 139 passed; data-gitignore.test.ts, pasted-attachments.test.ts, queued-messages.test.ts → 41 passed.

CI status at review time: the required check Unit, build, E2E, and package was still pending (run 33242626225) when this review was submitted; license/cla is green. This review is not gated on it — the local gate above is this run's evidence, and CI remains the authority for the merge. A follow-up comment will record the CI outcome.

Findings

🔹 Minor

1. packages/cezar/src/server/server.ts:4131 — nosniff on a response typed application/octet-stream may stop legitimate pasted images from rendering in the cockpit.

The hardening is right, but it lands on a route that serves two different things. persistImage (run.ts:3411-3416) derives its extension from the media type and falls back to 'img' for anything that is not PNG, JPEG, WebP or GIF, and splitAttachments (composer-images.ts:49) routes everything whose mediaType starts with image/ down that path. So an AVIF, BMP, HEIC or SVG file dragged onto the composer is stored as pasted-N.img, and IMAGE_TYPES (server.ts:4494-4499) has no img key — it is served as application/octet-stream, now with nosniff and content-disposition: attachment, and rendered through <img src> by ImageItem (thread-items.tsx:641).

Per the Fetch specification nosniff blocks only script and style destinations, so Chrome is unaffected, and content-disposition is ignored for subresource loads in every engine. Firefox, however, additionally enforces nosniff on image loads and refuses an image whose declared type is not an image type. I have not run a browser against this build, so I am flagging it as a risk to confirm rather than a demonstrated break — but it is worth confirming, because before this commit those files rendered by sniffing and now they are declared un-sniffable.

Independently of the browser question, serving a known-image payload as application/octet-stream is simply inaccurate, and the fix closes both concerns at once. Fix: have persistImage record the real media type (or a truthful extension) instead of collapsing everything unknown to .img, and widen IMAGE_TYPES to the formats the composer can actually produce — deliberately excluding svg, which is the one case where refusing to render is the correct outcome and where content-disposition: attachment is doing exactly its job. Worth a line in the QA pass either way (it is in the manual-QA comment).

2. packages/cezar/src/server/server.ts:3818 and packages/cezar/src/server/server.ts:3939 — the new orphan-attachment cleanup misses the two 409 rejection paths.

dropPersistedFiles is a good fix and its comment states the rule correctly — "every rejection below has to take them back out" — but it is wired into only the three 400s inside the currentRun?.status === 'queued' block (server.ts:3789, :3800, :3805). Two rejection paths persist files and then return without it:

• server.ts:3818, the terminal return c.json({ error: 'session closed' }, 409) in POST /runs/:id/messages, reached when neither enqueueMessage nor deferMessage accepts the message.
• server.ts:3939, if (!result.ok) return c.json({ error: result.error }, 409) in POST /runs/:id/continue, where the whole route has no cleanup at all.

Both are the ordinary race — a user finishes typing and attaches a file just as the session closes — so they are arguably more likely in practice than the queue-cap 400s that are covered, and they leave exactly the orphans the fix set out to prevent, unreclaimable by dropOrphanImages for the same reason (attachments live in the message text, not in queuedMessages[].images). Fix: hoist dropPersistedFiles above the ladder and call it on both, and add the same closure to the continue route. Related but smaller: the attachments-library copy made by persistFile is never dropped, so a rejected upload still archives. That is defensible — the library is "everything the user uploaded" and the folder is now gitignored and bounded — but worth a sentence in the doc comment so the asymmetry is deliberate rather than incidental.

3. packages/cezar/src/server/server.ts:3746 and packages/cezar/src/workflows/run.ts:788 — the two highest-severity fixes in this commit landed without regression tests, and one of those tests was explicitly requested last round.

The all-or-nothing rule is now a single !== comparison and the startRun loss note is a single if (lost.length); nothing in the suite pins either. grep for could not be saved to disk across packages/cezar/src and packages/web/src returns no test hit. The previous review listed this as test-coverage gap #2 with the shape spelled out — stub persistFile to return null for one of three attachments and assert the route reports the loss rather than answering 200 — and it did not land and was not mentioned in the response comment, so it reads as dropped rather than declined.

The same applies to the Major 2 hardening: no test asserts the images route's x-content-type-options, CSP or content-disposition, even though the sibling raw-file path's header is pinned at git-changes.test.ts:701. Security headers are precisely the thing a later refactor removes quietly. Fix: one route test per fix — a partial-failure 500 with the count in the body, and a header assertion on GET /api/v1/runs/:id/images/:file for both a known extension and an unknown one.

4. packages/web/src/components/composer/composer-images.ts:17,23,42 — the naming drift carried over from the previous review is still open, and unacknowledged.

The module is composer-images.ts, the interface is PendingImage, the encoder is fileToPendingImage, and the composer's state is images/setImages/imagesRef — all of which now routinely hold CSVs and logs, and the JSDoc saying "attachment" makes the mismatch more conspicuous rather than less. CODE_REVIEW.md lists naming drift under Minor, so this does not block; I am re-raising it only because the response comment accounted for every other finding explicitly and this one silently, and a rename gets more expensive with every surface that adopts the old name. PendingAttachment / fileToPendingAttachment / attachments would do it; the filename can stay if the churn is a concern.

5. packages/cezar/src/workflows/pasted-attachments.test.ts:380 — the library-cap test allocates and writes a real 520 MB file to exercise a 512 MB constant.

writeFileSync(old, Buffer.alloc(520 * 1024 * 1024)) costs a ~520 MB resident allocation in the vitest worker plus 520 MB of real disk I/O, for one assertion. I measured it at 597 ms locally, which is tolerable here, but npm test is a required check and vitest runs several workers in parallel on a 7 GB GitHub-hosted runner — this is the kind of fixture that turns into an intermittent OOM or a disk-space failure on someone else's machine rather than a clean red.

The test itself is the right test and should stay. Fix, in order of preference: make ATTACHMENTS_LIBRARY_MAX_BYTES injectable (a constructor option or an env override on RunManager) so the case can assert the same behavior at a few kilobytes; or, if the constant should stay private, create the fixture sparsely — writeFileSync(old, ''); truncateSync(old, 520 * 1024 * 1024) — which statSync().size reports at the full logical size, so trimAttachmentsLibrary behaves identically with near-zero allocation and I/O.

💅 Nit

1. packages/cezar/src/data-gitignore.test.ts:60 — the doc comment's "and none exists today" is not accurate, and it describes the guard's one blind spot.

The comment says computed path segments cannot be checked statically "and none exists today", but packages/cezar/src/automations/store.ts has several — join(this.dataDir, POLL_LOCK) at line 210 and join(this.dataDir, filename) at 276, 292, 310, 318 and 328. Those particular files are already in wanted, so nothing is currently unguarded; the problem is that the sentence tells the next reader the blind spot is empty when it is not, which is the exact reader who needs to know it is not. Suggest replacing it with a short note that computed segments exist in automations/store.ts and are covered by hand.

2. packages/cezar/src/server/server.ts:3801 — the queue-cap error still says "image limit" on a rejection a file caused.

Now that incomingAttachments folds files.length into the tally, a user who stacks four CSVs onto a queued run is told "too many queued images — 8 image limit across the stack". "attachment limit" would match what is actually being counted; the accompanying comment already uses the right word.

3–4. The previous review's two nits are still open, both of which were explicitly the author's call and remain so. composer.tsx:327's conditional two-arg onSubmit still guards a JavaScript behavior that needs no guard, and the images/files schema maxima (4 × 7,000,000 chars each) still sum above GLOBAL_BODY_LIMIT (32 MiB) for a scripted client that reads the bounds. Neither blocks; noting them only so the thread stays complete.

💥 Breaking Changes

• No exported/public symbol removed or renamed without a deprecation path — attachmentSaveError and trimAttachmentsLibrary are new and private/module-local; everything else added this round is additive.
• No function signature changed in a breaking way — copyToAttachmentsLibrary's callers changed the value of name, not the signature.
• No required type field removed or narrowed — taskFiles is optional in both the store and, now, the contract mirror (packages/contract/src/runs.ts:157), so old runs.json files still parse (CODE_REVIEW.md, State-file compatibility).
• No HTTP route URL removed or renamed; no method changed for an existing operation.
• No field removed or retyped in an existing response shape — the contract drift flagged as Minor 4 last round is now closed, so the mirror and the routes agree.
• No event or message name renamed or removed; the new loss note reuses the existing note type.
• No CLI command or flag renamed or removed; no machine-parsed output format changed.
• No config key renamed and no default changed silently.
• BACKWARD_COMPATIBILITY.md §3 — a new run-data file must be registered with ensureDataGitignore in the same PR. This was the previous review's blocker and it is now satisfied, with a test that will keep it satisfied. The .ai/cezar/ state contract is intact.

Two behavior changes are worth naming even though I do not consider them breaking, because both change what an existing client observes:

• POST /runs/:id/messages and POST /runs/:id/continue now answer 500 on a partial attachment write where they previously answered 200 with a truncated set. That is the requested fix and the correct trade — a silent partial loss is strictly worse — but it is a status-code change on an existing route for a case that used to succeed, so it belongs in the release notes.
• GET /api/v1/runs/:id/images/:file gained three response headers. See Minor 1 for the one case where that is more than hardening.

🧪 Test Coverage

The commit adds 221 lines of test against 175 lines of production code, and the tests are pointed at the failure rather than at the code. Three deserve specific credit:

• data-gitignore.test.ts guards the rule instead of the instance, and its second case will fail for a state file nobody has written yet. That is the difference between fixing a bug and closing a class of bug, and it is the right answer to a finding whose whole problem was that it is invisible from inside this repository.
• The provenance test scopes its assertion to the two run ids it created, because earlier cases in the same file archive into the same shared library folder — a detail that would otherwise have made it flaky the moment someone added a case above it.
• The composer paste tests fixed the stub as well as the code: adding kind to the fake DataTransferItem is what makes the new filter actually exercised, and the pasteText case pins that plain-text paste still reaches the textarea, which is the regression the widened filter could plausibly have caused.

Remaining gaps, all mapped to findings above:

1. No test for the all-or-nothing partial-failure rule on either route, and none for the startRun loss note (Minor 3) — explicitly requested in the previous review.
2. No test for the images route's new security headers (Minor 3), while the sibling raw path's header is pinned at git-changes.test.ts:701.
3. The library-cap test's fixture is 520 MB of real bytes (Minor 5) — the assertion is right, the fixture is not.
4. Nothing exercises the two uncovered orphan-cleanup paths (Minor 2), which is consistent with the code not covering them.

None of these changes the verdict. The behavior this PR ships is covered where it matters most — the sanitizer, the intake paths, the wire shapes, the taskFiles/taskImages split, and now the gitignore rule.

[pat-lewczuk]

🧪 Manual QA instructions (needs-qa)

This PR is approved and requires manual QA (needs-qa, no skip-qa). It is queued in merge-queue but the QA-approval gate holds it until qa-approved is added. QA reviewer: when you pick it up, move it to qa by swapping the labels (remove merge-queue, add qa), then run the routes below.

One setup note that decides whether P0 is testable at all: run the cockpit against a consumer repository, not against the cezar checkout itself. cezar's own root .gitignore carries a blanket .ai/cezar/ entry, so the entire P0 area is invisible from inside this repo — that is exactly why the original bug shipped. Point the cockpit at any other git repository with a clean working tree.

P0 — user uploads must not reach the user's git working tree

Where to click

• A cockpit running against a consumer git repository (not the cezar checkout)
• /new — the composer on the new-task form
• A terminal in that consumer repository, running git status

What to verify

• Before attaching anything, git status in the consumer repo is clean. Attach a file through the paperclip on /new, send the task, then run git status again → still clean, with no untracked .ai/cezar/attachments/ entry.
• Open .ai/cezar/.gitignore in the consumer repo → it contains an attachments/ line alongside runs/, tmp/ and launch-key. On a repo that cezar had already initialized before this build, the line should have been appended on the next launch rather than requiring a fresh setup — confirm an existing install picks it up.
• Confirm the file actually landed: .ai/cezar/attachments/ exists and holds a copy, and the run's own directory (.ai/cezar/runs/<runId>-images/) holds the original under a recognizable name.

What can go wrong

• attachments/ missing from the generated .gitignore, so a dragged CSV, production log or .env-shaped export shows up in the user's git status and is one git add -A from a public repo. This is the blocker the previous review found; it is fixed in code and covered by a unit test, but this is the only place it is observable end to end.
• An existing .ai/cezar/.gitignore not being updated in place (the append path), which would leave every already-onboarded project exposed while new ones look fine.
• Files landing outside .ai/cezar/ entirely, or with a name that escapes the directory — attach a file named ../../escape.txt and confirm it is written as escape.txt inside the run directory and nowhere else.

P0 — the file-serving route hands out user bytes safely

Where to click

• A task thread containing a pasted screenshot
• Browser devtools → Network, on the request to /api/v1/runs/<runId>/images/<file>
• The same URL pasted directly into the address bar

What to verify

• The response carries x-content-type-options: nosniff and content-security-policy: default-src 'none'; style-src 'unsafe-inline'; sandbox.
• A pasted PNG/JPEG/WebP/GIF screenshot still renders inline in the thread and still zooms, and its response has an image/* content type and no content-disposition header.
• Attach an SVG file, then open its served URL directly in the address bar → it downloads or shows as an attachment rather than rendering as a document. It must not execute script on the cockpit's origin; that origin holds the launch key.

What can go wrong

• This is the one regression risk worth the most attention (Minor 1 in the review). Paste or drag an image in a format outside PNG/JPEG/WebP/GIF — AVIF, BMP or HEIC are the realistic ones — and check whether it still renders in the thread. It is stored with an .img extension and served as application/octet-stream, which is now marked nosniff. Chrome is expected to be unaffected; Firefox enforces nosniff on image loads and may refuse to render it, where it rendered before this PR. Please test this in both Chrome and Firefox and report what you see either way — a "renders fine in both" result is just as useful as a break, because the review flagged this as a risk to confirm rather than a demonstrated failure.
• Existing screenshots in old transcripts failing to load after the upgrade (the route change is retroactive — it applies to files already on disk).

P1 — composer intake: paperclip, paste and drag-drop are equivalent

Where to click

• /new — the new-task composer
• A task thread's follow-up composer
• The Continue affordance on a finished task

What to verify

• All three intake paths accept a non-image file: click the paperclip and pick a .csv; copy a .csv in the OS file manager and press ⌘V/Ctrl+V in the textarea; drag a .log onto the composer. Each produces a named chip with a working "Remove <name>" control. The paste path is the one that changed this round — before, ⌘V of a non-image did nothing at all, silently.
• Pasting plain text still types into the textarea and attaches nothing.
• A non-image attachment renders as a named chip, not a broken thumbnail; a pasted image still renders as a thumbnail.
• Send a task with a file attached and confirm the agent receives a usable absolute path in its opening prompt — ask it to read the file back and check the contents match.
• Do the same on a follow-up message into a live session, on a message stacked onto a queued run, and through Continue on a finished run. All three should deliver the path note.

What can go wrong

• An empty (0-byte) file should be refused with the single line <name> is empty and must not bounce the whole message with a raw schema error; the typed draft must survive.
• A file over 5 MB should be refused with the existing "too large" line, again without losing the draft.
• The per-message cap is 4 attachments combined across images and files — attach five and confirm the fifth is refused with a readable line rather than silently dropped.
• Stacking onto a queued run: the 8-attachment stack cap now counts files as well as images. Confirm the rejection message appears rather than a 500 — note the wording still says "image limit" even when a file caused it (a known nit).
• Weird filenames: attach files named q3 report (final).csv, .env, and one with non-ASCII characters. Each should land on disk under a sanitized, still-recognizable name, and the path note should point at a file that actually exists.

P1 — perceived performance and responsiveness

Where to click

• Cold-load /new and a task thread that contains attachments (hard refresh, cache disabled)
• The same routes at a mobile viewport (375 px wide)

What to verify

• Cold load of /new shows a useful shell/loading state promptly and the composer becomes interactive without a visible stall; capture a screenshot as evidence.
• Attaching a large-but-legal file (~4 MB) does not freeze the textarea — typing stays responsive while the file is being encoded to base64 on the main thread.
• The chip row wraps sanely at the mobile viewport and the remove controls stay tappable.
• Light, dark and system themes all render the new chip affordance correctly.

What can go wrong

• A multi-megabyte attachment blocking the UI thread during base64 encoding, which would show up as a frozen composer rather than an error.
• The chip row overflowing its container or pushing the send button off screen on mobile.

P2 — known v1 gap, confirm it is only a gap

Where to click

• A task thread for a run started with a non-image file attached

What to verify

• The agent demonstrably received the file (ask it to read it), even though the thread's opening bubble does not show a chip or any trace of the attachment. This is a deliberate, documented v1 gap, not a delivery failure — the point of the check is to confirm the file reaches the agent despite being invisible in the transcript.
• If a partial write is easy to simulate in your environment (for example, by making the run directory read-only mid-flight), a task start that loses attachments should append a visible ⚠ … could not be saved to disk note to the transcript rather than starting silently. Skip this if it is impractical to stage — it is not currently covered by an automated test either, which is why it is called out.

Pass/fail

• All routes pass → remove the qa label and add merge-queue plus qa-approved (this clears the QA-approval gate)
• Any route fails → remove the qa label, add qa-failed, and leave a comment describing the failure.

[pat-lewczuk]

🤖 om-auto-review-pr completed: APPROVED. Lock released.

Re-reviewed head 9160590f in an isolated worktree, against the e97735d0 review from 2026-08-28. The blocker and all four majors are closed, and closed well — the .gitignore fix in particular ships a test that guards the rule (every literal join(dataDir, …) in the service must be in the wanted list, with the committable exceptions named) rather than just this one entry, which is a better answer than the one the review asked for.

Validation gate: npm run typecheck, npm run test:unit, npm run build (incl. check:pack) and npm run test:package are all green. npm test fails 6 suites — and a clean origin/main (ae9b38b2) fails the identical 6 in the same environment, run from the repository root so the scopes are comparable (327 files both sides). Not a subset, the same set; the failures are environmental (TMPDIR resolves inside a git repo here, so the health/projects cases that expect null see this checkout's real remote). No regression is attributable to this PR, and the PR's own 180 tests pass when run in isolation.

What is left is five minors and four nits, none of which matches this repository's Major criteria in CODE_REVIEW.md, so none of them blocks. Worth flagging three from the review body:

• The new nosniff header lands on a route that also serves .img files — pasted images in formats outside PNG/JPEG/WebP/GIF (AVIF, BMP, HEIC). Chrome should be unaffected; Firefox enforces nosniff on image loads and may refuse them where they rendered before. I have not run a browser against this build, so it is a risk to confirm rather than a demonstrated break — the manual-QA comment asks for it explicitly, in both browsers.
• dropPersistedFiles covers the three 400s but not the two 409s (server.ts:3818 and :3939), which are the likelier race in practice.
• The partial-failure test the previous review asked for did not land and was not mentioned, and the images-route headers are unpinned while the sibling raw path's header is pinned at git-changes.test.ts:701.

Labels: changes-requested → merge-queue; needs-qa retained, so with qaGate on, the QA-approval gate holds the merge until a QA reviewer adds qa-approved. priority-high and risk-high kept, with reasons in the label-rationale comment. Manual-QA instructions are posted separately.

autofix: skipped (not my PR — re-run with --autofix to fix it here). The five minors and four nits are left for @Damian-Szczepanski to pick up or decline; the PR is approvable as it stands either way.

in-progress was swapped for ci-monitoring: the required check Unit, build, E2E, and package was still pending at review time, so this run owes a CI-result follow-up. The PR is free for anyone to act on in the meantime — ci-monitoring is not a lock.

[pat-lewczuk]

🤖 om-auto-review-pr — CI result

CI settled inside the follow-up budget, and it is red — but not because of this PR. The failure is a pre-existing test-isolation bug in the release tooling that fires on any workflow re-run, which for a fork PR means every run.

What failed. Run 33242626225, check Unit, build, E2E, and package — one job step, Run packaged CLI E2E tests, 14 of 15 passing. Every other step is green: Typecheck ✅, Run unit tests ✅, Run server and cockpit unit suites ✅ (which confirms the 6 vitest suites that fail in my local environment are environmental — they are green on CI), Build ✅ including check:pack.

✖ the nightly channel stamps a dated version and publishes under the nightly tag
  AssertionError: Expected values to be strictly equal:
  + actual   '0.9.9-nightly.20260813.12.2'
  - expected '0.9.9-nightly.20260813.12'
  at packages/cezar/test/e2e/release-snapshot.test.ts:184

Root cause — packages/cezar/test/e2e/release-snapshot.test.ts:165. snapshot.ts:62-63 appends .${runAttempt} to a snapshot version when GITHUB_RUN_ATTEMPT > 1, deliberately, "so re-runs never collide" — that production behavior is correct and is not what is broken. The bug is in the test harness: runScript builds its child environment as { ...process.env, …extraEnv } (release-snapshot.test.ts:87-93), so anything the case does not explicitly pin leaks in from the surrounding workflow. The sibling PR-snapshot case at line 110 pins GITHUB_RUN_ATTEMPT: '1'; the nightly case does not, so it inherits the runner's real attempt number. This run was attempt 2, the script appended .2, and the hard-coded expectation missed.

Why this necessarily hits fork PRs. A fork PR's first workflow attempt is action_required — it waits for maintainer approval and never executes. Attempt 1 of this very run is recorded as action_required (2026-08-29T08:12:56Z); the run that actually executed is attempt 2. So GITHUB_RUN_ATTEMPT is ≥ 2 for effectively every fork contribution, and this case fails every time, regardless of the diff.

Evidence it is not this PR. Three independent lines:

• This PR touches no release tooling — not scripts/release-snapshot.mjs, not packages/cezar/src/release/, not test/e2e/. Its 19 files are the composer, the run manager, the server routes, the contract and the API client.
• npm run test:package passes 15/15 locally on this exact head (9160590f), where GITHUB_RUN_ATTEMPT is unset.
• Across the last 40 workflow runs, the other two failing pull_request runs are both attempt 1 and neither fails this test, while this run — the only attempt 2 — is the one that does. main is green on ae9b38b2, whose run is attempt 1. The failure correlates with the attempt number, not with the branch.

What this means for the verdict. The code approval stands: it was a judgement about the diff and nothing here changes it. But a red required check is a red required check, so I have moved the pipeline label off merge-queue — this PR must not be merged while the check is red.

I set blocked rather than changes-requested, deliberately, and want to be explicit about the deviation. changes-requested means the author owes a change; here @Damian-Szczepanski owes nothing — the fix belongs in release-snapshot.test.ts on main and this PR cannot make it. blocked is the label whose defined meaning is "progress depends on an external blocker", which is exactly the situation. The practical effect is the same where it matters: the PR is out of the merge queue and the merge skills will refuse it.

The unblock, in order of preference. Pin the ambient variable in the nightly case, matching what its sibling at line 110 already does — one line, and it fixes every future fork PR at the same time:

GITHUB_RUN_NUMBER: '12',
GITHUB_RUN_ATTEMPT: '1',
CEZ_RELEASE_CHANNEL: 'nightly',

Better still, since this is the second case to be bitten by inheritance, consider having runScript neutralize the whole GITHUB_* family by default and let each case opt in to the ones it means to assert. That would be a small PR against main, after which this PR goes green on its next run with no change to its own code.

As a stopgap a maintainer can re-run the failed job, but note that a re-run raises the attempt number rather than resetting it, so it will fail again for the same reason. There is no attempt number that makes this pass except 1.

Removing ci-monitoring: this follow-up is delivered and no further CI follow-up is owed from this run.

[Damian-Szczepanski]

Closing this from our side — the change already lives in our own build and we are no longer maintaining the branch it was proposed from. Nothing here is blocked on you; feel free to pick the idea up independently if it is useful. Thanks for taking a look.

[pat-lewczuk]

Thanks for saying so plainly — and no hard feelings at all about the withdrawal. Keeping a fork branch rebased against a moving main for two weeks is real work, and it is entirely reasonable to stop doing it once your own build has what you need.

For the record, and so this does not read as a rejection to anyone finding it later: the review verdict on this PR was approve. The blocker and all four majors were closed, and the final red CI was a pre-existing test-isolation bug in our own release tooling (release-snapshot.test.ts leaking ambient GITHUB_RUN_ATTEMPT into its child process, which fires on any workflow re-run — and for a fork PR, every run is a re-run). That was never yours to fix.

Two things came out of your work:

• The core of it shipped in feat(composer): accept PDF, TXT and MD attachments, not just images #951 — the composer now takes PDF, TXT and MD alongside images, files are materialized to disk and reach the agent as a path. Your feat(composer): non-image file attachments — materialized to disk, path-noted in the prompt #899/feat(composer): non-image file attachments, restored on current main #929 branch is where that design was worked out, including the detail that a path is the only form that survives the codex/opencode backends.
• The per-project attachment library did not ship with it, and that is the piece we are now picking up: feat(attachments): per-project attachment library #957. It carries the file's original name on the wire, copies user file attachments into .ai/cezar/attachments/ with content dedupe and collision-safe naming, and — this one is straight from your review round — adds attachments/ to ensureDataGitignore with a static guard test, so the next new state directory cannot repeat the "user uploads land in the user's git status" bug. Credited to you in the PR body and in the code.

We narrowed it in one place: files only, not images. A pasted screenshot has no filename to file it under, and putting a name on the image path would mean widening ContentBlock, which is the runner protocol. Named image uploads can join later behind an explicit paste-vs-pick distinction.

Thanks for the idea and for the care you put into the review rounds — both landed.