Review findings on PR #1527:
- The 'which goals are goal-aligned' set lived in two adjacent if-chains
(_objective_for and _require_budget_for_goal_aligned) that had to stay in
sync — a new goal-aligned goal added to one but not the other would slip
the budget guard. Both now read a single _GOAL_ALIGNED_OBJECTIVES table.
- The goal strings are the canonical PortfolioGoal enum values, not
re-declared string constants, so goal-value drift can't silently degrade
a goal to max-SAP; _target_sap reads the enum too.
- _scored_candidate_groups takes objective without a default (its only
caller passes it).
- scoring.py: 'cached: float | None' -> Optional[float] per the CLAUDE.md
'Use Optional over | None' rule.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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, 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>
'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>
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>
Extract the abandonment outcome strings to a single UnsuccessfulOutcome
Literal + UNSUCCESSFUL_OUTCOMES tuple in domain.abri.models, alongside
SlotCode. The differ's NEGATIVE_OUTCOMES now references the tuple and the
reason-code mapping is keyed on the Literal, so the two can no longer
drift and a mistyped outcome fails type-checking.
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>
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>
date_abandoned now resolves to the third failed attempt's confirmed
survey date, falling back to its last submission date, rather than the
processing date. This dates the OpenHousing cancellation to when the
job actually lapsed even if the trigger message lags or redelivers.
- domain: abandonment_date() encodes the confirmed-survey -> last-
submission fallback
- DealAbandonment carries both dates; abandon_job raises
AbandonmentDateUnknownError when a deal has neither
- last_submission_date now flows through the trigger message and request
- the injected clock is dropped: nothing reads now() any more
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
North-drop now runs once: the selection computes the usable (north-dropped)
ladder, feasibility filters it, and Sub-Ladder derivation consumes it.
Co-Authored-By: Claude Fable 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>
roof_construction codes group by FORM (empirical: 1=Flat 98%, 4/5/8=
Pitched 88-99%, 3=dwelling-above 100% over 7,974 certs; 7/9=premises-
above per #1452), so the filter matches families — an exact-code filter
would wrongly drop pitched neighbours lodged as 5/8. Historic prefixes
map to the same families; roof rooms and thatch stay unconditioned.
Harness ladder replay and telemetry mirror the new filter.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Evidence (439-pair harness, PR #1466): historic-vs-new age band agrees
52% exactly but 90% within one band (assessors re-band, skewing newer);
TFA agrees 45% within ±5% but 82% within ±20%. Equality/±5% steered the
cohort toward stale values where they engaged and relaxed everywhere
else. Band definitions are public so the harness's ladder replay shares
one source of truth.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
