Review · grok-ce-code-review-adversarial-01 · Image attachments in comments and replies

High

[x] RESOLVED/NO-FIX: Abort/Remove leaves occupying draft rows: remove() skips DELETE unless serverId is set (image_attachment_composer_controller.js:268), but ingest persists an uploading row before bytes stream and error JSON has no attachment id, so a discarded failure still occupies a 5-slot and ClaimBatch rejects attachments_not_ready.
[x] RESOLVED/NO-FIX: Retry of a ready client_key un-readies a successful upload: reserve_slot! always update!(state: :uploading) on the existing row (ingest.rb:86), so a lost-201 retry can then record_failure over a committed ready draft and block claim.
[x] RESOLVED/NO-FIX: Enqueue/drop/paste still run while submit is in flight: refreshSubmitState only disables submit buttons (image_attachment_composer_controller.js:113,431), so a new uploading row can be reserved before ClaimBatch's lock and fail or extra-bind the post.
[x] AUTO-FIX: Overlay cancel during submit reports a successful post as failed: Annotorious cancelForm removes the form and disconnect nulls this.form, then navigateAfterSubmit throws on this.form.closest (image_attachment_composer_controller.js:342) and the catch tells the user the message could not be posted.
[x] AUTO-FIX: Disconnect continues abandoned uploads: ensureBatch() uses unabortable fetch (image_attachment_composer_controller.js:176,237); after disconnect clears items, the in-flight upload() can still POST into the leaked batch.
[x] RESOLVED/NO-FIX: Alt-text PATCH races claim and can mutate submitted rows: alt is fire-and-forget on change and onSubmit never awaits it (image_attachment_composer_controller.js:154,284,297); submitted_metadata_is_immutable returns early when stale image_attachment_batch_id_in_database is present (image_attachment.rb:154), so a pre-claim loaded update! can write alt text after bind.
[ ] ESCALATE: Cancelled composers exhaust the 6-open-batch cap for 24h: each first upload POSTs a new batch and disconnect never discards it (batches_controller.rb:25); after six attach-and-cancel cycles too_many_open_batches tells the user to discard one, but there is no destroy endpoint.

Medium

[x] RESOLVED/NO-FIX: decoder_busy is persisted as failed (ingest.rb:50) so a transient decoder slot blocks submit of sibling ready images until the user clicks Retry, against the retryable-contention invariant.
[x] AUTO-FIX: Storage/IO failures in ingest re-raise raw StandardError after recording upload_failed (ingest.rb:52), so rescue_from ImageAttachments::Error never runs and the composer gets a non-JSON 500.
[x] RESOLVED/NO-FIX: Scheduler-independent draft byte cap is only checked at batch create (batches_controller.rb:33); six open batches can then ingest up to 6x50MB and exceed MAX_OUTSTANDING_DRAFT_BYTES.
[x] RESOLVED/NO-FIX: Multipart request.content_length is passed as declared_length (attachments_controller.rb:15), so envelope overhead can 422 a legitimate file near the 20MB limit before the stream check.
[x] RESOLVED/NO-FIX: Client never enforces the 50MB total: maxTotalSize is wired but unused (image_attachment_composer_controller.js:15).
[x] AUTO-FIX: PostgreSQL qualification never asserts adapter_name and the 5-file race stays outcome-only (concurrency-qualification.yml:24, image_attachment_concurrency_test.rb:109).
[x] RESOLVED/NO-FIX: test/system/image_attachments_test.rb is not in specialized.system_claimed and bin/rails test does not run system tests, so picker/reply/reopen/viewer Playwright evidence is not CI-gated.
[x] RESOLVED/NO-FIX: Viewer test titled keyboard/focus only clicks thumbnail and close (image_attachments_test.rb:183); no Arrow/Escape/activeElement assertions.
[x] AUTO-FIX: Browser suite never posts two images (SECOND_IMAGE_PATH unused at image_attachments_test.rb:32); multi-image bind is only proven at the request layer.
[x] AUTO-FIX: No recurring rewarm of submitted variants: ImageAttachmentThumbnailJob comment claims reconciliation will produce them (image_attachment_thumbnail_job.rb:5) but orphan reconciliation only purges parentless rows.
[x] RESOLVED/NO-FIX: get_annotation description still says cropped PNG only (get_annotation_tool.rb:5) even though detail now always includes attachments arrays agents must fetch with the same bearer before url_expires_at.
[x] AUTO-FIX: Composer XHR has no timeout (image_attachment_composer_controller.js:207) and Retry does not abort a prior in-flight request (:205), so a hung upload can keep submit blocked.

Nit

[x] RESOLVED/NO-FIX: Failed composer items are styled via [data-state="failed"] instead of a BEM modifier (application.css:2756).
[x] RESOLVED/NO-FIX: Private blob streaming headers/Range handling are duplicated across browser, API, and screenshot media controllers (image_attachment_media_controller.rb:56).
[x] RESOLVED/NO-FIX: Attachment CSS (~200 lines) was appended to the monolithic application.css (:2598) rather than a component stylesheet.
[x] AUTO-FIX: Ingest and DraftPresenter duplicate failure strings that can drift (draft_presenter.rb:8).
[x] AUTO-FIX: Draft open-batch/byte caps live in the controller (batches_controller.rb:22) instead of the batch model/service.
[x] RESOLVED/NO-FIX: Go attachments,omitempty drops empty arrays on remarshal (internal/screenote/types.go:91); CLI annotation get uses RawMessage today so this is latent.
[x] RESOLVED/NO-FIX: Paste system test never asserts the textarea kept text/plain (image_attachments_test.rb:84).
[x] RESOLVED/NO-FIX: repositionForm is a pass-through alias of requestAnnotationFormPosition (annotorious_controller.js:505).
[x] AUTO-FIX: CSRF tests under with_forgery_protection only assert deny, not the composer's X-CSRF-Token success path (attachments_controller_test.rb:99).