admin/goodwe-addon
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

SAFETY-07 review: defend the three wiring mutations that survived

Browse Source 
The reviewer's finding was correct and the shape of it matters: everything the
first cut graded was in peakguard.py, and grading the module is not the same as
grading the guard. Three one-line changes to main.py's CALL SITE left all twelve
suites green while a default install parked at 0 W in the quarter that sets the
month's bill.

B2 was the serious one. My own comment described that defect - "that entity is
optional and empty on a default install, so until now the suspension could not
fire on the site that needed it" - and nothing could regress it. A comment that
documents a fix nobody can test is the same shape as a corrected comment nobody
tests, which this project has already been caught by once.

WHAT CHANGED

The suppression conjunction moved out of main.py and into
peakguard.shaping_suspended(), for the reason watchdog.loop_is_following() and
freeze.looks_frozen() live where they do: the check that grades a safety
predicate must assert on THE SHIPPED EXPRESSION, not on a transcription that can
drift apart from it in silence. Its docstring carries both terms and why neither
is sufficient alone. main.py's call site now says READ THAT FUNCTION FIRST, and
records that a reviewer mutated it back to its historical broken form with every
suite staying green.

test_peakguard.py part 7 gains a section that EXECUTES the three mutations
rather than describing them. Each one is put in place on the shipped module and
the REAL Controller.cycle() is driven with it, on the dangerous fixture: a
default install (no peak_forecast_entity, so peak_fc is None and peak_at_risk is
False forever) with maintenance in its CHARGE phase, so a P_MAINT_SHAPE `hi <= 0`
limit is live, and the guard genuinely engaged on the measurement. That the
mutations are monkeypatched onto the shipped module is itself load-bearing: if
main.py inlined either expression again the patch would do nothing and these
checks would pass vacuously.

    SHIPPED   suspended=True    loop -> 1498 W, limited by peak    +1497.8 W
    B1        suspended=False   CONTRADICTION between limits           0.0 W
    B2        suspended=False   CONTRADICTION between limits           0.0 W
    B2b (forecast term dropped instead) - caught by its own check

B3 is graded three ways, because the single-cycle form is only INFLATED and the
failure is what happens on the next cycle:

  * the value main.py actually hands decide() is recorded off the real call, with
    the measured battery at 0 W and the command standing at +3000 W;
  * the one-cycle difference is asserted to be exactly the standing command - the
    whole defect in one line, watts that were asked for and never arrived counted
    as if they had;
  * both forms are then run forward against the REAL controller's own projection
    with an inverter that never follows:

        intent-sized floor    4482 5000 5000 5000 ...  SATURATED at cycle 2
        delivered-power floor 1482 1482 1482 1482 ...  stationary

  * and the call site is graded on parsed syntax, so a rename cannot quietly
    reintroduce it: `decide(batt_w=...)` must unparse to `self.batt`.

SAFETY-24 is now named in peakguard.py, in its own section, with the number that
makes it real - 878 Wh of forced discharge on the 2026-08-20 replay - and the
statement that this must not go to a client install before that ticket lands. A
ticket nobody can find from the code is a ticket nobody reads before shipping.

RED AND GREEN

Fifteen mutations run against the shipped modules, each reverted after: the six
new wiring ones (B1, B2, B2b, B3, B3b, and deleting the SAFETY-24 pointer) and
the eight from the first round, re-run to confirm they still bite. All fifteen
turn test_peakguard.py red. Full suite green, test_price.py and TARIFF-03's
included.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
This commit is contained in:
glenn schrooyen
2026-08-30 23:40:39 +02:00
co-authored by Claude Opus 5
parent 9f8b721a4d
commit 2d570d84ce
3 changed files with 262 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/app/main.py
+11 -5
View File
@@ -515,11 +515,17 @@ class Controller:
# default install, so until now the suspension could not fire on the
# site that needed it. `self.peak_guard` is measured from the meter
# and is always there.
suspend = peak_at_risk(self.peak_fc,
float(self.o.get("peak_cap_w", 3500)))
if self.peak_guard is not None and self.peak_guard.engaged:
suspend = True
if suspend:
#
# ⚠️ THE CONJUNCTION LIVES IN peakguard.py, NOT HERE, and that is not
# tidiness. It was inline, and a reviewer mutated it back to its
# historical broken form - re-gated on `peak_forecast_entity` alone -
# and every suite in this repository stayed green while a default
# install parked at 0 W in the quarter that sets the month's bill.
# Read shaping_suspended() before touching this: both terms are
# there, and why neither is sufficient alone.
if peakguard.shaping_suspended(
self.peak_guard, self.peak_fc,
float(self.o.get("peak_cap_w", 3500))):
# Edge-logged: level-logging here would write one line per cycle
# for as long as the event lasts and flush the 40-line action log
# of the very context that explains it.
goodwe_controller/app/peakguard.py
+50
View File
@@ -245,6 +245,21 @@ validate.py are untouched by this ticket, exactly as TARIFF-01 left them, and
14's `capacity_peak_limit_kw` stays declined - a kW key beside a W key is the
mismatch trap 14's own warning box describes.
------------------------------------------------------------------------------
⚠️ WHAT THIS GUARD DOES NOT HAVE: AN ENERGY BACKSTOP. SEE SAFETY-24.
The graded branch can FORCE DISCHARGE, and nothing in this file or in the loop
below it consults the state of charge - `soc_floor` belongs to the maintenance
cycle, and freeze.py already records that for the grid-following loop "the BMS
is the sole energy backstop". On the 2026-08-20 replay the guard asks for 878 Wh
in a day. Against a pack sitting near 9 % that is not nothing, and the guard will
keep asking as long as the meter says the quarter is over the ceiling.
That is deliberate for THIS card and it is filed as SAFETY-24. It is also the
reason this must not go to a client install before SAFETY-24 lands: on the
reference site the BMS is a known quantity and an operator is present, and on a
client site neither is true.
------------------------------------------------------------------------------
AND IT READS NO PER-PHASE QUANTITY, ON PURPOSE
@@ -258,6 +273,7 @@ not need re-testing. Nothing here indexes a phase.
from dataclasses import dataclass
from .arbiter import P_PEAK, Claim
from .control import peak_at_risk
from .tariff import QUARTER_S
# Why the projection may not be graded. Reasons, not booleans: the status page
@@ -454,6 +470,40 @@ def decide(projection, *, batt_w, cap_w, reading, achievable_w,
f"only give {ceil_top:.0f} W" if saturated else "")))
def shaping_suspended(decision, peak_forecast_w, cap_w) -> bool:
"""Must the maintenance charge-only shaping stand down this cycle?
THE ONE PLACE THIS CONJUNCTION IS WRITTEN, and it is public for the reason
`watchdog.loop_is_following()` and `freeze.looks_frozen()` are: the check
that grades it has to assert on THE SHIPPED EXPRESSION. A safety predicate
transcribed into its own test is one copy plus a future disagreement about
what the check means, and the copy that drifts is always the grading one.
It was inline in `main.py` and nothing could reach it; a reviewer mutated it
back to its historical broken form and all twelve suites stayed green.
⚠️ WHY SUPPRESSION AND NOT PRIORITY. The shaping is a LIMIT (`hi <= 0`) and
the peak guard's claim is a LIMIT (`lo >= 0`). arbiter.py clause 2 applies
EVERY limit at or above the winner's priority and takes the MOST RESTRICTIVE,
with no notion of one limit outranking another - which is precisely what
makes the safety bounds unconditional and must not be changed. Two crossing
limits are therefore a CONTRADICTION, which resolves to 0 W: the battery
PARKED, in the one quarter of the month that sets the bill. P_PEAK 50 >
P_MAINT_SHAPE 40 does not save it and never could.
⚠️ AND THE FIRST TERM IS NOT ENOUGH ON ITS OWN. `peak_forecast_entity` is
OPTIONAL and is empty on a default install, so `peak_forecast_w` is None
there and `peak_at_risk()` answers False forever. Before this card the
suspension therefore could not fire on the very site that needed it. The
second term is the MEASURED guard, which is always present wherever P1 is.
Deleting either term - or re-gating the whole thing on the forecast - parks
a default install at 0 W, silently, in the billed quarter. test_peakguard.py
part 7 drives the real cycle with each of those mutations in place.
"""
if peak_at_risk(peak_forecast_w, float(cap_w)):
return True
return decision is not None and decision.engaged
def claim(decision: PeakDecision) -> Claim | None:
"""The decision as one arbiter claim, or None when the guard is idle.
goodwe_controller/test_peakguard.py
+201 -9
View File
@@ -80,6 +80,7 @@ from app import peakguard
from app.arbiter import (
P_LOOP, P_MAINT_SHAPE, P_MAINTENANCE, P_PEAK, P_SAFETY, Claim, resolve,
)
from app.control import peak_at_risk
from app.freeze import FreezeWatchdogConfig, looks_frozen, rail_w
from app.main import Controller
from app.p1 import (
@@ -763,14 +764,21 @@ print("7. the wiring: the real Controller, the real cycle, the real status page"
class _Store:
data = {}
# ⚠️ PER INSTANCE. `data` was a CLASS attribute, which is fine while
# nothing writes it and a trap the moment part 7 forces a maintenance phase
# on one controller: every controller built afterwards would inherit it.
def __init__(self, data=None):
self.data = dict(data or {})
def set(self, *a):
pass
def set(self, key, value):
self.data[key] = value
def get_time(self, *a):
return None
def set_time(self, *a):
pass
class _Pub:
enabled = False
@@ -802,7 +810,7 @@ class _Hass:
return True
def built(db_path, opts=None):
def built(db_path, opts=None, store=None):
hass = _Hass()
c = Controller(dict(
{"setpoint_entity": "number.sp", "meter_source": "ha_signed",
@@ -810,7 +818,8 @@ def built(db_path, opts=None):
"max_w": LIVE_MAX_W, "gain": 0.6, "slew_w": 1000, "deadband_w": 15,
"target_grid_w": 0, "step_w": 10, "heartbeat_s": 10,
"stale_input_s": 15}, **(opts or {})),
hass, _Store(), _Pub(), stop=asyncio.Event(), peak_db_path=str(db_path))
hass, store or _Store(), _Pub(), stop=asyncio.Event(),
peak_db_path=str(db_path))
c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W
return c, hass
@@ -862,7 +871,8 @@ check("...and the status page says NOT COVERED rather than 'idle' or a green "
# block with no seconds remaining - so the whole setup is retried on a fresh
# Controller rather than left to flake. It is bounded at three attempts: a fourth
# would mean something is wrong that is not a boundary.
def wired_over_limit(db, want_floor_w, opts=None):
def wired_over_limit(db, want_floor_w, opts=None, store=None, batt_w=0.0,
standing_w=None):
"""A real Controller driven one real cycle over a real over-limit quarter.
⚠️ THE LOAD IS DERIVED FROM WHERE IN THE QUARTER THE WALL CLOCK LANDS, and
@@ -874,9 +884,14 @@ def wired_over_limit(db, want_floor_w, opts=None):
computed backwards from `remaining` at the moment of the run.
"""
for attempt in range(4):
ctl, hs = built(db if attempt == 0 else Path(f"{db}.{attempt}"), opts)
ctl.auto, ctl.soc, ctl.batt = True, 55.0, 0.0
ctl, hs = built(db if attempt == 0 else Path(f"{db}.{attempt}"), opts,
store=_Store(store) if store is not None else None)
ctl.auto, ctl.soc, ctl.batt = True, 55.0, batt_w
ctl.dev_min, ctl.dev_max = -LIVE_MAX_W, LIVE_MAX_W
if standing_w is not None:
# The command STANDING ON THE WIRE, which is what B3 below needs to
# differ from the measured battery power.
ctl.target = ctl.last_write = standing_w
now = datetime.now(timezone.utc)
blk = datetime.fromtimestamp(
int(now.timestamp() // QUARTER_S) * QUARTER_S, timezone.utc)
@@ -951,6 +966,184 @@ check("...and the command is still bounded by the rating, so a saturated guard "
"never asks the hardware for more than it has",
abs(hass2.writes[-1]) <= LIVE_MAX_W + 1e-6)
# ==============================================================================
# THE WIRING'S OWN MUTATIONS. Everything above this line grades `peakguard.py`,
# and a reviewer showed that is not the same thing as grading the guard: THREE
# one-line changes to main.py's CALL SITE left all twelve suites green while a
# default install parked at 0 W in the quarter that sets the month's bill.
#
# So these three are executed, not described. Each mutation is put in place on
# the SHIPPED module and the REAL `Controller.cycle()` is driven with it - which
# also proves main.py routes through the thing being mutated rather than
# inlining it, because a monkeypatch of an inlined expression would do nothing
# and these checks would pass vacuously.
print()
print(" the wiring's own mutations - a default install, maintenance charging")
# A DEFAULT INSTALL: `peak_forecast_entity` is absent, so `peak_fc` is None and
# `peak_at_risk()` answers False forever. That is the whole point - it is the
# configuration on which the old suspension could not fire.
MAINT_CHARGING = {"phase": "charge"}
_mainsrc = (HERE / "app" / "main.py").read_text(encoding="utf-8")
def maint_cycle(tag, want_floor_w=LIVE_MAX_W * 0.3, **kw):
"""One real cycle with maintenance in CHARGE and no forecast entity."""
return wired_over_limit(TMP / tag, want_floor_w, store=MAINT_CHARGING, **kw)
base, base_hass, BASE_NET, BASE_REM = maint_cycle("mut-base.db")
res0 = base.maint.tick(datetime.now(timezone.utc), base.soc)
print(f" maintenance phase {res0.phase!r}: owns_setpoint={res0.owns_setpoint}, "
f"charge_only={res0.charge_only}; peak_forecast_entity absent so "
f"peak_fc={base.peak_fc}")
check("the fixture really is the dangerous one: maintenance in its CHARGE phase "
"(a P_MAINT_SHAPE `hi <= 0` limit) on an install with no "
"peak_forecast_entity, and the guard genuinely engaged on the measurement",
res0.charge_only and not res0.owns_setpoint and base.peak_fc is None
and base.peak_guard is not None and base.peak_guard.engaged
and base.peak_guard.blind is None)
print(f" SHIPPED: suspended="
f"{peakguard.shaping_suspended(base.peak_guard, base.peak_fc, CAP_W)}, "
f"{base.reason}, target {base.target:+.1f} W")
check("SHIPPED: the shaping stands down, the arbiter does not contradict, and "
"the battery discharges into the peak",
peakguard.shaping_suspended(base.peak_guard, base.peak_fc, CAP_W)
and "CONTRADICTION" not in base.reason
and near(base.target, base.peak_guard.floor_w, 1.5)
and base.target > 0.0)
# ---- B1: the suppression removed altogether --------------------------------
_shipped_pred = peakguard.shaping_suspended
try:
peakguard.shaping_suspended = lambda d, fc, cap: False
b1, b1_hass, _, _ = maint_cycle("mut-b1.db")
finally:
peakguard.shaping_suspended = _shipped_pred
print(f" B1 (suppression removed): {b1.reason}, target {b1.target:+.1f} W")
check("B1: with the suppression gone the two LIMITS cross - `lo >= 0` from the "
"peak guard against `hi <= 0` from the shaping - and the arbiter answers "
"CONTRADICTION and 0 W. The battery is PARKED in the billed quarter, and "
"P_PEAK 50 > P_MAINT_SHAPE 40 does not save it",
"CONTRADICTION" in b1.reason and near(b1.target, 0.0)
and b1.peak_guard.engaged)
# ---- B2: re-gated on peak_forecast_entity, i.e. the historical bug ----------
# WARNING - THIS IS THE ONE THAT MUST NOT SHIP UNDEFENDED. It is verbatim the
# defect this card's own comment describes, and until this check existed the
# comment explained a fix that nothing could regress. That is the same shape as
# a corrected comment nobody tests, which this project has already been caught
# by once.
try:
peakguard.shaping_suspended = lambda d, fc, cap: peak_at_risk(fc, float(cap))
b2, b2_hass, _, _ = maint_cycle("mut-b2.db")
finally:
peakguard.shaping_suspended = _shipped_pred
print(f" B2 (re-gated on peak_forecast_entity): "
f"peak_at_risk({b2.peak_fc}, {CAP_W:.0f}) = "
f"{peak_at_risk(b2.peak_fc, CAP_W)}, {b2.reason}, "
f"target {b2.target:+.1f} W")
check("B2: gating the suspension on peak_forecast_entity ALONE parks a default "
"install at 0 W. That entity is optional and empty there, so peak_at_risk "
"is False forever and the shaping is filed against the guard's floor - "
"silently, in the quarter that sets the month's bill",
not peak_at_risk(b2.peak_fc, CAP_W)
and "CONTRADICTION" in b2.reason and near(b2.target, 0.0))
check("...and the shipped predicate differs from that mutation on exactly this "
"input, which is what makes the measured term load-bearing rather than "
"decorative",
_shipped_pred(base.peak_guard, None, CAP_W) is True
and peak_at_risk(None, CAP_W) is False)
check("...while the forecast term is not redundant either: on a site that DOES "
"set peak_forecast_entity, a forecast over the cap suspends the shaping "
"even before the measured guard engages",
_shipped_pred(None, CAP_W + 1000.0, CAP_W) is True
and _shipped_pred(None, None, CAP_W) is False)
# ---- B3: the floor sized from intent rather than from delivered power -------
# `decide()` is graded above with a hand-passed `batt_w`; nothing graded WHICH
# value main.py hands it. Recorded off the real call, with the measured battery
# and the standing command deliberately far apart.
STANDING = 3000.0
_seen = {}
_shipped_decide = peakguard.decide
def _recording_decide(projection, **kw):
_seen.update(kw)
return _shipped_decide(projection, **kw)
try:
peakguard.decide = _recording_decide
b3, b3_hass, _, _ = maint_cycle("mut-b3.db", batt_w=0.0, standing_w=STANDING)
finally:
peakguard.decide = _shipped_decide
print(f" B3: measured battery {b3.batt:+.0f} W, command standing on the wire "
f"{STANDING:+.0f} W; main.py handed decide() batt_w={_seen.get('batt_w')}")
check("B3: main.py sizes the floor from the MEASURED battery power, not from "
"the command. The utility bills what FLOWED, and `grid = load - batt` is "
"true of delivered power and false of intent",
near(_seen.get("batt_w"), b3.batt)
and not near(_seen.get("batt_w"), STANDING))
# THE CONSEQUENCE, ITERATED. One cycle of the intent form is merely inflated by
# the standing command; the failure is what happens on the NEXT cycle, when its
# own unmet ask has become the standing command. So both forms are run forward
# against the same frozen projection - the inverter never follows, so the meter
# never moves - and the two are allowed to separate over time rather than being
# judged at a single point where they need not differ.
_proj3 = PeakTracker.project(b3.p1.averager, datetime.now(timezone.utc))
_kw = dict(cap_w=CAP_W, reading=b3.peak.peak(), achievable_w=LIVE_MAX_W,
max_hold_s=b3.p1.averager.max_hold_s)
_intent = _shipped_decide(_proj3, batt_w=STANDING, **_kw)
check("one cycle of the intent form is exactly the delivered form plus the "
"command standing on the wire - which is the whole defect in one line: "
"watts that were asked for and never arrived, counted as if they had",
near(_intent.floor_w - b3.peak_guard.floor_w, STANDING, 1.5))
_i_cmd, _d_batt = STANDING, 0.0
_i_track, _d_track, _i_sat_at = [], [], None
for _k in range(1, 9):
_di = _shipped_decide(_proj3, batt_w=_i_cmd, **_kw)
_dd = _shipped_decide(_proj3, batt_w=_d_batt, **_kw)
_i_track.append(_di.floor_w)
_d_track.append(_dd.floor_w)
if _di.saturated and _i_sat_at is None:
_i_sat_at = _k
_i_cmd = _di.floor_w # the intent form's ask becomes what stands
# ...while the inverter delivers nothing, so the delivered form's input
# does not move at all.
print(f" ...iterated against a frozen meter and an inverter that never "
f"follows:")
print(f" intent-sized floor {[f'{v:.0f}' for v in _i_track]}"
f" -> SATURATED at cycle {_i_sat_at}")
print(f" delivered-power floor {[f'{v:.0f}' for v in _d_track]} -> stationary")
check("...and the consequence is the wind-up part 1 demonstrates, now on the "
"REAL controller's own projection: the intent-sized floor re-adds its own "
"unmet ask every cycle and reaches the rail SATURATED, while the "
"delivered-power floor does not move at all",
_i_sat_at is not None and _i_track[-1] > _i_track[0]
and len({f"{v:.3f}" for v in _d_track}) == 1
and not b3.peak_guard.saturated)
# Graded on the parsed call syntax too, so a rename cannot quietly reintroduce
# it: the keyword main.py passes must be the measured attribute.
_call = next(n for n in ast.walk(ast.parse(_mainsrc))
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute)
and n.func.attr == "decide")
_batt_src = ast.unparse(next(k.value for k in _call.keywords if k.arg == "batt_w"))
print(f" main.py's decide(batt_w=...) is `{_batt_src}`")
check("...and the call site says so in the source: the argument is the measured "
"battery attribute, not the standing command and not the resolved target",
_batt_src == "self.batt")
# SAFETY-24 must be pointed at from the module that needs it, not only from a
# review thread - a ticket nobody can find from the code is a ticket nobody
# reads before shipping this to a client install.
check("peakguard.py names SAFETY-24, the missing SoC backstop on the forced "
"discharge this guard can command", "SAFETY-24" in src)
# ==============================================================================
# CFG-03: no fourth defaults site.
cfg = (HERE / "config.yaml").read_text(encoding="utf-8")
val = (HERE / "app" / "validate.py").read_text(encoding="utf-8")
@@ -977,7 +1170,6 @@ check("peakguard.py reads no option of its own: it makes no `.get()` call at "
# test_validate.py's CFG-03 block spells out at length: the two diverging is a
# live defect in this repo for `maintenance_charge_w`, and it is not to be
# repeated on the key this guard's ceiling comes from.
_mainsrc = (HERE / "app" / "main.py").read_text(encoding="utf-8")
_fallbacks = {float(m) for m in
re.findall(r'peak_cap_w"\s*,\s*([0-9]+(?:\.[0-9]+)?)', _mainsrc)}
print(f" peak_cap_w: config.yaml ships {CAP_W:.0f}, main.py falls back to "