diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 6fd86b3..56cff2d 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -249,17 +249,35 @@ class Controller: ) # SAFETY-44. THE NAMES THE MACHINES READ, HANDED TO THE MIGRATION THAT # WRITES FOR THEM. This line used to read `migrate_unit_keys(store, - # UNIT_A)`, and the constant was wrong in exactly the way line 315's - # comment says the SAME constant would be wrong two hundred lines below - # it: on a declared roster the flat latch landed under + # UNIT_A)`, and the constant was wrong in exactly the way the + # `maint_migration` call below says the SAME constant would be wrong - + # ⚠️ this comment used to cite "line 315" and "two hundred lines below", + # and BOTH numbers were wrong (315 is the CFG-03 defaults note; the + # comment meant is ~54 lines down, and line numbers move). Cite the + # thing, not its coordinates. On a declared roster the flat latch + # landed under # `unit/inverter-a/`, which is on no roster, was read back under the # name it was handed, and stamped the marker. `roster_names()` is the # naming rule itself and returns `[UNIT_A]` on the flat shape, so the - # upgrade path every install runs today is byte-identical - and a - # declared roster now gets `misdirected`, no marker, and a retry. + # upgrade path every install runs today is byte-identical. + # + # ⚠️ AND `misdirected` IS UNREACHABLE FROM **THIS** CALL SITE, WHICH THE + # COMMIT MESSAGE ORIGINALLY CLAIMED AS A BENEFIT. `unit` is + # `roster_names(opts)[0]` and `build_roster()` names its entries from + # the same `roster_names()` list, so `unit` is a member of `roster` by + # construction and the refusal can never fire here. Review measured it: + # dropping the `roster` argument from the `_verify_moves()` call inside + # `migrate_unit_keys()` survives 28/28. + # + # THE ARGUMENT STAYS ANYWAY, and the reason is the one `_verify_moves()` + # already gives for keeping its own `roster=None` branch: it is what a + # NEW caller gets, and the grading it enables is measured at the + # function's own level in test_maintenance.py rather than through here. + # What is NOT true is that this call site buys the refusal, so that + # claim is gone rather than restated. _roster_names = roster_names(opts) self.store_migration = migrate_unit_keys( - store, _roster_names[0] if _roster_names else UNIT_A, _roster_names) + store, _roster_names[0], _roster_names) self.inverters = build_roster(opts, store, sign_cfg, delivery_cfg) # ⚠️ CFG-04. SAY WHICH SHAPE IS IN FORCE, BECAUSE THE OTHER ONE'S KEYS # ARE STILL SITTING IN THE UI. A declared `inverters:` roster reads none @@ -812,8 +830,45 @@ class Controller: # held at 0 W until an operator clears it, which is the conservative # direction; SAFETY-44's is a site DRIVING on a state file that says it # is latched. + # ⚠️ AND THE THREE CASES ARE THREE VARIABLES, NOT ONE SHARED FLAG. + # THIS IS WHERE THE FIRST VERSION OF THIS FIX WAS DEFEATED, BY ONE KEY. + # It wrote `namespaced_latched = True` for BOTH "on the roster" and "off + # the roster but not UNIT_A", so a single latched key belonging to a + # unit the operator had removed from `inverters:` vetoed the repair of + # the misdirected `inverter-a` latch sitting beside it. Measured on the + # shipped build, roster ['garage', 'shed'], store stamped so the + # migration cannot reach it: + # + # sign_tripped + unit/inverter-a/sign_tripped + # -> [('garage', True), ('shed', False)] repaired + # ...AND unit/old-shed/sign_tripped + # -> [('garage', False), ('shed', False)] ⚠️ DRIVING + # + # A site DRIVING on a state file that reads `sign_tripped: true`, which + # is the sentence this whole card exists to make false - and + # `tools/rehearse_roster.py` said so, on the unmodified tree, exit 1. + # + # ⚠️ IT IS ALSO INSTANCE #17 OF HABIT 4, IN THE EXTENSION MUTATION.md + # ADDED TWO DAYS AGO. The two fixtures written for this card each hold + # ONE arrangement - `_staged_store` has only `inverter-a`, `_other_store` + # has only `inverter-b` - and the flag is written twice with opposite + # intents only when BOTH are present. A per-key check whose keys have + # never disagreed WITHIN ONE STORE is a check over whichever key came + # last. The fixture below now holds both. + # + # THE THREE CASES, AND WHY THE ANSWER DIFFERS FOR EACH: + # on the roster a machine reads it: real protection, stop. + # off-roster, `UNIT_A` the broken migration's fingerprint - this + # IS the flat latch, misdirected. Repair. + # off-roster, other name a real unit out of today's config. Its + # latch is unread too, but it is NOT the flat + # key's misdirection, and re-arming a healthy + # sibling off it is DEV-02's defect. Stop - + # UNLESS a stranded `UNIT_A` latch is also + # present, because then the flat key IS + # accounted for and the repair is the point. on_roster = {inv.name for inv in self.inverters} - stranded, namespaced_latched = [], False + stranded, on_roster_latched, other_off_roster = [], False, False for k, v in data.items(): if not (k.startswith("unit/") and k.endswith("/sign_tripped")): continue @@ -822,12 +877,19 @@ class Controller: # `unit_key()` builds `unit//`, so the unit is field 1 - # split through the writer's own spelling rather than re-deriving. unit = k.split("/")[1] - if unit in on_roster or unit != UNIT_A: - namespaced_latched = True - else: + if unit in on_roster: + on_roster_latched = True + elif unit == UNIT_A: stranded.append(k) - if namespaced_latched or any(inv.sign_wd.tripped - for inv in self.inverters): + else: + other_off_roster = True + if on_roster_latched or any(inv.sign_wd.tripped + for inv in self.inverters): + return + if other_off_roster and not stranded: + # A removed unit's own latch, and no misdirected one. The flat key + # is that unit's mirror, so it carries no new information and + # re-arming `inverters[0]` from it would latch a healthy pack. return if stranded: _LOG.error( diff --git a/goodwe_controller/test_maintenance.py b/goodwe_controller/test_maintenance.py index 0040752..8b513aa 100644 --- a/goodwe_controller/test_maintenance.py +++ b/goodwe_controller/test_maintenance.py @@ -21,6 +21,7 @@ Read MUTATION.md before you report a survivor. import asyncio import dataclasses +import logging import re import sys from datetime import datetime, timedelta, timezone @@ -34,7 +35,8 @@ from app.inverter import UNIT_A from app.main import Controller from app.store import ( DEFAULTS, MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION, MAINT_UNIT_KEYS, - SCHEMA_KEY, SCHEMA_VERSION, migrate_maint_keys, unit_key, + SCHEMA_KEY, SCHEMA_VERSION, migrate_maint_keys, migrate_unit_keys, + unit_key, ) fails = [] @@ -1443,7 +1445,7 @@ print("13. FIXED (SAFETY-44): SAFETY-02's migration takes the roster too, and " # ITSELF: an install that has never run a version with namespaced keys, which is # what makes `migrate_unit_keys()` run at all. _lat_store = MemStore({"sign_tripped": True, - "sign_reason": "SIGN_DISAGREEMENT: measured pre-DEV-02"}) + "sign_reason": "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured"}) _lat, _ = controller(NAMED_OPTS, store=_lat_store) print(f" pre-DEV-02 flat latch + declared roster {_C2_NAMES} -> " f"{_lat.store_migration.explain()}") @@ -1480,9 +1482,9 @@ check("⚠️ ...and the OTHER units on the roster are NOT latched by it. The fl # operator actually produces. Only the GUARD repairs it. _staged_store = MemStore({ "sign_tripped": True, - "sign_reason": "SIGN_DISAGREEMENT: measured pre-DEV-02", + "sign_reason": "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured", unit_key(UNIT_A, "sign_tripped"): True, - unit_key(UNIT_A, "sign_reason"): "SIGN_DISAGREEMENT: measured pre-DEV-02", + unit_key(UNIT_A, "sign_reason"): "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured", SCHEMA_KEY: SCHEMA_VERSION}) _staged, _ = controller(NAMED_OPTS, store=_staged_store) print(f" already-stamped store + declared roster -> ran=" @@ -1519,10 +1521,111 @@ check("⚠️ a latch stranded under a name that is not `UNIT_A` is NOT re-armed "sibling off it is the defect DEV-02 removed", not any(i.sign_wd.tripped for i in _other.inverters)) +# ⭐ AND THE ARRANGEMENT NEITHER FIXTURE ABOVE HOLDS: **BOTH** KEYS AT ONCE. +# ⚠️ THIS IS INSTANCE #17 OF HABIT 4 AND IT WAS FOUND IN THIS CARD'S OWN FIX. +# The first version of the guard classified each key and then wrote ONE shared +# `namespaced_latched` flag for both "on the roster" and "off-roster but not +# UNIT_A". `_staged_store` above holds only an `inverter-a` latch and +# `_other_store` below holds only an `inverter-b` one, so the flag was never +# written twice with opposite intents in a single store - and a per-key check +# whose keys have never disagreed WITHIN ONE STORE is a check over whichever +# key happened to come last. Measured on that build, one added key: +# +# sign_tripped + unit/inverter-a/sign_tripped -> garage latched, repaired +# ...AND unit/old-shed/sign_tripped -> ⚠️ NOTHING latched +# +# A site DRIVING on a state file that reads `sign_tripped: true` - the sentence +# this card exists to make false - reached by an operator taking one unit out of +# `inverters:` after it had tripped. "A roster is configuration and it changes" +# is this module's own phrase, and the second inverter goes in this month. +_both_store = MemStore({ + "sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured", + unit_key(UNIT_A, "sign_tripped"): True, + unit_key(UNIT_A, "sign_reason"): "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured", + unit_key("old-shed", "sign_tripped"): True, + unit_key("old-shed", "sign_reason"): "SIGN_DISAGREEMENT: old-shed, since removed", + SCHEMA_KEY: SCHEMA_VERSION}) +_both, _ = controller(NAMED_OPTS, store=_both_store) +print(f" stranded `inverter-a` AND an unrelated off-roster `old-shed` -> " + f"{[(i.name, i.sign_wd.tripped) for i in _both.inverters]}") +check("⭐ A STRANDED `inverter-a` LATCH IS STILL REPAIRED WHEN AN UNRELATED " + "OFF-ROSTER LATCH SITS BESIDE IT. One shared flag let `old-shed` veto the " + "repair, and the site came up driving on a file that says it is latched", + _both.inverters[0].sign_wd.tripped) +check("...and the repair still lands on the roster's first unit, not on the " + "removed one", + _both_store.data.get(unit_key(_C2_NAMES[0], "sign_tripped")) is True) +check("⚠️ ...and the reason carried across is the FLAT one - the misdirected " + "latch's own - and not `old-shed`'s, which belongs to a different " + "physical inverter", + "old-shed" not in _both.inverters[0].sign_wd.reason) + +# ⚠️ AND THE OPERATOR MESSAGE IS ASSERTED, because review deleted it whole +# (`if stranded:` -> `if False:`) and 28/28 stayed green. `stranded` has no +# other consumer, so the list existed only to print a line nothing checked - +# and the line is the entire operator-facing half of this repair: it names the +# key that was stranded and the roster it was measured against. A repair an +# operator cannot see is a repair they cannot confirm. +_log_store = MemStore({ + "sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured", + unit_key(UNIT_A, "sign_tripped"): True, + unit_key(UNIT_A, "sign_reason"): "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured", + SCHEMA_KEY: SCHEMA_VERSION}) +_records = [] +_h = logging.Handler() +_h.emit = lambda r: _records.append(r.getMessage()) +logging.getLogger("goodwe").addHandler(_h) +try: + _logged, _ = controller(NAMED_OPTS, store=_log_store) +finally: + logging.getLogger("goodwe").removeHandler(_h) +_s44_lines = [m for m in _records if "SAFETY-44" in m] +print(f" SAFETY-44 operator lines logged: {len(_s44_lines)}") +check("⭐ the repair SAYS SO, at ERROR, naming SAFETY-44 - deleting this line " + "left the whole suite green, and it is how an operator learns their " + "latch was re-armed rather than never lost", + len(_s44_lines) == 1) +check("...and it names the STRANDED KEY, so the operator can find it in the " + "state file rather than being told something happened", + any(unit_key(UNIT_A, "sign_tripped") in m for m in _s44_lines)) +check("...and the ROSTER it was measured against, which is the half that says " + "WHY no machine read it", + any("garage" in m and "shed" in m for m in _s44_lines)) + +# ⭐ AND `misdirected` IS MEASURED AT THE FUNCTION'S OWN LEVEL, BECAUSE IT IS +# UNREACHABLE FROM THE CALL SITE. `Controller.__init__` passes +# `roster_names(opts)[0]`, which `build_roster()` also names its first entry +# from, so `unit` is a member of `roster` by construction and the refusal can +# never fire there. Review measured that: dropping the `roster` argument from +# `_verify_moves()` inside `migrate_unit_keys()` survives 28/28. The argument is +# kept for the reason `_verify_moves()` gives for its own `roster=None` branch - +# it is what a NEW caller gets by default - and a guard nothing exercises is a +# guard nobody knows works, so it is exercised HERE rather than claimed. +_mis_store = MemStore({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured"}) +_mis = migrate_unit_keys(_mis_store, UNIT_A, ["garage", "shed"]) +print(f" migrate_unit_keys(store, 'inverter-a', ['garage','shed']) -> " + f"ok={_mis.ok} misdirected={sorted(_mis.misdirected)}") +check("⭐ a WRONG `unit` - one that is not on the roster it was handed - comes " + "back `misdirected`, which is the refusal `migrate_maint_keys()` already " + "had and this one did not", + bool(_mis.misdirected) and not _mis.ok) +check("...and the schema marker is NOT stamped, so the migration runs again on " + "the next start rather than being recorded as finished - the one thing " + "store.py's header says must never happen", + _mis_store.data.get(SCHEMA_KEY) != SCHEMA_VERSION) +check("⚠️ ...while the SAME call with a unit that IS on the roster stamps the " + "marker - so the refusal is caused by the name and not by the fixture", + migrate_unit_keys( + MemStore({"sign_tripped": True, "sign_reason": "x"}), + "garage", ["garage", "shed"]).ok) + # The counterfactual, and it is the accepted baseline habit 7 asks for: the # same store, the same migration, one thing different - the roster shape. _flat_lat_store = MemStore({"sign_tripped": True, - "sign_reason": "SIGN_DISAGREEMENT: measured pre-DEV-02"}) + "sign_reason": "SIGN_DISAGREEMENT: SYNTHETIC FIXTURE, not measured"}) _flat_lat, _ = controller(FLAT_OPTS, store=_flat_lat_store) print(f" the same store on the FLAT config -> roster " f"{[(i.name, i.sign_wd.tripped) for i in _flat_lat.inverters]}") diff --git a/goodwe_controller/test_rehearse.py b/goodwe_controller/test_rehearse.py index 33c0219..2f8ff6a 100644 --- a/goodwe_controller/test_rehearse.py +++ b/goodwe_controller/test_rehearse.py @@ -411,6 +411,37 @@ check("...and `phase` is NOT among them, because its value 'idle' IS the " "phase" not in _m9_vanished) shutil.rmtree(Path(_m9).parent, ignore_errors=True) +# ⚠️ AND `M12` BOUNDS THE **MOVED-NOT-LOST** RULE, WHICH NOTHING DID. The +# suppression added with SAFETY-44 cites its own bound in a comment, and the +# first version of that comment cited M3 - which drops `sign_reason` from +# `LEGACY_UNIT_KEYS` and expects `LATCH KEY NOT UNDER A ROSTER NAME` - and the +# commit message cited it too. It is the wrong mutant, and so is M9: every key +# M9 vanishes is FLAT, so neither can satisfy the `unit/` gate the rule sits +# behind. THE BRANCH HAD NO NEGATIVE CONTROL AT ALL, in a card whose whole +# subject is checks that cannot reach the failure they name. +# +# ⚠️ AND THE HOLE IT LEFT WAS REAL. `DEFAULTS["sign_tripped"] is False`, so the +# ONLY non-default value a latch can hold is `True` - which means, before the +# `vanished_unit not in after.units` clause, ANY surviving latched roster unit +# suppressed ANY other unit's vanished latch. Measured on a build that drops a +# roster unit's `sign_*` while a sibling's survives at the same value: exit 0, +# "nothing vanished, nothing stranded", on a deleted SAFETY-02 latch. +_M12_OLD = 'tripped=bool(data.get(self.key("sign_tripped"), False)),' +_M12_NEW = ('tripped=bool(data.get(self.key("sign_tripped"), False))' + chr(10) + + ' if self.name != "shed" else False,') +_m12 = mutant_tree([("app/inverter.py", _M12_OLD, _M12_NEW)]) +_m12_code, _m12_out = run(Path(_m12), "state-legacy.json", "roster.yaml") +print(f" a roster unit that stops restoring its latch -> exit {_m12_code}") +check("⭐ M12: a ROSTER unit's latch that stops being read is still a finding - " + "the moved-not-lost rule suppresses a RENAME (the key left a name this " + "roster does not carry and arrived under one it does), never a LOSS", + _m12_code == RR.EXIT_FINDING) +check("...and the suppression is not what swallowed it: a key whose own unit is " + "ON the roster is never reported as migrated, whatever else carries the " + "same value", + "migrated unit/shed/" not in _m12_out) +shutil.rmtree(Path(_m12).parent, ignore_errors=True) + # ⚠️ AND THE INPUT FILE IS NEVER WRITTEN. The owner runs this against a copy of # a live state file; a tool that edited it would be a tool that had to be # trusted rather than one that cannot do harm. @@ -668,9 +699,19 @@ print("5b. NEGATIVE CONTROL, AND THE SPLITTER'S POSITIVE ONE: revert THE " # one that makes them disagree, and it is a negative control for part 5 at the # same time: put back the clause that counted ANY `unit/*/sign_tripped` as # protection, including one under a name no unit answers to. +# ⚠️ WHAT THIS CONTROL PINS IS THE SHIPPED SPELLING, NOT THE RULE, and review +# is owed that distinction. `mutant_tree()` asserts its target exists - rightly, +# because a mutation that did not happen yields a fabricated result - but the +# consequence is that EDITING the guard's line makes this file red at the +# ANCHOR before any behaviour is measured. Demonstrated: rewriting the same +# semantics as `on_roster = set()` leaves the anchor intact and IS caught, by +# M4's control; rewriting the line itself is caught by the anchor, which looks +# identical from the exit code and proves much less. Read 5b and 5c as controls +# over this text, and the behavioural coverage as living in test_maintenance.py +# section 13 and test_inverter.py, which construct stores rather than strings. _no_guard = mutant_tree([( "app/main.py", - " if unit in on_roster or unit != UNIT_A:", + " if unit in on_roster:", " if True: # MUTANT: any namespaced latch is protection")]) _ng_code, _ng_out = run(Path(_no_guard), "state-legacy.json", "roster.yaml") _NG = legs(_ng_out) @@ -706,8 +747,7 @@ print("5c. NEGATIVE CONTROL: revert THE CALL SITE, and the litter comes back " # `misdirected` refusal that stops a WRONG marker being stamped; it does not # buy the latch. Both edits stay. _CALL_NOW = (" self.store_migration = migrate_unit_keys(" + chr(10) + - " store, _roster_names[0] if _roster_names else " - "UNIT_A, _roster_names)") + " store, _roster_names[0], _roster_names)") _old_call = mutant_tree([( "app/main.py", _CALL_NOW, " self.store_migration = migrate_unit_keys(store, UNIT_A)")]) diff --git a/goodwe_controller/tools/rehearse_roster.py b/goodwe_controller/tools/rehearse_roster.py index cc5acc6..b3d807f 100644 --- a/goodwe_controller/tools/rehearse_roster.py +++ b/goodwe_controller/tools/rehearse_roster.py @@ -563,14 +563,43 @@ def diff(before: Snapshot, after: Snapshot) -> list: # # ⚠️ THE TEST IS NOT "the name appears somewhere". It is: the SAME bare # key, at the SAME value, under a key in `machine_read` - the set built - # by driving a real start, not by pattern-matching a namespace. A latch - # that genuinely went missing has no such key and is still a finding, - # which is what M3 (delete the flat rollback view) measures and what - # keeps this from being a softening of the check. + # by driving a real start, not by pattern-matching a namespace. + # + # ⚠️ THIS USED TO CITE "M3 (delete the flat rollback view)" AS WHAT + # BOUNDS IT, AND THAT CITATION WAS WRONG IN BOTH HALVES. M3 drops + # `sign_reason` from `LEGACY_UNIT_KEYS` and expects `LATCH KEY NOT UNDER + # A ROSTER NAME`; the delete-the-rollback-view mutant is M9, and every + # key M9 vanishes is FLAT, so it can never satisfy the `unit/` gate + # here. Neither mutation exercises this branch. The sentence "what keeps + # this from being a softening of the check" was not supported by the + # thing it named - and a citation is the worst possible carrier for that, + # because it reads as having been checked. The branch is now bounded by + # M12, written for it, in test_rehearse.py. moved_to = sorted( k for k in after.machine_read if k.rsplit("/", 1)[-1] == bare and after.data.get(k) == value) - if key.startswith("unit/") and moved_to: + # ⚠️ AND THE VANISHED KEY'S OWN UNIT MUST BE **OFF THE ROSTER**, WHICH + # THE FIRST VERSION OF THIS RULE DID NOT REQUIRE AND WHICH IS WHAT MADE + # IT A SOFTENING. The match was bare-key-plus-value over the whole of + # `machine_read`, with no identity - and `DEFAULTS["sign_tripped"] is + # False`, so the ONLY non-default value a latch can hold is `True`. + # Any surviving latched roster unit therefore suppressed any OTHER + # unit's vanished latch. Measured: a build that DROPS + # `unit/shed/sign_*` while `unit/garage/sign_*` survives, both latched + # by one site event so the reasons match too, reported + # + # none in LEG SAME-STEP: nothing vanished, nothing stranded ... + # NO FINDINGS in the rehearsed transitions + # + # exit 0, on a roster unit's SAFETY-02 latch that had been deleted. + # + # The legitimate case is a RENAME: the key left `inverter-a`, which is + # not on this roster, and arrived under `garage`, which is. A key whose + # own unit is still on the roster has not been renamed - it has been + # LOST - and no amount of the same value elsewhere makes that untrue. + vanished_unit = key.split("/")[1] if key.startswith("unit/") else None + if vanished_unit is not None and vanished_unit not in after.units \ + and moved_to: after.vanished_but_migrated.append( f"{key} = {value!r} -> {moved_to[0]}") continue