PR Review — Go CLI and REST contract (pass 1)

Diff reviewed: git diff origin/main..HEAD. Multi-persona union below.

High

• [x] AUTO-FIX: [correctness] screenote screenshot create uploads with the wrong content type and is rejected by the server: internal/cli/screenshot.go:834 always calls CreateScreenshot(..., "", reader), and client.go:1135 defaults an empty content type to application/octet-stream. The server sets params[:image].content_type from that multipart part, and Screenshot/ScreenshotImage#acceptable_image validates blob.content_type.in?(%w[image/png image/jpeg]) (app/models/screenshot.rb, app/models/screenshot_image.rb). So every real CLI upload attaches application/octet-stream and fails validation with 422, breaking a must-have command (plan A3/Unit 2). The CLI never sniffs/derives the type from the file extension. <!-- triage: "fails with 422" is contested — ActiveStorage attach re-identifies content_type from bytes via Marcel (identify defaults true), so real PNG/JPEG uploads currently pass (matches claude reviewer's nit); regardless, deriving content type from file extension is the robust fix and satisfies plan Unit 4 — auto-fixing rather than escalating since the fix is unambiguous -->
• [x] AUTO-FIX: [testing] The upload bug is fully masked by the tests: internal/screenote/client_test.go passes "image/png" explicitly and the Rails screenshots_controller_test.rb uses fixture_file_upload("test_image.png", "image/png"), so no test exercises the actual CLI code path (empty content type → octet-stream). Plan Unit 4 requires a test for the "expected content type fallback" and it does not cover the real default. <!-- triage: add test exercising the actual CLI content-type derivation/fallback path — required by plan Unit 4 -->

Medium

• [x] AUTO-FIX: [correctness] annotation list without --screenshot applies --limit/--offset per screenshot and then concatenates (internal/cli/annotation.go:98-117): the user's limit/offset are sent on each per-screenshot request, so the aggregate is neither limited nor offset correctly, and pagination.total is reported as len(annotations) (the count returned), not the true total — misleading output. <!-- triage: duplicate of claude-ce Medium (annotation list rework) — apply limit/offset to aggregate, report true total -->
• [x] AUTO-FIX: [correctness] annotation list aggregate silently caps at 100 screenshots via the hard-coded WithLimitOffset(Query(nil), 100, 0) (internal/cli/annotation.go:98); annotations on any screenshot beyond the first 100 are dropped with no indication to the user (plan cautions against silent truncation). <!-- triage: duplicate of claude-ce Medium — page through all screenshots instead of hard cap; plan A6 warns against silent truncation -->
• [x] AUTO-FIX: [reliability] annotation list aggregate does one API call per screenshot (N+1) and aborts the entire command if any single per-screenshot request errors (annotation.go:104-108) — a deleted/inaccessible screenshot fails the whole listing. <!-- triage: folded into annotation list rework — make per-screenshot errors non-fatal (skip inaccessible); N+1 is inherent to the aggregate design (no bulk endpoint = out of scope) -->

Nit

• [ ] ESCALATE: [security] screenote config prints the resolved API key in plaintext to stdout (internal/cli/config.go → writeJSON(resolved); Values.APIKey serializes as api_key), a footgun for CI logs; consider masking unless an explicit opt-in flag is passed. <!-- triage: unresolved after checking plan/brainstorm — plan says config prints "resolved config as JSON" but does not decide secret masking; security posture + multiple valid fixes make this a user product decision -->
• [x] AUTO-FIX: [maintainability] fileExists in internal/cli/root.go:621 is dead code — defined and never referenced. <!-- triage: duplicate of claude-ce Nit; dead code removal -->
• [x] RESOLVED/NO-FIX: [maintainability] The --interactive persistent flag (root.go:540) is wired but never read anywhere; no interactive prompting exists, so the flag is inert. <!-- triage: plan A6 defines --interactive as the interactivity contract; v1 has no prompts (interactive flows out of scope), so an inert placeholder flag is expected, not a defect -->
• [x] AUTO-FIX: [correctness] writeRawJSON (root.go:596-608) has a dead branch: after empty raw is replaced with {}, the later len(raw) == 0 check can never be true. <!-- triage: duplicate of claude-ce Nit; dead branch simplification -->
• [x] AUTO-FIX: [consistency] Output formatting differs across commands: raw-passthrough commands (project list, page list, screenshot list/create, annotation get) emit the server's compact JSON, while the aggregated annotation list and config emit 2-space-indented JSON. <!-- triage: standardize locally-constructed JSON to match server passthrough (compact); plan says preserve server JSON where possible -->
• [x] AUTO-FIX: [correctness] resolve_page (app/controllers/api/v1/screenshots_controller.rb) treats any all-digits --page/--page-id as a page ID, so a page literally named "123" cannot be selected by name; acceptable but undocumented. <!-- triage: plan Unit 2 requires documenting the exact page-selection behavior; doc fix -->
• [x] AUTO-FIX: [performance] ContractSerializer.annotation_detail iterates annotation.annotation_comments.includes(:user, :api_key), but ProjectScope.annotations preloads :annotation_comments without :api_key, causing a small extra query per detail request. <!-- triage: confirmed; nest-preload annotation_comments: [:user, :api_key] in ProjectScope.annotations -->