Skip to content
Hive

screenote · Image attachments in comments and replies

Review

reviews/codex-native-review-04.md

High

  • [x] RESOLVED/NO-FIX: Reject trailing-data image polyglots — <task-worktree>/app/services/image_attachments/ingest.rb:159-159 When a valid PNG/JPEG/WebP has bytes appended after its end marker, Marcel identifies the leading image and libvips' decoded.avg still succeeds. The original polyglot bytes are then stored and served by the original/download routes, so verify that decoding consumed the whole stream or persist only a canonical re-encode. <!-- triage: duplicate of the polyglot AUTO-FIX in codex-ce-code-review-04 -->
  • [x] RESOLVED/NO-FIX: Make failure updates conditional on the row still uploading — <task-worktree>/app/services/image_attachments/ingest.rb:297-300 When an aborted request and its same-key retry overlap, reload can observe uploading, then the retry can commit ready or removal can write its tombstone before this unconditional update obtains the row lock. The stale request then rewrites the successful or removed row to failed, blocking claim… <!-- triage: duplicate of the conditional failure-state AUTO-FIX in codex-ce-code-review-04 -->
  • [x] RESOLVED/NO-FIX: Replay claimed reopen batches before checking resolved state — <task-worktree>/app/controllers/annotation_comments_controller.rb:10-10 When a successful reopen response is lost, the first request has already made the annotation open and claimed the batch; retrying the identical request therefore exits here with 422 before ClaimBatch can return the existing reopen comment. This breaks submission replay and reports failure for… <!-- triage: duplicate of the claimed-reopen replay AUTO-FIX in codex-ce-code-review-04 -->
  • [x] RESOLVED/NO-FIX: Return all submitted composer fields in 422 responses — <task-worktree>/app/controllers/concerns/image_attachment_submission.rb:67-70 On any root, reply, or reopen validation failure, form contains only the batch and ready IDs, so the required 422 rehydration contract cannot restore the submitted message or root coordinates and viewport after a remount or reconnect. Include the permitted annotation/comment fields and reopen i… <!-- triage: duplicate of the complete 422 rehydration AUTO-FIX in codex-ce-code-review-04 -->
  • [x] RESOLVED/NO-FIX: Check the live Solid Queue scheduler in readiness — <task-worktree>/app/services/screenote/recurring_tasks.rb:31-36 In production, registered? becomes true solely because these names appear in checked-in YAML; it remains true if no Solid Queue scheduler booted, if the scheduler rejected the tasks, or if its heartbeat died. Readiness therefore reports healthy while cleanup never runs and drafts accumulate. <!-- triage: duplicate of the live-supervisor readiness AUTO-FIX in codex-ce-code-review-04 -->
  • [x] AUTO-FIX: Retain failed blob deletions for retry — <task-worktree>/app/services/image_attachments/ingest.rb:240-242 When a staged upload must be discarded and storage deletion fails, ActiveStorage::Blob#purge has already destroyed the blob record before calling the service. This rescue then drops the only durable key, so neither cleanup nor reconciliation can retry and repeated rejected uploads can leak untracked provider bytes. <!-- triage: plan lifecycle contract forbids unrecoverable primary-byte leaks -->
  • [x] RESOLVED/NO-FIX: Preserve uploaded drafts when Annotorious is cancelled — <task-worktree>/app/javascript/controllers/image_attachment_composer_controller.js:645-646 When a user cancels the Annotorious overlay after an upload has completed, this DELETE destroys the open batch and purges its ready attachments. The authoritative flow treats cancelForm like disconnect or navigation and keeps committed drafts for their 24-hour recovery window, so… <!-- triage: prior escalation A1 requires purge on explicit Annotorious cancel and preservation only on disconnect/navigation -->

Medium

  • [x] RESOLVED/NO-FIX: Return and validate the comment produced by the claim — <task-worktree>/app/controllers/annotation_comments_controller.rb:13-18 When a claimed reply batch is submitted from another reply composer in the same project, ClaimBatch returns its original AnnotationComment, but this controller discards that result and sends 201 for the requested annotation. No comment is created on that thread although the UI reports success; resp… <!-- triage: duplicate of the claimed-target binding and returned-comment-ID AUTO-FIXes in codex-ce-code-review-04 -->
  • [x] AUTO-FIX: Refuse expired batches on the resume endpoint — <task-worktree>/app/controllers/image_attachment_drafts/batches_controller.rb:13-13 When a tab reconnects after the 24-hour expiry, this endpoint still returns 200 with ready rows and preview URLs even though media, upload, and claim all reject that batch. The controller restores those rows as ready and enables submission, so every post then fails with batch_unusable. <!-- triage: Unit 2 requires resume access only for open unexpired batches -->
  • [x] RESOLVED/NO-FIX: Extend batch expiry while still holding the batch lock — <task-worktree>/app/services/image_attachments/ingest.rb:213-213 If the batch expires just after ensure_usable!, cleanup can acquire the batch lock and destroy it after the commit releases that lock but before this separate activity update. A decoded upload that was accepted before expiry is then purged and returned as a failure; move the expiry bump into the locked tra… <!-- triage: duplicate of the atomic ready-transition renewal AUTO-FIX in codex-ce-code-review-04 -->

Nit

No findings.

← Back to task

HiveDev, when it’s ready.

HiveDev is the hosted product being built around open-source Hive. It isn’t available yet; join the waitlist to hear when it is. We’ll use your email for HiveDev availability updates.

Read our privacy notice.