fix: release VPP control when IDLE at the reserve floor (#592)

[johanzander]

Summary

• Growatt VPP: when the plan says IDLE and the battery is at its configured minimum SoC, release remote control instead of holding battery_first, so the inverter and its BMS can sleep.
• Above the floor, nothing changes — the Question: How is IDLE used? #466 hold is preserved.

Root cause

_intent_to_vpp mapped every IDLE period to vpp_power=+1, remote control enabled, with no state-of-charge input at all. Because remote control stays enabled, _apply_period_vpp's needs_write is unconditionally true, so the command is re-asserted every 15-minute period to refresh the inverter's fallback timer (#404). Through a long overnight idle the inverter is therefore never handed back, and the BMS never idles down.

The battery_first hold is correct where IDLE is holding energy back for a later peak (#466): IDLE's own DP cost model (_idle_battery_flows) never credits battery discharge for load, so self-consumption must come from grid/solar. At the reserve floor there is no stored energy left to protect — the hold buys nothing and costs the inverter its sleep.

Fix

IDLE + at_reserve_floor → (0, False): released to the inverter's own load_first self-use.

at_reserve_floor is derived live in BatterySystemManager._at_reserve_floor() and threaded through apply_period exactly as block_passive_charging (#355) and strategic_intent (#413) already are. It is read fresh at every write — including the retry path (which fires 3–8 minutes later) and the discharge-inhibit path, both of which would otherwise default it to False and silently re-assert the hold.

Why release rather than vpp_power=0 with remote control still enabled. The reporter tested the latter and recommended it, but that is grid_first, and it is not flow-neutral: it holds the battery against charging, so passive solar surplus is bypassed to the grid. IDLE's DP cost model does credit that absorption. Releasing to load_first keeps it, matching today's hold exactly. The reporter's overnight test could not distinguish the two — there is no sun at 03:00. Their other listed suggestion (load_first) is what this implements.

The SoE conversion deliberately mirrors min_soe_kwh's own arithmetic (capacity * pct / 100), because the case that decides this branch is exact equality — a battery parked on its floor overnight.

Test plan

• ./scripts/quality-check.sh passes locally

• pytest -m slow passes (554 passed, 8 skipped). The VPP regression baseline did move, and the movement is itself the evidence — see "Corpus" below. The earlier revision of this PR claimed the baseline was unchanged and read that as flow-neutrality; that claim was vacuous and has been withdrawn.

• Observed on the real stack (docker-compose.ci.yml, ci-growatt-vpp scenario, libfaketime at 2025-01-15 20:00, battery sensor driven to the 10% floor). The DP independently planned Period 80 (20:00): Intent=IDLE — the reported condition, produced by the real optimizer. Reading the actual inverter registers, with the fix reverted and restored:

  	vpp_remote_control	Meaning
  Fix reverted	Enabled (power 1)	inverter commanded, BMS stays awake
  Fix restored	Disabled	inverter released, BMS can sleep

  (vpp_power still reading 1 afterwards is expected: set_growatt_vpp_period documents power_pct as ignored when remote_control_enabled is False.)

Evidence the test discriminates

• Reverted: return (0, False) if at_reserve_floor else (1, True) → return 1, True
• Result: 3 tests FAILED — test_idle_at_reserve_floor_releases_remote_control, test_idle_at_the_floor_releases_the_inverter, test_released_control_stops_re_asserting_every_period. The production-path test showed the bug directly: four identical power_pct: 1, remote_control_enabled: True writes across four consecutive periods, which is "the inverter is never released".
• Restored: tree clean, 60/60 green.
• The same mutation was also run end-to-end against the live stack (table above), not only against the unit suite.

Outcome-level coverage

• Production write path — test_vpp_idle_at_reserve_floor.py drives BatterySystemManager._apply_period_schedule and asserts the command that actually lands on the inverter, not _intent_to_vpp with hand-built arguments. This is what stops the new branch being unreachable in production while a unit test still passes.

• Execution model — test_vpp_simulator_branches.py::TestIdleAtReserveFloor proves the released command produces the same battery power as today's hold at the floor, under both solar surplus and load deficit, plus a guard rail showing it does discharge one kWh above the floor (i.e. why the gate is needed).

• Corpus — the VPP baseline, re-derived. The previous claim ("unchanged across 37 fixtures") could not have held: derive_vpp_commands never received at_reserve_floor, so the release branch was unreachable from the capture harness and the pin could not move whatever the fixtures contained. With the flag threaded through, the corpus exercises it, and the pin moves in exactly the shape the fix predicts:

  Half	Entries	Periods changed	Δ realized cost	Δ SoE
  v10.0.2 (historical plan, today's model)	23	234	0.000000000000	0.000000000000 kWh
  Current (today's plan)	27	265	0.000000000000	0.000000000000 kWh
  Total	50	499	0	0

  499 periods change their command from [1, True] to [0, False]; not one moves a joule or an öre. That is the corpus-level flow-neutrality evidence the withdrawn claim asserted without support.

  Re-pin method. Both pins moved, test_vpp_execution_of_the_baseline_plan_is_unchanged included — expected, since this PR edits the execution model that half replays the historical plan through. --repin-current skips that half by design, so the historical plans were extracted from the baseline's own plan fields and replayed via --from-plans, then --add-new restored the three post-tag plan: null fixtures. No tag checkout. Verified after writing: fixture set identical, zero plans altered on either half, only the 50 command arrays re-pinned — so the drift signal test_drift_from_the_released_version_is_recorded depends on is intact.

run_scenario_realized / R == P does not apply: that harness is Growatt TOU-only. vpp_simulator is VPP's execution model, and it is covered above.

Scope assessment

Local — stays inside the existing contract. _intent_to_vpp is already a pure (inputs) → (power_pct, remote_control_enabled) lookup fed by flags computed once upstream in BSM; this adds a fourth flag on the identical, twice-established path. Only one subclass overrides apply_period. TOU platforms never reach this code path and ignore the flag.

Workaround check: the diff adds nothing whose job is to route around an ordering, timing, or dependency problem. at_reserve_floor is a genuine new input the mapping needs and does not currently receive — same category as block_passive_charging. No fallback, no second construction site, no extra trigger.

Review round 2 — what changed

Both Stage 4 reviews (09:40 and 10:55) requested changes. All three findings are addressed:

1. at_reserve_floor was dead at 2 of 3 call sites (09:40). The = False default let both non-production callers keep passing while never supplying the real value.
   • _vpp_display_state now receives it via _planned_at_reserve_floor(), which derives the flag from the plan's SoE trajectory (the display counterpart to the write path's live SoC). Without this the schedule API reported the old hold for periods production releases, breaking _mode_display_fields's own no-fabrication contract.
   • derive_vpp_commands now threads a per-period flag from the simulated SoE trajectory, which is what makes the corpus evidence above mean anything.
2. _at_reserve_floor() could raise on an unreadable sensor (10:55, plus the inline comment on battery_system_manager.py:2816). get_battery_soc() is float | None, and the helper multiplied it raw — an uncaught TypeError on every period write, on every platform, from two apscheduler jobs with no exception handling. It now reuses _get_current_battery_soc()'s soc is not None and 0 <= soc <= 100 validation and holds on an invalid reading, logged. Holding is chosen deliberately: it is the safe direction and is exactly the pre-VPP idle mode #592 behaviour, since releasing is what could let self-use draw the battery down. Explicit and logged, not a silent fallback.
   • Writing that test RED surfaced a second bug the review did not mention: a negative SoC reading was releasing, because a negative SoE is trivially <= min_soe_kwh. The range check closes it.

Known limitation — please read before merging

How far the battery can actually fall under released self-use is the inverter's own discharge_stop_soc, not BESS's min_soc — and in VPP mode BESS never writes that register (initialize_hardware returns before sync_soc_limits, per #309). If the inverter's own floor sits below the configured min_soc, released self-use can draw the gap between them.

This is not new to this PR: LOAD_SUPPORT (#413) and SOLAR_STORAGE already release control the same way at any SoC, so this extends an existing exposure to IDLE-at-floor rather than creating one, bounded by the gap between the two floors (zero on a correctly configured inverter). vpp_simulator models the release as a hold at min_soe_kwh, i.e. it assumes the two floors agree.

Ships experimental pending hardware confirmation from the reporter that (a) the BMS does sleep, and (b) the battery does not discharge below the configured minimum.

Documentation

• docs/INVERTER_PLATFORMS.md — new "IDLE at the reserve floor" subsection under IDLE semantics, including the caveat above.
• docs/agents/bess-knowledge.md — the IDLE/VPP mapping paragraph stated the hold unconditionally and would otherwise have gone stale.
• CHANGELOG.md — one line under ## [Unreleased] → Fixed.

Refs #592 — deliberately not Closes, since the reporter still needs to confirm on real hardware.

[bess-agent]

@claude-bot review

[bess-agent]

@claude-bot review

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

test-permission-check-only, will be replaced

[bess-manager-claude-bot]

(Disregard my previous one-line comment on this PR — that was a tooling check, not the review.)

Review: fix/issue-592-vpp-idle-at-floor

Root cause match

Matches #592. The reporter described battery_first (vpp_power=+1, remote control enabled) being re-asserted every period during an overnight IDLE at min SoC, keeping the BMS awake. The PR traces this to _apply_period_vpp's needs_write = remote_control_enabled or (...), which is unconditionally True while remote control stays enabled — confirmed correct by reading core/bess/solax_modbus_growatt_controller.py.

Does the fix hold everywhere it needs to? — No, two of three call sites of the changed function were left un-updated

_intent_to_vpp gained a fourth parameter, at_reserve_floor: bool = False (core/bess/solax_modbus_growatt_controller.py:349), and the new branch:

core/bess/solax_modbus_growatt_controller.py:428
    return (0, False) if at_reserve_floor else (1, True)

There are three call sites. Only one — _apply_period_vpp (the production hardware-write path, line ~509) — passes the real, live-computed flag. The other two keep the default False, so for them the new branch is dead code:

1. core/bess/simulation/vpp_simulator.py:93-98 (derive_vpp_commands, not touched by this PR):

power_pct, remote_control_enabled = controller._intent_to_vpp(
    grid_charge,
    discharge_rate,
    block_passive_charging,
    intents[period],
)

This is the function core/bess/tests/unit/vpp_capture.py::simulate_plan (via scripts/capture_vpp_baseline.py) uses to build the 37-fixture VPP regression corpus. The PR description's central evidence for flow-neutrality is:

"pytest -m slow passes; the v10.0.2 VPP regression baseline is unchanged on all 37 fixtures — 24 of which contain IDLE-at-floor periods. No re-baselining, which is the corpus-level confirmation that no energy flow moves."

That's not what this shows. Because at_reserve_floor always defaults to False here, _intent_to_vpp's new branch is unreachable through this harness regardless of what any fixture's SoE trajectory does — every one of those "24 IDLE-at-floor" fixtures still gets (1, True) out of _intent_to_vpp when this baseline is captured. The baseline is unchanged because the mapping change is never exercised by it, not because it's flow-neutral. Per docs/agents/rules.md's testing section ("A new test must be seen to fail without its fix... 'The suite is green' is evidence the suite is satisfied, never that the behavior holds"), this specific piece of evidence needs to be re-derived or the claim withdrawn. (The TestIdleAtReserveFloor class in test_vpp_simulator_branches.py does legitimately pin flow-neutrality — but it constructs VppCommand(0, False) by hand and calls vpp_command_to_power directly, bypassing _intent_to_vpp/derive_vpp_commands entirely, so it can't catch a wiring defect in the mapping itself, only in the physics model.)

2. core/bess/solax_modbus_growatt_controller.py:432-449 (_vpp_display_state, not touched by this PR):

def _vpp_display_state(
    self,
    grid_charge: bool,
    discharge_rate: int,
    block_passive_charging: bool = False,
    strategic_intent: str = "",
) -> tuple[int, bool]:
    """Display-facing alias for _intent_to_vpp().
    ...
    """
    return self._intent_to_vpp(
        grid_charge, discharge_rate, block_passive_charging, strategic_intent
    )

This feeds _mode_display_fields() (core/bess/inverter_controller.py:682, called from get_period_settings()), i.e. the schedule data the API/UI shows the user. _mode_display_fields's own docstring states it "Never fabricates a label the hardware doesn't back." After this PR ships, for any IDLE period the UI predicts to be at the reserve floor, the API will still report vpp_power_pct: 1, vpp_remote_control: true — the old hold — even though production will actually write vpp_power_pct: 0, vpp_remote_control: false. That's a real, user-facing accuracy regression this PR introduces on a path with an explicit no-fabrication contract, and neither the new tests nor the fixture corpus cover it (no test references _vpp_display_state together with the reserve floor).

Both gaps exist for the same structural reason: giving the new parameter a default (= False) let both call sites keep compiling and keep passing their existing tests without anyone having to notice they don't supply the real value. A required parameter (no default) would have turned both into TypeErrors the moment any existing test exercised them, surfacing the gap immediately instead of shipping it silently.

Fixing this doesn't look like a redo of the whole PR: _vpp_display_state's fix is straightforward, since get_period_settings() already has the period's planned SoE available and can derive at_reserve_floor from it the same way _at_reserve_floor() derives it from the live one. derive_vpp_commands is more involved — it currently derives all commands before simulate_vpp computes any SoE, so wiring this through means computing/threading a per-period at_reserve_floor (from the plan's own SoE trajectory) into that loop rather than deriving commands eagerly, up front.

Is the change minimal?

Yes for the plumbing that is there — the at_reserve_floor parameter follows the exact established pattern of block_passive_charging/strategic_intent, threading through _apply_period_schedule, the retry path, and apply_discharge_inhibit correctly (verified each one re-reads live SoC rather than reusing a stale value). The _at_reserve_floor() helper's SoE arithmetic matches min_soe_kwh's own (total_capacity * pct / 100.0, same order of operations) as claimed. No scope creep, no new classes, no Optional/hasattr/silent-fallback violations, no hardcoded entity IDs. The gap isn't scope creep — it's incomplete scope: the new input needed to reach every caller of the function it was added to, and didn't.

Would the new tests catch a regression?

The production-write-path tests (test_vpp_idle_at_reserve_floor.py) and the mapping tests (test_solax_modbus_growatt_vpp.py) are well constructed — they drive _apply_period_schedule end to end, assert the actual command, and the PR's mutation-testing note (reverted the fix, watched 3 tests fail) satisfies the "write it RED" rule. But they only cover the one call site that was actually wired up. They give no signal on the two gaps above.

Scope assessment statement

Present and appropriately labeled "Local" per rules.md's Debugging Protocol step 9 — I agree with that characterization for the write-path change itself. It just needs to additionally cover the two other callers of the function it's editing.

Verdict: Request changes. The core write-path fix looks correct and well tested. Before merging, either wire at_reserve_floor through _vpp_display_state and derive_vpp_commands (recommended, so the display and the corpus baseline both reflect real behavior), or explicitly scope this PR to the write path only, drop the corpus-baseline claim from the PR description, and file a follow-up issue for the display/baseline gaps.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Summary

Root-cause match: Yes. The diff traces cleanly to the reported bug — _intent_to_vpp's IDLE branch unconditionally returned (1, True), and because _apply_period_vpp's needs_write is True whenever remote_control_enabled, the command was re-asserted every period, so the inverter was never released. The fix ((0, False) if at_reserve_floor else (1, True)) addresses that directly, and I verified initialize_hardware really does return before sync_soc_limits for control_mode == "vpp" (solax_modbus_growatt_controller.py:648-653) — so the PR's "Known limitation" section is correct, and it actually corrects a wrong premise in the issue's own analysis comment (which claimed the min-SoC register is always synced at startup).

Would the new tests catch a regression? Mostly yes — test_vpp_idle_at_reserve_floor.py drives the real _apply_period_schedule write path (not just the mapping function) and pins the "stop re-asserting" behavior, and test_vpp_simulator_branches.py::TestIdleAtReserveFloor proves flow-neutrality at the floor. Good outcome-level coverage. However, see the blocking correctness finding below — the new code path doing the reserve-floor check is not itself exercised in a state that's realistic for production (sensor unavailable / None), and no test covers it.

Minimal / no scope creep? Yes — the diff stays inside _intent_to_vpp's existing (inputs) → (power_pct, remote_control_enabled) contract, and the fourth flag follows the same threading pattern already used for block_passive_charging/strategic_intent. Docs and changelog updates match the change.

Scope assessment statement: Present and accurate — PR body states this is a "Local" fix per the Debugging Protocol.

Blocking finding

BatterySystemManager._at_reserve_floor() (core/bess/battery_system_manager.py:2796-2819) does:

current_soe = (
    self.battery_settings.total_capacity
    * self.controller.get_battery_soc()
    / 100.0
)

get_battery_soc() → _get_sensor_value("battery_soc") is documented and typed to return float | None: "Returns: float: The sensor value, or None if the sensor is unavailable, unknown, or could not be read" (ha_api_controller.py:1419-1433). This codebase already has an established pattern for this exact fallibility — _get_current_battery_soc() (battery_system_manager.py:1737-1744) explicitly checks soc is not None and 0 <= soc <= 100 before using it, with a warning + fallback when it isn't. _at_reserve_floor() skips that check entirely, so a transient "unavailable"/"unknown" HA sensor state raises an uncaught TypeError (float * NoneType) instead of degrading gracefully.

This isn't just a theoretical edge case — it's now reachable from three call sites added by this PR, with different (and in two cases, worse) failure behavior than before:

• _apply_period_schedule (battery_system_manager.py:2718) — wrapped by update_battery_schedule's outer try/except, so this fails the entire period write (grid_charge, discharge_rate, everything), not just the reserve-floor decision, and is silently absorbed as "Failed to update battery schedule".
• The period retry closure (battery_system_manager.py:2870, inside retry_period_write) — this runs via a bare apscheduler DateTrigger job with no exception handling at all, so a failure here bypasses _runtime_failure_tracker.record_failure(...) entirely — unlike the existing if not success: branch it sits next to, which is precisely how failures are supposed to surface to the dashboard banner.
• apply_discharge_inhibit (battery_system_manager.py:3492) — registered directly as an every-minute CronTrigger job (backend/app.py:424-429) with no surrounding try/except in battery_system_manager.py. A sensor hiccup here silently breaks discharge-inhibit monitoring for that tick with no failure-tracker entry, unlike every other failure path in this file.

Also worth noting: _at_reserve_floor() is called unconditionally in _apply_period_schedule for every inverter platform (TOU Growatt, Solis, Huawei, SolaX, ...), not just Growatt VPP where the flag is actually consumed — so this crash risk is not scoped to the platform this PR is fixing.

None of the new tests exercise this: MockHomeAssistantController.settings["battery_soc"] is always set to a real float (10.0/50.0/the default 50), never None, so the gap is completely untested. Suggest mirroring _get_current_battery_soc()'s validation (or reusing it) before requesting changes are re-reviewed.

Verdict

REQUEST_CHANGES — solid root-cause fix and strong outcome-level test design, but the new _at_reserve_floor() helper needs to handle get_battery_soc() -> None the way the rest of this file already does, since as written it turns a routine, transient HA sensor hiccup into an unhandled exception on every period write, across every inverter platform, not just Growatt VPP.

[bess-manager-claude-bot]

self.controller.get_battery_soc() can return None ("unavailable"/"unknown" HA sensor state — see ha_api_controller.py's _get_sensor_value, typed float | None). Multiplying that into current_soe raises an uncaught TypeError instead of degrading gracefully.

This codebase already has the fix for this exact fallibility: _get_current_battery_soc() a few hundred lines up validates soc is not None and 0 <= soc <= 100 before using it. _at_reserve_floor() should do the same (or reuse it) rather than reading the sensor raw.

This matters more than a normal null-check nit because of where this method is now called from: the retry closure in _schedule_period_retry (line ~2870) has no surrounding try/except at all, and apply_discharge_inhibit (line ~3492) runs as a bare every-minute APScheduler job with none either — both bypass _runtime_failure_tracker entirely on this exception, unlike every other failure path in this file. And since this is computed unconditionally in _apply_period_schedule for every platform (not just Growatt VPP), it's a new crash risk for Solis/Huawei/SolaX/TOU-Growatt installs too, not just the VPP path this PR targets.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Verdict: REQUEST_CHANGES

The fix's core mapping (_intent_to_vpp's new at_reserve_floor branch, _at_reserve_floor()'s live-SoC read with fail-safe holding) matches #592's root cause and the maintainer's own recommended fix (release to load_first, not grid_first). The production write path is well covered — test_vpp_idle_at_reserve_floor.py drives _apply_period_schedule end to end and would clearly catch a regression of the reported bug. But two things need fixing before merge.

1. Rule violation — forbidden getattr(obj, key, default)

core/bess/inverter_controller.py:697 (_planned_at_reserve_floor):

soe = getattr(self.current_schedule, "state_of_energy", None)
if not soe or period >= len(soe):
    return False

docs/agents/rules.md → Python: "Never use hasattr, getattr(obj, key, default), or any silent fallback." .github/claude-bot.md's checklist repeats this explicitly. current_schedule is typed DPSchedule | None (inverter_controller.py:176), and DPSchedule.state_of_energy is a required constructor arg with no default — the attribute always exists on a real instance. The only thing getattr is masking here is current_schedule being None. That should be:

if self.current_schedule is None:
    return False
soe = self.current_schedule.state_of_energy
if period >= len(soe):
    return False

Small in isolation, but it's a bright-line rule the checklist calls out by name, not a style nit.

2. Off-by-one: _planned_at_reserve_floor reads the wrong end of the period

Same method, next line:

return soe[period] <= self.battery_settings.min_soe_kwh

The docstring says this checks whether the battery is at the floor entering period — matching what BatterySystemManager._at_reserve_floor() reads live (the SoC that exists before this period's write). But state_of_energy[period] is not that. The only production writer of this field is _create_updated_schedule (battery_system_manager.py:2254):

combined_soe[target_period] = period_data.energy.battery_soe_end

— explicitly battery_soe_end, not battery_soe_start (both are distinct fields on EnergyData, populated separately in dp_battery_algorithm.py:1024-1025: battery_soe_start=soe, battery_soe_end=next_soe). So state_of_energy[period] is the SoE after period's own action runs, i.e. the SoE entering period + 1, not period. _planned_at_reserve_floor(period) is answering the question for the wrong period — it's reading one step ahead of what _at_reserve_floor() reads for the same period at write time.

Concretely: at the boundary period where the plan first reaches the floor (battery discharges down to exactly min_soe_kwh during that period), the live write-time check would see the battery still above the floor entering that period (hold expected), but _planned_at_reserve_floor would already report "at floor" for that same period, one period too early — and symmetrically one period too late when the plan leaves the floor. This directly contradicts the PR's own stated contract for this method: "it must answer the same question, or the UI shows a hold for periods production releases."

This also isn't reachable by the elsewhere-correct soe_trajectory convention this same PR establishes in vpp_simulator.py — there, _simulate's loop passes command_at(t, soe) where soe is the running entering SoE at the point command_at is invoked (core/bess/simulation/vpp_simulator.py, _simulate's loop body), i.e. index t's entering state — the correct convention. _planned_at_reserve_floor reuses the same "index by period" idea against a differently-shaped production array (combined_soe, which stores ending SoE) without the shift.

Why the new tests don't catch this: TestDisplayAgreesWithWhatIsWritten in test_vpp_idle_at_reserve_floor.py builds its fixture with state_of_energy=[soe] * 97 — a uniform value across every index. Under a uniform trajectory, soe[period] and soe[period - 1] are identical, so the off-by-one is invisible. A fixture with a varying SoE trajectory (e.g. one that crosses the floor between period - 1 and period) would discriminate this; the current one can't, regardless of which side of the bug the implementation is on.

Scope / minimality

Scope is otherwise appropriate — the vpp_simulator.py restructuring (splitting simulate_vpp/simulate_vpp_commands/_simulate) is justified in the PR body and is a direct consequence of the flag becoming SoE-dependent, not unrelated cleanup. The "Known limitation" section (VPP mode never writing discharge_stop_soc) is disclosed honestly and I confirmed it against solax_modbus_growatt_controller.py:647-655 (initialize_hardware returns early for control_mode == "vpp" before super().initialize_hardware()/sync_soc_limits would run) — accurate, not overstated.

Scope assessment statement

Present and reasonable ("Local" — the PR explains why _intent_to_vpp gaining a fourth input flag stays within its existing pure-lookup contract).

---

Please fix both issues in _planned_at_reserve_floor (the getattr and the index) and add a test where the SoE trajectory actually varies across the boundary period, so the display-vs-write agreement claim is actually exercised.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Summary

Root cause and fix match the linked issue (#592) exactly: _intent_to_vpp mapped every IDLE period to a battery_first hold with no SoC input, so an empty battery at the reserve floor never released the inverter and its BMS never slept. The fix threads a live at_reserve_floor flag through the write path (battery_system_manager.py), the display path (_planned_at_reserve_floor), and the simulator/corpus (vpp_simulator.py), releasing to load_first rather than the reporter's suggested power=0/enabled (grid_first) — correctly reasoned, since grid_first would bypass passive solar surplus to the grid instead of absorbing it, which IDLE's DP cost model credits.

Two rounds of prior review findings (dead flag at 2/3 call sites, unguarded TypeError on an unreadable SoC) are visibly addressed with dedicated regression tests (test_unreadable_soc_holds_rather_than_releasing, test_out_of_range_soc_holds_rather_than_releasing).

Test coverage is strong and outcome-focused (per rules.md's testing rules): test_vpp_idle_at_reserve_floor.py drives the real production write path (_apply_period_schedule) rather than asserting on _intent_to_vpp alone, so the branch can't go dead in production while a unit test still passes — exactly the failure mode the file's own docstring calls out. test_vpp_simulator_branches.py::TestIdleAtReserveFloor proves flow-neutrality at the outcome level (same vpp_command_to_power result as the old hold), with a guard rail (test_release_would_discharge_above_the_floor) showing the branch does matter above the floor. The scope-assessment section required by the checklist is present and accurate — this stays inside _intent_to_vpp's existing (inputs) → (power_pct, remote_control_enabled) contract, adding one more input flag the same way block_passive_charging and strategic_intent already are.

Blocking: shipped docs contradict the PR's own evidence

Two doc additions in this PR assert a claim the PR body explicitly withdrew.

docs/INVERTER_PLATFORMS.md:397-399 (as it lands on the branch):

"Verified across the fixture corpus: the v10.0.2 VPP regression baseline is unchanged by this fix, on all 37 fixtures, 24 of which contain IDLE-at-floor periods."

docs/agents/bess-knowledge.md:311:

"...and there is no headroom to discharge — so the VPP regression baseline is unchanged."

But the PR description's own "Test plan" section says the opposite, in these words:

"The VPP regression baseline did move, and the movement is itself the evidence... The earlier revision of this PR claimed the baseline was unchanged and read that as flow-neutrality; that claim was vacuous and has been withdrawn."

And the "Corpus" section quantifies it: 50 baseline entries / 499 periods change their command from [1, true] to [0, false] — which is exactly what core/bess/tests/unit/data/baselines/vpp_baseline_v10_0_2.json shows in this diff (over 1000 changed lines, 1,true → 0,false throughout). I confirmed via git log that both doc lines were written in the first commit (c05b58e) and never touched by the later commit that actually re-pins the baseline (43295e6 test: re-pin the VPP baseline commands, plans untouched) or the commit that fixed the off-by-one SoE index (900f5e5).

This isn't a nit: docs/agents/bess-knowledge.md is explicitly required reading for any agent answering BESS behavior questions (per this repo's own CLAUDE.md), so shipping a factually wrong "baseline unchanged" claim there actively misinforms future work — and it directly contradicts the "claim... has been withdrawn" language sitting right above it in the same PR. What's actually unchanged is realized_cost and SoE (0.000000000000 delta per the Corpus table) — the commands changed by design in every IDLE-at-floor period. Both doc passages need to say that: baseline commands re-pinned in 499 periods across 50 entries, cost/SoE unchanged.

Non-blocking observation

_planned_at_reserve_floor (core/bess/inverter_controller.py) special-cases period == 0 to read soe[0] directly rather than soe[period - 1], to avoid Python's negative-index wraparound. The docstring justifies this by asserting period 0 is always "the optimization period" (i.e. combined_soe[0] freshly written as current_soe), citing _create_updated_schedule. That holds when the active schedule's optimization_period == 0, but I don't see it holds generally: for a schedule re-optimized mid-day, index 0 is a past (historical) period, and battery_system_manager.py's own SOE-progression loop (~line 1876-1922) fills historical indices with event.energy.battery_soe_end — the SoE leaving that period, same convention as every other index — not a freshly-captured "entering" value. That would make soe[0] semantically the wrong quantity for a historical period 0. This is display-only (get_period_settings, not the live-SoC write path), so it can't misbehave the hardware, and I don't see a test exercising get_period_settings(0) on a schedule where optimization_period != 0 to confirm either way. Worth a look, not a blocker.

Checklist

• Fix matches the linked issue's root cause: yes.
• Tests would catch a regression: yes — several would fail RED without the fix (confirmed by the PR's own revert-and-run evidence), and they assert outcomes/production paths, not just the mapping.
• Change is minimal / no scope creep: yes — the wider touch surface (display, simulator, three call sites) is required by the correctness fixes from prior review rounds, not scope creep, and is explained in the PR body.
• Rule violations: none found in Architecture/Error-handling/Comments/Security categories (no Optional[x], no hasattr/getattr fallback, no new production classes, no entity-ID hardcoding, no exception-string matching).

Requesting changes only for the doc/PR-evidence contradiction above — that's a two-line fix in each of the two files.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Review: #619 (fix/issue-592-vpp-idle-at-floor)

Root cause match: Confirmed against issue #592's own text. The reporter described IDLE holding battery_first overnight at min SoC, preventing BMS sleep, and suggested either "load first" or "power 0 at soc min" as fixes, explicitly noting they'd tested the latter ("forced power to 0 when soc is at min ... recommend this"). The PR's root-cause section (_intent_to_vpp mapping every IDLE to vpp_power=+1/remote-control-enabled with no SoC input, re-asserted every period per #404's fallback-timer refresh) matches the reported mechanism exactly, and the fix explains — correctly — why it implements the reporter's other suggestion (load_first/release) rather than their tested one: power=0 with remote control still enabled is grid_first, which bypasses passive solar surplus to the grid instead of absorbing it, unlike battery_first's hold. That's a real behavioral difference the reporter's overnight test (no sun at night) couldn't have caught. Good catch, well-reasoned choice.

Traced the implementation: _at_reserve_floor() (battery_system_manager.py:2793) reuses _get_current_battery_soc()'s validation (so it can't raise on None/out-of-range SoC — verified _get_current_battery_soc already wraps everything in try/except and returns None on failure) and mirrors min_soe_kwh's own total_capacity * pct / 100.0 formula (verified against settings.py:153), so the floor comparison is exact-equality-safe as claimed. _apply_period_vpp's needs_write = remote_control_enabled or (remote_control_enabled != self._last_written_vpp_remote_control) (verified in solax_modbus_growatt_controller.py) confirms the "stops re-asserting" claim: once released, both sides of the or are False on subsequent identical periods.

Tests / regression coverage: Very strong, multi-layered:

• test_vpp_idle_at_reserve_floor.py drives the actual production write path (_apply_period_schedule), not just _intent_to_vpp with hand-built args — this is the right level, since a unit test on the mapping alone would pass even if the flag never reached it in production (which is exactly what happened in round 1).
• TestIdleAtReserveFloor in test_vpp_simulator_branches.py proves flow-neutrality at the outcome level (battery power, not just the command), per docs/agents/testing.md's "assert the outcome, not the command" guidance, with a guard-rail test proving the release would discharge one kWh above the floor (i.e., why the gate matters).
• The corpus re-pin (499 command changes, 0 cost/SoE delta) is credible: I verified derive_vpp_commands genuinely required a real soe_trajectory before (it previously received nothing at all, matching the PR's own admission that the earlier "unchanged baseline" claim was vacuous), and vpp_capture.py's move from eager derive_vpp_commands to lazy simulate_vpp is what makes the release branch reachable during capture.
• Round-2 fixes (dead flag at 2 of 3 call sites, crash-on-unreadable/negative-SoC) are exactly the right things to have caught in earlier rounds, and are independently tested (test_unreadable_soc_holds_rather_than_releasing, test_out_of_range_soc_holds_rather_than_releasing).

I traced the SoE-indexing logic in _planned_at_reserve_floor (the "entering vs leaving" off-by-one it flags) against test_the_crossing_period_still_displays_the_hold and it checks out: state_of_energy[p] is genuinely the leaving-value per _create_updated_schedule, and reading p-1 for the entering value is correct.

Scope: Matches the stated "local" assessment — _at_reserve_floor/at_reserve_floor is threaded as a new input alongside the already-established block_passive_charging/strategic_intent pattern, not a new responsibility bolted onto an unrelated method. The vpp_simulator.py refactor (splitting _derive_vpp_command/_simulate, adding simulate_vpp_commands) is larger than the core fix, but it's justified: it's the direct fix for the round-1 finding that the corpus evidence was vacuous, not speculative cleanup.

Rule compliance: No Optional[x], no hasattr/silent-fallback getattr, no new classes, all sensor access goes through _controller.get_battery_soc() / _get_current_battery_soc() (no hardcoded entities), no exception-message string matching. CI is green across all checks (algorithm, fast, E2E, docker, quality, frontend).

Known limitation: The PR is upfront that VPP mode never writes the inverter's discharge_stop_soc register (#309), so if that floor sits below the configured min_soc, released self-use could draw the gap — explicitly documented, scoped as pre-existing exposure shared with #413/#413's SOLAR_STORAGE, and the PR deliberately doesn't Closes #592 pending the reporter's hardware confirmation. That's the right call, not a blocker.

No correctness bugs or rule violations found.

Verdict: APPROVE