diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 7dba693..6fd86b3 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -247,7 +247,19 @@ class Controller: hold_s=float(opts.get("delivery_hold_s", DELIVERY_OPTION_DEFAULTS["delivery_hold_s"])), ) - self.store_migration = migrate_unit_keys(store, UNIT_A) + # 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/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. + _roster_names = roster_names(opts) + self.store_migration = migrate_unit_keys( + store, _roster_names[0] if _roster_names else UNIT_A, _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 @@ -751,23 +763,79 @@ class Controller: legacy_tripped = bool(data.get("sign_tripped", False)) if not legacy_tripped or not self.inverters: return - # ⚠️ THE QUESTION IS "IS ANY NAMESPACED LATCH SET", AND IT IS ASKED OF - # THE STORE RATHER THAN OF THE ROSTER. At a roster of two a trip on B - # leaves A untripped, so re-arming A off a flat key that B's own trip - # mirrored would latch a healthy inverter on every restart - the - # flat-key defect this card removed, coming back through its own guard. - # And the STORE is the right place to ask: it holds every - # `unit/*/sign_tripped` this install has ever written, including one - # belonging to a unit that is not in TODAY's roster (a roster is - # configuration and it changes), whereas the roster only knows what is - # configured right now. The flat key carries new information only when - # nothing namespaced is latched at all. - namespaced_latched = any( - bool(v) for k, v in data.items() - if k.startswith("unit/") and k.endswith("/sign_tripped")) + # ⚠️ THE QUESTION IS "IS ANY NAMESPACED LATCH SET **THAT A MACHINE + # READS**", AND THE LAST CLAUSE IS SAFETY-44's SECOND HALF. At a roster + # of two a trip on B leaves A untripped, so re-arming A off a flat key + # that B's own trip mirrored would latch a healthy inverter on every + # restart - the flat-key defect DEV-02 removed, coming back through its + # own guard. That much stands, and it is why the question is asked of + # the STORE's keys rather than of the roster's objects. + # + # ⚠️ BUT IT USED TO ACCEPT **ANY** `unit/*/sign_tripped`, and the + # paragraph here defended that: the store "holds every one this install + # has ever written, including one belonging to a unit that is not in + # TODAY's roster ... whereas the roster only knows what is configured + # right now". THE PREMISE IS TRUE AND IT ARGUES THE WRONG WAY. A latch + # under a name no unit answers to is not protection an operator has - it + # is a key nothing reads, which is the definition of the state this + # method exists to repair. Counting it as protection made the guard + # return early on precisely the install that needed it, and that is not + # hypothetical: it is where the migration above USED to put the latch + # on every declared roster, so the guard was blinded by its own upgrade. + # + # THE MIGRATION FIX DOES NOT REACH THOSE INSTALLS. A store stamped by + # the shipped version already has the marker, so `migrate_unit_keys()` + # returns at the version check and never runs again. This guard is the + # ONLY repair path for a site that upgraded before this card, and it + # re-arms onto `inverters[0]` - which lands the latch back under a name + # the roster reads. + # ⚠️ AND "OFF THE ROSTER" IS NOT ENOUGH ON ITS OWN - IT HAS TO BE THE + # MIGRATION'S OWN FINGERPRINT, WHICH IS `UNIT_A`. The first draft of + # this treated EVERY off-roster latch as litter, and test_inverter.py's + # DEV-02 restart case caught it: there the store carries + # `unit/inverter-b/sign_tripped = True` from B's own trip while the + # roster being graded holds only A, and the blunt rule re-armed a + # HEALTHY A off the flat key that B's trip had mirrored - DEV-02's + # defect rebuilt inside its own guard, which is exactly what the + # paragraph above was right to be afraid of. + # + # `UNIT_A` is the discriminator because it is the ONLY name the broken + # migration ever wrote. `roster_names()` falls back to a POSITIONAL + # `inverter-N`, so `inverter-a` is not a name this code hands to anyone + # else. A latch stranded under any OTHER name is a real unit that is + # out of today's config - a roster is configuration and it changes - + # and it still counts as protection. + # + # ⚠️ THE RESIDUAL, STATED RATHER THAN LEFT TO BE FOUND: a site whose + # own first unit is NAMED `inverter-a`, latched, and then taken out of + # the roster gets re-armed on `inverters[0]`. That is a healthy unit + # 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. + on_roster = {inv.name for inv in self.inverters} + stranded, namespaced_latched = [], False + for k, v in data.items(): + if not (k.startswith("unit/") and k.endswith("/sign_tripped")): + continue + if not bool(v): + continue + # `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: + stranded.append(k) if namespaced_latched or any(inv.sign_wd.tripped for inv in self.inverters): return + if stranded: + _LOG.error( + "SAFETY-44: this store carries a LATCHED sign-disagreement " + "failsafe under %s, and that unit is NOT on today's roster " + "%s - so no machine reads it and the site came up DRIVING on " + "a state file that says it is latched. Re-arming below.", + ", ".join(sorted(stranded)), sorted(on_roster)) first = self.inverters[0] # The one state this method exists for. reason = str(data.get("sign_reason", "") or "") diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index 324f270..90ecbd4 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -253,7 +253,7 @@ class MigrationReport: return out -def migrate_unit_keys(store, unit: str) -> MigrationReport: +def migrate_unit_keys(store, unit: str, roster=None) -> MigrationReport: """Move the pre-DEV-02 flat per-inverter keys into `unit`'s namespace. `unit` is the FIRST inverter in the roster, which on every install that can @@ -261,6 +261,24 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: version that knew about more than one inverter is already at SCHEMA_VERSION and is not touched. + ⚠️ `roster` IS THE NAMES THE MACHINES READ, AND IT IS SAFETY-44. This + function shipped WITHOUT it, taking a hardcoded `inverter.UNIT_A` at its one + call site - which is, exactly, the MAINT-05 defect `_verify_moves()` spends + a paragraph describing, still live in the sibling that paragraph was written + beside. On a store that ran pre-DEV-02 flat and declares + `inverters: [garage, shed]`, the latch moved to `unit/inverter-a/ + sign_tripped`, was read back under that same name, was SATISFIED, and + stamped the marker - so the next start skipped the migration too and NO + inverter came up latched on an install whose state file says `sign_tripped: + true`. A disarmed SAFETY-02 guard, every symptom of a clean migration. + + `roster_names(opts)` answers this BEFORE any `Inverter` exists - it is + defined to, for `MqttPublisher`, which is constructed before the roster for + the same reason - so the "there is no roster yet" that kept this argument + out was never true. On the flat shape it returns `[UNIT_A]`, so the upgrade + every install runs today passes `unit == roster[0] == UNIT_A` and is + byte-identical. + THE THREE RULES, each one a case that would otherwise be a wrong answer: 1. IT NEVER OVERWRITES AN EXISTING NAMESPACED VALUE. A store that has both @@ -302,7 +320,7 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: rep.ran = True _move_flat_keys(store, unit, LEGACY_UNIT_KEYS, rep) - _verify_moves(store, rep) + _verify_moves(store, rep, roster) if rep.ok: store.set(SCHEMA_KEY, SCHEMA_VERSION) @@ -383,12 +401,26 @@ def _verify_moves(store, rep: MigrationReport, roster=None) -> None: itself. `test_maintenance.py` section 9c calls this twice with the SAME wrong `unit` over one store, which is the shape nothing in the suite had. - `roster=None` means "not graded", and the one caller that passes it is - `migrate_unit_keys()`, which runs BEFORE `build_roster()` for the reason its - own docstring gives - the `Inverter` objects restore their latch in their - constructors, so the keys have to be in place before there is a roster to - check against. That gap is real and is recorded as a running check rather - than left to a comment: see test_maintenance.py section 13. + `roster=None` still means "not graded", and SAFETY-44 is what it cost. This + paragraph used to end here, saying that the one caller passing it was + `migrate_unit_keys()`, "which runs BEFORE `build_roster()` ... so the keys + have to be in place before there is a roster to check against", and calling + that gap real. THE FIRST HALF IS TRUE AND THE CONCLUSION IS NOT. The keys do + have to be written before the `Inverter`s restore from them - but the NAMES + do not come from the `Inverter`s. `roster_names(opts)` reads them straight + off the options, before anything is constructed, which is the whole reason + it exists (`MqttPublisher` is built before the roster too). So the argument + that could have graded that write was available at the call site the entire + time, and the sibling function three screens down had already been fixed by + passing it. + + ⚠️ SO NO CALLER PASSES `None` TODAY, and this branch is kept for one reason: + it is what a NEW caller gets by DEFAULT. A future migration that forgets the + roster gets the ungraded read-back and stamps its marker over a namespace no + machine reads - silently, exactly as this one did. That is the shape of both + defects, and the gap is recorded as a running check rather than left to a + comment: test_maintenance.py section 13, and test_rehearse.py part 5, which + drives it end-to-end through the harness an operator actually runs. """ for target, value in rep.moved.items(): got = getattr(store, "data", {}).get(target, "<>") diff --git a/goodwe_controller/test_maintenance.py b/goodwe_controller/test_maintenance.py index 9d5429d..0040752 100644 --- a/goodwe_controller/test_maintenance.py +++ b/goodwe_controller/test_maintenance.py @@ -34,7 +34,7 @@ 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, - migrate_maint_keys, unit_key, + SCHEMA_KEY, SCHEMA_VERSION, migrate_maint_keys, unit_key, ) fails = [] @@ -1411,39 +1411,33 @@ check("...while the SAME site with the house importing more than the drain " # --------------------------------------------------------------------------- # print() -print("13. RECORDED, NOT FIXED: SAFETY-02's migration has the same " - "wrong-namespace hole, and it is a LATCH") +print("13. FIXED (SAFETY-44): SAFETY-02's migration takes the roster too, and " + "the guard is no longer defeated by the key the defect wrote") -# ⚠️ FOUND WHILE FIXING THIS CARD'S OWN INSTANCE, AND WRITTEN DOWN AS A RUNNING -# CHECK RATHER THAN A COMMENT SO IT CANNOT BE FORGOTTEN OR QUIETLY "FIXED" BY AN -# UNRELATED EDIT. It is the SAME defect one module over, and it costs more. +# ⚠️ THIS SECTION USED TO BE THE SPECIFICATION OF A DEFECT. It was written while +# fixing MAINT-05's own instance, as a RUNNING check rather than a comment, with +# the instruction that "the card that fixes it should expect them to go red and +# should replace them". SAFETY-44 is that card and this is that replacement. # -# WHAT IT IS. `Controller.__init__` calls `migrate_unit_keys(store, UNIT_A)` - -# a hardcoded constant, where the maintenance migration now passes the roster's -# own first name. It has to run BEFORE `build_roster()`, because each -# `Inverter` restores its sign latch in its constructor, so there is no roster -# to name yet and `_verify_moves()` is passed `roster=None`. On a DECLARED -# roster the pre-DEV-02 flat `sign_tripped: true` therefore lands under -# `unit/inverter-a/sign_tripped`, which no unit on this site reads. +# WHAT IT WAS. `Controller.__init__` called `migrate_unit_keys(store, UNIT_A)` - +# a hardcoded constant, where the maintenance migration already passed the +# roster's own first name. On a DECLARED roster the pre-DEV-02 flat +# `sign_tripped: true` landed under `unit/inverter-a/sign_tripped`, which no +# unit on the site reads, and `_verify_moves()` read it back under the name it +# had been handed and was satisfied. # -# WHY THE EXISTING GUARD DOES NOT CATCH IT, WHICH IS THE PART WORTH PINNING. -# `_grade_store_migration()` re-arms the latch when the flat key says tripped -# and NOTHING NAMESPACED IS LATCHED. The misdirected write IS namespaced and IS -# latched, so `namespaced_latched` is True, the guard returns, and no inverter -# on the roster comes up tripped. The guard is defeated by the very key the -# defect wrote. +# AND THE GUARD DID NOT CATCH IT, WHICH WAS THE PART WORTH PINNING. +# `_grade_store_migration()` re-arms when the flat key says tripped and NOTHING +# NAMESPACED IS LATCHED - and the misdirected write IS namespaced and IS +# latched. The guard was defeated by the very key the defect wrote. # -# WHY IT IS NOT FIXED HERE. The remedy is to name the migration's target from -# the config before any `Inverter` exists, which means factoring -# `build_roster()`'s naming rule out of `build_roster()` so there is still only -# ONE spelling of it - inverter.py and SAFETY-02's latch semantics, with its own -# mutation battery over test_inverter.py and test_watchdog.py. MAINT-05 moves -# the maintenance history and nothing about the sign latch. -# -# ⚠️ AND IT IS A DISARMED SAFETY GUARD, NOT LOST HISTORY. Read the checks below -# as the SPECIFICATION OF A DEFECT: every one of them is asserting the WRONG -# behaviour, on purpose, and the card that fixes it should expect them to go -# red and should replace them. +# ⚠️ THE REASON IT WAS LEFT - "there is no roster to name yet, because each +# `Inverter` restores its latch in its constructor" - WAS THE HALF-TRUTH THAT +# KEPT IT ALIVE. The keys must be written before the `Inverter`s read them; the +# NAMES do not come from the `Inverter`s. `roster_names(opts)` reads them off +# the options before anything is constructed, which is what it already existed +# to do for `MqttPublisher`. The factoring this section said the fix would need +# had been done a card earlier. # # ⚠️ NO `schema_version` IN THIS STORE, AND THAT IS THE PRE-DEV-02 SHAPE # ITSELF: an install that has never run a version with namespaced keys, which is @@ -1457,25 +1451,74 @@ print(f" latch keys in the store: " f"{sorted(k for k in _lat_store.data if k.endswith('sign_tripped'))}") print(f" roster came up tripped: " f"{[(i.name, i.sign_wd.tripped) for i in _lat.inverters]}") -check("the flat pre-DEV-02 latch is migrated into `unit/inverter-a/` - a name " - "that is NOT on this roster, so no unit reads it. This is the same " - "wrong-namespace write `migrate_maint_keys()` now refuses, in the " - "migration that runs before there is a roster to grade against", - _lat_store.data.get(unit_key(UNIT_A, "sign_tripped")) is True - and unit_key(_C2_NAMES[0], "sign_tripped") not in _lat_store.data) -check("⚠️ AND NO INVERTER ON THE ROSTER COMES UP LATCHED, on a store whose file " - "still reads `sign_tripped: true`. That is DEV-02's own sentence for the " - "worst case - a disarmed safety guard on an install that believes it is " - "protected", - not any(i.sign_wd.tripped for i in _lat.inverters) +check("⭐ the flat pre-DEV-02 latch is migrated into the ROSTER'S OWN first " + "namespace and NOT into `unit/inverter-a/` - the same wrong-namespace " + "write `migrate_maint_keys()` refuses, now refused in the migration that " + "runs before the `Inverter` objects exist", + _lat_store.data.get(unit_key(_C2_NAMES[0], "sign_tripped")) is True + and unit_key(UNIT_A, "sign_tripped") not in _lat_store.data) +check("⭐ AND AN INVERTER ON THE ROSTER COMES UP LATCHED, which is the whole " + "card: a store whose file reads `sign_tripped: true` no longer produces " + "a site that drives", + _lat.inverters[0].sign_wd.tripped and _lat_store.data["sign_tripped"] is True) -check("...and `_grade_store_migration()` is DEFEATED BY THE KEY THE DEFECT " - "WROTE, which is why this needs a card rather than a tweak: its " - "'is anything namespaced latched' test is satisfied by the misdirected " - "value itself, so it returns instead of re-arming", - any(k.startswith("unit/") and k.endswith("/sign_tripped") and v - for k, v in _lat_store.data.items()) - and not any(i.sign_wd.tripped for i in _lat.inverters)) +check("...and the latch that came back carries the REASON, not just the " + "boolean - an operator meeting a FAILSAFE banner with no sentence under " + "it has been told the site stopped and not why", + "SIGN_DISAGREEMENT" in _lat.inverters[0].sign_wd.reason) +check("⚠️ ...and the OTHER units on the roster are NOT latched by it. The flat " + "key is one site-level boolean and re-arming every unit off it would be " + "DEV-02's defect in the opposite direction - a healthy pack parked " + "because its sibling once disagreed", + not any(i.sign_wd.tripped for i in _lat.inverters[1:])) + +# ⚠️ AND THE OTHER PATH, WHICH THE MIGRATION FIX ALONE DOES NOT REACH: a store +# ALREADY STAMPED by the shipped version. `migrate_unit_keys()` returns at its +# `from_version >= SCHEMA_VERSION` check and never moves anything, so the +# stranded `unit/inverter-a/sign_tripped` just sits there. This is the STAGED +# shape - "upgrade now, declare the roster later" - and it is the one an +# operator actually produces. Only the GUARD repairs it. +_staged_store = MemStore({ + "sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured pre-DEV-02", + unit_key(UNIT_A, "sign_tripped"): True, + unit_key(UNIT_A, "sign_reason"): "SIGN_DISAGREEMENT: measured pre-DEV-02", + SCHEMA_KEY: SCHEMA_VERSION}) +_staged, _ = controller(NAMED_OPTS, store=_staged_store) +print(f" already-stamped store + declared roster -> ran=" + f"{_staged.store_migration.ran}, came up tripped: " + f"{[(i.name, i.sign_wd.tripped) for i in _staged.inverters]}") +check("⚠️ PRECONDITION: the migration really does NOT run on this store - the " + "marker is already there - so anything below is the guard's doing and " + "not the migration's", + not _staged.store_migration.ran) +check("⭐ and the roster's first unit STILL comes up latched: the guard no " + "longer counts a latch stranded under `UNIT_A`, which is not on this " + "roster, as protection an operator has", + _staged.inverters[0].sign_wd.tripped) +check("...and the re-armed latch is written where a machine reads it, so the " + "NEXT start finds it without needing the guard again", + _staged_store.data.get(unit_key(_C2_NAMES[0], "sign_tripped")) is True) + +# ⚠️ THE NEGATIVE CONTROL FOR THAT NARROWING, because "off the roster" on its +# own was too blunt and test_inverter.py's DEV-02 restart case caught it: a +# latch stranded under a name that is NOT `UNIT_A` is a real unit out of +# today's config, and it MUST still count as protection. +_other_store = MemStore({ + "sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: B disagreed, mirrored flat", + unit_key("inverter-b", "sign_tripped"): True, + unit_key("inverter-b", "sign_reason"): "SIGN_DISAGREEMENT: B disagreed", + SCHEMA_KEY: SCHEMA_VERSION}) +_other, _ = controller(NAMED_OPTS, store=_other_store) +print(f" stranded latch under `inverter-b` -> came up tripped: " + f"{[(i.name, i.sign_wd.tripped) for i in _other.inverters]}") +check("⚠️ a latch stranded under a name that is not `UNIT_A` is NOT re-armed " + "onto the roster: it belongs to a unit the operator has taken out of " + "the config, the flat key is its own mirror, and re-arming a healthy " + "sibling off it is the defect DEV-02 removed", + not any(i.sign_wd.tripped for i in _other.inverters)) + # 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, diff --git a/goodwe_controller/test_rehearse.py b/goodwe_controller/test_rehearse.py index ba24b51..33c0219 100644 --- a/goodwe_controller/test_rehearse.py +++ b/goodwe_controller/test_rehearse.py @@ -587,33 +587,32 @@ shutil.rmtree(Path(_tree2).parent, ignore_errors=True) # --------------------------------------------------------------------------- # print() -print("5. SAFETY-44: a pre-DEV-02 flat store meeting a DECLARED ROSTER, which " - "is the path nobody had established either way") +print("5. SAFETY-44: a pre-DEV-02 flat store meeting a DECLARED ROSTER - " + "FIXED, with each half of the fix as its own negative control") # --------------------------------------------------------------------------- # -# The question on the card: `migrate_unit_keys(store, UNIT_A)` takes a hardcoded -# constant and has no `misdirected` guard, while `migrate_maint_keys(store, -# unit, roster)` takes the roster and does. Is that asymmetry safe? +# The question on the card: `migrate_unit_keys(store, UNIT_A)` took a hardcoded +# constant and had no `misdirected` guard, while `migrate_maint_keys(store, +# unit, roster)` takes the roster and does. That asymmetry was NOT safe, and +# this part used to assert the defect. It now asserts the FIX. # -# ⚠️ MEASURED, NOT ARGUED. The two checks below are the answer, and they are -# written as REQUIREMENTS so that a fix to `migrate_unit_keys` turns them red -# and this file has to be updated deliberately rather than drifting. +# ⚠️ THE FIX IS TWO EDITS AND THEY DO DIFFERENT JOBS, WHICH IS THE WHOLE REASON +# THIS PART IS SHAPED THE WAY IT IS. Measured, per leg, by reverting each edit +# on its own (5b and 5c below): +# +# SAME-STEP leg STAGED leg +# both edits (shipped) clean latch armed, litter +# guard reverted latch armed ⚠️ DISARMED +# call site reverted latch armed, litter latch armed, litter +# +# So the GUARD carries the safety and the CALL SITE carries the hygiene. Saying +# that plainly matters more than it looks: a reader who assumes the roster-aware +# migration is what re-arms the latch would delete the guard as redundant during +# a tidy-up and take the STAGED column with it - and STAGED is the path +# "upgrade now, declare the roster later" produces, which is the path an +# operator actually takes. code44, out44 = run(HERE, "state-legacy.json", "roster.yaml") -print(f" flat store + roster ['garage','shed'] -> exit {code44}") -check("⚠️ SAFETY-44 IS A DEFECT AGAIN: a store that ran pre-DEV-02 flat and " - "declares a multi-inverter roster comes up with its SAFETY-02 latch " - "DISARMED - the file says `sign_tripped: true` and not one inverter is " - "latched", - code44 == RR.EXIT_FINDING and "LATCH CAME UP DISARMED" in out44) -check("...and the harness names the two latch keys SAFETY-44 asks about, by " - "the namespace they were stranded in", - "unit/inverter-a/sign_tripped" in out44 - and "unit/inverter-a/sign_reason" in out44) -check("...and the mechanism is visible in the report: the keys are stranded " - "under `inverter-a`, which is not on the roster, so no machine reads them", - "STRANDED STORE KEY" in out44 and "['garage', 'shed']" in out44) -# ⚠️ PER LEG, NOT A COUNT OVER THE WHOLE REPORT. See `legs()` for the three -# builds this distinction was measured on and for what the count missed. _L = legs(out44) +print(f" flat store + roster ['garage','shed'] -> exit {code44}") print(f" legs in the report: {sorted(_L)}") for _leg in ("STAGED", "SAME-STEP"): print(f" LEG {_leg}: " @@ -622,74 +621,125 @@ check("the report is split into legs this check can grade SEPARATELY - the " "tool prints a `LEG ` marker, because counting a finding's text over " "the whole report cannot tell one red leg from two", set(_L) == {"STAGED", "SAME-STEP"}) -check("⚠️ SAME-STEP is disarmed: the roster is declared in the SAME step as " - "the upgrade, the migration runs and misdirects the latch to " - "`inverter-a`", - "LATCH CAME UP DISARMED" in _L.get("SAME-STEP", "")) -check("⚠️ AND IT IS NOT THE SAME-STEP PATH ALONE - the STAGED leg reaches the " - "same disarmed latch by a DIFFERENT route: the flat start stamps " - "`schema_version`, so the roster start's migration takes its early " - "return and the latch never moves at all", - "LATCH CAME UP DISARMED" in _L.get("STAGED", "")) - -# ⚠️ AND THE CONTRAST, WHICH IS WHAT STOPS THE CHECK ABOVE BEING VACUOUS. The -# SAME state file onto a roster whose first entry IS `inverter-a` comes up -# LATCHED. The two runs differ in one thing - the first roster name - so the -# finding is attributable to it and to nothing else. -code_ok, out_ok = run(HERE, "state-legacy.json", "roster-inverter-a.yaml") +check("⭐ SAFETY-44 IS FIXED ON BOTH PATHS: a store that ran pre-DEV-02 flat " + "and declares a multi-inverter roster comes up LATCHED - neither leg " + "reports a disarmed guard", + "LATCH CAME UP DISARMED" not in out44) +check("...and the latch is under a name THE ROSTER READS, which is the whole " + "of the claim - `garage` is on the roster and `inverter-a` is not", + all("after : which units came up latched : ['garage']" in v + for v in _L.values())) +check("⚠️ the SAME-STEP leg is now CLEAN OUTRIGHT: the migration writes the " + "latch straight into the roster's first namespace, so there is no " + "stranded copy left behind and nothing for an operator to read past", + "none in LEG SAME-STEP" in _L.get("SAME-STEP", "")) +check("⚠️ ...while the STAGED leg still reports the two STRANDED keys, and " + "that is REPORTED not hidden: its flat start stamped `schema_version` " + "before the roster existed, so the migration takes its early return and " + "the old copies stay in the file with nothing reading them. The latch is " + "armed anyway - by the guard - and the litter is true", + "STRANDED STORE KEY" in _L.get("STAGED", "") + and "unit/inverter-a/sign_tripped" in _L.get("STAGED", "")) +check("...and the run as a whole is a FINDING rather than silence, because " + "keys nothing reads are still keys nothing reads", + code44 == RR.EXIT_FINDING) check("...and no leg's text has swallowed the CLOSING SUMMARY, which repeats " "every finding from every leg - the failure that made the last leg read " "red whenever any leg was", not any("FINDING(S) - this transition is not clean" in v for v in _L.values())) +check("⚠️ and the key that MOVED is not reported as lost: the before-snapshot " + "holds `unit/inverter-a/sign_tripped = True` and the after-snapshot " + "holds it under `garage`, three lines apart in the same report. A " + "VANISHED finding there is the harmless-case alarm CONTROL-C exists to " + "prevent", + "migrated unit/inverter-a/sign_tripped = True -> " + "unit/garage/sign_tripped" in out44 + and "VANISHED STORE KEY" not in out44) # --------------------------------------------------------------------------- # print() -print("5b. THE SPLITTER'S POSITIVE CONTROL: a build in which exactly ONE leg " - "is clean") +print("5b. NEGATIVE CONTROL, AND THE SPLITTER'S POSITIVE ONE: revert THE " + "GUARD, and exactly one leg goes red") # --------------------------------------------------------------------------- # # ⚠️ A PER-LEG CHECK THAT HAS NEVER SEEN THE LEGS DISAGREE IS A COUNT WITH -# EXTRA STEPS. Every fixture above has both legs red, so nothing so far -# separates "reads each leg" from "reads the whole report twice". This part -# builds the one case that does: `migrate_unit_keys()` handed the roster's own -# first name instead of the hardcoded `UNIT_A`. -# -# ⚠️ AND WHAT IT MEASURES IS ALSO THE ANSWER TO "IS THAT THE FIX". It is NOT. -# It repairs the SAME-STEP path only. The STAGED path is untouched, because -# there the flat start stamps `schema_version = 1` and the roster start's -# migration takes its `from_version >= SCHEMA_VERSION` early return before the -# name it would be handed matters at all. A fix that stops at this line would -# close the half an operator is least likely to take and leave the half that -# "upgrade first, declare the roster later" produces. -_one_leg = mutant_tree([( +# EXTRA STEPS, and MUTATION.md's extension says one that has seen them disagree +# in a single arrangement is a total over that arrangement. This build is 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. +_no_guard = mutant_tree([( "app/main.py", - " self.store_migration = migrate_unit_keys(store, UNIT_A)", - " self.store_migration = migrate_unit_keys(store, " - "roster_names(opts)[0])")]) -_ol_code, _ol_out = run(Path(_one_leg), "state-legacy.json", "roster.yaml") -_OL = legs(_ol_out) + " if unit in on_roster or unit != UNIT_A:", + " 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) for _leg in ("STAGED", "SAME-STEP"): print(f" LEG {_leg}: " - f"{'DISARMED' if 'LATCH CAME UP DISARMED' in _OL.get(_leg, '') else 'clean'}") -check("⭐ with `migrate_unit_keys()` handed the roster's own first name, the " - "SAME-STEP leg comes up CLEAN - so the splitter really does read the " - "legs separately, and a check that only counted would have missed this", - "LATCH CAME UP DISARMED" not in _OL["SAME-STEP"]) -check("⚠️ ...while the STAGED leg is STILL DISARMED on the same build: that " - "line is not the whole fix, because the staged path never reaches it - " - "the first start stamps `schema_version` and the second start's " - "migration returns early", - "LATCH CAME UP DISARMED" in _OL["STAGED"]) -check("...and the run as a whole is still a finding, so nobody reads this " - "partial fix as done", - _ol_code == RR.EXIT_FINDING) -shutil.rmtree(Path(_one_leg).parent, ignore_errors=True) + f"{'DISARMED' if 'LATCH CAME UP DISARMED' in _NG.get(_leg, '') else 'clean'}") +check("⭐ with the off-roster clause reverted the STAGED leg comes up " + "DISARMED - so the guard's roster test is load-bearing, and it is the " + "half of the fix that repairs an install already stamped by the shipped " + "version", + "LATCH CAME UP DISARMED" in _NG.get("STAGED", "")) +check("⚠️ ...while the SAME-STEP leg stays ARMED on the very same build - the " + "legs DISAGREE, so the splitter really does read them separately and a " + "check that only counted findings would have missed this", + "LATCH CAME UP DISARMED" not in _NG.get("SAME-STEP", "")) +check("...and the run is a finding, so the partial build is never read as " + "clean", + _ng_code == RR.EXIT_FINDING) +shutil.rmtree(Path(_no_guard).parent, ignore_errors=True) +# --------------------------------------------------------------------------- # print() -print("5c. back to the shipped code") +print("5c. NEGATIVE CONTROL: revert THE CALL SITE, and the litter comes back " + "without the latch going out") +# --------------------------------------------------------------------------- # +# ⚠️ THIS IS THE CONTROL THAT KEEPS 5b HONEST ABOUT WHAT IT PROVED. 5b shows +# the guard carries the latch; on its own that is an argument for deleting the +# roster-aware migration as decoration. This build is the measurement instead: +# hand `migrate_unit_keys()` the hardcoded `UNIT_A` again, with the guard left +# fixed. The latch stays armed - the guard catches it - and the SAME-STEP leg +# gains the two stranded keys it does not have on the shipped code. The +# migration fix buys a store with nothing dead in it, and buys the +# `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)") +_old_call = mutant_tree([( + "app/main.py", _CALL_NOW, + " self.store_migration = migrate_unit_keys(store, UNIT_A)")]) +_oc_code, _oc_out = run(Path(_old_call), "state-legacy.json", "roster.yaml") +_OC = legs(_oc_out) +for _leg in ("STAGED", "SAME-STEP"): + print(f" LEG {_leg}: " + f"{'DISARMED' if 'LATCH CAME UP DISARMED' in _OC.get(_leg, '') else 'clean'}") +check("⚠️ with `UNIT_A` handed back to the migration the latch is STILL " + "armed on both legs - the guard is what re-arms it, and saying so is " + "the difference between this fix and a story about it", + "LATCH CAME UP DISARMED" not in _oc_out) +check("⭐ ...but the SAME-STEP leg is no longer clean: the two SAFETY-02 keys " + "are written under `inverter-a` on a roster of ['garage', 'shed'] and " + "nothing reads them - which is the state the shipped code does not " + "produce", + "STRANDED STORE KEY" in _OC.get("SAME-STEP", "") + and "none in LEG SAME-STEP" not in _OC.get("SAME-STEP", "")) +shutil.rmtree(Path(_old_call).parent, ignore_errors=True) + +# --------------------------------------------------------------------------- # +print() +print("5d. THE CONTRAST: the same file onto a roster that keeps the name") +# --------------------------------------------------------------------------- # +# ⚠️ AND THE CONTRAST, WHICH IS WHAT STOPS EVERY CHECK ABOVE BEING VACUOUS. The +# SAME state file onto a roster whose first entry IS `inverter-a` comes up +# LATCHED AND CLEAN. The two runs differ in one thing - the first roster name - +# so a finding above is attributable to it and to nothing else. +code_ok, out_ok = run(HERE, "state-legacy.json", "roster-inverter-a.yaml") check("the SAME state file onto a roster whose first entry IS `inverter-a` " - "comes up latched and clean - so the finding above is caused by the " - "roster NAME and not by the fixture being tripped", + "comes up latched and clean - so anything part 5 reports is caused by " + "the roster NAME and not by the fixture being tripped", code_ok == RR.EXIT_OK and "LATCH CAME UP DISARMED" not in out_ok) diff --git a/goodwe_controller/tools/rehearse_roster.py b/goodwe_controller/tools/rehearse_roster.py index 69e2150..cc5acc6 100644 --- a/goodwe_controller/tools/rehearse_roster.py +++ b/goodwe_controller/tools/rehearse_roster.py @@ -460,6 +460,7 @@ class Snapshot: # finding: see `diff()`. self.stranded_at_default = [] self.vanished_at_default = [] + self.vanished_but_migrated = [] @property def unit_keys(self): @@ -550,6 +551,29 @@ def diff(before: Snapshot, after: Snapshot) -> list: if value in (None, DEFAULTS.get(bare)): after.vanished_at_default.append(f"{key} = {value!r}") continue + # ⚠️ AND A KEY THAT MOVED TO A NAMESPACE A MACHINE READS HAS NOT + # VANISHED, WHICH IS THE SAME CORRECTION ONE PARAGRAPH UP, ONE CARD + # LATER. SAFETY-44's fix made `migrate_unit_keys()` write under the + # ROSTER's first name instead of the hardcoded `inverter-a`, so on a + # renamed roster the before-snapshot holds `unit/inverter-a/ + # sign_tripped = True` and the after-snapshot holds + # `unit/garage/sign_tripped = True`. Measured on that build: two + # VANISHED findings for a LATCH THAT IS SITTING ON THE SAME PAGE OF THE + # REPORT, three lines above them, under the name the roster reads. + # + # ⚠️ 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. + 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: + after.vanished_but_migrated.append( + f"{key} = {value!r} -> {moved_to[0]}") + continue findings.append(Finding( "VANISHED STORE KEY", f"{key} = {value!r} was in the store before the " @@ -846,6 +870,15 @@ def report_transition(leg_id, name, before, after, findings) -> None: mark = " " if key in after.data else " (absent, defaulted) " print(f" {key}{mark}") + if after.vanished_but_migrated: + print() + print(" keys that MOVED to the namespace this roster reads - not " + "lost, and shown") + print(" because a rename that relocates a LATCH is the operator's " + "business:") + for text in after.vanished_but_migrated: + print(f" migrated {text}") + if after.stranded_at_default or after.vanished_at_default: print() print(" keys whose value IS the default `Store.load()` supplies, so "