test(address2uprn): cover ambiguity withholding + override dedup; add ADR-0057

- unit tests for resolve_group_ambiguity (distinct addresses withheld,
  same-address re-listing kept, order preserved)
- Postgres integration tests for upsert_all's backstop dedup
- ADR-0057 recording the "confirm UPRNs before finalise" decision

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

docs/adr/0057-uprn-confirmation-precedes-finalise.md

@@ -0,0 +1,80 @@
+# UPRN confirmation precedes finalise; the bulk-upload review is one two-tab page and the portfolio "unmatched" tab is retired
+
+## Status
+
+proposed
+
+## Context
+
+The bulk-upload pipeline is a chain of SQS-driven Lambda stages:
+`address2uprn → landlord-overrides → combiner → (review) → finaliser`.
+`address2uprn` matches each input row independently against per-postcode EPC
+candidates, keeping the best candidate above a `lexiscore >= 0.7` floor. There
+is **no cross-row uniqueness guard**: nothing stops one UPRN being the best
+match for two *different* addresses. The textbook case is a coarse EPC record
+`42 Moreton Road` (no flat token) that `Flat 1, 42 Moreton Road` and
+`Flat 2, 42 Moreton Road` both clear — the building-number guard passes and the
+flat guard never fires, so both distinct flats collapse onto one UPRN.
+
+Downstream this is corrosive, not just noisy:
+
+- The `property` identity insert is `on_conflict_do_nothing` on the partial
+  unique index `(portfolio_id, uprn) WHERE uprn IS NOT NULL`. Two distinct
+  addresses carrying the same UPRN therefore **merge into a single property** —
+  one address silently loses its identity.
+- `_build_overrides` (the finaliser) keys `property_overrides` on
+  `(property_id, override_component, building_part)`. Both merged rows emit the
+  same tuple, so the single `INSERT ... ON CONFLICT DO UPDATE` raises
+  `CardinalityViolation` ("cannot affect row a second time").
+
+The classifier and finaliser were built assuming a UPRN is always intact and
+unique; it is not. Separately, a *post-onboarding* portfolio-level "Unmatched
+properties" tab let users fix no-UPRN properties after the fact — too late (the
+identity merge has already happened) and in a different place from the rest of
+review.
+
+## Decision
+
+1. **`address2uprn` emits a UPRN only when confident *and* unambiguous.** Within
+   a postcode group, a UPRN that is the best match for two or more rows whose
+   *normalised* input addresses differ is **withheld** (dropped to null) on
+   every such row; a UPRN shared only by rows with the *same* normalised address
+   is a genuine re-listing and is kept. `address2uprn` emits an
+   `address2uprn_status` column (`matched | ambiguous_duplicate | unmatched |
+   invalid_postcode | error`); withheld rows keep their `lexiscore` for triage.
+   A `lexiscore` review band `[0.7, X)` that also withholds shaky matches is a
+   follow-on once `X` is calibrated (see Consequences).
+2. **One pre-finalise review page, two tabs**, styled like the retired unmatched
+   tab:
+   - **Addresses** — the flagged rows (`unmatched` / `ambiguous_duplicate` /
+     low-score); the user resolves each via OS Places (reusing the existing
+     `MatchAddress` / `searchAddresses` / `useAssignUprn` flow + `postcode_search`
+     cache), producing a valid residential UPRN, or explicitly marks *no-UPRN*.
+   - **Classification** — the existing description→value verify gate, semantics
+     unchanged (every `Unknown` must be mapped; the finaliser still fails loud on
+     an unresolved value).
+   Finalise is enabled only when **both** tabs are clear.
+3. The finaliser runs only after confirmation, so every row carries a confirmed
+   UPRN **or** an explicit no-UPRN (a first-class terminal state: `property`
+   identity with `uprn NULL`, no `property_overrides`, building-passport shows
+   the "not matched yet" card).
+4. **Retire the portfolio-level "Unmatched properties" tab** — properties now
+   arrive pre-matched, so post-onboarding UPRN cleanup is unnecessary.
+5. `PropertyOverridePostgresRepository.upsert_all` keeps a **defensive
+   last-write-wins dedup** as a backstop for the single-statement invariant, but
+   it is no longer the primary fix.
+
+## Consequences
+
+- Distinct addresses can no longer silently merge into one property; the
+  `CardinalityViolation` can no longer arise from real data.
+- Onboarding reaching "complete" now implies confirmed identities, so portfolio
+  pages need no unmatched tab.
+- The ambiguous/low-score subset adds bounded user work at review time; confident
+  rows pass untouched.
+- The `lexiscore` review-band threshold `X` needs calibrating against a real
+  portfolio's score distribution before it is switched on; until then only the
+  ambiguous-duplicate rule (which needs no threshold) is active.
+- Migration is safe stage-by-stage: withheld UPRNs are caught by the *existing*
+  unmatched tab until the two-tab page ships, and the tab is removed only after
+  it does — so an ambiguous property always has somewhere to be resolved.

tests/backend/address2UPRN/test_resolve_group_ambiguity.py

@@ -0,0 +1,70 @@
+"""Unit tests for ``resolve_group_ambiguity`` (ADR-0057).
+
+Each address is matched independently, so one UPRN can be the best match for
+two *different* addresses (a coarse EPC record absorbing several real
+addresses). Withholding that UPRN is a pure function of a postcode group's
+``(uprn, normalised_address)`` pairs — no S3, EPC API, or scoring needed — so
+it is unit-tested directly here.
+"""
+
+from __future__ import annotations
+
+from backend.address2UPRN.scoring import resolve_group_ambiguity
+
+
+def test_distinct_addresses_sharing_one_uprn_are_withheld() -> None:
+    # Flat 1 and Flat 2 both matched the same coarse EPC UPRN — distinct
+    # addresses, so both are withheld rather than silently merged.
+    result = resolve_group_ambiguity(
+        [
+            ("100", "flat 1 42 moreton road"),
+            ("100", "flat 2 42 moreton road"),
+        ]
+    )
+    assert result == [(None, "ambiguous_duplicate"), (None, "ambiguous_duplicate")]
+
+
+def test_same_address_listed_twice_keeps_the_uprn() -> None:
+    # A genuine re-listing of one property (identical normalised address) is a
+    # real duplicate, not an ambiguous match — the UPRN is kept on both.
+    result = resolve_group_ambiguity(
+        [
+            ("100", "42 moreton road"),
+            ("100", "42 moreton road"),
+        ]
+    )
+    assert result == [("100", "matched"), ("100", "matched")]
+
+
+def test_distinct_uprns_are_each_matched() -> None:
+    result = resolve_group_ambiguity(
+        [
+            ("100", "42 moreton road"),
+            ("101", "44 moreton road"),
+        ]
+    )
+    assert result == [("100", "matched"), ("101", "matched")]
+
+
+def test_none_uprn_is_unmatched() -> None:
+    result = resolve_group_ambiguity([(None, "99 nowhere lane")])
+    assert result == [(None, "unmatched")]
+
+
+def test_order_is_preserved_across_mixed_group() -> None:
+    # A withheld pair, an unmatched row, and a clean match — all in one group;
+    # the output aligns positionally with the input.
+    result = resolve_group_ambiguity(
+        [
+            ("100", "flat 1 42 moreton road"),  # ambiguous (with row 3)
+            (None, "no epc candidate"),  # unmatched
+            ("100", "flat 2 42 moreton road"),  # ambiguous (with row 0)
+            ("200", "sole match road"),  # matched
+        ]
+    )
+    assert result == [
+        (None, "ambiguous_duplicate"),
+        (None, "unmatched"),
+        (None, "ambiguous_duplicate"),
+        ("200", "matched"),
+    ]

tests/repositories/property/test_property_override_postgres_repository.py

@@ -0,0 +1,102 @@
+"""Integration tests for ``PropertyOverridePostgresRepository.upsert_all``.
+
+The conflict handling lives entirely in SQL (``INSERT ... ON CONFLICT
+(property_id, override_component, building_part) DO UPDATE``), so it can only be
+verified against a real Postgres -- the ``db_engine`` fixture in
+``tests/conftest.py`` spins one up per test. The batch-dedup below is exactly
+the case an in-memory fake cannot catch: only a real ``ON CONFLICT`` statement
+raises ``CardinalityViolation`` when a batch would touch one target row twice
+(the ADR-0057 backstop).
+"""
+
+from __future__ import annotations
+
+from collections.abc import Iterator
+
+import pytest
+from sqlalchemy import Engine
+from sqlmodel import Session, select
+
+from infrastructure.postgres.property_override_table import PropertyOverrideRow
+from repositories.property.property_override_postgres_repository import (
+    PropertyOverridePostgresRepository,
+)
+from repositories.property.property_override_repository import PropertyOverrideInsert
+
+
+@pytest.fixture
+def session(db_engine: Engine) -> Iterator[Session]:
+    with Session(db_engine) as s:
+        yield s
+
+
+def _insert(
+    property_id: int,
+    override_value: str,
+    description: str,
+    *,
+    building_part: int = 0,
+    component: str = "property_type",
+) -> PropertyOverrideInsert:
+    return PropertyOverrideInsert(
+        property_id=property_id,
+        portfolio_id=820,
+        building_part=building_part,
+        override_component=component,
+        override_value=override_value,
+        original_spreadsheet_description=description,
+    )
+
+
+def _all_rows(session: Session) -> list[PropertyOverrideRow]:
+    return list(session.exec(select(PropertyOverrideRow)).all())
+
+
+def test_duplicate_conflict_key_in_one_batch_collapses_to_last(
+    session: Session,
+) -> None:
+    # arrange: two batch rows sharing the conflict key
+    # (property_id, override_component, building_part) — before the backstop this
+    # raised CardinalityViolation ("cannot affect row a second time").
+    repo = PropertyOverridePostgresRepository(session)
+
+    # act
+    repo.upsert_all(
+        [
+            _insert(742961, "House", "House: Semi-Detached"),
+            _insert(742961, "House", "House: Mid-Terrace"),
+        ]
+    )
+    session.commit()
+
+    # assert: one row, last occurrence wins (matches ON CONFLICT DO UPDATE).
+    rows = _all_rows(session)
+    assert len(rows) == 1
+    assert rows[0].override_value == "House"
+    assert rows[0].original_spreadsheet_description == "House: Mid-Terrace"
+
+
+def test_distinct_conflict_keys_are_all_written(session: Session) -> None:
+    # arrange: dedup must only collapse exact conflict-key duplicates — a
+    # different property_id, building_part, or component is a distinct row.
+    repo = PropertyOverridePostgresRepository(session)
+
+    # act
+    repo.upsert_all(
+        [
+            _insert(742961, "House", "House: Semi-Detached"),
+            _insert(742962, "House", "House: Mid-Terrace"),  # different property
+            _insert(742961, "House", "ext", building_part=1),  # different part
+            _insert(742961, "SolidBrick", "solid", component="wall_type"),  # component
+        ]
+    )
+    session.commit()
+
+    # assert
+    assert len(_all_rows(session)) == 4
+
+
+def test_empty_batch_is_a_noop(session: Session) -> None:
+    repo = PropertyOverridePostgresRepository(session)
+    assert repo.upsert_all([]) == 0
+    assert _all_rows(session) == []