fix: make accessibility PATCH a real merge patch instead of a full replace

[koinsaari]

Summary

• PATCH /places/{id}/accessibility previously did a blind full replace: submitting one field silently wiped every other previously-stored component. UpsertProfile now applies the request body as an RFC 7396 JSON Merge Patch on top of the current row (omitted = untouched, explicit null = cleared).
• Read-merge-write now happens inside one transaction using SELECT ... FOR UPDATE, so two concurrent PATCHes on the same place can no longer clobber each other's just-committed fields.
• Fixed two bugs found while building this: captureRawBody was wired to run before bodySizeLimiter, silently defeating the 1MB request cap; and it mistreated chunked/unknown-length bodies (ContentLength == -1) as empty, rejecting legitimate requests.
• Added a length cap on SourceReports entries (internal/validation) — the one remaining free-text field on this endpoint.

Test plan

• make build
• make lint (0 issues)
• make test (unit, all packages green, validation coverage 98.2%)
• make test-integration (all packages green, including a new goroutine-based concurrent-PATCH test and explicit-null/partial-submission tests against real Postgres via testcontainers)
• Verified internal/api/v1/server.gen.go is byte-identical to what openapi.yaml currently generates

🤖 Generated with Claude Code

[claude]

Claude finished @koinsaari's task in 6m 20s —— View job

---

Review: PR #125 — accessibility PATCH merge-patch safety

• Read REVIEW.md and CLAUDE.md
• Gather context (diff, changed files)
• Review core invariants (pure data layer, deterministic AuditFlags, validation ordering, stdlib-only HTTP)
• Review SQL/GORM parameterization and JSONB Valuer/Scanner symmetry
• Review merge-patch logic correctness (RFC 7396 semantics, concurrency)
• Review test coverage for new/changed code paths
• Post findings and set commit status (blocked — see note below)

No blocking issues. This is a solid fix — traced the merge-patch logic end to end (internal/place/repository.go:127-205) and it holds up:

• RFC 7396 semantics are correct: jsonpatch.MergePatch against {} for new profiles, explicit null clears a component, omitted fields survive the merge. Verified against the nullable: true additions in api/openapi.yaml (needed for the request validator to accept explicit nulls).
• AuditFlags are always recomputed by Engine.WithAuditFlags inside the prepare callback after the merge and before persistence, unconditionally overwriting any component's flags — a client can't smuggle flags through the merge patch (internal/a11y/engine.go:63-83).
• Validation → a11y → persist ordering is preserved: validation.AccessibilityProfile runs in the handler before UpsertProfile, which applies prepare (a11y + provenance) inside the same transaction as the write (cmd/api/main.go:350-363).
• The SELECT ... FOR UPDATE + single transaction correctly serializes concurrent updates. For the concurrent-create race (no row to lock yet), the retry-on-23505 wrapper (internal/place/repository.go:127-135) correctly falls through to a merge on the second attempt — matches the place_id UNIQUE constraint in 000002_create_accessibility_profiles.up.sql and is exercised by the new goroutine test.
• captureRawBody/bodySizeLimiter ordering fix is correct: since HandlerWithOptions applies Middlewares in list order as handler = mw(handler), the last entry becomes outermost. Putting bodySizeLimiter last now makes it run before captureRawBody's io.ReadAll, so the 1MB cap is actually enforced. Switching to plain io.ReadAll also fixes the ContentLength == -1 (chunked) mishandling.
• UpsertProfileIngestion correctly keeps full-replace semantics (ingestion writes canonical facts, not user-submitted diffs) — untouched by this diff, as it should be.
• No SQL string concatenation; all queries go through GORM parameter binding or explicit ? placeholders.

🟡 Nit

cmd/api/main.go:369 — the ErrInvalidPatch 400 response constructs apiv1.PatchPlaceAccessibility400JSONResponse{Error: err.Error()} and leaves Fields as its zero value. ValidationError.Fields (internal/api/v1/server.gen.go:289-292) has no omitempty and the schema marks fields required + non-nullable array, so this response serializes as "fields":null — inconsistent with the contract every other 400 on this endpoint honors via validationError(). Easy fix: route it through validationError() with a single field-level entry instead of constructing the response literal directly.

Fix this →

---

Note: Setting the claude-code-review commit status failed with 403 Resource not accessible by integration — the token available to this job lacks statuses:write permission. Since there were no Important findings, this would have been state=success; a maintainer may want to check the workflow's token permissions if the status check is expected to gate merges.

[claude]

Claude finished @koinsaari's task in 4m 0s —— View job

---

Review: PR #125 — accessibility PATCH merge-patch safety (re-review after push)

• Read REVIEW.md and CLAUDE.md
• Diff new commits against previous review (was the Fields:null nit addressed?)
• Re-check core invariants (pure data layer, deterministic AuditFlags, validation ordering, stdlib-only HTTP)
• Review SQL/GORM parameterization and JSONB Valuer/Scanner symmetry
• Review test coverage for new/changed code paths
• Post findings and set commit status (blocked — see note below)

No blocking issues. This is the same commit (f1d6361) as the prior review, re-verified end to end.

• The previously-flagged 🟡 nit (ErrInvalidPatch 400 response leaving Fields as null) is already fixed in this commit — cmd/api/main.go:369 now routes through validationError([]validation.FieldError{{Field: "body", Reason: err.Error()}}), and there's a dedicated regression test for it (cmd/api/integration_test.go: TestHandlePatchAccessibility_InvalidPatchReturnsPopulatedFields, plus internal/place/repository_integration_test.go: TestRepository_UpsertProfile_InvalidPatchReturnsErrInvalidPatch). No further action needed.
• RFC 7396 merge-patch semantics check out (internal/place/repository.go:127-206): jsonpatch.MergePatch against {} for new profiles, explicit null clears a component, omitted fields survive — exercised by TestRepository_UpsertProfile_PartialSubmissionPreservesOtherComponents and TestRepository_UpsertProfile_ExplicitNullClearsComponent.
• AuditFlags are unconditionally recomputed by Engine.WithAuditFlags inside the prepare callback, after the merge and before persistence (internal/a11y/engine.go:63-83) — a client can't smuggle flags through the patch on any component it touches.
• Validation → a11y → persist ordering holds: validation.AccessibilityProfile runs on the raw request body in the handler before UpsertProfile is called, and prepare (a11y + provenance) runs inside the same transaction as the write.
• Verified the middleware-ordering fix directly against the generated code, not just the source: HandlerWithOptions in server.gen.go:364-369 applies Middlewares as handler = middleware(handler) in list order, so the last entry (bodySizeLimiter) becomes outermost and now runs before captureRawBody's io.ReadAll — the 1MB cap is genuinely enforced.
• SELECT ... FOR UPDATE + single transaction correctly serializes concurrent updates on an existing row; for concurrent creates (no row to lock yet), the retry-on-23505 wrapper (internal/place/repository.go:127-135) falls through to a merge on the second attempt, matching the place_id unique constraint — exercised by the new goroutine-based TestRepository_UpsertProfile_ConcurrentPatchesBothSurvive.
• internal/api/v1/server.gen.go diff is clean — only the embedded base64 OpenAPI spec bytes changed (reflecting the new nullable: true component schemas); no handwritten struct/handler drift.
• UpsertProfileIngestion is untouched and correctly retains full-replace semantics (canonical ingestion writes facts, not user diffs).
• No SQL string concatenation; all queries use GORM parameter binding.
• New validation.AccessibilityProfile and the repository's merge/retry paths have direct unit/integration coverage for every meaningful branch (nil, valid, too-many-entries, oversized source/value; create, update, partial, explicit-null, invalid-patch, not-found, concurrent).

I wasn't able to run go build/go test in this sandbox (the tool run for go commands required interactive approval that wasn't available), so this review is based on static reading of the diff and generated code rather than a live build/test run.

Note: Setting the claude-code-review commit status failed again with 403 Resource not accessible by integration — same as the previous run. Since there are no Important findings, this would have been state=success. A maintainer should check the workflow's token permissions (needs statuses: write) if this check is expected to gate merges.