From c34b5a29226210cbe790ec0ff3cafbe0e0279459 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 30 Aug 2026 15:50:08 +0200 Subject: [PATCH 1/4] TEST-06: close the three gaps that survived SAFETY-18's suite green None of the three is a bug in the shipped code. They are gaps in what the suite could detect, and the next person to touch freeze.py had less protection than the passing count implied. 1. WHERE check_meter_frozen() runs in cycle(). Moving it to the top left the whole file green. Pinned by CONSEQUENCE, not by source order, so a refactor that keeps the semantics stays green: - the alarm must land on the SAME cycle the command reaches the rail (at the top it grades the previous command against this cycle's meter and arrives a cycle late); - the dead-poll-task composition: poll task dies, stale_input_s is crossed, the arbiter commands 0 W, and on THAT cycle the alarm is already clear - because the detector ran after the 0 W write. wired() now returns a per-cycle trace, which is what makes "on which cycle" an observation rather than a hope. 2. The NOT COVERED status row. The old check only asserted that an uncovered transport is SILENT, which a page claiming `armed` on a guard that cannot run also satisfies. The string is now asserted in both directions: the uncovered row must say NOT COVERED, name the meter_source and homewizard_local, and warn; the covered row must still say armed, or "does not say armed" would be satisfied by deleting the armed branch. 3. meter_freeze_s. Pinned in test_validate.py by MAINT-04's regex technique (no PyYAML in the image), and in test_freeze.py by its consequences: the shipped value drives the replays, must stay inside the 900 s billing quarter, and 3600/hold_s must stay inside the shipped deadband. A Controller built with the key ABSENT must land on the same number, which pins main.py's fallback without touching CFG-03. All three demonstrated red before being accepted green, and every new check was mutated to confirm it can fail; one candidate assertion ("no new notification while parked at 0 W") was DELETED because no mutation could break it, and replaced by the exact notification count across a revived transport, which can. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC --- goodwe_controller/test_freeze.py | 348 ++++++++++++++++++++++++++--- goodwe_controller/test_validate.py | 55 +++++ 2 files changed, 377 insertions(+), 26 deletions(-) diff --git a/goodwe_controller/test_freeze.py b/goodwe_controller/test_freeze.py index 09f0f81..89838fa 100644 --- a/goodwe_controller/test_freeze.py +++ b/goodwe_controller/test_freeze.py @@ -2,7 +2,10 @@ python3 test_freeze.py -Three parts, and the last two are the ones that matter: +Six parts. Parts 2 and 3 are the acceptance argument; parts 5 and 6 exist +because TEST-06 found three mutations that survived parts 1-4 green - the +evaluation POSITION inside cycle(), the NOT COVERED status string, and the +shipped `meter_freeze_s` itself: 1. the predicate's boundaries and the edge-trigger, table-driven; 2. SILENT ON REAL CONVERGED DATA. `scenarios/p1-age-2026-08-26.jsonl.gz` is @@ -18,6 +21,14 @@ Three parts, and the last two are the ones that matter: repo is next to this one (or `$GOODWE_SIM`), and an equivalent local stub otherwise. Both are exercised; the run says which one it used. + 4. THE WIRING: the real `Controller.cycle()`, asserted on the service call + that leaves the process; + 5. WHERE IN `cycle()` the detector is evaluated - by consequence, not by + source order. It runs after `write_setpoint()`, and that is what makes two + compositions safe (see the section for both); + 6. WHAT THE STATUS PAGE SAYS about an uncovered transport, and the shipped + threshold read back out of `config.yaml`. + ⚠️ Part 3 runs in WALL CLOCK - `unchanged_s` is `time.monotonic()` and the transport really polls - so it drives the detector at `hold_s=10`, not the shipped 300. The THRESHOLD is config; the MECHANISM is what is under test, and @@ -57,6 +68,7 @@ import asyncio import gzip import json import os +import re import subprocess import sys import time @@ -79,6 +91,29 @@ HERE = Path(__file__).resolve().parent # The live install's numbers, which are the ones the exposure is written at. LIVE_MAX_W = 5000.0 +# --------------------------------------------------------------------------- # +# THE SHIPPED THRESHOLD, READ FROM config.yaml (TEST-06 / AC3) +# --------------------------------------------------------------------------- # +# ⚠️ `meter_freeze_s: 300` used to appear in this file only as the literal 300, +# retyped six times, so `300 -> 3600` in config.yaml left every check here +# green. Everything below is now driven off the SHIPPED value, and the value +# itself is pinned in test_validate.py the way MAINT-04 pins the other defaults. +# +# Regex, not PyYAML: the add-on image installs py3-aiohttp/py3-paho-mqtt only, +# and a check that skips on ImportError is not a check. +_CFG_TXT = (HERE / "config.yaml").read_text(encoding="utf-8") + + +def _cfg_default(key): + """The single ` key: ` default line in config.yaml's options block.""" + found = re.findall(rf"^ {key}: (-?\d+(?:\.\d+)?)\s*$", _CFG_TXT, re.M) + return float(found[0]) if len(found) == 1 else None + + +SHIPPED_HOLD_S = _cfg_default("meter_freeze_s") +SHIPPED_DEADBAND_W = _cfg_default("deadband_w") +SHIPPED_MAX_W = _cfg_default("max_w") + def check(name, cond): print(f" {'ok ' if cond else 'FAIL'} {name}") @@ -135,6 +170,35 @@ check("the reason names both terms", "unchanged over 300" in wd.reason and "command over 4750" in wd.reason) +# --------------------------------------------------------------------------- # +# 1b. the SHIPPED threshold, and the two properties it has to have (AC3) +# --------------------------------------------------------------------------- # +# ⚠️ This is the number the whole ticket is an argument about, and it was pinned +# nowhere: `meter_freeze_s: 300 -> 3600` used to leave this file and +# test_validate.py both green. test_validate.py pins the literal in MAINT-04's +# manner; here it is pinned by its CONSEQUENCES, so a schema-legal value that +# gives up the argument goes red even though it is a legal value. +print("the shipped meter_freeze_s (AC3)") +check(f"config.yaml's numeric defaults were read (freeze {SHIPPED_HOLD_S}, " + f"deadband {SHIPPED_DEADBAND_W}, max_w {SHIPPED_MAX_W})", + None not in (SHIPPED_HOLD_S, SHIPPED_DEADBAND_W, SHIPPED_MAX_W)) +check("the code default and the shipped default are the same number - " + "FreezeWatchdogConfig() is a default, config.yaml is the ship", + FreezeWatchdogConfig().hold_s == SHIPPED_HOLD_S) +check("this file's `tuning()` really is config.yaml's deadband, so the " + "mutual-exclusion arithmetic below is about the shipped law", + tuning().deadband_w == SHIPPED_DEADBAND_W) + +# The upper bound is money: config.yaml's own comment says 300 s is "still +# inside the 900 s quarter that bills the capacity tariff". A hold that outlives +# the quarter notifies after the peak is already billed. +QUARTER_S = 900.0 +check(f"the alarm still lands inside the {QUARTER_S:.0f} s billing quarter " + f"({SHIPPED_HOLD_S:.0f} s hold)", SHIPPED_HOLD_S < QUARTER_S) +# The lower bound is the conjunction itself - see the derivation below, which +# now runs off the shipped numbers rather than off two retyped literals. + + # --------------------------------------------------------------------------- # # 2. silent on 40 minutes of real converged behaviour # --------------------------------------------------------------------------- # @@ -183,10 +247,10 @@ def replay(hold_s, max_w, cycle_floor_s=15.0): for mw in (2000.0, LIVE_MAX_W): - wd_r, peak_cmd, peak_unchanged, fired = replay(300.0, mw) + wd_r, peak_cmd, peak_unchanged, fired = replay(SHIPPED_HOLD_S, mw) print(f" max_w {mw:.0f}: worst |command| {peak_cmd:.0f} W (rail " f"{rail_w(mw):.0f} W), worst unchanged {peak_unchanged:.1f} s " - f"(hold 300 s), episodes {wd_r.episodes}") + f"(hold {SHIPPED_HOLD_S:.0f} s), episodes {wd_r.episodes}") check(f"max_w {mw:.0f}: never alarms on the converged capture", fired is None) check(f"max_w {mw:.0f}: and the rail term had margin to spare", peak_cmd < rail_w(mw)) @@ -194,7 +258,7 @@ for mw in (2000.0, LIVE_MAX_W): # The margin on the OTHER term, as numbers rather than as trust. Both figures # are quoted in config.yaml and freeze.py, so they are re-measured every run and # a capture swapped underneath them cannot leave the prose saying the old thing. -_, _, worst_unchanged, _ = replay(300.0, LIVE_MAX_W) +_, _, worst_unchanged, _ = replay(SHIPPED_HOLD_S, LIVE_MAX_W) gaps, last, prev = [], rows[0]["t"], None for r in rows: if prev is None or r["pwr"] != prev: @@ -208,7 +272,8 @@ check("worst observed unchanged_s is the 8 s the docs quote", 7.5 < worst_unchanged < 8.5) check("longest distinct-reading interval is the 15.4 s the docs quote", 15.0 < max(gaps) < 16.0) -check("300 s is at least 15x the worst of the two", 300.0 >= 15 * max(gaps)) +check(f"the shipped {SHIPPED_HOLD_S:.0f} s is at least 15x the worst of the two", + SHIPPED_HOLD_S >= 15 * max(gaps)) # --------------------------------------------------------------------------- # # 2b. TEL-05's case, reconstructed, because the capture cannot contain it @@ -226,22 +291,26 @@ check("300 s is at least 15x the worst of the two", 300.0 >= 15 * max(gaps)) # -14.2 W) rather than invented: one hour of a perfectly healthy converged meter # that happens not to move. print("TEL-05's low-power case - a converged meter that legitimately repeats") -tel05 = FreezeWatchdog(FreezeWatchdogConfig(hold_s=300)) +tel05 = FreezeWatchdog(FreezeWatchdogConfig(hold_s=SHIPPED_HOLD_S)) t5 = tuning(LIVE_MAX_W) tgt, iw, sc = 0.0, 0.0, 0 bare_would_fire = False tel05_cmd = 0.0 -for k in range(3600): +# Long enough that the REJECTED bare threshold has to get its chance even if +# someone raises meter_freeze_s - the mutation must be caught by the pins above, +# not by this case going red for an incidental reason. +TEL05_S = max(3600, int(SHIPPED_HOLD_S) + 300) +for k in range(TEL05_S): if k % 15 == 0: # cycle_floor_s, the only trigger left d = compute(prev_w=tgt, grid_w=-14.2, actual_w=tgt, tuning=t5, sat_count=sc, i_w=iw) tgt, iw, sc = d.target_w, d.i_w, d.sat_count tel05_cmd = max(tel05_cmd, abs(tgt)) # The REJECTED design, written out so the case can be shown to be real. - bare_would_fire = bare_would_fire or float(k) >= 300 + bare_would_fire = bare_would_fire or float(k) >= SHIPPED_HOLD_S tel05.evaluate(float(k), tgt, LIVE_MAX_W, T0 + timedelta(seconds=k)) -print(f" an hour at -14.2 W: worst |command| {tel05_cmd:.0f} W, " - f"unchanged_s reaches 3599 s, episodes {tel05.episodes}") +print(f" {TEL05_S / 60:.0f} min at -14.2 W: worst |command| {tel05_cmd:.0f} W, " + f"unchanged_s reaches {TEL05_S - 1} s, episodes {tel05.episodes}") check("the bare unchanged_s threshold TEL-05 rejected WOULD fire here", bare_would_fire) check("the conjunction does not - this is what the rail term buys", @@ -255,8 +324,11 @@ check("...and the command stayed nowhere near the rail", # figure is inside the deadband, so the command there is parked at 0 W. If # someone lowers meter_freeze_s or narrows the deadband, this is the check that # should stop them. -DEADBAND_W = 15.0 -slowest_healthy_w = 3600.0 / 300.0 +DEADBAND_W = SHIPPED_DEADBAND_W # config.yaml's, not a retyped 15 +slowest_healthy_w = 3600.0 / SHIPPED_HOLD_S +print(f" at the shipped {SHIPPED_HOLD_S:.0f} s hold, the slowest healthy " + f"meter that still looks unchanged is {slowest_healthy_w:.1f} W, against " + f"a {DEADBAND_W:.0f} W deadband") check("a healthy meter slow enough to look unchanged is inside the deadband", slowest_healthy_w < DEADBAND_W) _park = compute(prev_w=0.0, grid_w=-slowest_healthy_w, actual_w=0.0, @@ -454,11 +526,17 @@ class _Hass: # runaway under test never happens - which would have made this a check # that passes for the wrong reason. self.batt = 0.0 + # ⚠️ The dead-poll-task case (AC1's second half): every input reads + # None, exactly as it does when the transport task has died while the + # controller keeps cycling. + self.dead = False async def limits(self, entity): return (-5000.0, 5000.0, 1.0) async def number(self, entity, invert=False): + if self.dead: + return None return {"sensor.grid": self.grid, "sensor.soc": 50.0, "sensor.batt": self.batt}.get(entity) @@ -479,29 +557,54 @@ class _Source: self.unchanged_s = unchanged_s +# ⚠️ meter_freeze_s is deliberately NOT in here. A controller built with the key +# absent must land on the shipped number by itself, and that is asserted below - +# supplying it here would test this dict instead of the shipped default. +STALE_INPUT_S = 15.0 + + +def _opts(**over): + o = {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off", "max_w": LIVE_MAX_W, "gain": 0.6, + "slew_w": 1000, "deadband_w": SHIPPED_DEADBAND_W, + "target_grid_w": -10, "step_w": 10, "heartbeat_s": 10, + "stale_input_s": STALE_INPUT_S} + o.update(over) + return o + + +def notes_of(hass): + """The HA persistent notifications that have left the process so far.""" + return [d for dom, svc, d in hass.calls + if (dom, svc) == ("persistent_notification", "create")] + + def wired(unchanged_s, grid, cycles=60, **over): + """Returns (hass, controller, trace). + + `trace` is one row per cycle - (command after the cycle, the watchdog's + level, notifications raised so far) - which is what AC1 needs: the + QUESTION IS NOT WHETHER THE ALARM HAPPENS BUT ON WHICH CYCLE. + """ async def go(): hass = _Hass(grid) - c = Controller(dict( - {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", - "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", - "meter_source": "off", "max_w": LIVE_MAX_W, "gain": 0.6, - "slew_w": 1000, "deadband_w": 15, "target_grid_w": -10, - "step_w": 10, "heartbeat_s": 10, "stale_input_s": 15}, - **over), hass, _Store(), _Pub(), stop=asyncio.Event()) + c = Controller(_opts(**over), hass, _Store(), _Pub(), + stop=asyncio.Event()) c.auto = True c.dev_min, c.dev_max = -5000.0, 5000.0 c.p1_source = _Source(unchanged_s) + trace = [] for _ in range(cycles): await c.read_inputs() await c.cycle() - return hass, c + trace.append((c.target, c.freeze_wd.active, len(notes_of(hass)))) + return hass, c, trace return asyncio.run(go()) -hass_f, ctl_f = wired(3600.0, 4000.0) -notes = [d for dom, svc, d in hass_f.calls - if (dom, svc) == ("persistent_notification", "create")] +hass_f, ctl_f, trace_f = wired(3600.0, 4000.0) +notes = notes_of(hass_f) # The status row lives behind `p1_enabled`, which is what a homewizard_local # install has. Flipped here rather than plumbing a whole P1 transport in - the # row's own content is what is under test. @@ -522,16 +625,209 @@ check("the event log names it", any("METER FROZEN" in e for e in ctl_f.events)) check("and it does NOT command 0 W - this cut is an alarm only", abs(ctl_f.target) >= rail_w(LIVE_MAX_W)) -hass_n, ctl_n = wired(3600.0, -14.2) +hass_n, ctl_n, _ = wired(3600.0, -14.2) check("wired up, TEL-05's converged case still raises nothing", not [1 for dom, svc, _ in hass_n.calls if svc == "create"]) -hass_m, ctl_m = wired(2.0, 8000.0) +hass_m, ctl_m, _ = wired(2.0, 8000.0) check("wired up, a real railed load against a moving meter raises nothing", not [1 for dom, svc, _ in hass_m.calls if svc == "create"] and abs(ctl_m.target) >= rail_w(LIVE_MAX_W)) -check("an uncovered transport (no unchanged_s) raises nothing, and says so", +check("an uncovered transport (no unchanged_s) raises no notification", not [1 for dom, svc, _ in wired(None, 4000.0)[0].calls if svc == "create"]) + +# --------------------------------------------------------------------------- # +# 5. WHERE in cycle() the detector is evaluated (TEST-06 / AC1) +# --------------------------------------------------------------------------- # +# ⚠️ WHY THIS IS NOT STYLE. `check_meter_frozen()` runs LAST, after +# `write_setpoint()`, and moving it to the top of `cycle()` used to leave this +# whole file green. It is not a tidiness preference, it is the reason two +# separate compositions are safe, and each is asserted here by an OBSERVABLE +# CONSEQUENCE rather than by the source order - so a refactor that keeps the +# semantics (renaming it, inlining it, calling it from a helper that runs at the +# end) stays green, and one that changes WHICH command it judges goes red: +# +# (a) BOTH TERMS MUST DESCRIBE THE SAME MOMENT. The command it grades has to +# be the one that just went on the wire, judged against how long the meter +# that chose it has stood still. Evaluated at the top it grades the +# PREVIOUS cycle's command against THIS cycle's meter - a pairing that was +# never true of any moment - and the alarm arrives a whole cycle late. +# (b) THE DEAD-POLL-TASK COMPOSITION. When the poll task dies the inputs go +# stale, the arbiter commands 0 W, and the detector - evaluated after that +# write - correctly sees an unrailed command and CLEARS. At the top of the +# cycle it would still be looking at the railed command from before the +# failsafe, and would keep the alarm standing over a controller that is +# already commanding 0 W. +print("WHERE the detector is evaluated in cycle() - the ordering AC1 protects") + +_railed = [i for i, (t, _, _) in enumerate(trace_f) + if abs(t) >= rail_w(LIVE_MAX_W)] +_alarmed = [i for i, (_, _, n) in enumerate(trace_f) if n >= 1] +_first_railed = _railed[0] if _railed else None +_first_alarm = _alarmed[0] if _alarmed else None +print(f" ramp to the rail: first railed command on cycle {_first_railed}, " + f"first notification on cycle {_first_alarm}, over {len(trace_f)} cycles") +# Without this the equality below could be satisfied on cycle 0, where "the same +# cycle" and "one cycle later" are indistinguishable. +check("the command takes several cycles to reach the rail, so 'same cycle' and " + "'one cycle later' are different observations", + _first_railed is not None and _first_railed >= 1 + and len(trace_f) > _first_railed + 1) +check("the alarm lands on the SAME cycle the command reaches the rail - " + "evaluated after write_setpoint() it grades the command now on the wire; " + "moved to the top of cycle() it would grade the previous one and arrive a " + "cycle late", + _first_alarm is not None and _first_alarm == _first_railed) + + +def dead_poll(after_cycles=20, dead_cycles=4, revived_cycles=20): + """Alarm on a railed command, then kill the poll task and keep cycling. + + The real thing: the transport task dies, every input reads None, and after + `stale_input_s` the arbiter's P_SAFETY stop commands 0 W. `inputs_bad_since` + is backdated rather than slept through - the clock is wall clock, and the + mechanism rather than the wait is what is under test. + """ + async def go(): + hass = _Hass(4000.0) + c = Controller(_opts(), hass, _Store(), _Pub(), stop=asyncio.Event()) + c.auto = True + c.dev_min, c.dev_max = -5000.0, 5000.0 + c.p1_source = _Source(3600.0) + for _ in range(after_cycles): + await c.read_inputs() + await c.cycle() + before = (c.target, c.freeze_wd.active, len(notes_of(hass))) + hass.dead = True + trace = [] + for _ in range(dead_cycles): + await c.read_inputs() + if c.inputs_bad_since is not None: + c.inputs_bad_since = (datetime.now(timezone.utc) + - timedelta(seconds=STALE_INPUT_S + 1)) + await c.cycle() + trace.append((c.target, c.freeze_wd.active, len(notes_of(hass)))) + # ...and then the transport comes back, against a meter that is still + # frozen. The loop re-rails and this is a NEW episode - the alarm is + # allowed, indeed required, to speak again. + hass.dead = False + revived = [] + for _ in range(revived_cycles): + await c.read_inputs() + await c.cycle() + revived.append((c.target, c.freeze_wd.active, len(notes_of(hass)))) + return before, trace, revived, notes_of(hass), c + return asyncio.run(go()) + + +_before, _dead, _revived, _all_notes, _ctl_d = dead_poll() +_zeroed = [i for i, (t, _, _) in enumerate(_dead) if t == 0.0] +print(f" poll task killed: alarming at {_before[0]:+.0f} W " + f"(active={_before[1]}, {_before[2]} notification(s)) -> " + + ", ".join(f"cycle {i}: {t:+.0f} W active={a} notes={n}" + for i, (t, a, n) in enumerate(_dead))) +check("the episode really was standing before the poll task died - otherwise " + "the release below is vacuous", + _before[1] and _before[2] == 1 and abs(_before[0]) >= rail_w(LIVE_MAX_W)) +check("a dead poll task drives the command to 0 W", + bool(_zeroed) and _zeroed[0] == 0) +check("...and on THAT SAME cycle the alarm is already clear, because the " + "detector ran after the 0 W write and not before it", + bool(_zeroed) and not _dead[_zeroed[0]][1]) +check("...and it stays clear for as long as the failsafe holds it there", + _dead and not any(a for _, a, _ in _dead)) +# ⚠️ "and it raises nothing new while parked" is NOT asserted here, and that is +# deliberate: with the alarm cleared there is no mutation of this code that +# could make a notification appear during those cycles, so the assertion could +# not fail and would have been decoration. The real claim - that the outage did +# not add notifications - is carried by the exact count below, which CAN fail. + +# ⚠️ AND THE OTHER HALF, or "no second notification" is a claim no mutation can +# break: when the transport comes back against a meter that is STILL frozen the +# loop re-rails, and that is a new episode which MUST speak. What must not +# happen is a second notification stacking up beside the first. +_re_railed = [i for i, (t, _, _) in enumerate(_revived) + if abs(t) >= rail_w(LIVE_MAX_W)] +print(f" transport revived: command re-rails on cycle " + f"{_re_railed[0] if _re_railed else None}, " + f"{_ctl_d.freeze_wd.episodes} episode(s), {len(_all_notes)} notification(s), " + f"ids {sorted({n.get('notification_id') for n in _all_notes})}") +check("a revived transport against a still-frozen meter re-rails the command", + bool(_re_railed)) +check("...which is a SECOND episode, and it alarms again on the cycle it rails", + _ctl_d.freeze_wd.episodes == 2 + and bool(_re_railed) and _revived[_re_railed[0]][2] == _before[2] + 1) +check("...on the same notification_id, so the second REPLACES the first " + "instead of stacking a second one beside it", + len(_all_notes) == 2 + and len({n.get("notification_id") for n in _all_notes}) == 1) + + +# --------------------------------------------------------------------------- # +# 6. what the status page SAYS about coverage (TEST-06 / AC2) +# --------------------------------------------------------------------------- # +# ⚠️ The check above only ever asserted that an uncovered transport is SILENT, +# which a status page claiming `armed` on a guard that cannot run also satisfies. +# The AC was that the detector reports uncovered transports *and says so*, so +# the string is what gets asserted here, in both directions: the uncovered row +# must not claim `armed`, and the covered row must still claim it - or "does not +# say armed" would be satisfiable by deleting the armed branch outright. +print("what the status page SAYS about coverage") + + +def freeze_rows(unchanged_s, grid, **over): + """The freeze-detector rows of the real checks(), on a p1 install.""" + _h, ctl, _t = wired(unchanged_s, grid, cycles=3, **over) + # p1_enabled is what a homewizard_local install has; the row's own content + # is what is under test, not the transport that feeds it. + ctl.p1_enabled = True + return ctl, [r for r in ctl.checks() + if r["text"].startswith("freeze detector")] + + +_ctl_u, _rows_u = freeze_rows(None, 4000.0, meter_source="ha_dsmr") +_ctl_c, _rows_c = freeze_rows(2.0, -14.2) +print(f" uncovered: {_rows_u[0]['text'] if _rows_u else '(no row at all)'}") +print(f" covered: {_rows_c[0]['text'] if _rows_c else '(no row at all)'}") +check("an uncovered transport gets exactly one freeze-detector row", + len(_rows_u) == 1) +check("...and it says NOT COVERED", + bool(_rows_u) and "NOT COVERED" in _rows_u[0]["text"]) +check("...and it does NOT claim the guard is armed - that would be the status " + "page lying on behalf of a guard that cannot run", + bool(_rows_u) and "armed" not in _rows_u[0]["text"]) +check("...and it names the meter_source it is not covering, so the operator " + "knows which install this is", + bool(_rows_u) and "ha_dsmr" in _rows_u[0]["text"]) +check("...and it names homewizard_local as the transport that would be covered", + bool(_rows_u) and "homewizard_local" in _rows_u[0]["text"]) +check("...flagged as a warning rather than a silent ok", + bool(_rows_u) and not _rows_u[0]["ok"] and _rows_u[0]["warn"]) + +check("a COVERED transport gets exactly one freeze-detector row", + len(_rows_c) == 1) +check("...and that one DOES say armed - without this, 'the uncovered row does " + "not say armed' is satisfied by deleting the armed branch", + bool(_rows_c) and "armed" in _rows_c[0]["text"] + and "NOT COVERED" not in _rows_c[0]["text"]) +check("...and it quotes the hold and the rail it is armed at, derived from the " + "config the controller was actually built with", + bool(_rows_c) + and f"{_ctl_c.freeze_wd.cfg.hold_s:.0f} s unchanged" in _rows_c[0]["text"] + and f"{rail_w(LIVE_MAX_W):.0f} W" in _rows_c[0]["text"]) +check("...as an ok row, since a covered armed guard is not a warning", + bool(_rows_c) and _rows_c[0]["ok"] and not _rows_c[0]["warn"]) + +# AC3's other half, through the built object rather than through a literal: the +# opts dict does NOT carry meter_freeze_s, so this is the number an install that +# leaves the key at its config.yaml default actually lands on. +print(f" a Controller built with meter_freeze_s ABSENT holds " + f"{_ctl_c.freeze_wd.cfg.hold_s:.0f} s (config.yaml ships " + f"{SHIPPED_HOLD_S:.0f} s)") +check("a Controller built with the key absent lands on the SHIPPED hold, so " + "config.yaml and main.py's fallback cannot drift apart unnoticed", + _ctl_c.freeze_wd.cfg.hold_s == SHIPPED_HOLD_S) + print() if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}") diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index 23a029e..b7faa3f 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -253,6 +253,61 @@ check("...and the shipped charge rate is at or under the shipped ceiling", check("...and the shipped discharge rate too, so it warns about nothing", _shipped["maintenance_discharge_w"] <= _shipped["max_w"]) +# --------------------------------------------------------------------------- # +print("the shipped meter_freeze_s, pinned (TEST-06 / AC3)") +# ⚠️ WHY A NUMBER GETS ITS OWN CHECK. `meter_freeze_s: 300` is the threshold +# SAFETY-18's entire argument is about, and before this it was pinned NOWHERE: +# 300 -> 3600 left test_freeze.py AND this file green, because the schema +# accepts int(60,3600) and no check ever read the shipped value. The schema +# bounds what is CONFIGURABLE; this pins what is SHIPPED, which is a different +# question and the one the docs answer. +# +# Read by regex like _shipped above, for the same reason: no PyYAML in the +# image, and a check that skips on ImportError is not a check. +_freeze_s = [float(v) for v in re.findall( + r"^ meter_freeze_s: (\d+(?:\.\d+)?)\s*$", _txt, re.M)] +_freeze_schema = re.findall(r"^ meter_freeze_s: int\((\d+),(\d+)\)\s*$", + _txt, re.M) +check(f"config.yaml carries exactly one meter_freeze_s default {_freeze_s}", + len(_freeze_s) == 1) +check("...and exactly one schema range for it", len(_freeze_schema) == 1) +_hold_s = _freeze_s[0] if _freeze_s else None +_lo, _hi = ((float(_freeze_schema[0][0]), float(_freeze_schema[0][1])) + if _freeze_schema else (None, None)) +check("the shipped meter_freeze_s is still the measured 300 s that freeze.py, " + "config.yaml's own comment and test_freeze.py are all written at", + _hold_s == 300.0) +check(f"...and it is inside its own schema range ({_lo}-{_hi} s)", + None not in (_lo, _hi, _hold_s) and _lo <= _hold_s <= _hi) + +# The two properties the number has to have, DERIVED rather than restated - so +# they go red on a value that is merely schema-legal. Both come straight from +# config.yaml's own comment on the key. +# +# upper: the alert must land inside the 900 s quarter that bills the capacity +# tariff, or the notification arrives after the money is spent; +# lower: the slowest a HEALTHY meter can tick a 1 Wh register and still look +# unchanged for hold_s is 3600/hold_s W, and that must stay inside the +# deadband or the conjunction's two terms stop being mutually +# exclusive (freeze.py, "WHERE hold_s COMES FROM"). +_QUARTER_S = 900.0 +_shipped_deadband = [float(v) for v in re.findall( + r"^ deadband_w: (\d+(?:\.\d+)?)\s*$", _txt, re.M)] +check(f"config.yaml carries exactly one deadband_w default {_shipped_deadband}", + len(_shipped_deadband) == 1) +_db = _shipped_deadband[0] if _shipped_deadband else None +_slowest_healthy_w = 3600.0 / _hold_s if _hold_s else None +print(f" shipped meter_freeze_s {_hold_s} s: alert inside the " + f"{_QUARTER_S:.0f} s billing quarter, and a healthy meter slow enough to " + f"look unchanged for it is {_slowest_healthy_w} W against a " + f"{_db} W deadband") +check(f"...the alarm still lands inside the {_QUARTER_S:.0f} s billing quarter", + _hold_s is not None and _hold_s < _QUARTER_S) +check("...and a healthy meter that looks unchanged for it is still inside the " + "deadband, so the two terms stay mutually exclusive", + _slowest_healthy_w is not None and _db is not None + and _slowest_healthy_w < _db) + # --------------------------------------------------------------------------- # print("the KNOWN cross-file default divergence, pinned rather than commented") # ⚠️ WHAT THIS GUARDS, AND WHY IT ASSERTS THE BUG RATHER THAN THE FIX. -- 2.54.0 From 611d3dbcdfc67d46f7f95a221dbd44f5b0446837 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 30 Aug 2026 17:05:28 +0200 Subject: [PATCH 2/4] TEST-06 review follow-ups: two notification surfaces, the real ordering boundary, the schema bounds 1. notes_of() could not tell this guard's alarm from anybody else's. It selected by SERVICE, so against #12's app/ (which adds goodwe_output_clamp) eight assertions went red, AC1's core check among them. Blanket id-keying is not the repair and was measured, not guessed: selector vs #12's app/ id renamed by service (as committed) 8 red 1 red by id alone 3 red 6 red by id or title (shipped) 3 red 1 red all_notes_of() is the whole surface; notes_of() is this guard's, recognised by the id it should carry OR the title it writes - the `or` is what keeps the one assertion whose subject IS the id able to fail on its own. The three surface-wide claims stay surface-wide, each with a comment naming which surface it is about and why the next guard's author must decide rather than narrow them. They are the three that go red against #12, which is the correct signal. 2. The ordering comment named the wrong boundary. N3 - moving the call to immediately before write_setpoint() - is green and correctly so: self.target is assigned two lines earlier, write_setpoint() clamps only a local and can skip the write entirely. So the invariant is "after `self.target = resolution.target_w`", and the detector grades pre-clamp INTENT, not the wire. Corrected in test_freeze.py and in main.py's own note above the call (comment-only; I merge second and own the conflict). N8 - moving the call above the assignment - is RED 4. 3. The schema range's own bounds are now pinned. int(60,3600) -> int(1,86400) survived green: an operator could set meter_freeze_s: 86400 and the guard is dead with nothing red. Both bounds are re-derived rather than restated. 4. find_hwsim() crashed the whole suite on a checkout one level below the root (HERE.parents[1] -> IndexError, reproduced at C:/goodwe_controller). Guarded. And part 3 now names its transport in the summary: the rig hwsim and the stub are not equivalent - the rig path failed 1 run in 3 - so a green log has to say which one backed it. Also: the "revived transport re-rails" check is labelled a PRECONDITION like its sibling; freeze_rows() records that flipping p1_enabled exercises the row's content and never its gate; and the note about the deleted assertion is corrected - it was REDUNDANT, not undecidable. The reviewer falsified it in one line (move the hass.call above `if not fired: return`, RED 9). "I could not construct a falsifier, therefore it cannot fail" was exactly the inference this file exists to distrust. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC --- goodwe_controller/app/main.py | 22 ++- goodwe_controller/test_freeze.py | 234 +++++++++++++++++++++++------ goodwe_controller/test_validate.py | 30 ++++ 3 files changed, 233 insertions(+), 53 deletions(-) diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 7d6f1ab..f4c6b86 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -406,12 +406,22 @@ class Controller: self.reason = resolution.explain() await self.write_setpoint(self.target) - # --- SAFETY-18, and it runs LAST on purpose -------------------------- - # Both terms of the conjunction describe the state that now exists: the - # command that has just gone on the wire, and how long the meter that - # chose it has been standing still. Evaluated before write_setpoint() - # it would be reasoning about the PREVIOUS command against THIS cycle's - # meter - the one combination that is true of neither moment. + # --- SAFETY-18, and it runs AFTER `self.target` is assigned ---------- + # ⚠️ THE BOUNDARY IS THE ASSIGNMENT ABOVE, NOT write_setpoint(). This + # comment used to say "the command that has just gone on the wire", + # which is wrong twice over and was corrected by TEST-06's reviewer: + # `check_meter_frozen()` grades `self.target` - the arbiter's resolved + # intent - and `write_setpoint()` clamps only a local `value` and can + # skip the write entirely, so that number may never have gone anywhere. + # + # What both terms of the conjunction must describe is the SAME moment: + # the command this cycle resolved, against how long the meter that + # chose it has been standing still. Evaluated before + # `self.target = resolution.target_w` it would grade the PREVIOUS + # cycle's command against THIS cycle's meter - the one combination that + # was true of neither moment. Moving it between the assignment and + # write_setpoint() is therefore harmless; moving it above the + # assignment is not, and test_freeze.py part 5 fails on exactly that. await self.check_meter_frozen() async def check_meter_frozen(self) -> None: diff --git a/goodwe_controller/test_freeze.py b/goodwe_controller/test_freeze.py index 89838fa..a6bc24c 100644 --- a/goodwe_controller/test_freeze.py +++ b/goodwe_controller/test_freeze.py @@ -349,11 +349,19 @@ check("...and the same replay DOES alarm once both terms are made reachable", # 3. a genuinely frozen meter, over real HTTP, through the real transport # --------------------------------------------------------------------------- # def find_hwsim(): - """The rig's own fake meter, if the infra repo is reachable.""" + """The rig's own fake meter, if the infra repo is reachable. + + ⚠️ `HERE.parents[1]` only exists when this package sits at least two levels + below the filesystem root - it raised IndexError and took the WHOLE suite + down on a checkout at `C:/goodwe_controller`, which is a failure of the + lookup, not of the thing being tested. Guarded, so a shallow checkout falls + through to the stub like any other missing sibling repo. + """ cands = [] if os.environ.get("GOODWE_SIM"): cands.append(Path(os.environ["GOODWE_SIM"]) / "hwsim.py") - cands.append(HERE.parents[1] / "goodwe" / "sim" / "hwsim.py") + if len(HERE.parents) > 1: + cands.append(HERE.parents[1] / "goodwe" / "sim" / "hwsim.py") for c in cands: if c.exists(): return c @@ -451,7 +459,15 @@ def with_server(argv, port, fn): hw = find_hwsim() -print(f"frozen meter over HTTP - using {'the rig hwsim at ' + str(hw) if hw else 'the local stub'}") +# ⚠️ WHICH TRANSPORT BACKED PART 3 IS PART OF THE RESULT, and it is repeated in +# the summary at the end of the run. The two are not equivalent: pointed at the +# real `sim/hwsim.py` the rig path failed 1 run in 3 (a 16 s window against a +# 10 s hold, when hwsim.serve() is slow to start), while the stub is tuned never +# to fail. A green log that does not say which one ran is a green log that can +# have quietly substituted the weaker stand-in - the same shape as an assertion +# that cannot fail, from the other end. +PART3_TRANSPORT = f"the rig hwsim at {hw}" if hw else "the local stub" +print(f"frozen meter over HTTP - using {PART3_TRANSPORT}") if hw: def argv_for(mode): return [HWSIM_DRIVER, "8791" if mode == "freeze" else "8792", mode, @@ -574,12 +590,65 @@ def _opts(**over): return o -def notes_of(hass): - """The HA persistent notifications that have left the process so far.""" +# ⚠️ TWO SURFACES, AND EVERY ASSERTION BELOW SAYS WHICH ONE IT IS ABOUT. +# The first cut of this helper selected by SERVICE, which measured "any guard's +# notification" while every assertion built on it was named for THIS guard's. +# That was true only because `check_meter_frozen()` was the sole +# `persistent_notification.create` in `app/`; SAFETY-03 (#12) adds a second one +# (`goodwe_output_clamp`) raised by scenarios that rail the command on purpose, +# and eight assertions here went red against it - AC1's core check included. +# +# Blanket id-keying is NOT the repair. Three of those eight are claims about the +# WHOLE notification surface, and narrowing them to this guard's id would have +# silently shrunk a claim rather than fixed a conflation - which is the defect +# class this card exists to close. So both surfaces are available by name, and +# each assertion picks one deliberately. +FREEZE_NOTE_ID = "goodwe_meter_frozen" + + +def all_notes_of(hass): + """EVERY HA persistent notification raised this run, whoever raised it.""" return [d for dom, svc, d in hass.calls if (dom, svc) == ("persistent_notification", "create")] +def _is_freeze_note(d): + """Is this notification the freeze detector's? + + ⚠️ THE ID **OR** THE TITLE, and the `or` is measured rather than tidy. + Keyed on the id alone, renaming the id empties every list in this file and + SIX assertions collapse together - so the one check whose subject IS the id + could no longer fail on its own, in the file about assertions that cannot + fail. Recognising the guard by what it says as well leaves the rename + exactly one red: the id's own check. Measured both ways: + + selector vs #12's app/ id renamed + by service (the first cut) 8 red 1 red + by id alone 3 red 6 red + by id or title (this) 3 red 1 red + + The union cannot re-admit another guard's alarm unless that guard also + calls itself frozen, which would be a confusable alarm on its own account. + """ + return (d.get("notification_id") == FREEZE_NOTE_ID + or "frozen" in str(d.get("title", "")).lower()) + + +def notes_of(hass): + """Only THIS guard's - see _is_freeze_note().""" + return [d for d in all_notes_of(hass) if _is_freeze_note(d)] + + +def notes_by_text_of(hass): + """This guard's, recognised by what it SAYS and never by its id. + + Exists for the two assertions whose subject IS the id: selected by the id, + both would be true by construction. + """ + return [d for d in all_notes_of(hass) + if "frozen" in str(d.get("title", "")).lower()] + + def wired(unchanged_s, grid, cycles=60, **over): """Returns (hass, controller, trace). @@ -604,7 +673,8 @@ def wired(unchanged_s, grid, cycles=60, **over): hass_f, ctl_f, trace_f = wired(3600.0, 4000.0) -notes = notes_of(hass_f) +notes = notes_of(hass_f) # THIS guard's +all_notes = all_notes_of(hass_f) # the whole surface # The status row lives behind `p1_enabled`, which is what a homewizard_local # install has. Flipped here rather than plumbing a whole P1 transport in - the # row's own content is what is under test. @@ -612,9 +682,22 @@ ctl_f.p1_enabled = True rows = [c for c in ctl_f.checks() if "METER MAY BE FROZEN" in c["text"]] print(f" frozen at +4000 W: command ran to {ctl_f.target:.0f} W, " f"{len(notes)} notification(s), {len(rows)} red status row(s)") -check("the shipped cycle() raises exactly one HA notification", len(notes) == 1) +# SURFACE-WIDE, deliberately. "The controller alarmed once" is a different and +# stronger claim than "the freeze guard alarmed once", and it is the one this +# row has always made. When a second guard lands (SAFETY-03's +# `goodwe_output_clamp` is the first), this assertion is where somebody decides +# whether the scenario is still meant to raise exactly one alarm in total - a +# decision, made here, rather than a claim quietly narrowed to keep it green. +check("the whole notification surface carried exactly one alarm this run", + len(all_notes) == 1) +# THIS GUARD'S, keyed on the id. +check("...and the freeze detector is the one that raised it", len(notes) == 1) +# ⚠️ Selected by TEXT, not by id - see notes_by_text_of(). This is the only +# assertion whose subject is the id, so it is the one that must survive being +# the only thing a rename breaks. check("...with a fixed notification_id so repeats replace, not stack", - notes and notes[0].get("notification_id") == "goodwe_meter_frozen") + [d.get("notification_id") for d in notes_by_text_of(hass_f)] + == [FREEZE_NOTE_ID]) check("...and it says the controller is still running", bool(notes) and "STILL RUNNING" in notes[0]["message"]) check("the status page carries a failed row saying so", @@ -625,39 +708,66 @@ check("the event log names it", any("METER FROZEN" in e for e in ctl_f.events)) check("and it does NOT command 0 W - this cut is an alarm only", abs(ctl_f.target) >= rail_w(LIVE_MAX_W)) +# ⚠️ THESE THREE ARE SURFACE-WIDE ON PURPOSE, and they are the three that go red +# against #12's `app/`. That is the correct signal and not a bug in them: each +# says "this scenario is quiet", which is a stronger claim than "this guard is +# quiet", and it is the claim a reader of a converged install wants. Whoever +# lands a second notifying guard has to come here and decide, per scenario, +# whether the new alarm belongs in it - `notes_of()` is one word away for the +# narrower reading, and choosing it must be a decision with a reason. +# (They also used to match `svc == "create"` in ANY domain, which was looser +# still.) hass_n, ctl_n, _ = wired(3600.0, -14.2) -check("wired up, TEL-05's converged case still raises nothing", - not [1 for dom, svc, _ in hass_n.calls if svc == "create"]) +check("wired up, TEL-05's converged case leaves the whole surface silent", + not all_notes_of(hass_n)) hass_m, ctl_m, _ = wired(2.0, 8000.0) -check("wired up, a real railed load against a moving meter raises nothing", - not [1 for dom, svc, _ in hass_m.calls if svc == "create"] - and abs(ctl_m.target) >= rail_w(LIVE_MAX_W)) -check("an uncovered transport (no unchanged_s) raises no notification", - not [1 for dom, svc, _ in wired(None, 4000.0)[0].calls if svc == "create"]) +check("wired up, a real railed load against a moving meter leaves the whole " + "surface silent", + not all_notes_of(hass_m) and abs(ctl_m.target) >= rail_w(LIVE_MAX_W)) +check("an uncovered transport (no unchanged_s) leaves the whole surface silent", + not all_notes_of(wired(None, 4000.0)[0])) # --------------------------------------------------------------------------- # # 5. WHERE in cycle() the detector is evaluated (TEST-06 / AC1) # --------------------------------------------------------------------------- # -# ⚠️ WHY THIS IS NOT STYLE. `check_meter_frozen()` runs LAST, after -# `write_setpoint()`, and moving it to the top of `cycle()` used to leave this -# whole file green. It is not a tidiness preference, it is the reason two -# separate compositions are safe, and each is asserted here by an OBSERVABLE -# CONSEQUENCE rather than by the source order - so a refactor that keeps the -# semantics (renaming it, inlining it, calling it from a helper that runs at the -# end) stays green, and one that changes WHICH command it judges goes red: +# ⚠️ WHERE THE BOUNDARY ACTUALLY IS - AND IT IS NOT write_setpoint(). +# The first version of this comment said the detector runs "after +# write_setpoint()" and grades "the command that has just gone on the wire". +# TEST-06's reviewer disproved both halves by mutation: +# +# N3 move check_meter_frozen() to immediately BEFORE write_setpoint() +# -> green, and CORRECTLY green +# +# because `self.target = resolution.target_w` is assigned two lines above the +# write, `write_setpoint()` clamps only a local `value`, and it can skip the +# write entirely (`if not (changed or force): return`). So what the detector +# grades is `self.target` - the arbiter's resolved intent, PRE device clamp and +# possibly never emitted - and the invariant these checks pin is: +# +# check_meter_frozen() runs AFTER `self.target = resolution.target_w` +# +# That distinction is not pedantry. A check more robust than its own +# explanation is how the next person moves the call ABOVE the assignment +# believing they had preserved the stated rule. +# +# Both properties below are asserted by an OBSERVABLE CONSEQUENCE rather than by +# source order, so semantic-preserving refactors stay green (measured by the +# reviewer: renaming the method, and calling it from a helper that itself runs +# last, both survive - the suite drives `await c.cycle()` and never names the +# method): # # (a) BOTH TERMS MUST DESCRIBE THE SAME MOMENT. The command it grades has to -# be the one that just went on the wire, judged against how long the meter -# that chose it has stood still. Evaluated at the top it grades the +# be the one THIS cycle resolved, judged against how long the meter that +# chose it has stood still. Evaluated above the assignment it grades the # PREVIOUS cycle's command against THIS cycle's meter - a pairing that was # never true of any moment - and the alarm arrives a whole cycle late. # (b) THE DEAD-POLL-TASK COMPOSITION. When the poll task dies the inputs go -# stale, the arbiter commands 0 W, and the detector - evaluated after that -# write - correctly sees an unrailed command and CLEARS. At the top of the -# cycle it would still be looking at the railed command from before the -# failsafe, and would keep the alarm standing over a controller that is -# already commanding 0 W. +# stale, the arbiter resolves 0 W, and the detector - evaluated after that +# resolution lands in self.target - correctly sees an unrailed command and +# CLEARS. Evaluated above the assignment it would still be looking at the +# railed command from before the failsafe, and would keep the alarm +# standing over a controller that is already commanding 0 W. print("WHERE the detector is evaluated in cycle() - the ordering AC1 protects") _railed = [i for i, (t, _, _) in enumerate(trace_f) @@ -674,9 +784,9 @@ check("the command takes several cycles to reach the rail, so 'same cycle' and " _first_railed is not None and _first_railed >= 1 and len(trace_f) > _first_railed + 1) check("the alarm lands on the SAME cycle the command reaches the rail - " - "evaluated after write_setpoint() it grades the command now on the wire; " - "moved to the top of cycle() it would grade the previous one and arrive a " - "cycle late", + "evaluated after `self.target = resolution.target_w` it grades the " + "command THIS cycle resolved; evaluated above that assignment it grades " + "the previous one and arrives a cycle late", _first_alarm is not None and _first_alarm == _first_railed) @@ -684,7 +794,7 @@ def dead_poll(after_cycles=20, dead_cycles=4, revived_cycles=20): """Alarm on a railed command, then kill the poll task and keep cycling. The real thing: the transport task dies, every input reads None, and after - `stale_input_s` the arbiter's P_SAFETY stop commands 0 W. `inputs_bad_since` + `stale_input_s` the arbiter's P_SAFETY stop resolves 0 W. `inputs_bad_since` is backdated rather than slept through - the clock is wall clock, and the mechanism rather than the wait is what is under test. """ @@ -716,11 +826,14 @@ def dead_poll(after_cycles=20, dead_cycles=4, revived_cycles=20): await c.read_inputs() await c.cycle() revived.append((c.target, c.freeze_wd.active, len(notes_of(hass)))) - return before, trace, revived, notes_of(hass), c + # ⚠️ by TEXT: "the second episode REPLACED the first" is a claim about + # the id, so the list it is checked against must not be selected by the + # id - that would make it true by construction. + return before, trace, revived, notes_by_text_of(hass), c return asyncio.run(go()) -_before, _dead, _revived, _all_notes, _ctl_d = dead_poll() +_before, _dead, _revived, _freeze_notes, _ctl_d = dead_poll() _zeroed = [i for i, (t, _, _) in enumerate(_dead) if t == 0.0] print(f" poll task killed: alarming at {_before[0]:+.0f} W " f"(active={_before[1]}, {_before[2]} notification(s)) -> " @@ -732,15 +845,21 @@ check("the episode really was standing before the poll task died - otherwise " check("a dead poll task drives the command to 0 W", bool(_zeroed) and _zeroed[0] == 0) check("...and on THAT SAME cycle the alarm is already clear, because the " - "detector ran after the 0 W write and not before it", + "detector ran after 0 W was resolved into self.target, not before it", bool(_zeroed) and not _dead[_zeroed[0]][1]) check("...and it stays clear for as long as the failsafe holds it there", _dead and not any(a for _, a, _ in _dead)) -# ⚠️ "and it raises nothing new while parked" is NOT asserted here, and that is -# deliberate: with the alarm cleared there is no mutation of this code that -# could make a notification appear during those cycles, so the assertion could -# not fail and would have been decoration. The real claim - that the outage did -# not add notifications - is carried by the exact count below, which CAN fail. +# ⚠️ "and it raises nothing new while parked" is NOT asserted here, and the +# reason matters more than the deletion. It was dropped as REDUNDANT, not as +# undecidable: TEST-06's reviewer falsified it in one line - move the +# `hass.call` above `if not fired: return` so the detector notifies every cycle, +# and notifications appear in exactly these parked cycles (that mutation takes +# this file RED 9). My first note here claimed no mutation could break it, which +# was the very inference this file exists to distrust, made about this file. +# The deletion stands because the exact count below catches that same mutation +# nine times over - no coverage was lost - and the honest form of the claim is +# "I tried to break it and here is what happened", never "I could not, so it +# cannot". # ⚠️ AND THE OTHER HALF, or "no second notification" is a claim no mutation can # break: when the transport comes back against a meter that is STILL frozen the @@ -750,17 +869,28 @@ _re_railed = [i for i, (t, _, _) in enumerate(_revived) if abs(t) >= rail_w(LIVE_MAX_W)] print(f" transport revived: command re-rails on cycle " f"{_re_railed[0] if _re_railed else None}, " - f"{_ctl_d.freeze_wd.episodes} episode(s), {len(_all_notes)} notification(s), " - f"ids {sorted({n.get('notification_id') for n in _all_notes})}") -check("a revived transport against a still-frozen meter re-rails the command", + f"{_ctl_d.freeze_wd.episodes} episode(s), {len(_freeze_notes)} " + f"notification(s) from this guard, " + f"ids {sorted({n.get('notification_id') for n in _freeze_notes})}") +# A PRECONDITION, not a discriminating check - the same role as "the command +# takes several cycles to reach the rail" above, and labelled the same way: +# without it the two checks below are satisfied by a controller that never came +# back. The crudest falsifier (never file the loop claim) takes most of this +# file red, which is the signature of a precondition rather than a check; the +# narrowest one measured is a stale-input stop that never releases. +check("PRECONDITION: a revived transport against a still-frozen meter re-rails " + "the command", bool(_re_railed)) check("...which is a SECOND episode, and it alarms again on the cycle it rails", _ctl_d.freeze_wd.episodes == 2 and bool(_re_railed) and _revived[_re_railed[0]][2] == _before[2] + 1) +# ⚠️ STABILITY, not the value - the value is pinned once, above. Renaming the +# id must fail that check and NOT this one; the two say different things and +# each has to be able to go red on its own. check("...on the same notification_id, so the second REPLACES the first " "instead of stacking a second one beside it", - len(_all_notes) == 2 - and len({n.get("notification_id") for n in _all_notes}) == 1) + len(_freeze_notes) == 2 + and len({n.get("notification_id") for n in _freeze_notes}) == 1) # --------------------------------------------------------------------------- # @@ -776,7 +906,16 @@ print("what the status page SAYS about coverage") def freeze_rows(unchanged_s, grid, **over): - """The freeze-detector rows of the real checks(), on a p1 install.""" + """The freeze-detector rows of the real checks(), on a p1 install. + + ⚠️ WHAT THIS DOES NOT COVER. The rows live behind `if self.p1_enabled:`, and + the flag is set here by hand rather than by building a real P1 transport - + inherited from part 4's fixture. So everything below proves "WHEN the row is + produced it says the right thing", and nothing below proves "the row is + produced on homewizard_local and not otherwise". That gate is uncovered; + AC2 is about the string, and widening the fixture belongs to whoever covers + the gate. + """ _h, ctl, _t = wired(unchanged_s, grid, cycles=3, **over) # p1_enabled is what a homewizard_local install has; the row's own content # is what is under test, not the transport that feeds it. @@ -829,6 +968,7 @@ check("a Controller built with the key absent lands on the SHIPPED hold, so " _ctl_c.freeze_wd.cfg.hold_s == SHIPPED_HOLD_S) print() +print(f"part 3 (the live HTTP transport) ran against: {PART3_TRANSPORT}") if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}") sys.exit(1) diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index b7faa3f..4645058 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -308,6 +308,36 @@ check("...and a healthy meter that looks unchanged for it is still inside the " _slowest_healthy_w is not None and _db is not None and _slowest_healthy_w < _db) +# ⚠️ AND THE RANGE'S OWN BOUNDS. "300 sits inside the range" is worth nothing +# while the range itself is unpinned: widened to `int(1,86400)` an operator can +# configure `meter_freeze_s: 86400` and the guard is dead for a day at a time, +# with every check in this repo still green. That mutation survived the first +# cut of this PR and is what this block closes. Both numbers are argued for in +# config.yaml's own comment and in freeze.py, and each is re-derived here: +# +# 60 s the floor freeze.py calls NOT A RECOMMENDATION - there a healthy +# meter can look unchanged at 60 W, four times the shipped deadband, so +# the conjunction's two terms stop being mutually exclusive and only +# the margin is left; +# 3600 s the ceiling - already four times the billing quarter, so the top of +# the range has given up detecting a freeze inside the quarter that +# bills it. It is the last bound between a supported configuration and +# a disabled guard, and moving it is a decision rather than a tidy-up. +print(f" schema range {_lo}-{_hi} s: at the floor a healthy meter that looks " + f"unchanged is " + + (f"{3600.0 / _lo:.0f} W" if _lo else "?") + + f" against a {_db} W deadband; the ceiling is " + + (f"{_hi / _QUARTER_S:.1f}x" if _hi else "?") + + " the billing quarter") +check(f"the schema range itself is still the argued 60-3600 s (found " + f"{_lo}-{_hi})", (_lo, _hi) == (60.0, 3600.0)) +check("...a floor that gives up the mutual exclusion and keeps only the " + "margin, which is why config.yaml calls it not a recommendation", + None not in (_lo, _db) and 3600.0 / _lo > _db) +check("...and a ceiling already past the billing quarter, so no value the " + "schema still accepts can alarm inside the quarter that bills it", + _hi is not None and _hi > _QUARTER_S) + # --------------------------------------------------------------------------- # print("the KNOWN cross-file default divergence, pinned rather than commented") # ⚠️ WHAT THIS GUARDS, AND WHY IT ASSERTS THE BUG RATHER THAN THE FIX. -- 2.54.0 From d31a12b7c62f354c8b92713d86e51f41a12bed43 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 30 Aug 2026 17:10:42 +0200 Subject: [PATCH 3/4] TEST-06: the id-selection note said 'the only assertion'; there are two Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC --- goodwe_controller/test_freeze.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/goodwe_controller/test_freeze.py b/goodwe_controller/test_freeze.py index a6bc24c..d9e41e6 100644 --- a/goodwe_controller/test_freeze.py +++ b/goodwe_controller/test_freeze.py @@ -692,9 +692,10 @@ check("the whole notification surface carried exactly one alarm this run", len(all_notes) == 1) # THIS GUARD'S, keyed on the id. check("...and the freeze detector is the one that raised it", len(notes) == 1) -# ⚠️ Selected by TEXT, not by id - see notes_by_text_of(). This is the only -# assertion whose subject is the id, so it is the one that must survive being -# the only thing a rename breaks. +# ⚠️ Selected by TEXT, not by id - see notes_by_text_of(). One of the two +# assertions whose subject IS the id (the other is the stability check in part +# 5), so it must not be selected by the thing it is checking, and it has to be +# able to fail while everything else stays green. check("...with a fixed notification_id so repeats replace, not stack", [d.get("notification_id") for d in notes_by_text_of(hass_f)] == [FREEZE_NOTE_ID]) -- 2.54.0 From 4b521b2df0e665a6f58c0fd7956acd0753892206 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 30 Aug 2026 18:11:21 +0200 Subject: [PATCH 4/4] ci: trigger a run on the rebased head No content change. The gate never created a run for 0942228d - the newest run stayed on d31a12b7, the pre-rebase head - so `checks / unit` had nothing to report and sat pending. Why no run was created is CI-02 and belongs to CI-01's owner; this commit only asks for one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC -- 2.54.0