Review · claude-opus-5-ce-code-review-adversarial-03 · Image attachments in comments and replies

High

[x] AUTO-FIX: Removal tombstones count toward the five-file ceiling at commit but not at reservation, so a composer breaks permanently after five removals: reserve_slot! counts batch.image_attachments.active_drafts.count (app/services/image_attachments/ingest.rb:99) while ensure_slot_available! counts every row — tombstones included — with batch.image_attachments.where.not(id: attachment.id).count < ImageAttachment::MAX_FILES (app/services/image_attachments/ingest.rb:254); "attach five, remove one, attach a replacement" therefore streams and fully decodes the new file and then raises too_many_files ("You can attach up to 5 images.") while the rail shows four, record_failure turns the rejected row into a failed row that occupies an active_drafts slot of its own, and since RemoveAttachment keeps the tombstone "until claim/discard/expiry" (app/services/image_attachments/remove_attachment.rb:5-7) every subsequent upload in that batch fails the same way with no recovery short of cancelling the composer. total_byte_size (app/models/image_attachment_batch.rb:95) and DraftPresenter.batch (:21) both use active_drafts, and outstanding_draft_bytes documents its deliberate opposite choice (:55-57), which makes the commit-side count the one unintentional inconsistency; test/services/image_attachments/ingest_test.rb:107 ("rejects a sixth file") never involves a removal, so nothing in the suite covers remove-then-replace.

Medium

[x] AUTO-FIX: A replayed batch that was claimed by a different composer is handed back untyped and crashes the annotation endpoint: ClaimBatch#replay returns batch.claimed_parent — an Annotation or an AnnotationComment (app/services/image_attachments/claim_batch.rb:60-66, app/models/image_attachment_batch.rb:86-88) — and AnnotationsController#respond_created(result.parent) then calls annotation.viewport and reports annotation_id: annotation.id (app/controllers/annotations_controller.rb:57-66), so posting a reply/reopen batch's public ID to POST /screenshots/:id/annotations raises NoMethodError (500) instead of an ordinary 422, while the mirror case in AnnotationCommentsController#respond_created ignores result.parent entirely (app/controllers/annotation_comments_controller.rb:29-44) and answers 201 created without creating a comment; submission_batch only verifies uploader and project, never that the batch's claimed parent matches the endpoint (app/controllers/concerns/image_attachment_submission.rb:19-29), and plan Unit 4's verification list names "cross-composer batches" while test/controllers/image_attachment_submission_test.rb has no such case.
[x] AUTO-FIX: Composer thumbnails re-download the full original from the server even though the picked File is already in memory: DraftPresenter.preview_url mints image_attachment_media_path(attachment, :original) (app/services/image_attachments/draft_presenter.rb:45) and renderItem assigns it straight to thumb.src (app/javascript/controllers/image_attachment_composer_controller.js:722-724), so a five-image 50MB message pulls all 50MB back through the app server — streamed by BlobStreaming under private, no-store so nothing is cached — purely to paint a rail thumbnail; drafts correctly never warm variants (plan Unit 5 step 2), which makes URL.createObjectURL(item.file) for locally-picked items the only way to avoid the round trip, with the server preview kept for resumed rows where item.file is null (:239-250).

Nit

[x] AUTO-FIX: ImageAttachmentDraftCleanupJob.enqueue_for_startup! (app/jobs/image_attachment_draft_cleanup_job.rb:15-21) is never called by any boot script, initializer, config, or test — bin/docker-entrypoint:109 wires only ReconcileScreenshotProcessingJob.enqueue_for_startup! — so it is unexercised dead API copied from a precedent that is wired; either call it alongside the existing entrypoint line or drop it and rely on the 15-minute recurring schedule.
[x] AUTO-FIX: Ingest#stream_to! loops on while (chunk = io.read(CHUNK_SIZE)).present? (app/services/image_attachments/ingest.rb:113), so a 64KB chunk consisting only of whitespace bytes is treated as EOF and silently truncates the upload into a later invalid_image; IO#read(n) already returns nil at EOF, so while (chunk = io.read(CHUNK_SIZE)) is the correct and intent-revealing test.
[x] AUTO-FIX: WriteAltText writes alt text onto a removal tombstone: it only re-resolves the row by ID under the batch lock with no state guard (app/services/image_attachments/write_alt_text.rb:27-30), so a PATCH that lands after RemoveAttachment#purge_removed_image has nulled alt_text (app/services/image_attachments/remove_attachment.rb:80-86) re-populates the field on a row the composer already discarded; guard on !attachment.removal_tombstone? so the write answers not-found like the rest of the removal contract.
[x] AUTO-FIX: Screenote::Readiness#ready? now re-reads and ERB-evaluates config/recurring.yml from disk on every health probe: recurring_cleanup_registered? (app/services/screenote/readiness.rb:74-76) calls RecurringTasks.registered?, which re-runs path.read plus ERB.new(...).result and YAML.safe_load per call (app/services/screenote/recurring_tasks.rb:39-45); the schedule cannot change without a redeploy, so memoize it per environment.
[x] AUTO-FIX: NotificationMailer#annotation_workspace_url re-implements the workspace deep link instead of reusing the existing helper: app/mailers/notification_mailer.rb:58-67 hand-builds version_id plus the available_viewports check that PageWorkspaceNavigation#page_workspace_path_for / #validated_workspace_viewport already own (app/controllers/concerns/page_workspace_navigation.rb:12-28), so the digest CTA and every in-app link can drift apart on the next change to workspace routing.