From d5c157721253e51922d050e1332118e75f118c59 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 4 Sep 2026 11:13:13 +0200 Subject: [PATCH] SAFETY-30: pin WHICH thresholds each sign detector is armed with, not just what the numbers are MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW FOUND A MUTATION SURVIVOR THAT UNDOES THE WHOLE PRODUCT OF AC 5, and it is a coverage defect rather than a behaviour one - the shipped code arms the detectors correctly today. In `_arm_sign_watchdogs()`, collapsing inv.sign_wd.arm(self.sign_cfg if one else self.unit_sign_cfg, ...) to `arm(self.sign_cfg, ...)` throws the re-derived 250 W / 85 s per-unit pair away and arms every unit with SAFETY-02's meter-facing 300 W / 180 s - and left ALL 19 FILES GREEN. Reproduced here before fixing anything: fresh per-mutation tree, no __pycache__, mounted :ro, graded through tools/mutation_run.sh; -31 B; 19 pass, 0 fail. WHY NOTHING CAUGHT IT. `grep -rn "sign_wd.cfg" test_*.py` found exactly ONE threshold assertion, part 7's N=1 arm at (300.0, 1.0) - which the mutation PRESERVES. Nothing at a roster of two read `inv.sign_wd.cfg` at all. The ledger mutates the VALUES in SIGN_OPTION_DEFAULTS (M10 250->300, M11 325->180, both correctly red through the config pins) and never the WIRING that delivers them. Part 3's behavioural checks cannot close the gap either, and the comment now says why: B is reversed at ~2000 W with short holds, so it trips under EITHER pair, and a fault big enough and long enough to trip under both thresholds is evidence about neither. THE FIX - test_watchdog.py part 3, on the real two-inverter Controller. Every `inv.sign_wd.cfg` equals the per-unit pair, `site_sign_wd.cfg` equals the site pair, and each detector's `against` is its own instrument. Graded against LITERALS, because `== self.unit_sign_cfg` cannot tell "the per-unit pair" from "whatever this Controller was handed". Both pairs are printed. ⚠️ AND THE FIXTURE HAD TO CHANGE FOR THE PIN TO MEAN ANYTHING - the recursion worth naming, because it is the same defect class as the one being closed. `_sign_site` set `sign_disagree_s`, `unit_sign_disagree_s` AND `site_sign_disagree_s` all to 1 s, with two of the three magnitude gates at 300 W. A bare equality there discriminates on `min_w` alone: a mis-wiring that delivered the right magnitude with the WRONG HOLD compares EQUAL to the pair it was supposed to have. A check comparing two things that happen to be equal cannot see the difference it names (MUTATION.md's third failure mode). The three pairs are now pairwise distinct IN BOTH COORDINATES - 250 W / 1 s per unit, 320 W / 2 s site, 300 W / 3 s shipped - a PRECONDITION asserts that distinctness so a later edit collapsing them goes red rather than quietly vacuous, and a second PRECONDITION asserts the literals are the pairs this Controller actually holds. VERIFIED BY BREAKING IT. Three mutations, each on its own freshly copied tree with no __pycache__, mounted :ro, through tools/mutation_run.sh: M14 arm() handed self.sign_cfg at every roster -31 B RED (was the survivor; the AC 5 WIRING check names the wiring) M15 right min_w, SHIPPED hold - hold-only mis-wire +78 B RED, same pin M16 right min_w, SITE hold - hold-only, and it still trips inside the drive +83 B RED, and the ONLY red in test_watchdog.py M16 is the one that says the hold half of the equality bites on its own. Unmutated tree: 19 pass, 0 fail, purged and fingerprinted. Ledger rows M14-M16 and the write-up in DECISIONS/SAFETY-30-per-unit-sign.md section 5; a CHANGELOG bullet. NOT IN THIS PR, so it gets a card rather than a quiet fix: a stuck per-unit power sensor would false-trip the new per-unit check at N>=2. Already-merged SAFETY-31 has the identical exposure on the same fixture. Pre-existing N>=2 class, unreachable at the deployed roster of one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 11 +++ .../DECISIONS/SAFETY-30-per-unit-sign.md | 54 +++++++++++ goodwe_controller/test_watchdog.py | 92 ++++++++++++++++++- 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 7efde7d..0dc6273 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -67,6 +67,17 @@ evidence. row - and the site detector's window is never so much as opened. `unit_sign_disagree_*` and `site_sign_disagree_*` do nothing on a one-inverter install. +* **The threshold WIRING is pinned, not just the threshold values.** Review + found that `arm(self.sign_cfg if one else self.unit_sign_cfg, ...)` could be + collapsed to `arm(self.sign_cfg, ...)` — arming every unit with the + meter-facing pair instead of the re-derived per-unit one — with all 19 test + files still green, because nothing at a roster of two read `sign_wd.cfg` and + the trips in the fixture happen under either pair. `test_watchdog.py` part 3 + now asserts the pair and the instrument on **every** detector against + literals, and the fixture's three threshold pairs were made pairwise distinct + in both coordinates so that a hold delivered from the wrong pair cannot + compare equal to the right one. M14–M16 in + `DECISIONS/SAFETY-30-per-unit-sign.md` §5. * **Known and recorded rather than fixed:** the site comparison is defeated by a square-wave load whose half period is about one control cycle, which holds it opposed for ~1000 s at *both* rosters. The N=1 control arm is what says this is diff --git a/goodwe_controller/DECISIONS/SAFETY-30-per-unit-sign.md b/goodwe_controller/DECISIONS/SAFETY-30-per-unit-sign.md index 96b4338..2b8ffd7 100644 --- a/goodwe_controller/DECISIONS/SAFETY-30-per-unit-sign.md +++ b/goodwe_controller/DECISIONS/SAFETY-30-per-unit-sign.md @@ -377,6 +377,9 @@ size-preserving edit is what TEST-15 turned into a false survivor once already. | M11 | `watchdog.py` | `site_sign_disagree_s` 325 → 180, i.e. **the number carried** | **+0 B** | **RED** — the measured-215 s check and the CFG-03 pin | | M12 | `arbiter.py` | `sign_site` scoped `SCOPE_UNIT` | **+0 B** | **RED** — `test_arbiter.py` **and** `test_peakguard.py` | | M13 | `validate.py` | the `unit_sign_disagree_w` rule removed | −126 B | **RED** — `test_validate.py` | +| **M14** | `main.py` | **`arm()` handed `self.sign_cfg` at EVERY roster** — the WIRING, not the values | −31 B | **SURVIVOR at review — GAP CLOSED**, then RED. See below | +| M15 | `main.py` | per-unit `arm()` given the right `min_w` and the SHIPPED hold — a HOLD-ONLY mis-wire | +78 B | **RED** — the same wiring pin | +| M16 | `main.py` | per-unit `arm()` given the right `min_w` and the SITE hold — hold-only again, and behaviour-preserving | +83 B | **RED** — the wiring pin, and the ONLY red in `test_watchdog.py` | ### M4 is recorded as a survivor because it was one @@ -400,6 +403,57 @@ cycles, zero calls, printed. A counter can only be zero for one reason. The mutation was re-run against it and is red; the run that found it was discarded along with its tree, so this is a result and not an artefact. +### M14: the VALUES were pinned and the WIRING was not + +M10 and M11 mutate the numbers in `watchdog.SIGN_OPTION_DEFAULTS` and both are +red through the config pins. Neither of them touches the line that DELIVERS +those numbers to a detector: + +```python +inv.sign_wd.arm(self.sign_cfg if one else self.unit_sign_cfg, + AGAINST_METER if one else AGAINST_OWN_POWER) +``` + +Drop the conditional — `arm(self.sign_cfg, ...)` — and every unit is armed with +SAFETY-02's meter-facing pair instead of the re-derived per-unit one. At review +that left **all 19 files green**. `grep -rn "sign_wd.cfg" test_*.py` found +exactly one threshold assertion, and it is §7's N=1 arm — `(300.0, 1.0)` — +which the mutation PRESERVES. Nothing anywhere read `inv.sign_wd.cfg` at a +roster of two. + +The behavioural checks in part 3 cannot close it and it is worth saying why: B +is reversed at ~2000 W and every hold in that fixture is short, so B trips under +either pair. **A fault big enough and long enough to trip under both thresholds +is evidence about neither.** + +What the survivor costs in the field is the whole product of §3: at +`config.yaml`'s own defaults the per-unit check would run at `sign_disagree_s` +**180 s instead of 85 s** — 2.1× slower to park a reversed inverter — and its +gate would move from 250 W to 300 W, so a unit reversing a share between the two +becomes invisible, while `SIGN_OPTION_DEFAULTS`, `config.yaml` and §3 all go on +describing a pair the running code does not use. With CI green. + +**Closed by a wiring pin in `test_watchdog.py` part 3**, on the real +two-inverter Controller: every `inv.sign_wd.cfg` equals the per-unit pair, +`site_sign_wd.cfg` equals the site pair, and each detector's `against` is its +own instrument — all graded **against literals**, since `== self.unit_sign_cfg` +cannot tell *the per-unit pair* from *whatever this Controller was handed*. + +⚠️ **And the fixture had to be changed for that pin to mean anything.** +`_sign_site` set `unit_sign_disagree_s: 1` **and** `site_sign_disagree_s: 1` +**and** `sign_disagree_s: 1`, with two of the three magnitude gates at 300 W. A +bare equality there discriminates on `min_w` alone: a mis-wiring that delivered +the right magnitude with the wrong hold would compare EQUAL to the pair it was +supposed to have. That is this document's own §5 lesson and MUTATION.md's third +failure mode — a check comparing two things that happen to be equal cannot see +the difference it names — reappearing inside the fix for it. The three pairs are +now pairwise distinct **in both coordinates** (250 W / 1 s, 320 W / 2 s, +300 W / 3 s), a PRECONDITION check asserts that distinctness so a later edit +collapsing them goes red rather than quietly vacuous, and both pairs are +**printed**. M15 and M16 exist to show the hold half actually bites: right +magnitude, wrong hold, red both times — and M16, whose wrong hold still trips +inside the drive, is red on the wiring pin and on **nothing else** in that file. + ## 6. What this card does NOT close * **The flapping-load blind spot in the SITE comparison.** §3, at length. Present diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index 84170be..7ce60b8 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -1342,13 +1342,31 @@ def _sign_site(hass, store=None, **over): stopped by circulation while this section is trying to grade a PER-UNIT attribution, because then every "only B is parked" check below would be measuring the wrong guard. + + ⚠️ THE THREE THRESHOLD PAIRS ARE PAIRWISE DISTINCT IN BOTH COORDINATES, + AND THAT IS LOAD-BEARING RATHER THAN COSMETIC. This fixture is the only + place in the suite where `_arm_sign_watchdogs()` runs at a roster of two, so + it is the only place that can see WHICH pair reaches WHICH detector. The + first version of it set every hold to 1 s and two of the three magnitude + gates to 300 W; a detector armed with the shipped pair, or with the site + pair, or with the right magnitude and the wrong hold, then compared EQUAL + to the pair it was supposed to have, and the wiring check below would have + passed on three of those four arrangements. That is MUTATION.md's third + failure mode written into a fixture - a check comparing two things that + happen to be equal cannot see the difference it names - and the + precondition beside the check is what stops a later edit collapsing them + again without a red line. + + Anyone changing a hold here: the PER-UNIT hold is what part 3's trip timing + is read against, and the SITE hold must still expire inside `_drive()`'s + window (14 cycles x 0.2 s), which part 5 depends on. """ opts = {"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", "meter_source": "off", "max_w": 2000, "gain": 0.6, - "sign_disagree_w": 300, "sign_disagree_s": 1, + "sign_disagree_w": 300, "sign_disagree_s": 3, "unit_sign_disagree_w": 250, "unit_sign_disagree_s": 1, - "site_sign_disagree_w": 300, "site_sign_disagree_s": 1, + "site_sign_disagree_w": 320, "site_sign_disagree_s": 2, "circulation_hold_s": 3600, "auto_start": True} opts.update(over) c = Controller(opts, hass, store if store is not None else _MemStore(), @@ -1392,6 +1410,76 @@ async def _one_reversed(): _c30, _h30, _s30 = asyncio.run(_one_reversed()) _a30, _b30 = _c30.inverters + +# ⚠️ THE WIRING PIN FOR AC 5, AND IT IS A DIFFERENT CLAIM FROM EVERY +# BEHAVIOURAL CHECK IN THIS SECTION. The rest of part 3 grades what the +# detectors CONCLUDED; this grades WHICH THRESHOLDS THEY WERE HANDED, and the +# gap between the two was live. Replacing +# `arm(self.sign_cfg if one else self.unit_sign_cfg, ...)` with +# `arm(self.sign_cfg, ...)` in `Controller._arm_sign_watchdogs()` throws the +# re-derived per-unit pair away and arms every unit with SAFETY-02's +# meter-facing one - and left ALL 19 FILES GREEN. Nothing at a roster of two +# read `inv.sign_wd.cfg` at all; the one place that does is part 7's N=1 arm, +# which that edit PRESERVES. +# +# WHY THE TRIPS BELOW CANNOT CATCH IT, said here because "the suite is green" +# and "nothing was wrong" are the two readings this comment exists to separate. +# B is reversed at ~2000 W - far over 250 W and far over 300 W - and every hold +# in this fixture is short enough to expire inside `_drive()`. A fault that is +# big enough and long enough to trip under BOTH pairs is evidence about +# neither. Only an equality on the pair itself can tell them apart. +# +# WHAT THE SURVIVOR COSTS IN THE FIELD, at config.yaml's own defaults: the +# per-unit check runs at `sign_disagree_s` 180 s instead of the re-derived 85 s +# - 2.1x slower to park a reversed inverter - and its gate moves from 250 W to +# 300 W, so a unit reversing a share between the two becomes invisible, while +# watchdog.SIGN_OPTION_DEFAULTS, config.yaml and DECISIONS section 3 all go on +# describing a pair the running code does not use. +_UNIT_PAIR = (250.0, 1.0) # `unit_sign_disagree_w` / `_s` - one per unit +_SITE_PAIR = (320.0, 2.0) # `site_sign_disagree_w` / `_s` - one per site +_SHIPPED_PAIR = (300.0, 3.0) # `sign_disagree_w` / `_s` - N=1 only +_armed = {inv.name: (inv.sign_wd.cfg.min_w, inv.sign_wd.cfg.hold_s) + for inv in _c30.inverters} +_armed_site = (_c30.site_sign_wd.cfg.min_w, _c30.site_sign_wd.cfg.hold_s) +print(f" armed per unit: {_armed}") +print(f" armed site: {_armed_site} " + f"shipped pair, wanted by NOBODY here: {_SHIPPED_PAIR}") +check("PRECONDITION: the three pairs are pairwise distinct IN BOTH " + "COORDINATES, so an equality below can fail on the magnitude alone, on " + "the hold alone, or on both. This fixture originally gave all three the " + "same 1 s hold and two of them the same 300 W gate, and a mis-wiring " + "that delivered the right magnitude with the wrong hold would then have " + "compared EQUAL to the pair it was supposed to have", + len({_UNIT_PAIR, _SITE_PAIR, _SHIPPED_PAIR}) == 3 + and len({p[0] for p in (_UNIT_PAIR, _SITE_PAIR, _SHIPPED_PAIR)}) == 3 + and len({p[1] for p in (_UNIT_PAIR, _SITE_PAIR, _SHIPPED_PAIR)}) == 3) +check("PRECONDITION: and all three are pairs this Controller actually holds, " + "so the literals below are the fixture's own numbers and not three " + "constants that agree with nothing. In particular the shipped pair is " + "still constructed and still a reachable mis-wiring target", + (_c30.unit_sign_cfg.min_w, _c30.unit_sign_cfg.hold_s) == _UNIT_PAIR + and (_c30.site_sign_cfg.min_w, _c30.site_sign_cfg.hold_s) == _SITE_PAIR + and (_c30.sign_cfg.min_w, _c30.sign_cfg.hold_s) == _SHIPPED_PAIR) +check("⚠️ AC 5 WIRING. EVERY per-unit detector on this roster of two is armed " + "with the PER-UNIT pair - 250 W / 1 s here, `unit_sign_disagree_w` and " + "`unit_sign_disagree_s` in the field. Graded against LITERALS rather " + "than against `_c30.unit_sign_cfg`, which cannot tell 'the per-unit " + "pair' from 'whatever this Controller was handed'. This is the line that " + "goes red when `_arm_sign_watchdogs()` stops choosing between the two " + "pairs and hands every unit the site-facing one", + len(_armed) == 2 and all(p == _UNIT_PAIR for p in _armed.values())) +check("...and the SITE detector is armed with the SITE pair - 320 W / 2 s " + "here - which differs from the per-unit pair AND from the shipped pair " + "in both coordinates, so this equality is not carried by a number the " + "fixture happens to hold twice", + _armed_site == _SITE_PAIR) +check("...and each detector is pointed at its own INSTRUMENT as well as at its " + "own thresholds: the units at their own 0x0518 power, the site at the " + "meter. They are two arguments of one call and a pin on one is not a pin " + "on the other", + all(inv.sign_wd.against == AGAINST_OWN_POWER for inv in _c30.inverters) + and _c30.site_sign_wd.against == AGAINST_METER) + _mid = _s30[3] print(f" cycle 3: stood {[round(x) for x in _mid['stood']]} " f"measured {[round(x) for x in _mid['meas']]} "