Properties 741843/741872 (uprns 100060714155/100021944594) failed
modelling_e2e subtasks 6ac7841a-9338-4efe-97e5-7e0d19b3055d and
c5450f03-5b0d-4068-a3b3-d1470bc0af57 with a bare StopIteration: their
SAP-Schema-15.0 (2011-era LIG-lodged) certs identify the main dwelling's
building part as "Main building" rather than "Main Dwelling", so
from_api_string fell to OTHER and left the property with no MAIN part —
crashing wall_recommendation.py's unguarded next(...).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review findings on PR #1527:
- The overlay constants, ScoredOption builder, ventilation dependency and
selected_types helper come from the shared _optimiser_fixtures module
(landed on the fabric-first base) instead of local copies; the boiler
overlay is the shared BOILER_OVERLAY (SAP Table 4a code 104, a mains-gas
combi) rather than code 201, which is neither a boiler nor a heat pump.
_IWI_OVERLAY (solid-wall internal, type 3) stays local — no shared
equivalent — and the carbon stubs stay bespoke (the shared StubScorer has
no CO2 knob).
- The optimise_package_fabric_first import is lifted to module scope.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An unrecognised schema_type used to raise ValueError and abort the whole
call; now from_api_response logs a warning and returns None so one
unmapped cert doesn't break a batch. Schema 15.0 already has a mapper
(PR #1531) so it's unaffected; only genuinely unmapped versions skip.
OpenHousing logs a job but silently drops the appointment when no bookable
resource is sent, so the survey date never lands. log_job already sends the
configured default_resource; amend_job omitted the parameter unless the
request carried an explicit surveyor. Default amend's resource to the
configured surveyor too, and fail loudly at config load on a blank
ABRI_RELAY_DEFAULT_RESOURCE so the misconfiguration can't ship an empty
resource again.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review findings on PR #1526:
- tests/domain/modelling/_optimiser_fixtures.py is the one home for the
overlay constants, the ScoredOption builder, the additive per-kind
StubScorer and the forced ventilation dependency; test_optimiser.py and
test_optimiser_fabric_first.py had byte-identical copies of each
(and _StubScorer / _VentStubScorer fold into one parameterised stub).
- Fixture worlds are domain-plausible per team convention: the fabric-vs-
heating contrast is a £12,000 EWI against a £3,200 gas boiler rather
than a £500 heat pump undercutting a £1,000 cavity wall; heating
overlays carry real identities (SAP Table 4a code 104 for the boiler,
a PCDF index for the heat pump) instead of code 201 doubling as both;
whole-dwelling double glazing is £3,500, not £500.
- Dead knobs removed: the unused _ROOF_OVERLAY, the always-zero roof
gain, the duplicate _BOILER_OVERLAY, and the nested conditional
expressions in the interaction stubs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-attempted entering this SAP-15.0 cert's 4 real N/E/S/W windows (gov-API
sap_windows, 2.516 m^2 each) as separate Elmhurst Openings rows instead of
the accepted single combined South row, to test whether it closes the
residual ~2pt gap (engine 53 vs Elmhurst worksheet 51).
Reproduced live the documented elmhurst_lib.py window-grid limitation:
adding a 2nd row wipes the previously-added row's width/height to 0.00,
in both same-session and fresh-session-per-window attempts. Not fixable
via current Playwright automation. Restored the assessment to its original
single South 10.06 m^2 row (no PDFs re-downloaded, no worksheet change).
Separately confirmed our own engine already models this cert's windows
per-orientation correctly from gov-API sap_windows via
solar_gains.py's ORIENTATION_BY_SAP10_CODE cascade -- this is an Elmhurst
build-tooling ceiling, not an engine or mapper bug. sap_score=53 remains
unchanged; no calculator/mapper code was touched. Full accuracy suite
re-run clean (66 passed/67 skipped/1 xfailed + corpus test passed).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
These are 2011 SAP-2009 lodgements; this calculator implements current
RdSAP10, which has since revised several U-value tables. Closeness to
lodged was never evidence of mapper correctness (nor is a gap evidence
against it) -- the prior commit's comments and worklist entries wrongly
framed 16/20 "matching lodged" as validation and the other 4 as
anomalies needing explanation. Corrected the comments to state what the
batch actually shows: all 20 structurally distinct certs map without
error and produce plausible scores. Real accuracy validation still
needs an Elmhurst RdSAP10 rebuild on the same methodology as the
engine, which remains open.
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.
Review feedback: cavity and internal wall insulation are competing options,
so a package selecting both read as nonsense even in a synthetic fixture.
The behaviour under test (a dependency triggered in both phases injects
once) now uses an airtightness pair that genuinely coexists — cavity wall
in phase 1, double glazing re-entering in phase 2 — with the same numbers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents behaviour already delivered: with no SAP target both phases run
max-gain, so the envelope still consumes the budget first. Test passed on
arrival.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents behaviour already delivered: an empty phase 1 meets no target, so
phase 2 optimises every group — identical to a plain run. Test passed on
arrival.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents behaviour already delivered: phase 2 optimises every group phase 1
did not consume, with signals re-scored against the fabric-applied dwelling,
so glazing skipped on raw-baseline merit re-enters when it closes the target.
Test passed on arrival.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents behaviour already delivered by the phase-2 structure: the leftover
budget is the envelope minus the committed fabric cost, so a heating system
the plain optimiser would buy can be priced out. Test passed on arrival.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
'Cancelled' and 'no show' were separate strings in the outcome vocabulary,
but HubSpot only ever emits the one dropdown value 'Cancelled / No Show' -
so neither phantom string could ever match. Replace both with the real
value (mapped to NOACCESS, best-guess pending client), which also fixes the
long-standing differ mismatch: the abandonment trigger now actually fires on
Cancelled / No Show. Drops the now-unused REQT enum member.
Co-Authored-By: Claude Opus 4.8 <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>
Complete the outcome-to-code table so all five UNSUCCESSFUL_OUTCOMES map:
cancelled -> REQT (new enum member) and no show -> NOACCESS join the
existing three. Only no answer -> CARDED is firm; the rest are best-guesses
for client confirmation, noted inline with alternatives.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the CARDED placeholder in abandon_reason_for_outcome with the
real outcome-to-code table from the client: No Answer->CARDED,
Tenant Refusal->CRA (ambiguous, client follow-up pending),
Not Viable->NOACCESS. Only these three outcomes trip the abandonment
trigger today; any empty/unrecognised outcome raises
UnmappableOutcomeError since abandon_reason is mandatory.
Co-Authored-By: Claude Opus 4.8 <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>
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>
