feat(optimizer): Phase 4a — platform capability model + execution_model leaf

[johanzander]

What

Phase 4a of the optimizer target architecture (design doc §4/§4c, decisions D1 and D2). The optimizer could not see which platform would execute its plan: discharge_rate_is_load_following had zero occurrences in dp_battery_algorithm.py, pwl_window_dp.py and action_selector.py, so a SolaX or an SPH was handed the same action space as a Growatt MIN.

D1 — new leaf core/bess/execution_model.py. Imports settings and dp_constants and nothing else (pinned by a test that parses its own import graph). It holds intra_period_discharge_gate, relocated out of battery_system_manager — so simulation/inverter_simulator no longer imports the orchestrator — and INTENT_TO_MODE, which InverterController.INTENT_TO_MODE now references instead of copying.

D2 — PlatformCapabilities. Discharge lattice, control model, mode vocabulary, minimum commandable gear, and discharge-rate semantics as the tri-state the design asked for: ceiling / target / absent — the period_list platforms have no per-period rate to interpret as either. BatterySystemManager builds it from the live controller and passes it to optimize_battery_schedule, which threads it everywhere discharge_resolution_kw used to go. One object, one construction site. The #282 minimum-gear rule (floor(threshold / step) + 1), previously restated in three places, now has one home.

Behaviour

Neutral, by requirement — goldens and the 36-fixture corpus are bit-identical, so 4b's delta stays readable. pytest -m slow: 538 passed, 5 skipped. pytest -m "not slow": 1863 passed, 31 skipped.

One intended change — #580. _residual_cover_p plans a delivery, which is exact only where the inverter throttles to the real load. It was added unconditionally before, so on native SolaX / SPH / Solis / Huawei the DP planned a delivery the hardware will not produce (the #282 shape). It is now gated on load_support_delivers_exact_cover. Growatt keeps the candidate in both control modes — on solax-modbus VPP the rate register is a forced power, but #413 makes LOAD_SUPPORT release the period to native load-following, so the cover is delivered exactly (see the review thread below; the first cut of this PR got that wrong).

Mutation evidence

The #580 test asserts the plan (chosen action and the resulting grid import), not a candidate list. With the capability gate removed from _residual_cover_p, both parametrisations fail:

FAILED test_execution_model.py::test_cover_candidate_is_not_planned_where_the_cover_is_not_delivered[target]
FAILED test_execution_model.py::test_cover_candidate_is_not_planned_where_the_cover_is_not_delivered[absent]
2 failed, 12 passed

Two further pins were watched fail the same way after review (details in the review thread): the VPP-mode pin with the gate keyed on the register's semantics, and test_every_shipped_controller_satisfies_the_invariant with Solis's inherited True reintroduced.

Current: 19 passed in this file; fast suite 1868 passed / 31 skipped; slow suite 538 passed / 5 skipped.

Not absorbed, deliberately: #579 and #571

The plan asks 4a to take _value_slope_below and the new public has_value_cell_below with it. It did not, for two reasons recorded in the plan and design doc:

• fix: price the discharge gate off the cell the battery is actually in #579 is unmerged — there is no code on main to move.
• D1 does not relocate that code. Both fixes live in dp_battery_algorithm._record_marginal_value. 4a moves the gate (the two-line ceiling function), not the value estimator that decides its input.

Taking #579 would additionally have flipped 142 of 2168 golden gate booleans inside the phase that is required to be behaviour-neutral — exactly the confounding the sequencing exists to prevent. Land #579, then #571, each measuring its own delta.

Docs

optimizer-architecture.md (normative) gains an "as built, Phase 4a" note under P3 and lists execution_model.py among the files it governs; the plan and design doc record 4a as built and restate the #579/#571 queue.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y2NCCmmhjBxgFGQE6zZLTS

[johanzander]

Code review (high) — three findings, all fixed in 9806f2c

1. Cover-candidate gate was over-broad on solax-modbus VPP (medium). Gating _residual_cover_p on discharge_rate_is_load_following asks about the rate register. On solax-modbus Growatt in VPP mode that register is a forced power — but LOAD_SUPPORT never writes a rate there: #413 disables remote control for that intent and hands the period to the inverter's own load-following self-use (_intent_to_vpp). The cover is delivered exactly, so the first cut of 4a silently withdrew #466's sunrise-crossover saving from that platform for no fidelity gain — contradicting the design's own platform table ("Growatt VPP — no rate for load support since #413, natively load-following") and _residual_cover_p's own docstring.

Fixed with a second, separately-declared capability rather than a cleverer derivation: load_support_delivers_exact_cover, declared per controller next to discharge_rate_is_load_following and carried on PlatformCapabilities. True on TOU-register platforms and on solax-modbus in both modes; False on native SolaX (never got #413 — gap note in _vpp_display_state) and on the period-list platforms. discharge_rate_is_load_following keeps its own meaning for the intra-period gate, which does write a rate.

New pin, watched fail with the gate keyed on the register's semantics:

FAILED test_execution_model.py::test_cover_candidate_survives_where_load_support_load_follows_without_a_ceiling
1 failed, 16 passed

2. Solis read two ways (low). It declares CONTROL_MODEL = "period_list" but inherited the base class's discharge_rate_is_load_following = True, so the optimizer saw "no per-period rate" while _apply_period_schedule raised its ceiling to 100 as if it load-followed. Solis now declares both explicitly (as SPH already did, for the same stated reason), and BatterySystemManager reads both its planning and its apply-time gate through one platform_capabilities property.

3. Shared mutable mode vocabulary (low). INTENT_TO_MODE was one dict shared by the module constant, InverterController.INTENT_TO_MODE and every capability default — one in-place write would have rewritten the vocabulary for every platform at once, and 4b is slated to build commands from it. Now a MappingProxyType, pinned by a test. Instances are unhashable as a result; stated in the docstring rather than worked around, since nothing keys on one.

Re-verified after the fixes: goldens and the 36-fixture corpus still bit-identical (pytest -m slow: 538 passed, 5 skipped), fast suite 1866 passed / 31 skipped, ./scripts/quality-check.sh 0 errors 0 warnings.

Scope note: #580 now narrows to native SolaX / SPH / Solis / Huawei — Growatt keeps the candidate in both control modes. Changelog, plan and design doc updated to match.

[bess-agent]

Code review — full diff pass

Reviewed as a single diff pass, with source-context reads of every touched controller, the four _residual_cover_p call sites, and the BSM write path. pytest -m "not slow": 1866 passed, 31 skipped.

Verified correct

• All four _residual_cover_p call sites (action_selector._discharge_candidates ×2, dp_battery_algorithm._run_dynamic_programming, pwl_window_dp._pwl_candidate_values_at, _pwl_window_seed_points) are gated identically — no pass/replay action-set drift.
• min_discharge_gear_index is arithmetically identical to the three deleted copies (math.floor vs int(np.floor(...)), both on a strictly positive rate_step); grep confirms no remaining duplicates.
• No stale discharge_resolution_kw= kwarg callers remain (prod, tests, synthetic harness).
• The VPP mode: LOAD_SUPPORT forces fixed grid_first discharge rate instead of releasing control #413 claim underpinning load_support_delivers_exact_cover=True for solax-modbus VPP checks out in _intent_to_vpp (line 377: LOAD_SUPPORT -> (0, remote_control=False)), so the two-capability split is load-bearing and correctly assigned.
• INTENT_TO_MODE has no mutating consumers ([] and .get only), so the MappingProxyType swap is runtime-safe today.
• INTENT_TO_MODE: ClassVar[...] = INTENT_TO_MODE resolves to the module global (class bodies fall back to globals) — not a NameError.

No correctness defects in the capability threading itself; the refactor is behaviour-preserving on the default path and the one intended #580 change is correctly scoped.

Findings

1. core/bess/execution_model.py:158 — medium. __post_init__ validates discharge_rate_semantics against the tri-state, but does not enforce the cross-field invariant DISCHARGE_RATE_ABSENT ⇒ not load_support_delivers_exact_cover. That invariant is currently upheld only by three hand-written ClassVars (SPH, Huawei, Solis) — and this PR exists because one of them was wrong (Solis inherited the base-class True while declaring CONTROL_MODEL = "period_list").

Scenario: the next period_list controller added (or an existing one edited) inherits load_support_delivers_exact_cover = True; from_controller then builds semantics=ABSENT, exact_cover=True, and _residual_cover_p plans an off-lattice partial cover on hardware with no per-period rate at all — the exact #282/#580 shape this phase closes. test_period_list_platforms_have_no_rate_at_all asserts only discharge_rate_semantics, so nothing catches it.

A three-line raise ValueError in __post_init__ (or deriving load_support_delivers_exact_cover=False when semantics is ABSENT) makes the class of bug unrepresentable rather than re-caught in review.

2. core/bess/inverter_controller.py:64 — low. INTENT_TO_MODE: ClassVar[dict[str, str]] = INTENT_TO_MODE annotates a dict but binds a MappingProxyType. The annotation is now false, and no mypy runs in CI (quality-check.sh and the workflows have no mypy step), so nothing flags it. A future platform needing a different mode vocabulary writes the natural {**InverterController.INTENT_TO_MODE, ...} — fine — but any in-place variant (INTENT_TO_MODE["X"] = ..., .update(...)) type-checks against the declared dict and fails only at import time with a TypeError. ClassVar[Mapping[str, str]] states the actual contract.

Minor

The PR body's mutation-evidence block names test_cover_candidate_is_not_planned_where_the_rate_is_not_a_ceiling, but the committed test is test_cover_candidate_is_not_planned_where_the_cover_is_not_delivered — the evidence is real, the quoted names are stale.

[johanzander]

Bot review — both findings fixed in 717c7b5

1. Cross-field invariant not enforced (medium) — agreed and fixed. __post_init__ now rejects discharge_rate_semantics == "absent" together with load_support_delivers_exact_cover=True. The point is well taken: the invariant was upheld only by three hand-written ClassVars, and this branch exists partly because one of them was wrong.

Pinned two ways — the pair directly, and every shipped controller read through from_controller. I checked the second one actually catches the real mistake rather than just the constructed one: reintroducing Solis's inherited True fails

FAILED test_execution_model.py::TestDischargeRateSemantics::test_every_shipped_controller_satisfies_the_invariant
ValueError: load_support_delivers_exact_cover cannot be True when discharge_rate_semantics is 'absent'

where the old semantics-only assertion stayed green — which was the finding's exact argument.

Every other combination stays legal, including target + exact cover: that is solax-modbus VPP, and deriving False from ABSENT instead of raising would have been fine, but narrowing further would undo 9806f2c.

2. ClassVar[dict] annotating a MappingProxyType (low) — agreed and fixed. Now ClassVar[Mapping[str, str]]. Correct that nothing would have caught it: there is no mypy step in quality-check.sh or the workflows.

Minor (stale test names in the PR body) — fixed. The body now names test_cover_candidate_is_not_planned_where_the_cover_is_not_delivered, and its #580 scope paragraph is corrected to native SolaX / SPH / Solis / Huawei.

Re-verified: fast suite 1868 passed / 31 skipped, slow suite 538 passed / 5 skipped (goldens still bit-identical), ./scripts/quality-check.sh 0 errors 0 warnings.

Thanks for the _intent_to_vpp:377 and INTENT_TO_MODE-consumer verifications — those were the two claims the previous round rested on.