Place repo profile write methods + API handler extraction

[koinsaari]

Summary

• Adds UpsertProfile to internal/place — create-or-replace keyed on place_id, always overwrites. This is the API write path.
• Adds UpsertProfileIngestion — same but no-ops when user_verified=true, returns (written bool) for telemetry. This is the ingestion write path.
• Refactors PatchPlaceAccessibility in cmd/api to delegate its write to UpsertProfile. Engine orchestration, 404 check, and audit log entry stay in the handler. Behaviour unchanged — existing API integration suite passes unchanged.

Test plan

• go test ./... — all unit tests pass
• go test -tags integration ./internal/place/... -run "TestRepository_UpsertProfile|TestRepository_UpsertProfileIngestion" — 6 integration tests pass
• go test -tags integration ./cmd/api/... — 65 existing API integration tests pass unchanged
• go vet ./... — clean

Part of #10.

🤖 Generated with Claude Code

[claude]

Claude finished @koinsaari's task in 3m 53s —— View job

---

PR Review

• Gather context and read instructions
• Read changed files
• Analyze code for correctness, security, and design issues
• Post review findings
• Set commit status (skipped — integration token lacks repo:status permission)

---

The refactor is clean and the test coverage is thorough. One behavioral regression in the audit log, one minor race worth noting.

---

🔴 Important — Audit action is always "create" after this change

cmd/api/main.go:323

wasCreate := input.ID == ""          // always true — clients never send profile id
if err := s.places.UpsertProfile(ctx, id, &input); err != nil {
    return nil, err
}
auditAction := "update"
if wasCreate {
    auditAction = "create"           // so this always fires
}

input is the deserialized request body. The id field on AccessibilityProfile is DB-generated (gen_random_uuid()) and tagged json:"id,omitzero" — clients never send it. So input.ID is "" on every request, wasCreate is always true, and every write to write_logs records action "create" regardless of whether the row already existed. This is a silent behavioral regression from the original code, which tracked create vs. update correctly inside the transaction.

Minimal fix — add a created return value to UpsertProfile:

// repository.go
func (r *Repository) UpsertProfile(ctx context.Context, placeID string, profile *models.AccessibilityProfile) (created bool, err error) {
    err = r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
        // ...
        if errors.Is(loadErr, gorm.ErrRecordNotFound) {
            // ...
            created = true
            return tx.Create(profile).Error
        }
        // update path — created stays false
        // ...
    })
    return created, err
}

Then in the handler:

created, err := s.places.UpsertProfile(ctx, id, &input)
if err != nil {
    return nil, err
}
auditAction := "update"
if created {
    auditAction = "create"
}

Fix this →

---

🟡 Nit — Place check and profile write are no longer atomic

cmd/api/main.go:316–325

The original code checked place existence inside the same transaction as the profile write. Now the 404 check is a standalone query and UpsertProfile opens its own transaction:

s.db.WithContext(ctx).First(&models.Place{}, "id = ?", id)   // tx 1
s.places.UpsertProfile(ctx, id, &input)                       // tx 2

If a place is deleted between these two calls, UpsertProfile's tx.Create(profile) will hit the FK constraint and return a DB error, which the handler propagates as a 500 rather than a 404. Narrow race in practice, but worth tracking.

---

What looks good

• All GORM queries use parameterised placeholders — no string-concatenated user input anywhere in the new code.
• UpsertProfileIngestion correctly guards user_verified rows, and the three ingestion tests (InsertWhenAbsent, OverwritesNonVerified, SkipsUserVerified) cover all branches.
• The UpsertProfile integration tests cover create, update, and the "always overwrites user_verified" case.
• Validation → a11y engine → persist ordering is preserved in the handler (WithAuditFlags / DetectConflicts both happen before UpsertProfile).
• No accessibility scoring or decision logic introduced — pure data layer preserved.
• Removing the section-header comments is a clean-up win; the code reads fine without them.

[claude]

Claude finished @koinsaari's task in 3m 42s —— View job

---

PR Review

• Gather context and read instructions
• Read REVIEW.md and CLAUDE.md
• Read changed files and diff
• Analyze for correctness, security, and design issues
• Post review findings
• Set commit status (skipped — integration token lacks repo:status permission)

---

The previous 🔴 fix (returning created bool from UpsertProfile) is architecturally correct and the handler now uses it properly. One new finding: the tests for UpsertProfile don't assert on the created return value — the very field that drives the audit action.

---

🔴 Important — created return value is never asserted in the UpsertProfile tests

internal/place/repository_integration_test.go:246, 274, 279

Both tests that exercise UpsertProfile discard the created return value with _:

// TestRepository_UpsertProfile_CreatesWhenAbsent
if _, err := repo.UpsertProfile(ctx, placeID, profile); err != nil { // created not checked

// TestRepository_UpsertProfile_UpdatesWhenPresent
if _, err := repo.UpsertProfile(ctx, placeID, first); err != nil {   // created not checked
if _, err := repo.UpsertProfile(ctx, placeID, second); err != nil {  // created not checked

created is the primary behavioral output of UpsertProfile — it's what the handler at cmd/api/main.go:329 uses to choose between "create" and "update" for the audit log. If the logic regresses (e.g. created always returns false), both tests would still pass. Per REVIEW.md, meaningful branches of new exported repository functions must be covered.

Minimal fix — add assertions in the existing tests:

// In TestRepository_UpsertProfile_CreatesWhenAbsent
created, err := repo.UpsertProfile(ctx, placeID, profile)
if err != nil { t.Fatalf(...) }
if !created { t.Errorf("created = false, want true on first insert") }

// In TestRepository_UpsertProfile_UpdatesWhenPresent
created, err := repo.UpsertProfile(ctx, placeID, first)
if err != nil { t.Fatalf(...) }
if !created { t.Errorf("created = false, want true on first call") }

_, err = repo.UpsertProfile(ctx, placeID, second)  // second call: created must be false
// optionally: if created { t.Errorf("created = true, want false on update") }

Fix this →

---

🟡 Nit — Place check and profile write are still non-atomic

cmd/api/main.go:316–323

Previously noted; still present after this push. The existence check and the profile upsert run in separate transactions. A deleted-between-calls place causes a FK constraint error from UpsertProfile, which propagates as a 500 rather than a 404. Narrow race in practice.

---

What looks good

• The previous 🔴 regression (audit action always "create") is now correctly fixed: UpsertProfile returns (created bool, err error) and the handler uses it at cmd/api/main.go:329–331.
• All GORM queries use parameterised placeholders — no string-concatenated user input in the new code.
• UpsertProfileIngestion correctly guards user_verified rows, and the three ingestion tests cover all branches including the skip case.
• WithAuditFlags → DetectConflicts → UpsertProfile ordering is preserved in PatchPlaceAccessibility (cmd/api/main.go:305–323).
• user_verified is now included in the UpsertProfile updates map — needed for the API path to overwrite human-verified records.
• Pure data layer: no accessibility scoring or decision logic introduced.
• 65 existing API integration tests pass unchanged.

[claude]

Claude finished @koinsaari's task in 2m 21s —— View job

---

PR Review

• Gather context and read instructions
• Read REVIEW.md and CLAUDE.md
• Read changed files and diff
• Analyze for correctness, security, and design issues
• Post review findings
• Set commit status (skipped — integration token lacks repo:status permission)

---

Both 🔴 findings from previous reviews are resolved. No new blocking issues.

---

No blocking issues — previous fixes are correct

Previous 🔴 #1 — Audit action always "create": Fixed. UpsertProfile now returns (created bool, err error) and the handler at cmd/api/main.go:328–331 uses it to choose "create" vs "update". ✅

Previous 🔴 #2 — created return value not asserted in tests: Fixed. TestRepository_UpsertProfile_CreatesWhenAbsent asserts created == true on insert (line 250–252), and TestRepository_UpsertProfile_UpdatesWhenPresent asserts created == true on the first call and created == false on the second (lines 282–284, 291–293). Both meaningful branches are now covered. ✅

---

🟡 Nit — Place check and profile write are still non-atomic

cmd/api/main.go:316–323 — previously noted, still present. The existence check and the profile upsert run in separate transactions. A deleted-between-calls place causes a FK constraint violation from tx.Create(profile), which propagates as 500 rather than 404. Narrow race in practice.

---

What looks good

• AuditFlags (stored per-component inside Components JSONB) are included in the UpsertProfile updates map via "components": profile.Components — computed flags are persisted on update too. ✅
• WithAuditFlags → DetectConflicts → UpsertProfile ordering is preserved in PatchPlaceAccessibility. ✅
• All GORM queries use parameterised placeholders. ✅
• UpsertProfileIngestion correctly guards user_verified rows; 3 ingestion tests cover all branches. ✅
• user_verified is included in the UpsertProfile updates map — API write path correctly overwrites human-verified records. ✅
• 65 existing API integration tests pass unchanged per the PR test plan. ✅

[claude]

Claude finished @koinsaari's task in 2m 28s —— View job

---

PR Review

• Read REVIEW.md and CLAUDE.md
• Read changed files and diff
• Analyze for correctness, security, and design issues
• Post review findings
• Set commit status (skipped — integration token lacks repo:status permission)

---

No blocking issues — all three findings from previous review rounds are resolved.

---

Previous findings: all resolved ✅

🔴 #1 — Audit action always "create": Fixed. UpsertProfile returns (created bool, err error) and the handler at cmd/api/main.go:324–327 uses it to choose "create" vs "update". ✅

🔴 #2 — created return value not asserted in tests: Fixed. TestRepository_UpsertProfile_CreatesWhenAbsent asserts created == true on insert; TestRepository_UpsertProfile_UpdatesWhenPresent asserts created == true on the first call and created == false on the second. ✅

🟡 Nit — Place check and profile write non-atomic: Fixed in this push. Both the place-existence check (tx.First(&models.Place{})) and the profile write now run inside the same db.Transaction at internal/place/repository.go:122–154. The ErrPlaceNotFound sentinel propagates cleanly through GORM's transaction and is caught at cmd/api/main.go:318, returning a 404 as expected. ✅

---

What looks good

• Validation (WithAuditFlags → DetectConflicts) runs before UpsertProfile — ordering invariant preserved. ✅
• PatchPlaceAccessibility422 is returned for conflict errors; ErrPlaceNotFound correctly maps to 404; other DB errors propagate as 500. ✅
• All GORM queries use parameterised placeholders — no string-concatenated user input. ✅
• clause.Returning{} on the update path ensures existing is populated with the post-update DB state; *profile = existing copies it back so both audit.Log and the 200 response use the persisted record. ✅
• UpsertProfileIngestion correctly forces user_verified = false on insert and skips the write when the existing row has user_verified = true. All three branches are covered by tests. ✅
• New ErrPlaceNotFound sentinel is exported and tested in TestRepository_UpsertProfile_PlaceNotFound. ✅
• 65 existing API integration tests pass unchanged per the PR test plan. ✅
• Pure data layer: no accessibility scoring or decision logic introduced. ✅