Review findings on PR #1527:
- The objective is required (no sap_rating default) on _repair_to_target,
_best_repair_candidate and _rescored_groups: every caller already passes
it, and a default would let a future call path silently optimise SAP for
a carbon/bill goal while pyright stayed green. The default stays on the
public optimise_package / optimise_package_fabric_first entry points.
- _best_repair_candidate hoists objective(current) out of the candidate
loop: current is loop-invariant, so for the Energy-Savings bill objective
this was one full BillDerivation.derive per candidate per repair iteration
for the same score.
Co-Authored-By: Claude Fable 5 <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>
Review findings on PR #1526, behaviour-preserving:
- No fabric groups, or phase 1 committed nothing → delegate to the plain
optimise_package: the phase-2 prefix would be empty and its re-scoring
would reproduce the role-1 signals the groups already carry, one full
SAP-calculator run per Option for zero effect (the common case on
already-insulated stock).
- Phase 1 consumed every group → return the fabric package instead of a
phase-2 pass that can only select the empty package.
- _rescored_groups takes start_sap from the caller: the post-fabric
baseline score is the phase-1 package score already in hand, not a
fresh calculator run.
- fabric_types → phase_one_types: the set holds everything phase 1
committed, injected dependencies included — that inclusion is what the
outstanding-dependencies filter relies on, so the name must not invite
narrowing it to FABRIC_MEASURE_TYPES.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
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>
