Dan's review on #1546:
- Stream the workbook to a /tmp temp file and S3Client.upload_file it, instead
of render_workbook -> bytes -> put_object. render_workbook now saves straight
to a path; the orchestrator renders to a temp file, multipart-uploads it, and
always cleans it up. Restores ADR-0065's "never an in-memory BytesIO" decision
(the OOM path at the 100k-row cap).
- Move the raw `SELECT ... FROM scenario` out of the Lambda handler into
ScenarioNamesPostgresRepository, so the handler stays composition-only and all
SQL lives in repositories/.
- current_sap_points goes through _float, matching its Optional[float] read-model
type and the sibling numeric facts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "latest EPC per UPRN" selection ordered by e.id DESC, but row-id does not
track recency: a gov-EPC cert re-ingested after a PasHub survey lands with a
higher e.id, so the header/perf reads silently picked the stale gov cert (wrong
TFA, lodgement, property_type, current band) on 3 of portfolio 838. Order by the
effective lodgement date (registration -> completion -> inspection) DESC NULLS
LAST, e.id DESC tiebreak.
Compose the descriptive fabric from the structured SAP fields a re-survey lodges
(no gov-EPC prose): walls/roof/floor from the main epc_building_part, heating and
hot water from epc_main_heating_detail + the water-heating codes, decoded with
the SAP engine's own code space (fabric_description.py). Structured fabric wins
where present and falls back to the gov-EPC prose otherwise; windows and lighting
stay on prose (their per-element codes do not reconstruct the dwelling phrase).
Add built_form (epc_property, code-decoded) and property_age_band (building part
construction_age_band -> RdSAP date range) columns.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two fixes so a multi-scenario export over a PasHub-fetched portfolio is correct:
1. Plan selection: take the NEWEST plan per (property, scenario), not the
is_default one. is_default is one-per-property (not per-scenario), so it
cannot scope a multi-scenario export — a property's default sits under a
single scenario, leaving every other scenario with zero rows. _ROWS_SQL and
_MEASURES_SQL now DISTINCT ON (property_id) ORDER BY created_at DESC, id DESC.
2. Effective-EPC descriptive block: match the header facts + current performance
by UPRN (the latest epc_property for the source), because the PasHub re-fetch
lands records with property_id = NULL — a property_id join silently fell back
to a stale gov-EPC cert (wrong 2013-2025 lodgement dates / int property_type)
or nothing. Lodgement date coalesces registration -> completion -> inspection
(PasHub surveys carry only inspection_date). Descriptive prose elements stay
on the property_id gov-EPC fallback (a survey lodges structured fields, not
gov-EPC prose).
Result on portfolio 838 / scenarios 1303+1304: both sheets populate 205 rows;
property_type text on 204/205; lodgement dates 2026 on 200/205. Repository tests
updated (uprn fixtures, newest-plan selection + a newest-wins case); 22 export
tests pass, pyright clean.
First successful live run surfaced three issues:
1. Email looked rubbish (a giant raw presigned URL). Now sends a proper HTML
email with a 'Download documents' button plus a plain-text fallback, and a
summary (N documents across M properties, expiry). Email delivery is now
best-effort: a transport failure no longer loses an already-built package
(the link is still on sub_task.outputs), and the SMTP connect has a 30s
timeout so an unreachable SES endpoint fails fast instead of hanging to the
900s Lambda timeout.
2. Every folder was 'address unavailable (...)': the resolver read property.address,
but these are HubSpot deals with no property row. It now uses the deal's
dealname from hubspot_deal_data.
3. No logs / no idea why a run took ~9 minutes: the worker's INFO logs were
dropped (Lambda root logger defaults to WARNING). The handler now raises the
level, and the orchestrator logs per-phase timing and volume (gather+plan,
packaged N files / X MB, upload, email, total) so the slow phase is visible.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ADR-0034's nearby-postcode broadening degrades to a genuine
NoSameTypeComparablesError whenever the default 1000m/30-postcode reach has no
same-type comparable nearby (e.g. property_id=752685, portfolio 824 — the only
Maisonette within reach). Adds one configurable extra widening step
(EpcComparablePropertiesRepository.candidates_near now accepts
widen_nearby_postcodes), tried only when the normal-radius walk falls short of
`minimum` matches. modelling_e2e's handler wires this to a 3000m/60-postcode
PostcodesIoClient as the single wider step.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A live test returned 'no documents could be packaged' for 91 correctly-resolved
properties. Root cause: no upload source populates uploaded_files.landlord_property_id
(pashub/magic-plan/audit set hubspot_deal_id; ECMK sets hubspot_listing_id), so
matching on landlord_property_id found zero rows.
Match fix: the worker now joins uploaded_files -> hubspot_deal_data (on deal_id) ->
landlord_property_id, taking the property identity from the bridge. The repository
returns a small PropertyDocument read-model instead of the infra ORM row, so the
orchestrator no longer names infrastructure.postgres.* (resolves the leak dancafc
flagged) and the s3_upload_timestamp cast is gone. Coverage is limited to
deal-id-linked sources; listing_id/uprn-only files are a noted follow-up.
Observability: the empty-selection failure now carries stage counts
(selected/matched/planned/skipped) in both the message (-> the worker WARNING log)
and details (-> sub_task.outputs), and the run logs those counts. The next failure
says 'matched 0 documents' instead of failing opaquely.
ADR-0060 + CONTEXT.md updated (matching decision + considered options).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR review (dancafc):
- introduce UprnMatch NamedTuple (datatypes/address_match.py) for the
(uprn, address, lexiscore, certificate_number) return, replacing the bare
4-tuple in get_uprn_from_epc_df / get_uprn_from_historic_epc /
HistoricEpcResolver.resolve_uprn. Tuple-compatible, so unpacking is unchanged.
- rename get_uprn_with_epc_df -> get_uprn_from_epc_df (+ callers).
- type resolve_group_ambiguity via a GroupDecision NamedTuple and trim its
docstring.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 1 of confirming UPRNs before finalise. address2uprn matched each
row independently, so one UPRN could be the best match for two distinct
addresses (a coarse EPC record absorbing several real addresses, e.g.
flats in a block). Those distinct addresses were then silently merged by
the property identity insert, and collided in property_overrides.
resolve_group_ambiguity() withholds a UPRN claimed by >=2 distinct
normalised addresses within a postcode group (keeps genuine same-address
re-listings), and the handler now emits an address2uprn_status column
(matched | ambiguous_duplicate | unmatched | invalid_postcode | error).
Withheld rows drop to a null UPRN but keep their lexiscore for triage on
the (upcoming) confirmation page.
Also adds the ADR-0057 backstop dedup in property_overrides upsert_all so
the ON CONFLICT statement can never double-touch a row.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On reflection the certificate number only needs to travel through the
address2uprn pipeline (result CSV, S3 output) as an internal value —
not persisted to property. Reverts the PropertyIdentityInsert /
property_table.py / property_postgres_repository.py changes; keeps
certificate_number flowing through get_epc_data_with_postcode,
get_uprn_with_epc_df, get_uprn_from_historic_epc, and the
address2uprn_certificate_number result column.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EpcClientService.search_by_postcode already returns the matched
certificate number alongside the UPRN, but it was dropped before
persistence. Thread it through get_epc_data_with_postcode ->
get_uprn_with_epc_df / get_uprn_from_historic_epc (using the historic
dataset's lmk_key) -> the address2uprn_certificate_number result
column -> PropertyIdentityInsert -> the property table's new
certificate_number column (assessment-model PR #362).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EpcSource widens to "expired" (ADR-0054; the column is TEXT — no
migration). "predicted"/"expired" form one slot family: _slot_sources
routes every slot read and slot-clearing delete through the family, so a
re-ingestion flipping the flavour replaces the row instead of stranding
its sibling. FakeEpcRepo mirrors the family.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review ask (dancafc): resolver/repository locals now carry explicit types
(matches: list[ScoredHistoricEpc], records: list[HistoricEpc], df:
pd.DataFrame, ...) so the flow reads without chasing callee signatures.
CLAUDE.md's Type Safety section gains the rule so future sessions enforce it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The row→domain mapper now names all 93 constructor arguments explicitly
instead of splatting a lowercased dict, takes a plain Mapping (a
DataFrame.to_dict("records") row) instead of a pandas Series, and ignores
columns the domain type doesn't know. A missing/renamed CSV column fails
loudly as a KeyError at the row. Both iterrows() call sites move to
to_dict("records") — pandas-stubs types iterrows' Series unparameterized,
which strict mode rejects. pandas-stubs + boto3-stubs[s3] make the stack
check clean: pyright strict is now 0 errors across the PR's files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire flag_fuel_mismatch into the two override-resolution paths (the property
repository and the modelling-e2e handler), keeping overlays_from a pure mapping.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compare fuel families (not exact codes), so a solid-fuel room heater refined to
smokeless/dual/biomass is consistent; only a different family (gas/electric on a
solid heater) is logged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
overlays_from now applies main_fuel after main_heating_system (stable sort), so
an explicit landlord fuel wins the natural-fuel default the heating archetype
drags, regardless of override row order. apply_simulations is last-wins.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
HistoricEpcS3Repository reached into utils/s3.py (read_csv_gz_from_s3 +
parse_s3_uri), the legacy utility that self-constructs boto3 inside free
functions. The other S3 repositories deliberately depend on the
infrastructure/s3 layer instead (UnstandardisedAddressListCsvS3Repository
injects a CsvS3Client). Bring historic EPC into line.
- Add GzipCsvS3Client(S3Client) in infrastructure/s3: read_csv_gz(key) ->
DataFrame (get_object + gzip decode).
- Inject it into HistoricEpcS3Repository; the bucket lives in the client and
the repo only builds the per-postcode key + maps rows (no S3/HTTP code).
Add with_default_s3_client(s3_root) for composition roots.
- Update main.py and the match_addresses_for_postcode seam to the factory.
- Repo tests inject a real GzipCsvS3Client over a controlled boto stub
(exact key assertions + AccessDenied); add a moto-based client test and a
factory test covering s3_root -> bucket+key.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQE5TsSuQTeNSCSz9A9GQf
The port accepts a normalised Postcode and rejects malformed/empty ones via
Postcode.is_valid() (PostcodeNotFound) — dropping the per-lookup postcodes.io
HTTP call and the cross-module use of the private _sanitise_postcode. Mapper
helper promoted to public.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
