Skip to content

feat(tasks): in-task drafts survive leaving the task - #940

Merged
pat-lewczuk merged 10 commits into
open-mercato:mainfrom
piotrchabros:cez/2422bc2e
Sep 14, 2026
Merged

pat-lewczuk merged 10 commits into
open-mercato:mainfrom
piotrchabros:cez/2422bc2e

Conversation

@piotrchabros

Copy link
Copy Markdown
Contributor

Closes #939 — spec .ai/specs/2026-08-30-thread-composer-draft-persistence.md.

What was broken

ThreadView rendered the shared <Composer> uncontrolled, so the reply text lived in the
composer's own useState and the pasted screenshots in another. Opening another task is a route
change; the component unmounted and both were gone, with no warning and no way back. In a
parallel-agent cockpit "check on the task next to this one, mid-sentence" is the product's core
loop — and every neighbouring composer (/new, the GitHub hand-off box) already kept its draft.
The thread, the highest-traffic text input in the product, was the one that forgot.

What this does

A server-side, per-run, per-surface draft store, and every editable input inside a task wired
to it: the reply composer (text and attachments), the review-notes box, the inline prompt and
queued-message editors, and the header's rename. It is deliberately invisible — no toast, no badge:
you come back and the text is where you left it, thumbnails intact, and an editor that had unsaved
text re-opens holding it.

Server

  • packages/contract/src/drafts.ts — every request/response shape as a zod schema. The surface
    vocabulary (composer | review-notes | task-prompt | title | message:<msgId>) is a validated
    path param
    , not an interpolated string: it reaches the filesystem as a path segment.
  • packages/cezar/src/runs/drafts.ts — plain files under .ai/cezar/drafts/<runId>/, in the
    same style as ui-state.ts and runs/store.ts: atomic tmp+rename 0600 writes, degrade to "no
    draft" on every read error (missing, unreadable, malformed, per-entry corrupt), bounded on every
    axis, plus a 64 MiB whole-store backstop that evicts least-recently-touched run directories
    and logs one line. No expiry, no age sweep, no count sweep.
  • A chained draftRoutes family, mounted under both spellings, validated as middleware:
    GET /runs/:id/drafts, PUT/DELETE /runs/:id/drafts/:surface, and the …/images trio.
    An empty PUT is a delete, so "cleared when emptied" is enforced server-side rather than by
    client politeness; every route 404s on an unknown run, and deleting or pruning a run deletes
    its drafts.

Cockpit

  • useDraft(runId, surface) is the only entry point — no component talks to the draft API.
    Seed once and never fight the typist (a late GET cannot overwrite live text), one
    debounced + serialized write per typing pause, flush on unmount / visibilitychange / pagehide,
    and clear only once the message has really landed — a rejected send keeps the draft and its
    blobs. A failed draft write is silent: it must never be louder than the message being written.
  • Composer gains an optional images / onImagesChange seam, mirroring the existing text
    seam exactly. /new deliberately stays uncontrolled (multi-MB base64 has no business in
    localStorage) and that is pinned by its own test — it is the one change here that could break a
    surface nobody edited.
  • Attachments upload when they are attached, not when the message is sent, so the bytes cross
    the wire once and the draft record only references them.

Departures from the spec, and why

Recorded in § As built of the spec file:

  1. An attachment is one self-describing blob (images/<id>.json) rather than raw bytes plus a
    metadata index — two writers for one fact can diverge; one atomic write cannot.
  2. The orphan sweep has a grace window: an image is POSTed on paste and only named by the draft
    on the next debounced PUT, so a naive sweep would delete what the user just pasted.
  3. Two flush paths the spec did not name: pagehide (a full navigation runs no unmount effects),
    and carrying a pending write across a surface swap — the thread does not unmount when the
    route's :id changes, so without this, walking from task A to task B mid-sentence dropped A's
    last edit. Both are pinned by tests; the second was found by its test.

Risk

Additive. ui-state.json, runs.json, the NDJSON streams and the /new localStorage draft are
untouched; a cockpit that never calls the new routes behaves exactly as before, and deleting
.ai/cezar/drafts/ at any time is safe. The one thing worth saying out loud, and it is in the
CHANGELOG: a message you typed and chose not to send now rests on disk in your repo directory
until you clear it — 0600, gitignored, deleted with its task.

Validation

npm run typecheck, npm test, npm run test:unit, npm run build, npm run test:package — all
green. Nine npm test failures are pre-existing in this environment (root-user chmod
"unwritable" cases and "outside a git repo" cases); verified byte-identical against the parent
commit, and the branch's failure set is a subset of the baseline's.

npm run test:e2e reports TEST_E2E_STATUS=skipped here — agent-browser could not be provisioned
(no network to the Chrome-for-Testing hosts), so packages/web/e2e/thread-drafts.e2e.ts (type in
task A → open task B → return → the text is there → send → it is gone) has not run. It needs a
machine that can install the browser.

Tests added: the store (round-trip, empty-write-deletes, corrupt-file-reads-empty, traversal
refused, orphan sweep + its grace, backstop evicts oldest and never the run being written), the
routes (surface vocabulary, caps, 404s, blob round-trip), contract parity, typed-bodies entries,
the hook (debounce, seed-once, late-GET, unmount/keepalive flush, surface swap, submit success and
failure, attachments), the composer seam (uncontrolled /new pinned; controlled add/remove/restore),
and one integration test per host.

🤖 Generated with Claude Code

Typing a reply, glancing at another task, and coming back used to cost the
message: `ThreadView` rendered the shared composer uncontrolled, so the text and
the pasted screenshots lived in the component a route change unmounted. In a
parallel-agent cockpit that is the core loop, not an edge case, and every
neighbouring composer (`/new`, the GitHub hand-off box) already kept its draft.

Adds a server-side, per-run, per-surface draft store and wires every editable
input inside a task to it: the reply composer (text + attachments), the review
notes, the inline prompt and queued-message editors, and the header rename.

- `packages/contract/src/drafts.ts` — every shape as a zod schema, with the
  surface vocabulary (`composer | review-notes | task-prompt | title |
  message:<id>`) as a validated path param rather than an interpolated string.
- `packages/cezar/src/runs/drafts.ts` — plain files under `.ai/cezar/drafts/`,
  atomic writes, degrade-to-empty on every read error, bounded on every axis
  plus a 64 MiB whole-store backstop that evicts least-recently-touched runs.
  An attachment is one self-describing `0600` blob, so a metadata index cannot
  drift from the bytes it names.
- A chained `draftRoutes` family, mounted under both spellings and validated as
  middleware. An empty PUT is a delete, so "cleared when emptied" is the
  server's rule, not client politeness; every route 404s on an unknown run, and
  deleting or pruning a run deletes its drafts.
- `useDraft(runId, surface)` is the cockpit's only entry point: seed once and
  never fight the typist, one debounced+serialized write per typing pause,
  flush on unmount / `visibilitychange` / `pagehide`, and clear only once the
  message has really landed — a rejected send keeps the draft and its blobs.
- `Composer` gains an optional `images`/`onImagesChange` seam mirroring the text
  one exactly; `/new` stays uncontrolled, pinned by its own test.

Deliberately invisible: no toast, no badge. You come back and the text is there.

Docs: BACKWARD_COMPATIBILITY §2 (routes) and §3 (`drafts/`), `drafts/` into
`ensureDataGitignore` (moved to its own module so it can be tested at all), the
design record in `.ai/specs/`, and a CHANGELOG entry naming the new
unsent-content-at-rest.

Closes open-mercato#939

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pat-lewczuk pat-lewczuk self-assigned this Aug 31, 2026
@pat-lewczuk pat-lewczuk added the in-progress Cezar agent is actively working this issue label Aug 31, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

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

@pat-lewczuk pat-lewczuk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review: feat(tasks): in-task drafts survive leaving the task

🎯 Summary

This PR adds a server-side, per-run, per-surface draft store and wires every editable input inside a task to it: the reply composer (text and pasted attachments), the review-notes box, the inline prompt and queued-message editors, and the header's rename. The server half is a new contract module (packages/contract/src/drafts.ts), a file-backed store (packages/cezar/src/runs/drafts.ts) writing .ai/cezar/drafts/<runId>/, and a chained draftRoutes family in packages/cezar/src/server/server.ts. The cockpit half is one hook, useDraft(runId, surface) (packages/web/src/routes/task-thread/thread-draft.ts), plus an optional controlled-images seam on the shared Composer. I reviewed all 32 changed files at head ee9a562 against base main, in an isolated worktree, and ran the full configured validation gate.

The engineering standard here is high, and several things deserve to be said before the findings. The threat model around :surface is right: it is a closed vocabulary validated as route middleware (draftSurfaceParamSchema) rather than in the handler, and the store re-checks it at the filesystem boundary (safeSegment), so the traversal risk of a path-segment-shaped parameter is closed twice over. The store follows the house degradation rules faithfully — every read path degrades to "no draft" on missing, unreadable, malformed and per-entry-corrupt input, and no read throws. Writes are atomic tmp+rename at 0600. BACKWARD_COMPATIBILITY.md gained both the §2 route entry and the §3 state-file entry in the same PR, .gitignore maintenance was extracted into a testable module and gained the drafts/ entry, and deleting or pruning a run now deletes its drafts. Test coverage is genuinely thorough — 16 store tests, 18 route tests, 16 hook tests, plus host-level tests per surface. The hook's hardest cases (a late GET losing to live text, carrying a pending write across a surface swap, holding the optimistic clear back until a send resolves) are each pinned by a test.

What I am requesting changes on is one architectural cost that the PR's own "Departures from the spec" section names but does not price: making each attachment one self-describing blob means the draft store cannot read an attachment's metadata without reading and JSON.parseing its bytes. That turns the routine, per-typing-pause draft write into tens to hundreds of milliseconds of synchronous, event-loop-blocking work on a single-threaded server that is concurrently streaming agent output over SSE. Everything else I found is minor or a nit.

Verdict

request changes — one major finding (packages/cezar/src/runs/drafts.ts:167): resolving draft image metadata reads and parses the full base64 blob, so a draft holding pasted screenshots blocks the server event loop on every debounced PUT, on every attachment upload, and on every task open. There is no documented waiver for it, and the fix is small and local. Nothing else blocks: there are no blockers, no contract breaks, no security regressions, and the validation gate is green.

🧪 Validation Gate

Command Status Notes
npm run typecheck ✅ PASS All four workspaces (contract, api-client, server, web) clean.
npm test ✅ PASS 331 files, 6244 tests, 0 failures. See the note below — the first run in this environment showed 6 failures that are environmental, not this branch's.
npm run test:unit ✅ PASS 36 tests, 0 failures.
npm run build ✅ PASS Server + web build; check:pack ok — 482 files, 85 under web/dist (shell + assets present).
npm run test:package ✅ PASS Clean.

On the npm test failures, and a correction to the PR body. My first run reported 6 failures — src/server/git.test.ts ("returns null outside a git repository"), src/server/health-forge.test.ts ("forge:null outside a repo"), src/server/projects-api.test.ts ("keeps the pre-workspace shape byte-identical"), and three siblings in the same family. All of them assert "this temp directory is not a git repository". They fail here because this reviewing agent runs with TMPDIR pointed inside the checkout (the CEZ_AGENT_TMPDIR behaviour from #785), so mkdtempSync(tmpdir()) lands inside a git repo and getRepoInfo correctly finds one. Re-running with TMPDIR=/tmp TMP=/tmp TEMP=/tmp npm test gives 331 files / 6244 tests, all passing. So the gate is genuinely green, and none of it is attributable to this branch. For the record, the PR body's "nine npm test failures are pre-existing in this environment" does not match what I observed — it was six here, and they are an artifact of the temp directory rather than of root-user chmod; the conclusion (not this branch's fault) is right, the count and the cause are not.

Not run: npm run test:e2e is not part of validation.commands, and packages/web/e2e/thread-drafts.e2e.ts — the one spec that actually exercises "type in task A → open task B → return → the text is there → send → it is gone" — did not run here either, for the same reason the author reported: the agent-browser provider cannot be provisioned without network access to the Chrome-for-Testing hosts. The headline user journey of this PR is therefore still unverified in a real browser by anyone. That is why I am asking for needs-qa rather than treating the unit tests as sufficient.

Findings

⚠️ Major

packages/cezar/src/runs/drafts.ts:167 — reading an attachment's metadata reads and parses its whole base64 payload, blocking the event loop on every draft write.

imageMeta() is implemented as readImage() followed by discarding data, and readImage() (line 146) does JSON.parse(readFileSync(imagePath(...), 'utf8')) on a file that contains the full base64 image. Because the blob is deliberately self-describing, there is no cheaper place to get {id, mediaType, name, bytes} from. Three hot paths pay for it:

  • writeChecked() (line 257) calls imageMeta() once per image named in the body, purely to answer "does this id exist?" — on every debounced PUT, i.e. every 500 ms typing pause.
  • resolve() (line 181), called from readRunDrafts() (line 198) and again from writeChecked()'s return value (line 293), reads every blob of every surface to build the listing — so each PUT pays for it a second time, and GET /runs/:id/drafts pays for it once per task open.
  • The upload route packages/cezar/src/server/server.ts:4338 calls readRunDrafts() just to count one surface's held images, so uploading the second screenshot re-reads and re-parses the first.

The concrete failure: with four attachments at the route's own ~5 MB decoded cap, I measured 79 ms to read and parse the four blobs (5 MiB payloads, Node 20, warm page cache) — and each PUT does that twice, so roughly 160 ms of blocked event loop per typing pause. With two ordinary ~1 MB screenshots it is still ~65 ms per pause. cezar serve is single-threaded and is simultaneously streaming run-event/ui-event frames over SSE for every live agent, so this stalls the entire cockpit — other tabs' transcripts, the health poll, every other project's requests — while the user types a message that has a screenshot in it. That is the feature's own headline path, not an edge case.

Suggested fix, smallest first: keep the metadata where the write already knows it and the read does not need the bytes.

  1. In writeChecked, replace the existence check with a stat rather than a parse — existsSync(imagePath(dataDir, runId, id)) answers the same question for the same cost as one syscall.
  2. Give the metadata its own tiny sidecar — images/<imageId>.meta.json holding {id, mediaType, name, bytes}, written by writeRunDraftImage in the same breath as the blob — and have imageMeta() read that. The "two writers for one fact can diverge" argument in the PR body still holds for the bytes, but a metadata sidecar that is only ever written once, alongside the blob, and whose absence degrades to "blob is gone" (exactly what resolve() already does) does not reintroduce the divergence the self-describing blob was chosen to avoid.

Either way GET …/images/:imageId keeps reading the full blob, which is correct — that route is asking for the bytes.

🔹 Minor

packages/cezar/src/runs/drafts.ts:463 — the store-budget check walks the entire drafts tree synchronously on every write. enforceStoreBudget() calls dirBytes(draftsRoot(dataDir)), a recursive readdirSync + statSync walk of every run's draft directory, and it runs on every debounced PUT (line 283) and every attachment upload (line 343) — even when the store is nowhere near the 64 MiB ceiling. For a user with a few hundred tasks that is hundreds of syscalls per typing pause, on the same event loop as the finding above. Suggest caching the total in module state, adjusting it by the delta of each write, and only re-walking when the cached figure crosses (say) 80 % of DRAFT_STORE_MAX_BYTES — or, more simply, only walking when the file being written grew.

packages/cezar/src/runs/drafts.ts:343 — the budget is enforced before the write, so the store can exceed its own ceiling by one blob. enforceStoreBudget() measures the tree and then atomicWriteJsonSync adds up to ~7 MB on top. The ceiling is a backstop rather than a hard quota, so this is not severe, but it is easy to make honest: pass the incoming byte count into enforceStoreBudget(dataDir, keepRunId, incomingBytes) and compare total + incomingBytes against the cap.

packages/web/src/api/client.ts:1540 — the keepalive flush silently drops drafts larger than ~64 KiB, which are exactly the drafts most worth saving. putRunDraft passes { init: { keepalive: opts?.keepalive } }, and the hook uses that path for the visibilitychange → hidden and pagehide flushes (thread-draft.ts:377). The Fetch standard caps the total body of in-flight keepalive requests at 64 KiB and rejects beyond it; DRAFT_TEXT_MAX is 100 000 characters. Since every draft-write failure is deliberately swallowed (thread-draft.ts:179), a user who closes the tab on a 70 KiB draft loses it with no signal at all. Suggest sending keepalive only when the serialized body is comfortably under the limit and falling back to a plain fetch above it — an ordinary request still usually completes on visibilitychange, and it is strictly better than one that is guaranteed to be rejected. Worth a test in thread-draft.test.tsx next to the existing "flushes with keepalive when the tab is hidden mid-sentence" case.

packages/web/src/routes/task-thread/thread-draft.ts:250 — inferring the send-clear from text === '' misfires on an attachment-only draft. clearedForSend is computed as next.length === 0 && latest.current.text === '', which is correct for a real send (the composer clears text first, then images — composer.tsx:367), but it is also true when a user with an images-only draft clicks the last thumbnail's remove button. In that case the blob is not eagerly deleted; it is left to sweepOrphanImages, which honours a 10-minute grace window and only runs on a subsequent write — so if the run has other surfaces and no further draft write happens, the orphan can sit on disk indefinitely (until the 64 MiB backstop or the run's deletion). The heuristic is also fragile against any future reordering of the composer's optimistic clear. Suggest making the intent explicit instead of inferring it: have submit() set a clearingForSend ref that setImages reads, and drop the text-emptiness test.

packages/web/src/routes/task-thread/run-header.tsx:482 — a restored title draft auto-opens an editor that commits on blur, so returning to a task can silently apply a rename the user walked away from. The effect calls begin.current(draft.text) as soon as draft.hasDraft is true, and TitleEditInput wires onBlur={editor.commit} (editable-title.tsx:68). Concretely: a user types "Fix the login b" over the title, navigates to another task, comes back an hour later, clicks anywhere in the thread — and the task is now named "Fix the login b" with no confirmation. Before this PR, walking away from a half-typed rename discarded it. Restoring the text is the intended behaviour and I am not arguing against it; auto-committing it on the next stray click is the part that is new and surprising. Suggest either not auto-opening the title editor (restore on the next explicit Rename click instead, which is what the other surfaces effectively do), or suppressing blur-commit for an editor that was opened by the draft rather than by the user until they touch the field. Either way this deserves a test in run-header.test.tsx beside "re-opens holding a half-typed rename that was never committed" — the existing suite pins the restore but never blurs the restored input.

💅 Nit

packages/cezar/src/server/server.ts:4350 and :4364 — the image routes validate :surface and then ignore it. Both the GET and the DELETE destructure only { id, imageId }, so an image attached to composer is readable and deletable through …/drafts/title/images/<id>. There is no cross-run leak (the run id still scopes it) and deleteRunDraftImage correctly scrubs the id from every surface, so nothing is wrong — but the URL shape promises a scoping the handler does not enforce. Either check that the surface actually names the image and 404 otherwise, or add a one-line comment saying the surface is present for URL symmetry only, so the next reader does not assume it is a check.

packages/web/src/routes/task-thread/thread-draft.ts:256image.id as string. The cast is safe (the dropped filter on line 242 already narrows held.id !== undefined), but the assertion loses that guarantee to a future edit. Filtering into a string[] first, the way flush() does on line 201, keeps it checked.

💥 Breaking Changes

  • No exported/public symbol removed or renamed without a deprecation path. ensureDataGitignore moved from a private function in packages/cezar/src/index.ts to an exported one in the new packages/cezar/src/data-gitignore.ts; it was never part of any public surface, and the move is purely additive.
  • No function signature changed in a breaking way. useTitleEditor gained beginWith alongside the existing begin, which is preserved and now delegates to it; Composer gained optional images / onImagesChange props mirroring the existing text seam, and remains uncontrolled when they are absent (pinned by the /new test in composer.test.tsx).
  • No required type field removed or narrowed. PendingImage.id is added as optional.
  • No HTTP route URL removed or renamed; no method changed. The draftRoutes family is entirely new and is chained into the same versioned table as runsRoutes, so it is mounted under both /api/v1/… and /api/v1/p/:projectId/… and inherits the three-way alias parity — route-parity.test.ts, versioned-surface.test.ts and bc-route-inventory.test.ts all pass on this branch.
  • No field removed or retyped in an existing response shape.
  • No event or message name renamed or removed; no payload field removed. The SSE and WebSocket vocabularies are untouched.
  • 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 plain files.
  • No config key renamed and no default changed silently. .ai/cezar/drafts/ is a new, entirely disposable state directory; runs.json, ui-state.json, the NDJSON streams and the /new localStorage draft are untouched, and a cockpit that never calls the new routes behaves exactly as before.
  • Where a contract had to change: nothing had to change. BACKWARD_COMPATIBILITY.md gained the §2 route entry and the §3 drafts/<runId>/ state entry in this same PR, both accurately describing what shipped, and CHANGELOG.md carries the user-facing entry including the explicit "a message you typed and chose not to send now rests on disk in your repo directory" disclosure. The new drafts/ line in DATA_GITIGNORE_ENTRIES is appended non-destructively to existing repos' .ai/cezar/.gitignore, which is what keeps a pasted screenshot out of the user's git history on upgrade.

🧪 Test Coverage

Coverage on this PR is strong and specific, and I want to be concrete about what it actually pins rather than just calling it adequate.

The store (packages/cezar/src/runs/drafts.test.ts, 16 cases) covers the round-trip; surface and run isolation; "an empty write deletes the entry"; emptying the last surface removing the run directory including blobs; refusal of an unknown surface id before it reaches the filesystem; refusal of a PUT naming an image the store never minted; a corrupt draft.json reading as absent; per-entry salvage of a malformed sibling; an unwritable store answering with a reason instead of throwing; a traversal-shaped image id reading as missing; a vanished blob dropping out of the listing while the text draft survives; the orphan sweep and its grace window; and the backstop evicting least-recently-touched runs while never evicting the run being written.

The routes (packages/cezar/src/server/drafts-api.test.ts, 18 cases) cover the surface vocabulary including the parameterized message:<id> member, a 400 on anything outside it, an omitted body defaulting to an empty draft rather than a 400, the text and image caps, 404 on every verb for an unknown run, deleting a run deleting its drafts, the upload-then-reference flow, re-validation of the per-surface cap against a non-cockpit client, a traversal-shaped image id rejected at the param validator, and blob delete dropping the reference that named it.

The hook (packages/web/src/routes/task-thread/thread-draft.test.tsx, 16 cases) covers seeding; the empty case; a body that is not a draft listing leaving the input working; one write per typing pause rather than per keystroke; a late GET not overwriting live text; an in-flight GET not becoming what a later remount restores; the unmount flush; the keepalive hidden-tab flush; the task switch showing the right draft; carrying a pending write across a task switch; clear-on-landed-send; a failed send leaving the draft written; upload-on-attach; thumbnail removal deleting the blob while the pre-send clear does not; rehydrating a stored attachment into a thumbnail; and the disabled hook reading and writing nothing. Each host (task-thread.test.tsx, run-header.test.tsx, review-panel.test.tsx, composer.test.tsx) adds its own integration case, and composer.test.tsx specifically pins that /new stays uncontrolled — the one change here that could have broken a surface nobody edited.

Gaps worth closing, all tied to findings above:

  1. A performance regression test for the major finding, in packages/cezar/src/runs/drafts.test.ts: write a draft holding four blobs near the size cap and assert that readRunDrafts (or the PUT path) does not read the blob payloads — easiest as a spy on readFileSync, or by asserting the byte volume read. Without it, the self-describing-blob cost can silently come back.
  2. run-header.test.tsx: a case that renders with a stored title draft, lets the editor auto-open, then fires blur on the input and asserts what happens to the title. Today's suite pins the restore but never blurs, which is exactly why the blur-commit consequence went unnoticed.
  3. thread-draft.test.tsx: a case for the over-64-KiB keepalive body, asserting the flush still reaches the server (however that is resolved), next to the existing hidden-tab case.
  4. thread-draft.test.tsx: a case for an attachment-only draft (no text) whose last thumbnail is removed, asserting the blob is deleted rather than left to the sweep — the clearedForSend misfire above.
  5. The e2e spec still has not run anywhere. packages/web/e2e/thread-drafts.e2e.ts is written and looks right, but neither CI nor this review executed it, so the end-to-end journey this PR exists for has not been observed in a browser. This PR is labelled needs-qa for that reason; it needs one pass on a machine that can provision the browser, or a manual walkthrough with screenshots.

@pat-lewczuk pat-lewczuk added changes-requested Reviewer requested changes feature New capability priority-medium Ordinary bug or feature risk-medium Ordinary change with tests needs-qa Requires manual QA before merge labels Aug 31, 2026
@pat-lewczuk

pat-lewczuk commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

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

  • 🚀 merge-queue — the re-review at head f2335c81 approved: every finding from both earlier rounds is fixed and the full validation gate is green.
  • 🧪 needs-qa — stays on, so the QA-approval gate holds the merge until a QA reviewer adds qa-approved; the e2e spec covering the headline journey has still never run in a real browser, so nobody has observed it end to end.
  • feature — this adds a new capability: unsent text and attachments now survive leaving a task.
  • 🔹 priority-medium — an ordinary feature; nothing here is release-blocking or security-relevant.
  • 🟡 risk-medium — a single-area change with thorough tests, but it adds a new route family and writes user content to disk, so it is not risk-low.

@pat-lewczuk

Copy link
Copy Markdown
Collaborator

Thanks @piotrchabros — 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 pat-lewczuk removed the in-progress Cezar agent is actively working this issue label Aug 31, 2026
@pat-lewczuk

pat-lewczuk commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

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

Re-review of head 4fc2f66: the review. All eight findings from the first pass are fixed and re-verified against the code, each with a test that fails without its fix. New this round: one blocker (the head no longer merges — CHANGELOG.md only, after #962/#963), two majors (a restored task-prompt draft leaks into the next task and into that task's store — reproduced; a PUT naming a vanished blob discards the user's text with it), three minors and one nit.

Validation in an isolated worktree with TMPDIR outside the checkout: typecheck, test:unit, build (check:pack ok — 504 files) and test:package green; npm test is 6665 passing with one failure, agent-profile-wiring.test.ts:82, which reproduces verbatim on the base commit af7e8289 and is main's, not this branch's. packages/web/e2e/thread-drafts.e2e.ts still has not run anywhere, which is what needs-qa is holding the merge for.

autofix: skipped (not my PR — re-run with --autofix to fix it here). No CI follow-up is owed: the only check on this head, license/cla, is already green and nothing is pending.

…five smaller review items

Review of open-mercato#940 found one major cost the PR's own "Departures from the spec" named but did not
price: an attachment was ONE self-describing blob, so the store could not read its metadata
without reading and parsing its bytes. A listing runs that once per attached image on every
debounced PUT, on every upload and on every task open — ~160 ms of blocked event loop per typing
pause at the four-attachment cap, on the single thread that is streaming every live agent's
output over SSE.

- The metadata gets a sidecar (`images/<id>.meta.json`), written once beside the blob and never
  rewritten, so it cannot diverge from it the way the sketch's index could. A listing reads the
  sidecar and confirms the blob with one `existsSync`; a PUT's "did the store mint this id?" is
  two stats and no parse; the upload route counts a surface's held images from `draft.json`
  alone. Only `GET …/images/:imageId` still reads the bytes — it is asking for them.
- The 64 MiB backstop no longer walks the whole drafts tree on every write: it keeps a running,
  one-directional estimate (it can send us walking too often, never let the store past the
  ceiling unnoticed) and does the exact walk within 80 % of the cap. It also counts the bytes it
  is about to allow, so the store cannot settle a whole attachment above its own ceiling.
- The tab-close flush uses `keepalive` only while the body fits. Fetch caps in-flight keepalive
  bodies at 64 KiB and rejects past it, and DRAFT_TEXT_MAX is 100 000 characters — so the flag
  made the longest draft the one guaranteed not to be saved.
- The composer now says which emptying a change is (`ImagesChangeReason`) instead of the host
  inferring "send" from the text also being empty. That inference was wrong for an
  attachment-only draft: removing its last thumbnail looked like a send, so the blob was left to
  the orphan sweep's ten-minute grace.
- A rename editor that re-opened itself from a draft no longer commits on blur until the user
  touches it. Restoring the text is the point; silently applying it on the next stray click, an
  hour later, is not. Enter and Escape are unchanged.
- The image routes' `:surface` is documented as URL symmetry rather than a scope check, and one
  `as string` becomes a filtered narrow.

Tests: a listing survives a corrupt-but-present blob (the perf property, stated as behaviour),
the sidecar sweeps with its blob, the ceiling counts the incoming write, the >64 KiB flush goes
out without `keepalive`, an attachment-only draft's last removal deletes its blob, a restored
rename survives a blur, and the composer tags its optimistic clear.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@piotrchabros

Copy link
Copy Markdown
Contributor Author

Thanks — the major finding was right, and the fix was small and local. Pushed as c82de55a. Every finding is addressed; here is what changed and where I departed from the suggestion.

⚠️ Major — drafts.ts metadata read parsed the whole blob

Fixed with the sidecar (suggestion 2), plus the cheaper existence check (suggestion 1).

  • images/<imageId>.meta.json holds {id, mediaType, name, bytes}, written by writeRunDraftImage in the same breath as the blob, blob first so a crash between the two leaves an attachment that is simply not listed and whose bytes the orphan sweep reclaims — never one advertised without its bytes.
  • imageMeta() reads the sidecar and confirms the blob with one existsSync. That second syscall is what keeps "a blob that vanished stops being listed" true, which the sidecar alone would have broken.
  • writeChecked()'s per-image check is now imageExists() — two stats, no parse.
  • The upload route's held count comes from countRunDraftImages(), which reads draft.json only, so uploading the second screenshot no longer touches the first.
  • deleteRunDraftImage removes both files; sweepOrphanImages derives the id from either filename so a sidecar cannot outlive its blob.

The perf property is pinned as behaviour rather than as a spy: lists an attachment WITHOUT reading its bytes corrupts the blob's content while leaving the file in place, so anything that parses it drops the image and anything that reads the sidecar does not.

🔹 Minor — the budget walked the tree on every write

enforceStoreBudget keeps a running per-dataDir estimate and does the exact dirBytes walk only once the estimate is within 80 % of the cap. The estimate is deliberately one-directional — it only grows between walks, so a deletion leaves it reading high and costs an unnecessary walk (the safe direction); it can never read low enough to let the store past the ceiling, because every walk resets it to the truth.

🔹 Minor — the budget could be exceeded by one blob

enforceStoreBudget(dataDir, keepRunId, incomingBytes), compared as total + incomingBytes, including in the eviction loop's exit test. Pinned by counts what is ABOUT to be written, so an attachment cannot push it over the ceiling.

🔹 Minor — keepalive silently dropped drafts over ~64 KiB

putRunDraft sends keepalive only while the serialized body is ≤ 56 KiB (under the spec's 64 KiB because the allowance is shared with any other in-flight keepalive request), and falls back to an ordinary request above it. Test added beside the existing hidden-tab case.

🔹 Minor — clearedForSend misfired on an attachment-only draft

Departed from the suggested fix. A clearingForSend ref set inside submit() cannot work: Composer.submitDraft calls setText('') and setImages([]) before onSubmit, so submit() runs after the clear it would need to have flagged. Instead the composer says which change it is — onImagesChange(next, reason) with ImagesChangeReason = 'edit' | 'submit', mirroring the existing text seam's shape — and useDraft reads that instead of inferring anything from the text. The text-emptiness test is gone. New test: an attachment-only draft whose last thumbnail is removed deletes its blob now rather than leaving it to the sweep's grace window.

🔹 Minor — a restored title draft auto-committed on blur

Kept the auto-open (it is the feature — "an editor that had unsaved text re-opens holding it") and suppressed the blur-commit instead, via TitleEditor.commitOnBlur: false for an editor opened by beginWith from a draft, true the moment setDraft is called or the user clicks the pencil. TitleEditInput's onBlur consults it, so the run-header's wrapped commit (which also clears the draft) stays the one that runs. Enter and Escape are unchanged. New test blurs the restored input, asserts no PATCH and the text still there, then types and blurs and asserts the rename lands. CHANGELOG amended, since this is a user-visible nuance of what the entry promises.

💅 Nits

  • The image routes now carry the comment: :surface is validated but deliberately not used to scope the lookup, because a blob is minted before any draft record names it — scoping would 404 the thumbnail the user just pasted and orphan its bytes when they remove it. The run id is what scopes an attachment.
  • image.id as string → filtered into a string[], the way flush() does.

Docs

BACKWARD_COMPATIBILITY.md §3 describes the two-file attachment layout; the spec's § As built records the sidecar and its reason, and the four smaller decisions settled here.

Validation

npm run typecheck, npm run test:unit (36), npm run build (check:pack ok — 482 files, 85 under web/dist) and npm run test:package (15) are green. npm test is 331 files / 6249 tests, 6245 passing, 3 failing — and you were right that my earlier count and cause were wrong. The three here are projects-api.test.ts (projectsDir expects ~/cezar/projects; this box's registry root is elsewhere) and two root-user chmod cases in migrations.test.ts. I ran those two files against unmodified ee9a5624 in a separate worktree and got the identical 3 failures, so the set is byte-identical to the branch baseline. With TMPDIR=/tmp set, which is what removes your six.

packages/web/e2e/thread-drafts.e2e.ts still has not run — agent-browser cannot be provisioned here (no network to the Chrome-for-Testing hosts). needs-qa remains correct, and gap 5 of your test-coverage list is still open.

Re-requesting review.

# Conflicts:
#	BACKWARD_COMPATIBILITY.md
#	packages/web/src/components/composer/composer-images.ts
#	packages/web/src/components/composer/composer.test.tsx
#	packages/web/src/components/composer/composer.tsx
# Conflicts:
#	BACKWARD_COMPATIBILITY.md
#	packages/web/src/components/composer/composer-images.ts
#	packages/web/src/components/composer/composer.test.tsx
#	packages/web/src/components/composer/composer.tsx
# Conflicts:
#	BACKWARD_COMPATIBILITY.md
#	packages/web/src/components/composer/composer-attachments.ts
#	packages/web/src/routes/task-thread/thread-draft.ts
# Conflicts:
#	CHANGELOG.md
#	packages/web/src/routes/task-thread/run-header.tsx
# Conflicts:
#	packages/cezar/src/server/typed-bodies.test.ts
@pat-lewczuk pat-lewczuk self-assigned this Sep 14, 2026
@pat-lewczuk pat-lewczuk added the in-progress Cezar agent is actively working this issue label Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-14T10:42:11Z. Other auto-skills will skip this PR until the lock is released.

Re-review of the updated head 4fc2f66. (The PR was assigned to @piotrchabros from the previous review's author handoff, not held by another automation — that lock was released on 2026-08-31.)

@pat-lewczuk pat-lewczuk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Re-review: feat(tasks): in-task drafts survive leaving the task

Verdict

request changes — two things: the head no longer merges into main, and the cross-task
guarantee this feature is built on has a hole I could reproduce. Everything from the first review is
genuinely fixed, each with a test that fails without the fix; the new findings are mine, from a
second pass at head 4fc2f66.

🎯 What changed since the last review

c82de55a addressed all eight findings from the previous review, and I re-verified each against the current code rather than the reply:

Previous finding Disposition
⚠️ Major — imageMeta() parsed the whole blob Fixed. images/<id>.meta.json sidecar (drafts.ts:184), blob confirmed with one existsSync; writeChecked uses imageExists (two stats, no parse); the upload route counts through countRunDraftImages, which reads draft.json only. Pinned as behaviour by lists an attachment WITHOUT reading its bytes — corrupting the blob's content while leaving the file in place is a better test than a readFileSync spy, and it is the right call.
🔹 budget walked the tree on every write Fixed. storeBytesEstimate, exact walk only within 80 % of the cap, one-directional so it can read high but never low.
🔹 budget could be exceeded by one blob Fixed. enforceStoreBudget(dataDir, keepRunId, incomingBytes), compared as total + incomingBytes including in the eviction loop's exit test.
🔹 keepalive silently dropped drafts > 64 KiB Fixed. 56 KiB threshold measured on the encoded body, plain fetch above it, test beside the hidden-tab case.
🔹 clearedForSend misfired on an images-only draft Fixed, better than suggested. onImagesChange(next, reason) makes the composer state which emptying it is instead of the hook inferring it. The departure is right: submit() runs after the optimistic clear, so the ref I proposed could not have worked.
🔹 a restored rename auto-committed on blur Fixed. TitleEditor.commitOnBlur, false for an editor opened by beginWith, true the moment the user types or opens it themselves. Enter and Escape unchanged, and the new test blurs then types.
💅 image routes ignore :surface Resolved with a reason — a blob is minted before any record names it, so scoping would 404 the thumbnail just pasted. Fair.
💅 image.id as string Fixed. Filtered into a string[].

Gap 5 of the old coverage list is still open by both our accounts: packages/web/e2e/thread-drafts.e2e.ts has not run anywhere. needs-qa stays.

Findings

⛔ Blocker

The head no longer merges into main. mergeable: CONFLICTING, mergeStateStatus: DIRTY. I
resolved the exact scope with git merge-tree --write-tree --name-only HEAD origin/main: only
CHANGELOG.md conflicts
— every code file, including composer.tsx and composer.test.tsx,
auto-merges. The cause is #962 (0.10.1 release cut) and #963 (changelog reformatted to one-line
entries) landing on main after your 2026-09-13 merge. Merge main forward and keep your feature
entry in the new format. I reviewed the head as pushed; nothing in the conflict touches the code
findings below.

⚠️ Major

packages/web/src/routes/task-thread/thread-items.tsx:82-99 — a restored prompt draft follows you
into the next task, and is written to that task's draft store.

UserBubble keeps editing and draft in its own useState and never resets them when
draftRunId changes, while the transcript row key for the task prompt is the constant 'task'
(session-transcript.tsx:90) — so the same component instance survives a route :id change, which
is exactly what useDraft is carefully built to survive internally. The new auto-open effect
(:93-99) is what makes this reachable without the user doing anything.

Reproduced, not inferred. Rendering ThreadView with run r1 (stored task-prompt draft
"DRAFT OF TASK ONE"), then re-rendering with run r2 (no drafts):

SCRATCH editors after switch: 1 [ 'DRAFT OF TASK ONE' ]
SCRATCH puts: [["/api/v1/runs/r2/drafts/task-prompt","{\"text\":\"DRAFT OF TASK ONE!\",\"images\":[]}"]]

So: visit a task that holds an unsaved prompt edit, walk to any other queued task, and that task's
prompt bubble is sitting open holding the first task's text over the second task's prompt. One
keystroke persists it into r2's draft store; ⌘↵ saves it as r2's prompt. That contradicts the
invariant thread-draft.ts:106-108 states outright ("a draft must never leak into another task's
box").

Attribution, so this lands fairly: the stale-editor mechanism predates this PR — I ran the same
scenario at base af7e8289, where opening the editor by hand and switching tasks leaves
[ 'TYPED IN TASK ONE' ] in the next task's bubble too. What this PR changes is that (a) the editor
now opens by itself whenever the visited task has a stored task-prompt draft, so no user action
is needed to get into the stale state, and (b) the leaked text is now written to the other run's
store, so it outlives the session.

Fix: give UserBubble the same render-time identity reset useDraft uses — track draftRunId in
state and, when it changes, setEditing(false), setDraft(text), setActionError(undefined).
Keying the row by run id would work too, but messageActions is keyed by the same string, so both
would have to move together. Worth a test in task-thread.test.tsx beside the existing draft cases:
render with r1 holding a task-prompt draft, rerender with r2, assert no editor is open.

packages/cezar/src/runs/drafts.ts:304-306 — a PUT naming a blob the store no longer holds
throws away the user's text as well, silently and for good.

writeChecked refuses the whole write with unknown image: <id> if any named id is missing. The
cockpit swallows every draft-write failure by design (thread-draft.ts:178-192), and
latest.current.images still holds the dead id — so every subsequent keystroke re-sends it, 400s
again, and that surface silently stops persisting until the user removes the attachment or reloads
(which shows them an empty composer, the text gone).

A blob can disappear under a live composer without anything being wrong:

  • Two clients, which this feature explicitly supports ("they survive a reload, a second
    browser"): browser B sends the message → clear() → an empty PUT → the surface is the run's
    last, so writeChecked:324-328 calls deleteRunDrafts, rm -rf-ing the run's whole draft
    directory, blobs included. Browser A still holds the attachment and its id; from then on A's
    draft never persists again.
  • The same whole-directory delete fires from any other surface's clear() (an Escape on a
    restored rename, a cancelled bubble edit) in the ~500 ms between an upload landing and the
    composer's debounced PUT naming it — see the minor below.

The strictness is right against a client inventing ids, and the fix keeps that: drop unknown ids
and store the text
, the way resolve() (:213-224) already drops a vanished blob from a listing.
Nothing unknown is ever stored either way, and the user keeps their sentence. drafts-api.test.ts:190
and drafts.test.ts:83 pin the current 400 and would need updating to "an unknown id is dropped,
the text survives" — which is the behaviour worth pinning.

🔹 Minor

packages/cezar/src/runs/drafts.ts:324-328 — the whole-directory delete bypasses the orphan grace
window.
ORPHAN_GRACE_MS exists precisely because a blob is uploaded before any record names it
(:99-106), and sweepOrphanImages honours it — but when the last surface is emptied, writeChecked
skips the sweep entirely and rm -rfs the run directory, taking a blob uploaded one second ago with
it. That is the trigger for the major above. Suggest: before deleteRunDrafts in the emptied branch,
check whether images/ holds a file younger than ORPHAN_GRACE_MS and, if so, write the empty
draft.json and let the sweep reclaim it later. Same for deleteRunDraftSurface:351-353.

packages/cezar/src/runs/drafts.ts:547-586 — the store budget has no refusal path, so the 64 MiB
ceiling only holds when the excess lives in other runs.
keepRunId is never evicted, so once the
eviction loop runs out of other runs the write proceeds regardless of the total. The per-surface cap
that is supposed to bound it counts only images named in draft.json (countRunDraftImages), so a
client that uploads blobs and never references them is never counted and never refused — the store
grows until the disk does. The cockpit cannot do this, but server.ts:4370 re-checks the cap
precisely because "a client that is not the composer" is in the threat model, and CODE_REVIEW.md
asks for the same. Suggest returning { ok: false, error: 'draft store is full' } from
writeRunDraftImage when the loop cannot get under the cap — the route already maps that to a 400.

packages/web/src/routes/task-thread/thread-draft.ts:296-307 — an initial GET that lands after a
clear() can re-seed the surface the user just resolved.
clear() resets dirty to false but
leaves seeded.current unset, and React Query replaces the cache wholesale when the in-flight fetch
settles, so the seeding effect (:341-344) fires against a listing the clear already superseded.
Narrow — it needs a clear() with no prior keystroke while the first GET is still in flight (the
realistic instance is cancelling a bubble editor immediately on arrival) — and one line closes it:
set seeded.current = key inside clear().

💅 Nit

CHANGELOG.md — the #938 entry picked up a stray space ((fixes\n #935) (#938)) in an entry
this PR does not otherwise touch. Worth dropping while you resolve the conflict in this file anyway.

🧪 Validation Gate

Run in an isolated worktree at 4fc2f66, TMPDIR pointed outside the checkout (this agent
otherwise runs with a temp dir inside a git repo, which fails ~6 "outside a git repository"
assertions for reasons that have nothing to do with any branch).

Command Status Evidence
npm run typecheck ✅ PASS All four workspaces clean (contract, api-client, server, web) — including the two compile-time guards this PR adds, contract-parity.drafts.test.ts and the five new typed-bodies param assertions.
npm test ⚠️ FAIL (pre-existing, not this branch) Test Files 1 failed / 351 passed (352), Tests 1 failed / 6665 passed (6666). The single failure is src/workflows/agent-profile-wiring.test.ts:82expected [ 'CEZ_API_URL', 'CEZ_BIN', …(6) ] to deeply equal [ 'CEZ_HANDOFF_FILE', …(5) ]. Reproduced verbatim on the base commit af7e8289 in a separate worktree (`Tests 1 failed
npm run test:unit ✅ PASS node:test core-module suite, 0 failures.
npm run build ✅ PASS Server + web; check:pack ok — 504 files, 85 under web/dist (shell + assets present).
npm run test:package ✅ PASS Packaged CLI E2E clean.

Not run: npm run test:e2e is outside validation.commands, and packages/web/e2e/thread-drafts.e2e.ts
— the only spec that exercises "type in task A → open task B → return → the text is there → send →
it is gone" — could not run here either (agent-browser cannot reach the Chrome-for-Testing hosts).
The headline journey is still unobserved in a real browser by anyone. That is what needs-qa is for,
and it is also how the cross-task major above would have been caught.

Duplicate check: git log origin/main -- packages/cezar/src/runs/drafts.ts packages/contract/src/drafts.ts packages/web/src/routes/task-thread/thread-draft.ts
is empty; no part of this work has landed on main by another route.

💥 Breaking Changes

Unchanged from the first review and still clean: the draftRoutes family is entirely new and chained
into the same versioned table as runsRoutes (so it inherits the three-way alias parity —
route-parity.test.ts, versioned-surface.test.ts, bc-route-inventory.test.ts pass here);
BACKWARD_COMPATIBILITY.md carries both the §2 route entry and the §3 drafts/<runId>/ state entry,
now describing the two-file attachment layout; PendingAttachment.id and the Composer
images/onImagesChange seam are additive and /new stays uncontrolled; ensureDataGitignore's
move out of index.ts is a private→exported move with no removed surface; the new drafts/ line is
appended non-destructively to an existing repo's .ai/cezar/.gitignore (data-gitignore.test.ts),
which is what keeps a pasted screenshot out of a user's git history on upgrade. A task with no stored
draft reads as {surfaces:{}} everywhere and behaves exactly as before this feature existed.

🧪 Test Coverage

The suite grew where the last review asked it to — the perf property as behaviour, the incoming-bytes
ceiling, the over-56-KiB keepalive body, the images-only thumbnail removal, and the blur-then-type
rename — and each new case fails without its fix. The gaps left are the ones behind the findings above:

  1. task-thread.test.tsx — render with r1 holding a task-prompt draft, rerender with r2, assert
    no editor is open and no PUT goes to r2 (the major).
  2. drafts.test.ts — a PUT naming one live and one vanished image keeps the text and stores the
    live id (the second major, once it is resolved).
  3. drafts.test.ts — emptying the last surface while a blob younger than ORPHAN_GRACE_MS sits in
    images/ leaves that blob alone.
  4. The e2e spec, on a machine that can provision a browser.

Reviewed at head 4fc2f66 in an isolated worktree. autofix: skipped (not my PR — re-run with --autofix to fix it here).

@pat-lewczuk pat-lewczuk removed their assignment Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

@piotrchabros, three things before this can go in, in the order I'd do them: (1) merge main forward — only CHANGELOG.md conflicts, after #962/#963 reformatted it; (2) reset UserBubble's own editing/draft state when draftRunId changes, because a restored task-prompt draft currently stays open over the next task and gets written to that task's store (reproduced — details and the failing scenario are in the review); (3) make a PUT that names a vanished blob drop the unknown id instead of refusing the whole write, so the user's text is not lost with the attachment.

Everything from the last round is genuinely fixed and each fix has a test that earns its place — the sidecar, the budget estimate, the 56 KiB keepalive threshold, the submit reason on the attachments seam, and commitOnBlur. Push the update and re-request review.

@pat-lewczuk pat-lewczuk removed the in-progress Cezar agent is actively working this issue label Sep 14, 2026
# Conflicts:
#	CHANGELOG.md
#	packages/web/src/routes/task-thread/run-header.tsx
#	packages/web/src/routes/task-thread/session-transcript.tsx
@pat-lewczuk pat-lewczuk self-assigned this Sep 14, 2026
@pat-lewczuk pat-lewczuk added the in-progress Cezar agent is actively working this issue label Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-14T19:41:44Z. Other auto-skills will skip this PR until the lock is released.

Re-review of the new head c5da40b (previous pass reviewed 4fc2f66), with --autofix. (The assignee is @piotrchabros from the last review's author handoff, not an automation lock — no in-progress label was held.)

pat-lewczuk and others added 2 commits September 14, 2026 19:43
# Conflicts:
#	packages/web/src/routes/task-thread/session-transcript.test.tsx
#	packages/web/src/routes/task-thread/session-transcript.tsx
…dead blob eating the text

Addresses the review on open-mercato#940 at head 4fc2f66.

- `thread-items.tsx`: `UserBubble` now resets its own `editing`/`draft` state when
  `draftRunId` changes, the way `useDraft` already resets its internals. The transcript
  keys the prompt row by the constant `'task'`, so walking to another task swapped the
  component's props instead of unmounting it — a restored `task-prompt` editor stayed open
  over the NEXT task's prompt and the first keystroke filed task A's sentence under task B.
- `drafts.ts`: a `PUT` naming a blob the store no longer holds now DROPS the unknown id and
  keeps the text, as `resolve()` already does for a listing. A blob can vanish under a live
  composer (a second browser sends the message and the run's draft directory goes with it),
  and since draft writes fail silently the old 400 threw away the user's sentence with no
  way to notice — and re-threw it away on every keystroke after.
- `drafts.ts`: the whole-directory delete on an emptied last surface now honours
  `ORPHAN_GRACE_MS`, so it cannot `rm -rf` an attachment uploaded seconds ago whose naming
  `PUT` has not fired yet. Same guard in `deleteRunDraftSurface`.
- `drafts.ts`: `enforceStoreBudget` reports whether it got under the ceiling, and an
  attachment upload refuses with `draft store is full` when it could not — eviction never
  touches the run being written, so that case had no bound at all.
- `thread-draft.ts`: `clear()` claims the seed, so an initial `GET` that lands afterwards
  cannot re-seed the surface the user just resolved.

Each fix has a test that fails without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@pat-lewczuk pat-lewczuk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Re-review: feat(tasks): in-task drafts survive leaving the task

Verdict

approve — every finding from both previous rounds is now fixed and re-verified against the
code, the head merges into main again, and the full validation gate is green. This was an
--autofix run, so the outstanding fixes are mine, pushed to this branch as f2335c81; the feature
itself is @piotrchabros's work throughout.

🎯 What changed since the last review

At c5da40b1 the branch carried only a merge of main — the five findings from the
last review were all
still open, and the merge had not cleared the conflict (it had moved: CHANGELOG.md auto-merges
now, but #953's row memoization landed on session-transcript.tsx in the meantime).

Previous finding Disposition
⛔ Blocker — the head no longer merges into main Resolved. Merged main (d988258f) forward in 97f6a246. The live conflict was no longer CHANGELOG.md but session-transcript.tsx/.test.tsx: #953 wrapped each transcript row in a MemoizedRow while this branch threaded runId into renderRowContent. Both sides are wanted, so TranscriptRow now takes runId and MemoizedRow's comparator compares it — dropping it from the comparator would have pinned a row to the first task's id, which is the same class of bug as the major below. mergeable is MERGEABLE again.
⚠️ Major — a restored task-prompt draft follows you into the next task Fixed (thread-items.tsx:97-111). UserBubble now does the render-time (runId, surface) reset useDraft does internally — during render, not in an effect, so no frame paints the outgoing text. Pinned by task-thread.test.tsx "does not carry a restored prompt draft into the next task", which reproduces the original scenario (render r1 holding a task-prompt draft → rerender r2) and fails without the reset with the editor still open holding task one's text.
⚠️ Major — a PUT naming a vanished blob throws away the text too Fixed (drafts.ts:304-312). Unknown ids are now dropped and the text stored, exactly as resolve() already treats a listing whose blob is gone. Nothing unknown is stored either way, so the strictness that mattered — a client cannot invent an id — is unchanged. drafts.test.ts and drafts-api.test.ts were repinned from the 400 to "the id is dropped, the text survives", plus a mixed case (one live, one vanished) asserting the live one is kept.
🔹 Minor — the whole-directory delete bypasses the orphan grace window Fixed (drafts.ts, new hasFreshBlob). Emptying the last surface no longer rm -rfs a run directory holding a blob younger than ORPHAN_GRACE_MS; it writes the empty record and lets the sweep reclaim it. Same guard in deleteRunDraftSurface. The existing "removes the run directory, blobs and all" test now drives a clock past the window, and a new sibling covers the fresh-blob case through to naming the attachment afterwards.
🔹 Minor — the store budget has no refusal path Fixed (drafts.ts). enforceStoreBudget returns whether it got under the ceiling, and writeRunDraftImage refuses with draft store is full (the route already maps that to a 400) when eviction could not — keepRunId is never evicted, so that case previously had no bound at all. The draft record path deliberately still proceeds: refusing there would drop the user's text over a few hundred bytes of JSON, which is the same mistake as the major above.
🔹 Minor — an initial GET landing after clear() re-seeds the surface Fixed (thread-draft.ts:296-312). clear() claims the seed. Worth noting the test needed two corrections to earn its place: the clear's own empty PUT response incidentally corrected the cache, so the case only reproduces with writes failing — which the hook explicitly promises to survive — and the assertion had to flush one render past ready, or it read the state before the seeding effect had run and passed either way.
💅 Nit — stray space in the #938 CHANGELOG entry Resolved by #963's reformat on main.

Findings

None. No new issues in this pass: the diff scan turns up no any, no unchecked cast, no console/alert outside tests in the production paths; the draftRoutes family still 404s on an unknown run before touching the filesystem, validates :surface and :imageId as middleware rather than interpolating them, and re-checks the per-surface cap server-side. Dropping unknown ids relaxes a 400 to a 200 on an unchanged response shape, so it is additive.

🧪 Validation Gate

Run in an isolated worktree at f2335c81, TMPDIR pointed outside the checkout.

Command Status Evidence
npm run typecheck ✅ PASS All four workspaces clean (contract, api-client, server, web).
npm test ✅ PASS Test Files 358 passed (358), Tests 6829 passed (6829). The agent-profile-wiring.test.ts failure I reported last time as pre-existing on main is gone — main fixed it independently, so this gate is fully green for the first time on this branch.
npm run test:unit ✅ PASS 36 pass, 0 fail.
npm run build ✅ PASS check:pack ok — 504 files, 85 under web/dist.
npm run test:package ✅ PASS 16 pass, 0 fail.

Each of the six new/changed tests was run against the pre-fix code to confirm it fails there — five in drafts/drafts-api/task-thread, and the re-seed case above after being corrected twice.

CI is pending at review time. The workflow for f2335c81 had not reported when this review was submitted (license/cla is green; Unit, build, E2E, and package had not started). The verdict is not waiting on it — the local gate above is this run's evidence, and required checks still gate the merge independently.

Not run: npm run test:e2e is outside validation.commands, and packages/web/e2e/thread-drafts.e2e.ts — the only spec that exercises "type in task A → open task B → return → the text is there → send → it is gone" — could not run here either (agent-browser cannot reach the Chrome-for-Testing hosts). The headline journey is still unobserved in a real browser by anyone, which is what needs-qa is for; it is also how the cross-task major would have been caught.

Duplicate check: git log origin/main -- packages/cezar/src/runs/drafts.ts packages/contract/src/drafts.ts packages/web/src/routes/task-thread/thread-draft.ts is empty; none of this work has landed on main by another route.

💥 Breaking Changes

Clean, unchanged from the previous passes: the draftRoutes family is entirely new and chained into
the same versioned table as runsRoutes (three-way alias parity — route-parity.test.ts,
versioned-surface.test.ts, bc-route-inventory.test.ts all pass here);
BACKWARD_COMPATIBILITY.md carries both the §2 route entry and the §3 drafts/<runId>/ state entry;
PendingAttachment.id and the Composer images/onImagesChange seam are additive and /new
stays uncontrolled; ensureDataGitignore's move out of index.ts is a private→exported move with no
removed surface. A task with no stored draft still reads as {surfaces:{}} everywhere and behaves
exactly as it did before this feature existed.


Re-reviewed at head f2335c81 in an isolated worktree. --autofix run: five findings fixed and pushed as a follow-up commit (fast-forward, no force-push) to this branch rather than carried to a replacement PR, since maintainerCanModify is set — that keeps #940 and its review history intact.

@pat-lewczuk pat-lewczuk added merge-queue Approved, ready to merge and removed changes-requested Reviewer requested changes labels Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🧪 Manual QA instructions (needs-qa)

Exercise in-task draft persistence — unsent composer text, unsent inline message edits, and pasted
attachments surviving a route change, a reload and a second browser. Approved in
this review; the local
gate is green, but packages/web/e2e/thread-drafts.e2e.ts has never run in a real browser, so the
headline journey below is unobserved. P0 rows are the cross-task isolation ones — that is where both
majors of the last round lived.

Priority Setup and action Expected result / boundary
P0 With two queued/waiting tasks A and B: type half a sentence into A's reply composer, navigate to B without sending. B's composer is EMPTY. Nothing of A's text appears. Return to A — the text is back, exactly as typed.
P0 In task A, open a queued message's inline editor (or the task prompt's) and change the text without saving. Navigate to B, which has no draft of its own. B's prompt/message bubbles are all CLOSED — no editor sits open holding A's text. Navigate back to A: its editor re-opens with the unsaved edit.
P0 Repeat the row above, but in B type one character into the prompt bubble after arriving. What is stored for B is only what you typed in B. Returning to A still shows A's own unsaved edit, unchanged.
P0 Task A: paste a screenshot into the composer so the thumbnail appears. In a SECOND browser (same project), open task A and send its message, clearing the draft. Back in the first browser, keep typing. The first browser's text keeps persisting — reload it and the sentence is still there. Before the fix the whole surface stopped saving silently.
P1 Type a reply in a task, then hard-reload the page (⌘R / F5) before the message is sent. The composer comes back holding the text, and any pasted attachment comes back as a thumbnail.
P1 Type a reply, then send it. Reload. The composer is empty — sending clears the draft. The sent message (with its attachments) is in the transcript.
P1 Type a reply while the agent is running, then close the tab mid-sentence and reopen the task. The text is there. (This is the keepalive/pagehide flush; try it once with a short draft and once with a long one, over ~56 KiB, which takes a different code path.)
P1 Force a send to fail (stop the server, or send in a task whose run was deleted), then look at the composer. The message is restored into the composer with its attachments intact, and survives a reload — a failed send must not clear the draft.
P1 Paste 4 images into one composer, then try a 5th. The 5th is refused with the per-draft cap message; the first four still send correctly.
P1 Start renaming a task title from a restored draft, then click away (blur) without typing. The rename does NOT auto-commit. Blur-then-type, and Enter/Escape, behave as before.
P2 On a fresh clone/upgrade, check .ai/cezar/.gitignore after using a draft with an attachment. It carries a drafts/ line, appended without disturbing existing lines — a pasted screenshot must never land in the user's git history.

Also worth a pass on mobile/narrow viewport for the composer and the inline editors, and a cold load
of a task that has a stored draft (the editor should open with the text, not flash empty first).

QA reviewer: move merge-queueqa when starting. All checks pass → replace qa with
merge-queue and qa-approved. A failure → replace qa with qa-failed and report the failed
action, expected result, and observation. Required QA approval still gates merge in this repository
(qaGate is on), so merge-queue alone will not let this land.

@pat-lewczuk pat-lewczuk added ci-monitoring and removed in-progress Cezar agent is actively working this issue labels Sep 14, 2026
@pat-lewczuk pat-lewczuk removed their assignment Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr completed: APPROVED. Lock released (swapped for ci-monitoring — CI had not reported at review time and this run still owes a result comment).

Re-review of head f2335c81: the review. This was an --autofix run and one fix iteration completed: the branch arrived with all five findings from the last round still open and a live conflict, so I merged main forward (resolving session-transcript.tsx in favour of keeping BOTH #953's row memoization and this branch's runId threading), then fixed the two majors and three minors and pushed f2335c81.

Delivery note for @piotrchabros: because maintainerCanModify is set on this PR, the fixes went to your branch as an ordinary fast-forward follow-up commit (no force-push) rather than to a replacement PR — that keeps #940, its two earlier reviews and this discussion intact. Revert f2335c81 if you would rather take any of it a different way; the reasoning for each fix is in the review.

merge-queue + needs-qa: the QA-approval gate holds the merge until a reviewer works through the manual QA instructions. The e2e spec covering the headline journey still has not run in any browser.

@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr — CI result

Green at head f2335c81run 34889747813. Unit, build, E2E, and package SUCCESS (typecheck, unit tests, server + cockpit suites, build, packaged CLI E2E, release-package verification all passed), license/cla SUCCESS, Publish npm snapshot skipped as expected. Matches the local gate in the review.

The run needed a maintainer to release it (fork PR, action_required); I approved it after reviewing the diff, so that is not something still waiting on anyone.

The PR is now MERGEABLE / CLEAN and approved. Removing ci-monitoring — this agent's follow-up is done and no further comment will come from it.

⚠️ Worth being precise about one thing, since the job name invites the opposite reading: the "E2E" in Unit, build, E2E, and package is the packaged CLI suite. packages/web/e2e/thread-drafts.e2e.ts — the only spec that drives the headline journey in a real browser — did not run in CI, and has still never run anywhere. The manual QA checklist is the only thing that will cover it; needs-qa stays and the QA gate holds the merge.

@pat-lewczuk
pat-lewczuk merged commit 00b77b4 into open-mercato:main Sep 14, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New capability merge-queue Approved, ready to merge needs-qa Requires manual QA before merge priority-medium Ordinary bug or feature risk-medium Ordinary change with tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement: in-task drafts survive leaving the task — restore unsent composer text and attachments

2 participants