Batcher profile wiring + ingestion conflict resolution

[koinsaari]

Summary

• Widens sources.Sink to carry (Place, *AccessibilityProfile) — profile travels as a sibling, never embedded on the place struct, avoiding any ORM cascade confusion
• Batcher buffers (place, profile) pairs, writes places first via UpsertBatch, then writes profiles using the UUIDs returned by flush
• runCanonical wires in UpsertProfileIngestion and resolveIngestionConflicts — hard conflicts from OSM tags demote the component to limited rather than rejecting (API path rejects; ingestion corrects)
• Integration tests cover: profile written when tags present, component downgraded on hard conflict, user_verified row survives ingest unchanged
• READMEs and CLAUDE.md updated for the widened Sink, new repo methods, and v1 tag mapping table

Test plan

• go test ./... — 293 unit tests pass
• go test -tags integration ./... — 389 tests pass including 2 new canonical pipeline tests
• CI green

Part of #10. Implements the ingestion-side conflict policy from #62 (component-level downgrade; profile-level overall_status intentionally untouched).

🤖 Generated with Claude Code

[claude]

Claude finished @koinsaari's task in 5m 16s —— View job

---

PR Review — Batcher profile wiring + ingestion conflict resolution

• Read REVIEW.md and CLAUDE.md
• Analyze PR diff and changed files
• Review core logic (batcher, main.go, conflict resolution)
• Check integration tests
• Post findings

---

No blocking issues. The architectural decisions are sound: profile as a Sink sibling (not embedded on Place) prevents GORM cascade confusion; the ingestion conflict resolution correctly computes audit flags then detects conflicts then demotes before writing; UpsertProfileIngestion correctly skips user_verified=true rows. All SQL uses parameterised placeholders. No accessibility scoring logic was introduced.

🟡 Nit 1 — resolveIngestionConflicts is unreachable with current OSM tag mapping

cmd/ingestion/main.go:231-261

mapEntrance always emits OverallStatus: models.StatusUnknown, so the conflict check (comp.OverallStatus != models.StatusAccessible → skip) can never fire for entrance components. mapRestroom and mapParking can only produce OverallStatus=accessible when their backing properties are not conflicting (WheelchairAccessible=true, HasDisabledSpaces=true), so the hard-conflict flags for those components can't appear either. The downgrade logic is forward-compatible code, which is fine — but TestRunCanonical_WritesAccessibilityProfiles uses a hand-crafted fakeCanonicalSource to exercise it, not a real OSM tag path. Worth a comment on the function explaining the current invariant and when this will matter. Fix this →

🟡 Nit 2 — Profile-level OverallStatus not reconciled when a component is downgraded

cmd/ingestion/main.go:245-252

c.OverallStatus = models.StatusLimited

resolveIngestionConflicts demotes the component but leaves profile.OverallStatus untouched. If a future canonical source emits OverallStatus=accessible (e.g. wheelchair=yes) plus a conflicting entrance component, the stored row will have accessible at the profile level but limited at the component level. Today this can't happen via OSM (see Nit 1), but it's worth tracking. Fix this →

🟡 Nit 3 — UpsertProfileIngestion sets written=true without checking rows affected

internal/place/repository.go:184-195

if err := tx.Model(&existing).Updates(updates).Error; err != nil {
    return err
}
written = true  // set before verifying RowsAffected

If the row is deleted by a concurrent transaction between the First and the Updates (PostgreSQL read-committed), the UPDATE touches 0 rows but written is still set. Stats will be slightly inflated in this race. UpsertProfile (API path) has the same pattern but uses Returning which at least guarantees the model reflects reality. Fix this →

🟡 Nit 4 — batcher doesn't express the dependency between writeProfile and downgradeProfile

cmd/ingestion/batcher.go:15-21

The nil checks are independent:

if profile == nil || b.writeProfile == nil {
    continue
}
if b.downgradeProfile != nil {
    b.profilesDowngraded += b.downgradeProfile(profile)
}
ok, err := b.writeProfile(ctx, p.ID, profile)

A caller that sets writeProfile but leaves downgradeProfile nil silently writes unresolved conflicts. Production wiring in runCanonical always pairs them, but the struct doesn't communicate this requirement. A doc comment on the struct field (or making downgradeProfile required when writeProfile is non-nil) would clarify the invariant. Fix this →

---

Invariant checks:

• ✅ All SQL uses parameterised placeholders (no string concatenation of external input)
• ✅ AuditFlags computed deterministically by WithAuditFlags before each write — same as API path
• ✅ Conflict resolution order correct: WithAuditFlags → DetectConflicts → demote → persist
• ✅ No HTTP framework introduced
• ✅ No accessibility scoring or decision logic added
• ✅ New functions covered by unit and integration tests for happy path, conflict path, and user_verified preservation

[claude]

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

---

• Read REVIEW.md and CLAUDE.md
• Analyze PR diff and changed files
• Review core logic (batcher, main.go, conflict resolution, repository)
• Check integration tests
• Post findings and set commit status

---

PR Review — re-review after push

No blocking issues. The push addresses Nit 3 from the previous review: UpsertProfileIngestion now captures the GORM result and gates written = result.RowsAffected > 0 correctly (internal/place/repository.go:191-195). Three nits from the prior review remain open.

Invariant checks (all pass):

• ✅ Conflict resolution order correct: WithAuditFlags → DetectConflicts → demote → persist in resolveIngestionConflicts
• ✅ AuditFlags computed by internal/a11y.Engine, never from client input
• ✅ All SQL/GORM uses parameterised placeholders
• ✅ No accessibility scoring or aggregated judgment added
• ✅ No HTTP framework introduced
• ✅ All new code paths covered by unit and integration tests

🟡 Nit 1 — resolveIngestionConflicts is unreachable via the current OSM tag mapper

cmd/ingestion/main.go:232-257

mapEntrance always emits OverallStatus: StatusUnknown, so DetectConflicts skips it (it only checks StatusAccessible components). mapRestroom and mapParking emit StatusAccessible only when their properties are non-conflicting (WheelchairAccessible=true, HasDisabledSpaces=true), so their hard-conflict flags can't appear alongside StatusAccessible. The function is correct forward-compatible code, but TestRunCanonical_WritesAccessibilityProfiles exercises it via a hand-crafted fake source, which can create false confidence that the real OSM path triggers it. A comment on the function would make the invariant explicit. Fix this →

🟡 Nit 2 — Profile-level OverallStatus not reconciled when a component is downgraded

cmd/ingestion/main.go:245-252

resolveIngestionConflicts demotes the conflicting component to StatusLimited but leaves profile.OverallStatus untouched. The integration test fixture for OSMID 1003 enters with OverallStatus: StatusAccessible at the profile level and a conflicting entrance component; after the downgrade the stored row has overall_status=accessible at the profile level but overall_status=limited at the component level. The test asserts on the component status but not the profile-level field, so this inconsistency passes undetected. Fix this →

🟡 Nit 4 — batcher doesn't enforce the pairing of writeProfile and downgradeProfile

cmd/ingestion/batcher.go:53-58

A caller that sets writeProfile but leaves downgradeProfile nil silently writes profiles without resolving conflicts. Production wiring in runCanonical always pairs them, but the struct doesn't communicate the requirement. A doc comment on writeProfile, or a guard in flushNow that returns an error when writeProfile is set but downgradeProfile is nil, would make the invariant explicit. Fix this →

---

Note: setting a commit status via gh api repos/.../statuses returned HTTP 403 (integration permission); the status could not be posted automatically.

[claude]

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

---

• Read REVIEW.md and CLAUDE.md
• Analyze latest commit (752fac4)
• Review remaining open nits (Nit 1, Nit 2) and new changes
• Post findings
• Set commit status (HTTP 403 — integration permission not available)

---

PR Review — re-review after 752fac4

No blocking issues. Nit 3 (RowsAffected, fixed in a86cf06) and Nit 4 (batcher pairing guard, fixed in 752fac4) are both resolved. The guard at batcher.go:44-46 is correct — flushNow now returns an error when writeProfile is set without downgradeProfile.

Invariant checks (all pass):

• ✅ Conflict resolution order correct: WithAuditFlags → DetectConflicts → demote → persist
• ✅ AuditFlags computed by internal/a11y.Engine, never from client input
• ✅ All SQL/GORM uses parameterised placeholders
• ✅ No accessibility scoring or aggregated judgment added
• ✅ No HTTP framework introduced
• ✅ UpsertProfileIngestion correctly skips user_verified=true rows and checks RowsAffected

Two nits from prior reviews remain open, plus one new nit from this push.

🟡 Nit 1 — resolveIngestionConflicts is unreachable via the current OSM tag mapper

cmd/ingestion/main.go:232-257

mapEntrance always emits OverallStatus: StatusUnknown, so DetectConflicts skips it. mapRestroom and mapParking emit StatusAccessible only when their backing properties are non-conflicting, so the hard-conflict flags for those components can't appear alongside StatusAccessible either. The function is correct forward-compatible code, but the integration test exercises it via a hand-crafted fake source, which may give false confidence that the real OSM path triggers it. A comment on the function explaining the current invariant would make this explicit. Fix this →

🟡 Nit 2 — Profile-level OverallStatus not reconciled when a component is downgraded

cmd/ingestion/main.go:250-252

resolveIngestionConflicts demotes conflicting components to StatusLimited but never updates profile.OverallStatus. The integration test fixture for OSMID 1003 has OverallStatus: StatusAccessible at the profile level — after the downgrade the stored row will have overall_status=accessible at the profile level but overall_status=limited on the component. The test at main_integration_test.go:248-250 asserts on the component status only; the inconsistency passes undetected. Fix this →

🟡 Nit 5 — New flushNow guard has no unit test

cmd/ingestion/batcher.go:44-46

if b.writeProfile != nil && b.downgradeProfile == nil {
    return fmt.Errorf("batcher: writeProfile set without downgradeProfile")
}

The guard is new and meaningful — it's the enforcement of the previously-implicit invariant from Nit 4. batcher_test.go has eight tests covering flush behaviour but none that triggers this error path. A short test (sink one place with writeProfile set and downgradeProfile nil, call flushNow, expect the error) would lock this in. Fix this →

---

Note: commit status could not be posted (HTTP 403 — integration permission).

[claude]

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

---

• Read REVIEW.md and CLAUDE.md
• Analyze latest commit and open nits
• Review changed files
• Post findings and set commit status

---

PR Review — re-review after d46d2a8

No blocking issues. Nit 5 (unit test for flushNow guard, raised in the prior review) is now resolved: TestBatcher_FlushNow_ErrorWhenWriteProfileWithoutDowngrade in batcher_test.go:248-270 correctly sinks a place with a non-nil profile, calls flushNow, and asserts an error is returned. Two nits from earlier reviews remain open.

Invariant checks (all pass):

• ✅ Conflict resolution order correct: WithAuditFlags → DetectConflicts → demote → persist in resolveIngestionConflicts
• ✅ AuditFlags computed by internal/a11y.Engine, never from client input
• ✅ All SQL/GORM uses parameterised placeholders
• ✅ No accessibility scoring or aggregated judgment added
• ✅ No HTTP framework introduced
• ✅ UpsertProfileIngestion correctly skips user_verified=true rows and gates written on RowsAffected > 0
• ✅ flushNow guard (writeProfile set without downgradeProfile) covered by new unit test

🟡 Nit 1 — resolveIngestionConflicts is unreachable via the current OSM tag mapper

cmd/ingestion/main.go:232-257

mapEntrance always emits OverallStatus: StatusUnknown, so DetectConflicts skips it (it only checks StatusAccessible components). mapRestroom and mapParking emit StatusAccessible only when their backing properties are non-conflicting (WheelchairAccessible=true, HasDisabledSpaces=true), so the hard-conflict flags for those components can't appear alongside StatusAccessible either. The function is correct forward-compatible code, but the integration test exercises it via a hand-crafted fake source, which may give false confidence that the real OSM path triggers it. A comment on the function explaining the current invariant would make this explicit. Fix this →

🟡 Nit 2 — Profile-level OverallStatus not reconciled when a component is downgraded

cmd/ingestion/main.go:250-252

resolveIngestionConflicts demotes conflicting components to StatusLimited but never updates profile.OverallStatus. The integration test fixture for OSMID 1003 has OverallStatus: StatusAccessible at the profile level — after the downgrade the stored row will have overall_status=accessible at the profile level but overall_status=limited on the component. The test at main_integration_test.go:248-250 asserts only on the component status; the inconsistency passes undetected. Fix this →

---

Note: commit status could not be posted (HTTP 403 — integration permission not available).

[koinsaari]

🟡 Nit 1 — resolveIngestionConflicts is unreachable via the current OSM tag mapper

cmd/ingestion/main.go:232-257

mapEntrance always emits OverallStatus: StatusUnknown, so DetectConflicts skips it (it only checks StatusAccessible components). mapRestroom and mapParking emit StatusAccessible only when their backing properties are non-conflicting (WheelchairAccessible=true, HasDisabledSpaces=true), so the hard-conflict flags for those components can't appear alongside StatusAccessible either. The function is correct forward-compatible code, but the integration test exercises it via a hand-crafted fake source, which may give false confidence that the real OSM path triggers it. A comment on the function explaining the current invariant would make this explicit. Fix this →

This just for forward compatibility for new sources. Can ignore for now

🟡 Nit 2 — Profile-level OverallStatus not reconciled when a component is downgraded

cmd/ingestion/main.go:250-252

resolveIngestionConflicts demotes conflicting components to StatusLimited but never updates profile.OverallStatus. The integration test fixture for OSMID 1003 has OverallStatus: StatusAccessible at the profile level — after the downgrade the stored row will have overall_status=accessible at the profile level but overall_status=limited on the component. The test at main_integration_test.go:248-250 asserts only on the component status; the inconsistency passes undetected. Fix this →

Profile level status is intentionally untouched. this is by design. it's supposed to only touch component level status.