ce-code-review pass 03 — claude-opus-5

Scope: git diff origin/main..HEAD (base 070cb10, 99 files, +7338/-102). Reviewers: correctness, security, testing, maintainability, project-standards, performance, api-contract, data-migration, reliability, julik-frontend-races, agent-native, learnings. The standalone adversarial pass runs as its own spawn; findings below were reached independently and two of them corroborate it.

High

[x] AUTO-FIX: Removal tombstones count toward the five-file ceiling at commit but not at reservation, so remove-then-replace permanently wedges a composer: reserve_slot! counts batch.image_attachments.active_drafts.count >= ImageAttachment::MAX_FILES (app/services/image_attachments/ingest.rb:99) while ensure_slot_available! counts every row including tombstones — batch.image_attachments.where.not(id: attachment.id).count < ImageAttachment::MAX_FILES (app/services/image_attachments/ingest.rb:254); commit cbb7be6 switched the reservation side to active_drafts and left the commit side behind, so "attach five, remove one, attach a replacement" streams and fully decodes the new file and only then raises too_many_files, record_failure turns the rejected row into a failed row that occupies an active_drafts slot of its own, and ClaimBatch#validate_attachments! then blocks the post with attachments_not_ready with no recovery short of abandoning the composer — total_byte_size (app/models/image_attachment_batch.rb:95) and DraftPresenter.batch both use active_drafts, which makes line 254 the one unintentional divergence, and test/services/image_attachments/ingest_test.rb:107 ("rejects a sixth file") never involves a removal.
[x] AUTO-FIX: A claimed batch replayed against the other composer endpoint 500s in one direction and silently succeeds without writing in the other: ClaimBatch#claim short-circuits to replay on next replay if batch.state_claimed? before validate_ownership! and hands back batch.claimed_parent, an Annotation or an AnnotationComment (app/services/image_attachments/claim_batch.rb:47,60-66), while submission_batch only verifies uploader and project and never that the claimed parent matches the endpoint (app/controllers/concerns/image_attachment_submission.rb:22-30) — so posting a reply batch's public ID to AnnotationsController#create reaches respond_created(result.parent) → annotation.viewport (app/controllers/annotations_controller.rb:17,57-66) and raises NoMethodError (500) instead of an ordinary 422, and posting a root batch's ID to AnnotationCommentsController#create discards the ClaimBatch.call return value entirely (app/controllers/annotation_comments_controller.rb:13) and answers 201 created having created no comment; plan Unit 4 names "cross-composer batches" in its required verification list and test/controllers/image_attachment_submission_test.rb has no such case — the response-loss retry tests only replay against the same endpoint.

Medium

[x] AUTO-FIX: Attaching or removing a file during the interrupted-upload settling window can orphan an in-flight item or leave an untracked ghost row: resumeBatch unconditionally clears and rebuilds this.items and the DOM on every poll tick while this.resuming is true (app/javascript/controllers/image_attachment_composer_controller.js:288-290), but enqueue() (:172) and remove() (:454) guard only on this.submitting, and the Remove button's disabled expression at :744 likewise omits this.resuming even though the alt-text input at :743 includes it — so the reconnect path that pass 02 added is itself racy against user input.
[x] AUTO-FIX: Over-length alt text returns a 500 instead of the machine-code error envelope every other draft endpoint returns: WriteAltText#call uses attachment.update!(alt_text: alt_text) (app/services/image_attachments/write_alt_text.rb:30) against a validates :alt_text, length: { maximum: MAX_ALT_TEXT } (app/models/image_attachment.rb:49), and ImageAttachmentDrafts::BaseController rescues only ImageAttachments::Error and RateLimitStore::Unavailable while ApplicationController rescues only ActiveRecord::RecordNotFound (app/controllers/application_controller.rb:14) — the browser's maxlength hides it, but plan Unit 2 step 3 requires a machine code and actionable text on every draft response, and no test covers alt text longer than MAX_ALT_TEXT.
[x] AUTO-FIX: The three new image_attachment_batches CHECK constraints are asserted to exist by name and never exercised: test/integration/database_constraint_contract_test.rb:24-27 lists image_attachment_batches_valid_state, _future_expiry, and _claim_state in the name-existence map only, and unlike image_attachments — whose _exclusive_parent and _submitted_ready constraints do get raw-SQL rejection tests (test/models/image_attachment_test.rb:100-124) — no direct-SQL insert ever proves a batch constraint rejects bad data; there is also no test/models/image_attachment_batch_test.rb at all, so usable?, expired?, and touch_activity! are only covered indirectly.
[x] AUTO-FIX: ImageAttachmentDraftCleanupJob.enqueue_for_startup! is dead code that mimics a boot-time safety net without providing one: it is a verbatim copy of ReconcileScreenshotProcessingJob.enqueue_for_startup! down to the raise job.enqueue_error body (app/jobs/image_attachment_draft_cleanup_job.rb:15-21), but bin/docker-entrypoint:109 calls only the screenshot job and nothing anywhere calls the attachment one — wiki/self-hosting.md:172 documents that startup enqueue as the established convention, and the sibling has a dedicated test while this method has none; either wire it into the entrypoint or delete it and rely on the 15-minute recurring sweep.
[x] AUTO-FIX: Every REST and MCP annotation-detail read preloads attachment blobs no serializer touches: Api::V1::ProjectScope.annotation_details preloads { image_attachments: { image_attachment: :blob } } for the root and every comment (app/services/api/v1/project_scope.rb:37-42), but ImageAttachment#as_contract_json reads only plain columns (app/models/image_attachment.rb:120-131) and ContractSerializer#image_attachment_media_url only calls generate_token_for (app/serializers/api/v1/contract_serializer.rb:153-158), whose payload is just the record ID — dead join weight on the exact path get_annotation / screenote annotation get hits repeatedly.
[x] AUTO-FIX: ImageDecoding::Guard.synchronize applies the full timeout twice, so a composer can block ~10s before it is told the decoder is busy: acquire_key(key, timeout) { acquire_global(timeout, &block) } (app/services/image_decoding/guard.rb:29) passes the same WAIT_SECONDS = 5 to both the per-batch and the global stage, and Ingest#decode! calls it with no timeout override — the keyed layer's own comment names multi-file composer work as its target case, which is exactly when both waits stack.
[x] AUTO-FIX: A failing COMMIT leaks the staged blob with no reconciliation path: Ingest#commit! sets attached = true as the last statement inside ImageAttachment.transaction (app/services/image_attachments/ingest.rb:205) while the cleanup is ensure discard_blob(blob) if blob && !attached (:211), so an exception raised by the COMMIT itself rolls the row back to uploading yet skips the purge; the blob was created outside the transaction by stage_blob! (:176, create_after_unfurling!) and ImageAttachmentOrphanReconciliationJob only scans ImageAttachment rows, so nothing ever reclaims it — moving attached = true to after the transaction block closes it.
[x] RESOLVED/NO-FIX: Design call — the agent read loop ends at metadata: attachments[].url requires both the caller's own Authorization: Bearer header and the 5-minute purpose token (app/controllers/api/image_attachment_media_controller.rb:20-33), but internal/screenote/client.go has no authenticated-fetch method and there is no screenote attachment fetch/download subcommand in internal/cli/, while internal/cli/config.go:8-30 deliberately reports only TokenSet bool — so an agent onboarded through the documented screenote login OAuth flow has no sanctioned way to turn the URL into bytes short of scraping ~/.config/screenote/config.toml, and the MCP surface has the same gap (the crop is inlined as cropped_image_base64; human-attached images are not). The plan's R9 only requires the metadata projection, so this is not a missing plan deliverable — flagging it because CLAUDE.md names the CLI the public agent contract and the headline workflow is "agent reads feedback with the CLI".

Nit

[x] AUTO-FIX: media_blob variant resolution is duplicated rather than shared: app/controllers/image_attachment_media_controller.rb:45-54 and app/controllers/media_controller.rb:25-35 have the same raw-variant early return, ALLOWED_VARIANTS lookup, and variant(key).image&.blob shape (plus near-identical no-decode-on-GET comments), differing only in the attached? guard and the extra download variant — both already include BlobStreaming, which is the natural home for a shared resolve_variant_blob.
[x] AUTO-FIX: The unsupported-media-type message is hardcoded in the composer JS and can drift from the server's: "Attachments must be PNG, JPEG, or WebP images." appears at several call sites in app/javascript/controllers/image_attachment_composer_controller.js (first at :131) with no link to ImageAttachments::Error::UNSUPPORTED_MEDIA_TYPE or ImageAttachment::ALLOWED_CONTENT_TYPES, and no test pins the two in sync.
[x] AUTO-FIX: The draft API declares two rate_limit calls without an explicit shared scope (app/controllers/image_attachment_drafts/base_controller.rb:16-23), so the user and IP budgets may be keyed per including controller rather than across the whole draft surface; if two independent budgets were intended, say so in a comment.
[x] AUTO-FIX: attachments is omitted rather than [] on the comment object returned by the REST write endpoints, because ContractSerializer.annotation_comment gates the key on url_options truthiness (app/serializers/api/v1/contract_serializer.rb:123-135) — harmless today since agent-authored replies and resolution comments can never carry attachments, but the same "comment" shape now differs across endpoints and nothing pins it.
[x] AUTO-FIX: The new attachment eager-load paths have no query-budget regression test despite an established convention: test/controllers/api/v1/annotation_attachments_contract_test.rb reimplements its own inline count_queries subscriber with a loose fixed ceiling instead of reusing capture_app_queries / assert_variant_records_preloaded from test/controllers/projects_controller_test.rb, and every new attachment test uses exactly one annotation/comment/attachment, so an N+1 in PagesController#load_workspace or annotation_details would not be caught.
[x] AUTO-FIX: No system test mounts a root and a reply composer at the same time, though plan Unit 3's verification list names "concurrent composers" explicitly — batch isolation across simultaneously mounted composers is currently unproven end to end.
[x] AUTO-FIX: The new wiki/log.d/*.md fragments use two different internal formats — three follow the repo-wide Action / Pages-updated / Source structure, three are bare YAML frontmatter with neither section — which may matter to whatever compiles them into wiki/log.md.

Coverage

Verified against the current worktree, not the first feature commit; findings already fixed by the fix(attachments) commits on this branch were dropped.
Correctness reproduced the tombstone-cap defect by execution (rolled-back transaction, no repo mutation); testing reproduced both cross-composer directions with a throwaway integration test.
Clean passes with no findings: project-standards (BEM, enums, service-object shape, MiniTest + fixtures, importmap, wiki fragments present and wiki/log.md untouched) and api-contract (attachments always present on detail root and every comment, absent on list rows, MCP golden key fixture preserved, Go nil-vs-empty round trip, url_expires_at derived from the same MEDIA_TOKEN_EXPIRY constant as generates_token_for).
Security found no exploitable authorization, ingest, CSRF, or delivery defect. Confirmed sound: draft reads are uploader-scoped and a same-project collaborator gets 404; the bearer route requires principal + scope + attachment-bound purpose token + submitted? + live project_access?; ingest derives media type from bytes, requires declared/extension agreement, and caps dimensions/pixels before a forced full decode; no provider redirect or durable URL; the digest email carries no attachment URL.
Dropped as deliberate plan boundaries: metadata-light list reads omitting attachments, and media denials collapsing to a single non-enumerating not_found.
Residual risks worth an operator's eye, not filed as findings: media authorization uses the denormalized image_attachments.project_id rather than the live parent's project (equivalent today — nothing relocates a page between projects); the bearer purpose token travels as a query parameter, so an external proxy access log would record it (bounded — the token alone grants nothing); a valid image with arbitrary trailing bytes is still stored; draft batches of a removed member survive to the 24h sweep while still consuming that user's caps; Rack buffers the full multipart body before the 20 MB check (bounded in the release image by Thruster's MAX_REQUEST_BODY); ImageAttachmentOrphanReconciliationJob runs its GROUP BY/HAVING aggregate before LIMIT 500, so cost scales with total submitted rows; RecurringTasks.schedule re-reads and ERB-evaluates config/recurring.yml on every /ready probe.
Untested branches noted but not filed: MAX_DIMENSION (only image_pixels_too_large is covered), the Range: branch of BlobStreaming#stream_blob, and cross-purpose token replay.