Both flagged mismatches were Elmhurst input errors (same silent-stale-value
contamination class as the earlier chimneys/wall-thickness bugs), not
parser or calculator bugs:
- cylinder_size: build_100010086084.py's water_heating() selected
DropDownListCylinderSize by raw value "2", but this DOM's option values
ARE their visible litre-band text (no "2" among them) — Playwright
silently no-ops on a non-matching value, leaving a prior cert's "Medium
(131-170L)" selection in place. Fixed to match by text ("Normal"), and
added the missing mapper dict entries (Normal/Large litre-suffixed
labels) to _ELMHURST_CYLINDER_SIZE_LABEL_TO_SAP10 (a real mapper-coverage
gap — the calculator raises UnmappedElmhurstLabel rather than silently
mis-mapping).
- boiler_flue_type: the boiler-code search dialog's combined "Balanced/Open
Flue" Table 4b category doesn't drive the separate, independently
selectable RadioButtonListFlueType field, which was left at an inherited
"Balanced" from a prior cert. Fixed space_heating() to explicitly select
"Open" to match the lodged gov-API code. This field isn't consumed by
Sap10Calculator (ML/generator-only), so it had zero effect on the SAP
score.
Re-downloaded elmhurst_summary.pdf/elmhurst_worksheet.pdf after the fixes
(Recommendations page confirmed clean). Elmhurst-PDF-inputs path moved
46->47 SAP from the cylinder-volume correction; gov-API SAP (53) and
Elmhurst's own worksheet (51) are unchanged. Full accuracy suites re-run
clean (67 passed, 67 skipped, 1 xfailed, same as before).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Elmhurst rebuild for this cert had two carryover bugs from the shared
Playwright session: TextBoxOpenChimneys silently inherited "2" from a prior
cert's build (this cert's register lodges open_fireplaces_count=0, which
the gov-API mapper already reads correctly), and wall thickness inherited
a stale 280mm. Also fixed a real bug in the space_heating() PCDB-ref-clearing
logic: input_value() returns the string "0" when already cleared, which is
truthy in Python, causing a spurious navigate-away that broke the next
E.goto() call.
Rebuilt cleanly (chimneys=0, wall thickness=220mm per RdSAP convention for
unrecorded solid-wall thickness, since CheckBoxWallThicknessUnknown does not
persist via Playwright in this tool version). Elmhurst's own worksheet score
moves from a contaminated 46/48 to 51, closing the gap against this engine's
53-on-gov-API-inputs from ~5-7pt to ~2pt. No mapper.py change was needed --
the gov-API mapper's open_chimneys_count=0 was correct all along.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Built UPRN 100010086084 (the largest gov-API-vs-lodged outlier from the
20-cert batch) in Elmhurst's accredited RdSAP10 tool to get real ground
truth, per feedback that lodged rating isn't a valid comparison target.
Elmhurst worksheet: SAP 46-48 (orientation-dependent), vs this engine's
53 on gov-API inputs -- a real ~5-7pt residual gap. Ruled out with
direct empirical tests: total_floor_area, missing party_wall_length,
region/climate (SAP rating always uses UK-average weather per Appendix
U -- confirmed inert), boiler/secondary efficiency (exact match, 66%/
63% both sides). Confirmed real but NOT blind-fixed (need more than one
cert's evidence, and a wall-U-value patch tested the wrong direction in
isolation): a wall-thickness-unknown U-value fallback mismatch (this
engine 1.70, Elmhurst's own "unknown" resolution 1.40) and a window
U-value divergence (1.85 vs 2.52 W/m2K for the same nominal glazing
pick). Single-window orientation confirmed a ~2pt partial contributor.
Also fixes a small, separate, evidenced gap the build surfaced: the
Elmhurst summary parser didn't recognise the live tool's "Medium
(131-170L)" cylinder-size label (only the bare "Medium").
Full write-up is in the RealCertExpectation comment for this cert in
test_real_cert_sap_accuracy.py and the worklist. Residual gap is
intentionally left open for follow-up, not force-closed.
Sampled all 20 unique property_ids that failed with "Unsupported EPC
schema: 'SAP-Schema-15.0'" across the sub_task history (114 total across
72 subtask rows), captured 20 real live certs, and pinned each as a
RealCertExpectation regression in the accuracy corpus.
16/20 land exact or within 1-3 SAP of the lodged rating, confirming the
door_count/WWHRS/FGHRS/draught-proofing defaults generalise beyond the
original two properties. The other 4 (all solid-brick, uninsulated,
age band C) diverge +6 to +12 — traced to a legitimate SAP-2009-vintage-
lodgement vs RdSAP10-recalculation methodology gap (u_wall() checked
directly against RdSAP10's own Table 6, returns the correct 1.70 — no
mapper defect), not a mapper bug, so pinned to the engine's observed
value rather than tuned to lodged.
Sync Planning Authority, Designated Area, Article 4 PD Rights, and
Listed Building from HubSpot into hubspot_deal_data so downstream
consumers of the existing HubSpot ETL process can read them.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Built the property-753950 cert (worklist P4, PR #1503) in the live RdSAP-10
Online tool via Playwright: SAP 44 (E), vs our engine's 42 and the lodged
2014 cert's 45 — a normal RdSAP-2012->SAP-10.2 recalculation spread, not a
defect surfaced by the multiple_glazed_proportion fix.
Also surfaced (and documented as a caveat, not fixed here) a pre-existing
SAP-16.2 gap: the cert lodges party_wall_length_m=0 on both floors despite
being semi-detached, which Elmhurst's validation rejects and this PR's
engine change doesn't touch.
uploaded_files has no property_id — matching is on landlord_property_id — so the
property_ids path was pointless indirection (look property.id up in property just
to translate back to landlord_property_id). The FE holds landlord_property_id per
row anyway.
task.inputs hand-pick key is now landlord_property_ids: str[] (was property_ids:
int[]). resolve_selection unions two landlord_property_id sources — project_codes
expanded via hubspot_deal_data, and the hand-picked ids taken as given — and no
longer queries the property table at all. Trigger-only change; the domain plan,
orchestrator and matching already work in landlord_property_id.
ADR-0060, CONTEXT.md and the request schema updated to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Property 753950 (uprn 100021969385, cert 0141-2860-6891-9124-5625,
SAP-Schema-16.2) hard-failed modelling_e2e: RdSapSchema17_1 requires
multiple_glazed_proportion as a non-optional int, and this 16.x cert omits
the field entirely, lodging only the multiple_glazing_type="ND" sentinel.
The cert's own window.description ("Fully double glazed") states the
glazing extent unambiguously, so _normalize_sap_schema_16_x now derives
0/100 from "single"/"double" wording when the field is absent, mirroring
the existing single-glazed multiple_glazing_type cascade. This is
worklist P4 (.claude/skills/expand-sap-accuracy-corpus/worklist.md) —
a flat default was tried previously and reverted because making such
certs mappable at all pulls them into the EPC-prediction donor pool and
tips near-tie similarity matches; deriving from explicit text (rather
than a blind default) was the suggested unblock.
Re-measured the component-accuracy gate as the worklist asked: it drops
(has_hot_water_cylinder 0.8687->0.8586, cylinder_insulation_type
0.3333->0.1667, door_count residual 0.3131->0.3333) via the same
donor-pool-composition mechanism as the prior #1245/ADR-0037
re-baselines, not a prediction-logic loosening. Re-baselined the floors
with that rationale recorded inline.
The 'all properties' selection resolved against property.portfolio_id, but a
portfolio spans multiple HubSpot projects — so 'all' pulled the whole portfolio,
not the project the user was looking at. The project↔property grain lives on
hubspot_deal_data, not property.
task.inputs is now {project_codes?: str[], property_ids?: int[], portfolio_id?}:
the route resolves the distinct landlord_property_id set as the union of every
property in the named project_codes (from hubspot_deal_data) and the hand-picked
property_ids; portfolio_id is optional and only names the package. Drops the
portfolio-scoped select_all. Cap now applies to the resolved set size.
ADR-0060, CONTEXT.md and the request schema updated to match. New router tests
cover project-code resolution, the union+dedup, null landlord_property_id drop,
and the empty-project rejection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses PR #1498 review:
- A per-file read failure (missing/deleted object, or a bucket the role can't
reach) is now a SkippedDocument(reason=unreadable), not a whole-run abort;
an all-unreadable selection still fails (no empty package). Best-effort on
the read path (ADR-0060).
- Each ZIP member is streamed to a temp file (S3DocumentDownloader.download,
boto download_file) and added from disk, so a single multi-GB member never
hits the heap — the 'never held whole in memory' claim is now true.
- The 409 double-submit guard is DB-arbitrated: the sub_task insert is
conditional on the task having none, so a race creates only one.
- Local env gets a placeholder recipient email so the route is exercisable.
- PackageEntry carries landlord_property_id so a read failure can be reported.
- TODO noting the UploadedFile.s3_upload_timestamp typing fix.
Two new tests: per-file read failure skips-and-reports; address fallback flows
to the folder name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per PR review: the FE writes the selection config
({portfolio_id, property_ids?, select_all?}) into the FE-owned task.inputs and
passes only task_id, so a large hand-picked selection never travels in an HTTP
body. The route reads task.inputs, resolves to landlord_property_ids, caps, and
pins the recipe onto sub_task.inputs as before. Declares the FE-owned inputs
column on the TaskRow mirror so the backend can read it and the test schema
builds it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves the authenticated requester's email (ADR-0059), resolves + caps the
property selection, pins the recipe (landlord_property_ids, recipient_email,
package_name) onto one pre-created sub_task (raw SQL, dodging the mirror
double-registration), and enqueues one message to the worker. Refuses
double-submits (409) and oversized selections (400).
Co-Authored-By: Claude Fable 5 <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>
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>
Review feedback (#1481): the address batcher and the Modelling Run batcher
implemented the same greedy packing; the core moves to
utilities/grouped_batching.py and both become thin wrappers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ADR-0056 amended: override and EPC own type/form facts; a property with
neither is Unknown whatever the legacy columns say. Mirror columns removed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DB access via parameterised SQL, not the infrastructure SQLModel mirrors:
importing those alongside the legacy backend.app.db.models mirrors of the
same tables double-registers them in the shared metadata and crashes the
app at import. Contained to the modelling package until the DDD cut-over.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
