diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 6eb01ac..a97875a 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -1623,37 +1623,99 @@ check, **before** the roster is declared: Developer tools -> Template: {{ states | map(attribute='entity_id') | select('search','goodwe') | join('\n') }} - ⚠️ **Without it the tool cannot see a change to the id composition at all**, - because both sides of its own before/after diff are composed by the same - build: rename the MQTT device and every id moves equally on both sides, and - the diff reports "nothing orphaned" about a change that orphans everything. - `tools/rehearsal/live-entities-recorded.txt` is the recorded stand-in that - keeps the check live in CI; the real export is what makes it true of *this* - install. The install carries roughly 49 `goodwe*` entities and this add-on - announces eight of them — the tool separates the ids it owns from the ids it - does not, rather than claiming orphans it cannot cause. + ⚠️ **Omitting it is a REFUSAL (exit 2), not a quieter run.** The orphan check + is graded against the ids the install actually has, and without that file it + is not graded against reality at all — so the tool will not run rather than + hand back a clean bill of health for a half of the transition it did not + check. If you genuinely have to run without it, pass `--no-live-entities`: + the run then carries a standing banner in the report header **and** on the + exit line saying the orphan check was not graded. The asymmetry used to be + the other way round — an unreadable file refused, a missing flag passed with + exit 0 and no disclosure at all — and the owner runs this once, at + commissioning, under time pressure. + + The install carries roughly 49 `goodwe*` entities and this add-on announces + eight of them. The tool separates the ids it owns from the ids it does not, + rather than claiming orphans it cannot cause. + +### The reference, which is loaded on every run + +`--reference` defaults to `tools/rehearsal/announced-reference.txt` and is read +**whether or not** you pass an entity list. It records what the build is +supposed to announce, three columns per row: + + + +⚠️ **All three, and the second and third are the ones an entity_id-only list +cannot see.** The tool's own before/after diff is composed twice by the same +build, so a change to the composition moves both sides equally and the diff +reports nothing. Worse, two changes orphan an install *without moving a single +composed entity_id*: renaming `mqtt.BASE` (every `unique_id` changes) and +changing `DEVICE["identifiers"]` (HA sees a new device). Home Assistant keys its +entity registry on `unique_id` and its device registry on the identifier, so +either one creates a **new** entity beside the old — and because discovery is +published `retain=True` with no removal payload, the old config stays on the +broker and the old entity stays in the registry, still named by every automation +that referenced it, never updating again. Both were measured coming back +**exit 0** before the reference carried three columns. + +`object_id` is deliberately **not** a column: `app/mqtt.py` records that HA +2026.8 does not honour it in a discovery payload, so renaming one is a true +no-op, and `test_rehearse.py` requires that mutation to **survive** — otherwise +"the reference notices any edit to `mqtt.py`" would be indistinguishable from +"the reference notices what HA keys on". ⚠️ **No result in this repository was produced against real state.** Everything recorded here and in `test_rehearse.py` was measured on the synthetic fixtures in `tools/rehearsal/`. -### What it found on the synthetic fixtures +### What it confirms on the synthetic fixtures -⚠️ **A pre-DEV-02 flat store that declares a multi-inverter roster comes up with -its SAFETY-02 latch DISARMED, unless the first roster entry happens to be named -`inverter-a`.** `Controller.__init__` calls `migrate_unit_keys(store, UNIT_A)` -with a hardcoded constant, so the latch lands in `unit/inverter-a/sign_tripped`; -a roster of `["garage", "shed"]` reads `unit/garage/sign_tripped`, which is -absent. And `_grade_store_migration()` — the guard written for exactly this — -sees the migration's own misdirected key satisfy its `namespaced_latched` test -and returns without re-arming. The file says `sign_tripped: true`, the banner is -green, and nothing is held at 0 W. **This is SAFETY-44, and the answer to it is -that the asymmetry is not safe on that path.** It is reported here and pinned by -`test_rehearse.py` part 5; the fix belongs to its own card, shown red first. +⚠️ **This is a SECOND INSTRUMENT reaching a conclusion this repository already +holds, not a discovery.** `test_maintenance.py` section 13 — *"RECORDED, NOT +FIXED: SAFETY-02's migration has the same wrong-namespace hole, and it is a +LATCH"* — already states it, already names `namespaced_latched`, and already +carries the sentence *"the guard is defeated by the very key the defect wrote"*. +That file is on `release/1.0` and this card does not touch it. What is new here +is that the finding is now reachable **from a state file the owner supplies** +rather than only from a fixture built in a test, and that it is reached by a +different route: a full `Controller` start diffed against its predecessor, +rather than assertions over a `MemStore`. Two instruments, one answer. -The same end state is reached whether the roster is declared in the same step as -the upgrade or a restart later — in the staged case because the flat start -stamps `schema_version` and the roster start's migration then returns early. +**The finding.** A pre-DEV-02 flat store that declares a multi-inverter roster +comes up with its SAFETY-02 latch **DISARMED**, unless the first roster entry +happens to be named `inverter-a`. `Controller.__init__` calls +`migrate_unit_keys(store, UNIT_A)` with a hardcoded constant, so the latch lands +in `unit/inverter-a/sign_tripped`; a roster of `["garage", "shed"]` reads +`unit/garage/sign_tripped`, which is absent. And `_grade_store_migration()` — +the guard written for exactly this — sees the migration's own misdirected key +satisfy its `namespaced_latched` test and returns without re-arming. The file +says `sign_tripped: true`, the banner is green, and nothing is held at 0 W. + +**Both routes reach it, and they are NOT the same defect.** The rehearsal +reports them as separate legs because they need separate fixes: + +| leg | how it gets there | +|---|---| +| `SAME-STEP` | the roster is declared in the same step as the upgrade. The migration runs, and is handed `UNIT_A`. | +| `STAGED` | the flat config runs once under 0.7.0+ first. That start stamps `schema_version = 1`, so the roster start's migration takes its `from_version >= SCHEMA_VERSION` early return and **the latch never moves at all**. | + +⚠️ **So "hand `migrate_unit_keys()` the roster's own first name" is NOT the +whole fix, and the rehearsal measures that rather than asserting it.** +`test_rehearse.py` part 5b builds exactly that change and reports: + + LEG STAGED: DISARMED + LEG SAME-STEP: clean + +The staged path never reaches the line that was changed. A fix that stops there +closes the half an operator is least likely to take and leaves the half that +"upgrade first, declare the roster later" produces. The guard side — +`namespaced_latched` counting a namespace that is not on the roster — is what +covers both, and a change there turns every SAFETY-44 check in part 5 red, which +is what a fix should do to a pin that specifies a defect. + +The fix belongs to its own card, shown red first. Part 5 pins the current +behaviour as a requirement so it cannot drift green by accident. ### The reverse direction: the retreat is supported, the round trip is not @@ -1666,9 +1728,20 @@ is supported and does not corrupt state.** Measured on the fixtures: so a single-FSM add-on finds a current history rather than one dated the day the roster was declared; * **it orphans eight Home Assistant entities** — the four per-unit rows for each - of the two units. Their history stops and any automation naming them stops - firing, silently. That is the price of the retreat and it is not recoverable - by advancing again: HA keeps the id it composed at first discovery. + of the two units. **Measured:** the add-on stops announcing them and stops + publishing their state, so their history gaps and any automation whose + trigger names them stops firing, silently and without an error. + + ⚠️ **What is NOT measured, and was previously overstated here:** whether + advancing again restores them. An earlier draft of this section said the + orphaning was "not recoverable by advancing again: HA keeps the id it composed + at first discovery" — that reasoning cuts the other way, since keeping the + composed id is precisely what would let a re-advertised entity land back on + it, and discovery is published `retain=True` with no removal payload. Nothing + in this repository has tested a re-advance against a real Home Assistant, so + the claim is withdrawn rather than restated in the other direction. Plan the + retreat around the measured part: **the eight rows stop updating for as long + as you are retreated.** ⚠️ **What is NOT supported is the round trip.** A maintenance cycle that completes *while the install is rolled back* is **discarded** when it diff --git a/goodwe_controller/MUTATION.md b/goodwe_controller/MUTATION.md index fd741ab..ad2a48c 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -251,6 +251,38 @@ same place twice, the check is decoration. this comparison come from* — and if a mutation of the subject moves both sides, the instrument is decoration. + ⚠️ **AND REVIEW FOUND TWO MORE IN THE SAME CARD, ONE OF THEM INSIDE THE FIX + FOR THE OTHER — the FOURTEENTH and FIFTEENTH.** Both are the pin, not the + tool, and both are the same sentence in a new costume: *a check whose data + cannot reach the failure it names.* + + * **Fourteenth — a COUNT cannot tell one red leg from two.** The rehearsal + reports two independent routes to the disarmed latch (`SAME-STEP` and + `STAGED`) and prints every finding **twice per red leg**: once in that + leg's own block and once in the closing summary. The check named *"NOT THE + SAME-STEP PATH ALONE"* was written `out.count("LATCH CAME UP DISARMED") + >= 2`, so **one** red leg always satisfied it. Measured on three builds: + shipped code (both legs red) → 4; `migrate_unit_keys()` handed the roster + name (SAME-STEP fixed) → 2, **passes**; a guard-side fix (both fixed) → 0. + The check reported `ok` on the build whose only remaining failure *is* the + path it says is not the only one. The fix is to parse the legs and require + the finding in **each**, which is habit 2 again: grade the thing, not a + total over the thing. + + * **Fifteenth — and it is INSIDE THAT FIX, which is the third time this file + has had to write that sentence.** The first leg-splitter opened on a + `LEG ` marker and never closed, so every line after the *last* marker + was attributed to that leg — including the closing summary, which repeats + every finding from every leg. The last leg therefore read red whenever + **any** leg was red, and the new per-leg check was the old count with extra + steps. It was caught by building the one fixture that separates them: a + tree in which `SAME-STEP` is genuinely clean and `STAGED` is not. The tool + now prints `END LEG ` and the splitter closes on it. + + The rule that falls out, and it is the cheap one: **a per-thing check that + has never seen the things DISAGREE is a total.** Part 5b exists to make + them disagree on every run. + * **CFG-04's `tools/supervisor_shape_probe.py` §B** — and this one is the rule reaching outside the suite. The probe is the committed evidence that `config.yaml`'s `inverters: []` default is load-bearing, and four documents diff --git a/goodwe_controller/test_rehearse.py b/goodwe_controller/test_rehearse.py index 3b3c54d..e68d0c6 100644 --- a/goodwe_controller/test_rehearse.py +++ b/goodwe_controller/test_rehearse.py @@ -6,7 +6,7 @@ history, or a Home Assistant entity that an automation is watching. This file grades the instrument. ⚠️ AN INSTRUMENT THAT HAS NEVER BEEN SEEN TO GO RED IS NOT EVIDENCE. This -repository's most-repeated defect, `MUTATION.md` habit 4 and now THIRTEEN +repository's most-repeated defect, `MUTATION.md` habit 4 and now FIFTEEN recorded instances, is "a check whose DATA cannot reach the failure it names" - and a rehearsal that reports "migration OK" because it compared a structure to itself is the purest example of it there is. So this file does two things in @@ -25,9 +25,18 @@ order, and the first one is the one that makes the second mean anything: harness that would go red on the real defect for the wrong reason too. - Part 4 ONE MUTATION THAT DELIBERATELY SURVIVES, with the reason written + Part 4 TWO MUTATIONS THAT DELIBERATELY SURVIVE, with the reason written down. A battery in which everything dies is a battery whose author - stopped as soon as the number looked good. + stopped as soon as the number looked good - and the `object_id` one + is what BOUNDS the four `app/mqtt.py` mutations above it: without a + change to that file that must NOT be caught, "the reference notices + any edit to mqtt.py" and "the reference notices what HA keys on" are + the same green. + + Part 5b THE PER-LEG CHECK'S OWN POSITIVE CONTROL: a build in which exactly + ONE of the two legs is clean. ⚠️ A per-thing check that has never + seen the things DISAGREE is a total with extra steps, and this file + shipped that defect twice - see `legs()`. Part 5 SAFETY-44's exact path, pinned as a finding, by name, in both directions. @@ -172,48 +181,13 @@ def run(tree: Path, state: str, after: str, *extra) -> "tuple[int, str]": "--before", str(fixtures / "flat.yaml"), "--after", str(fixtures / after), "--live-entities", str(fixtures / "live-entities-recorded.txt"), + "--reference", str(fixtures / "announced-reference.txt"), *extra], capture_output=True, text=True, env={**os.environ, "PYTHONDONTWRITEBYTECODE": "1"}) return proc.returncode, proc.stdout + proc.stderr -for label, (state, after) in CONTROLS.items(): - code, out = run(HERE, state, after) - check(f"{label}: the shipped code, {state} -> {after}, exits 0 with no " - f"findings", code == RR.EXIT_OK and "NO FINDINGS" in out) - -# ⚠️ AND THE SOFTENING IS NOT A HOLE. The SAME key stranded with a NON-default -# value is still a finding - that is the whole of part 5 - so the two cases are -# separated by the VALUE and not by the key name. -_fresh_code, _fresh_out = run(HERE, "state-fresh.json", "roster.yaml") -check("a fresh install DOES strand `unit/inverter-a/sign_tripped` - the key is " - "written and nothing on the roster reads it - and the report SAYS SO", - _fresh_code == RR.EXIT_OK - and "unit/inverter-a/sign_tripped = False" in _fresh_out) -check("...and does NOT call it a finding, because the value is the default; " - "the same key at True is the finding part 5 turns on, so the two cases " - "are separated by the VALUE and not by the key name", - "STRANDED STORE KEY: unit/inverter-a/sign_tripped" not in _fresh_out - and "STRANDED STORE KEY: unit/inverter-a/sign_tripped = True" - in "\n".join(l.strip() for l in - run(HERE, "state-legacy.json", "roster.yaml")[1].splitlines())) - -# ⚠️ 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. -_before_bytes = (FIX / "state-legacy.json").read_bytes() -run(HERE, "state-legacy.json", "roster.yaml", "--reverse", "--round-trip") -check("the state file the harness was pointed at is BYTE-IDENTICAL afterwards, " - "including after the reverse and round-trip legs, which write", - (FIX / "state-legacy.json").read_bytes() == _before_bytes) - - -# --------------------------------------------------------------------------- # -print() -print("3. ⭐ THE MUTATIONS: each one breaks the shipped migration or the " - "entity-id composition, and each must turn its control RED") -# --------------------------------------------------------------------------- # def mutant_tree(edits) -> str: """A private copy of the add-on with `edits` applied. @@ -243,6 +217,189 @@ def mutant_tree(edits) -> str: return str(dest) +def legs(out: str) -> dict: + """The report, split into its legs on the tool's `LEG ` markers. + + ⚠️ THIS FUNCTION IS THE FIX FOR A DEFECT IN THIS FILE, AND THE DEFECT IS + HABIT 4 ONE LEVEL UP. Part 5's check that the disarmed latch is "NOT THE + SAME-STEP PATH ALONE" was written as + `out.count("LATCH CAME UP DISARMED") >= 2`. The tool prints every finding + TWICE per red leg - once in that leg's own FINDINGS block and once in the + closing summary - so ONE red leg yields exactly 2 and the check passes. + Measured on three builds: + + shipped code, both legs red count 4 + fix at main.py:250 only -> STAGED alone red count 2 ok + partial guard fix on rep.ran -> SAME-STEP alone red count 2 ok + + The check named "NOT THE SAME-STEP PATH ALONE" reported `ok` on the build + whose remaining failure IS the same-step path alone - and, worse, applying + the fix DOCS.md names as the cause left all five part-5 checks green. A + count over a whole report cannot distinguish one leg from two; only reading + the legs separately can. + + ⚠️ AND IT CLOSES ON `END LEG`, WHICH IS THE SAME DEFECT ONE LEVEL DOWN. + The first version of this splitter opened on `LEG ` and never closed, + so every line after the LAST marker went to that leg - including the + closing summary, which repeats every finding from every leg. The last leg + then read red whenever ANY leg was red. Measured on a build whose SAME-STEP + leg is genuinely clean: it still reported SAME-STEP disarmed. Part 5b is + the running control for exactly that. + """ + out_legs, current = {}, None + for line in out.splitlines(): + if line.startswith(" LEG "): + current = line.strip()[4:].strip() + out_legs[current] = [] + elif line.startswith(" END LEG "): + current = None + elif current is not None: + out_legs[current].append(line) + return {k: "\n".join(v) for k, v in out_legs.items()} + + +for label, (state, after) in CONTROLS.items(): + code, out = run(HERE, state, after) + check(f"{label}: the shipped code, {state} -> {after}, exits 0 with no " + f"findings", code == RR.EXIT_OK and "NO FINDINGS" in out) + +# ⚠️ AND THE SOFTENING IS NOT A HOLE. The SAME key stranded with a NON-default +# value is still a finding - that is the whole of part 5 - so the two cases are +# separated by the VALUE and not by the key name. +_fresh_code, _fresh_out = run(HERE, "state-fresh.json", "roster.yaml") +# ⚠️ THE NAMESPACE IS READ OUT OF THE TOOL'S OWN REPORT, NOT SPELLED HERE. A +# hardcoded `unit/inverter-a/...` makes this check fail for the RIGHT reason on +# the shipped code and for the WRONG reason on a build that fixes the migration +# to use the roster's own name - it would look like the default-softening broke +# when what changed is the namespace. What this check is about is the VALUE. +_stranded_default = set(re.findall( + r"^\s+stranded\s+(unit/\S+/sign_tripped) = False$", _fresh_out, re.M)) +check("a fresh install DOES strand a `.../sign_tripped` key - it is written " + "and nothing on the roster reads it - and the report SAYS SO under its " + f"own heading (found: {sorted(_stranded_default)})", + _fresh_code == RR.EXIT_OK and len(_stranded_default) == 1) +check("...and does NOT call it a finding, because the value is the DEFAULT; " + "the same key at True is the finding part 5 turns on, so the two cases " + "are separated by the VALUE and not by the key name", + not any(f"STRANDED STORE KEY: {k}" in _fresh_out + for k in _stranded_default) + and "STRANDED STORE KEY: unit/inverter-a/sign_tripped = True" + in run(HERE, "state-legacy.json", "roster.yaml")[1]) + +# --------------------------------------------------------------------------- # +print() +print("2b. ⚠️ OMITTING THE INSTALL'S ENTITY LIST IS A REFUSAL, NOT A QUIETER " + "RUN") +# --------------------------------------------------------------------------- # +# ⚠️ THIS PART EXISTS BECAUSE THE ASYMMETRY WAS BACKWARDS AND IT WAS MEASURED. +# A `--live-entities` file that could not be READ refused with exit 2; the flag +# simply not being PASSED produced "NO FINDINGS" and exit 0 - and +# `grep -ci "live-entities|recorded list|composition"` over that report returned +# ZERO, so nothing in the artifact an owner reads said half the check had not +# run. Measured on the two entity mutations at the time: +# +# M5 device rename with the flag: exit 1 without: exit 0 +# M6 sensor rename with the flag: exit 1 without: exit 0 +# +# The owner runs this ONCE, at commissioning, under time pressure. A forgotten +# flag must not hand back a clean bill of health for the half of the transition +# that orphans entities. +def raw(*args): + """`(exit code, THE REPORT, the log)` - and the split matters. + + ⚠️ THE REPORT IS STDOUT AND THE CONTROLLER'S LOGGING IS STDERR. Grading + "is the degraded banner near the end of the report" over the two + CONCATENATED reads the last lines of the LOG - `SAFETY-02 was TRIPPED...`, + `unparseable timestamp...` - and reports a missing banner that is in fact + printed. That is not a hypothetical; it is how this check first failed. + """ + fx = HERE / "tools" / "rehearsal" + proc = subprocess.run( + [sys.executable, str(HERE / "tools" / "rehearse_roster.py"), + "--state", str(fx / "state-legacy.json"), + "--before", str(fx / "flat.yaml"), + "--after", str(fx / "roster-inverter-a.yaml"), *args], + capture_output=True, text=True, + env={**os.environ, "PYTHONDONTWRITEBYTECODE": "1"}) + return proc.returncode, proc.stdout, proc.stderr + + +_no_code, _no_out, _no_err = raw() +check("omitting --live-entities REFUSES with exit 2 rather than reporting a " + "clean transition it did not check", + _no_code == RR.EXIT_COULD_NOT_RUN and "REFUSED" in _no_out + _no_err) + +_deg_code, _deg_out, _ = raw("--no-live-entities") +check("...and the explicit opt-out RUNS, so the escape hatch exists", + _deg_code in (RR.EXIT_OK, RR.EXIT_FINDING)) +check("⚠️ ...carrying a standing banner in the report HEADER and again in " + "the closing block, because a report is skimmed from the bottom", + _deg_out.count("DEGRADED RUN") >= 2 + and "DEGRADED RUN" in "\n".join( + _deg_out.strip().splitlines()[-4:])) +check("...and the degraded report is not silent about what it did not do - " + "the words are in it, which is the grep that returned 0 before", + sum(w in _deg_out.lower() for w in + ("live-entities", "orphan check", "not graded")) == 3) +check("contradictory flags are refused rather than one silently winning", + raw("--no-live-entities", "--live-entities", + str(FIX / "live-entities-recorded.txt"))[0] + == RR.EXIT_COULD_NOT_RUN) +check("...and the banner really is at the END of the report, which is the " + "half of that claim a concatenated stdout+stderr could not have graded", + "DEGRADED RUN" in "\n".join(_deg_out.strip().splitlines()[-4:])) + +# ⚠️ AND THE COMPOSITION HALF IS NOT BEHIND THE FLAG AT ALL ANY MORE. The +# reference is loaded on EVERY run, so a degraded run still catches a renamed +# device - which is what makes `--no-live-entities` a genuinely bounded +# concession rather than "half the tool off". +_ref_missing = raw("--no-live-entities", "--reference", "/nonexistent.txt") +check("a missing or malformed reference is exit 2 on every run, because it is " + "the only reference in the tool that does not move with the code", + _ref_missing[0] == RR.EXIT_COULD_NOT_RUN) + +# ⚠️ AND THE VANISHED CHECK CARRIES THE SAME DEFAULT/NON-DEFAULT RULE, BOUNDED +# THE SAME WAY. `M9` below drops a NON-default key and must still be a finding; +# without it, "vanished at the default is not a finding" could widen into +# "vanished is not a finding" and nobody would see it. +_m9 = mutant_tree([("app/store.py", + " value = data.get(legacy, DEFAULTS.get(legacy))\n" + " store.set(target, value)", + " value = data.get(legacy, DEFAULTS.get(legacy))\n" + " store.data.pop(legacy, None)\n" + " store.set(target, value)")]) +_m9_code, _m9_out = run(Path(_m9), "state-legacy.json", "roster-inverter-a.yaml") +_m9_vanished = set(re.findall(r"VANISHED STORE KEY: (\S+) =", _m9_out)) +print(f" deleting the flat key as it migrates -> exit {_m9_code}, " + f"vanished {sorted(_m9_vanished)}") +check("⭐ a NON-default key that vanishes is STILL a finding - a migration " + "made to DELETE the flat rollback view store.py rule 2 requires it to " + "leave in place is caught, so the default-softening above is bounded by " + "its own positive", + _m9_code == RR.EXIT_FINDING + and {"last_completed", "phase_started", "last_start_attempt"} + <= _m9_vanished) +check("...and `phase` is NOT among them, because its value 'idle' IS the " + "default - the same mutation, the same file, and the rule splitting the " + "two on the VALUE rather than on the key", + "phase" not in _m9_vanished) +shutil.rmtree(Path(_m9).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. +_before_bytes = (FIX / "state-legacy.json").read_bytes() +run(HERE, "state-legacy.json", "roster.yaml", "--reverse", "--round-trip") +check("the state file the harness was pointed at is BYTE-IDENTICAL afterwards, " + "including after the reverse and round-trip legs, which write", + (FIX / "state-legacy.json").read_bytes() == _before_bytes) + + +# --------------------------------------------------------------------------- # +print() +print("3. ⭐ THE MUTATIONS: each one breaks the shipped migration or the " + "entity-id composition, and each must turn its control RED") +# --------------------------------------------------------------------------- # MUTATIONS = [ # (id, control, what it breaks, edits, the finding it must produce) ("M1", "CONTROL-B", @@ -298,6 +455,26 @@ MUTATIONS = [ '("grid", "goodwe_grid_power", "Grid power", "W", "power", "measurement", None),', '("grid", "goodwe_grid_power", "Grid power W", "W", "power", "measurement", None),')], "COMPOSITION DRIFT: this build announces sensor.goodwe_grid_power_w"), + + # ⚠️ M7 AND M8 ARE THE TWO CHANGES THAT ORPHAN AN INSTALL WITHOUT MOVING A + # SINGLE COMPOSED entity_id, so an entity_id-only reference cannot see them. + # Both came back exit 0 before the reference carried three columns. + ("M7", "CONTROL-A", + "the `unique_id` composition changes and NOT the entity_id: `mqtt.BASE` " + "is renamed, so every announced unique_id moves while every composed " + "entity_id stays byte-identical. HA's MQTT entity registry keys on " + "unique_id, so this is a NEW entity beside a dead one - and discovery is " + "retained with no removal payload, so the dead one stays", + [("app/mqtt.py", 'BASE = "goodwe_ctl"', 'BASE = "goodwe_controller"')], + "UNIQUE_ID DRIFT"), + + ("M8", "CONTROL-A", + "the DEVICE IDENTIFIER changes: HA's device registry keys on it, so the " + "whole install moves onto a new device and the old one keeps its dead " + "entities. Again no composed entity_id moves", + [("app/mqtt.py", '"identifiers": ["goodwe_rs485_controller"],', + '"identifiers": ["goodwe_rs485_ctl_v2"],')], + "DEVICE IDENTIFIER DRIFT"), ] for mid, control, what, edits, must_say in MUTATIONS: @@ -355,6 +532,26 @@ check("...and the store really did change shape underneath it, so this is a " "ns.inverter-a.sign_tripped" in _out and "unit/inverter-a/" not in _out) shutil.rmtree(Path(_tree).parent, ignore_errors=True) +# ⚠️ THE SECOND DECLARED SURVIVOR, AND IT IS THE ONE THAT BOUNDS M5-M8. Four +# mutations of `app/mqtt.py` are now required to go red; without a mutation of +# the same file that must NOT, "the reference notices any edit to mqtt.py" +# would be indistinguishable from "the reference notices what HA keys on". +# `object_id` is the case: `app/mqtt.py` lines 36-42 and 61-76 record that HA +# 2026.8 does NOT honour it in a discovery payload - the entity_id is composed +# from device name plus entity name - so renaming one changes nothing an +# install can see, and a tool that refused it would refuse a free change. +_tree2 = mutant_tree([("app/mqtt.py", + '("grid", "goodwe_grid_power", "Grid power",', + '("grid", "goodwe_grid_pwr", "Grid power",')]) +_code2, _out2 = run(Path(_tree2), *CONTROLS["CONTROL-A"]) +print(f" renaming an object_id (HA does not honour it) -> exit {_code2}") +check("renaming an `object_id` survives, because HA composes the entity_id " + "from the device and entity NAMES and ignores it - so the four mqtt.py " + "mutations above are red for what HA keys on, not for being edits to " + "mqtt.py", + _code2 == RR.EXIT_OK) +shutil.rmtree(Path(_tree2).parent, ignore_errors=True) + # --------------------------------------------------------------------------- # print() @@ -382,17 +579,82 @@ check("...and the harness names the two latch keys SAFETY-44 asks about, by " 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) -check("⚠️ AND IT IS NOT THE SAME-STEP PATH ALONE. Running the flat config " - "first and declaring the roster afterwards - the staged upgrade - " - "reaches the same disarmed latch, because the first start stamps " - "`schema_version` and the second start's migration then returns early", - out44.count("LATCH CAME UP DISARMED") >= 2) +# ⚠️ 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" legs in the report: {sorted(_L)}") +for _leg in ("STAGED", "SAME-STEP"): + print(f" LEG {_leg}: " + f"{'DISARMED' if 'LATCH CAME UP DISARMED' in _L.get(_leg, '') else 'clean'}") +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("...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())) + +# --------------------------------------------------------------------------- # +print() +print("5b. THE SPLITTER'S POSITIVE CONTROL: a build in which exactly ONE leg " + "is clean") +# --------------------------------------------------------------------------- # +# ⚠️ 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([( + "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) +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) + +print() +print("5c. back to the shipped code") 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", @@ -405,11 +667,15 @@ print("6. THE REVERSE DIRECTION: the retreat, and the return from it") # --------------------------------------------------------------------------- # code_r, out_r = run(HERE, "state-flat-070.json", "roster.yaml", "--reverse", "--round-trip") -_orphaned = set(re.findall(r"ORPHANED ENTITY: (sensor\.\S+)", out_r)) -print(f" the retreat orphans: {sorted(_orphaned)}") -check("the RETREAT is rehearsed at all, and it reports the entities it " - "orphans - the eight per-unit rows, which is the cost of backing out", - "THE RETREAT" in out_r and _orphaned == { +_RL = legs(out_r) +print(f" legs in the reverse run: {sorted(_RL)}") +_orphaned = set(re.findall(r"ORPHANED ENTITY: (sensor\.\S+)", + _RL.get("RETREAT", ""))) +print(f" the RETREAT leg orphans: {sorted(_orphaned)}") +check("the RETREAT is rehearsed as its own leg, and THAT LEG reports the " + "entities it orphans - the eight per-unit rows, which is the cost of " + "backing out", + {"RETREAT", "ROUND-TRIP"} <= set(_RL) and _orphaned == { f"sensor.goodwe_{u}_{k}" for u in ("garage", "shed") for k in ("setpoint", "battery_power", "battery_soc", "sign_latch")}) @@ -421,13 +687,13 @@ check("...and NOT one site entity: the eight ids the install has today survive " "sensor.goodwe_battery_power", "sensor.goodwe_p1_sample_age"))) check("...and the first pack's maintenance history SURVIVES the retreat, " "because `UnitHistory.mirror_flat` keeps the flat keys live - so the " - "retreat itself does not lose state", - "MAINTENANCE HISTORY LOST" not in out_r.split("THE ROUND TRIP")[0] - .split("THE RETREAT")[-1]) + "retreat itself does not lose state. ⚠️ Read from the RETREAT LEG " + "alone, so the round trip's own loss cannot be mistaken for it", + "MAINTENANCE HISTORY LOST" not in _RL["RETREAT"]) check("⚠️ ...but the ROUND TRIP does not: a maintenance cycle completed while " "the install is rolled back is DISCARDED when it re-advances, because " "`maint_schema_version` is already stamped and the migration skips", - "RETREAT-WINDOW WORK DISCARDED" in out_r) + "RETREAT-WINDOW WORK DISCARDED" in _RL["ROUND-TRIP"]) check("...and that is a leg whose data can reach the failure it names: the " "retreat window actually contains a completion, written through the " "shipped `UnitHistory`, at a timestamp the starting fixture does not have", diff --git a/goodwe_controller/tools/rehearsal/announced-reference.txt b/goodwe_controller/tools/rehearsal/announced-reference.txt new file mode 100644 index 0000000..7e2aa5a --- /dev/null +++ b/goodwe_controller/tools/rehearsal/announced-reference.txt @@ -0,0 +1,48 @@ +# WHAT THIS BUILD IS SUPPOSED TO ANNOUNCE, recorded so that a change to the +# composition has something to disagree with. THREE COLUMNS: +# +# +# +# ⚠️ ALL THREE, AND THE SECOND AND THIRD ARE THE HALF THE FIRST DRAFT MISSED. +# A reference that carries only entity_ids cannot see the two changes that +# orphan an install without changing a single composed id: +# +# `BASE` renamed (`goodwe_ctl` -> anything) every unique_id changes +# `DEVICE["identifiers"]` changed HA sees a NEW device +# +# Home Assistant's MQTT integration keys its entity registry on `unique_id` and +# its device registry on the device identifier. Change either and HA creates a +# NEW entity beside the old one - and because discovery is published with +# `retain=True` and nothing publishes a removal payload, the OLD config stays on +# the broker and the dead entity stays in the registry, still named by every +# automation that referenced it, never updating again. The composed entity_id is +# identical on both, so an entity_id-only comparison reports a clean transition. +# That is the same shape as the habit-4 this file's own card already caught +# once: a reference that does not carry the thing that moved. +# +# ⚠️ AND `object_id` IS DELIBERATELY NOT A COLUMN. `app/mqtt.py` lines 36-42 and +# 61-76 record that HA 2026.8 does NOT honour `object_id` in a discovery +# payload - the id is composed from device name plus entity name - so renaming +# one is a true no-op and a column for it would make this file refuse a change +# that costs nothing. The check is graded on what HA actually keys on. +# +# PROVENANCE, stated exactly: +# - recorded 2026-09-08 from `release/1.0` at `meter_source: firmware`, +# roster of one, by reading `mqtt.discovery_payloads(units=[])`. It is what +# the code announces. +# - `sensor.goodwe_p1_sample_age` is additionally recorded in `app/mqtt.py` as +# VERIFIED AGAINST THE LIVE SYSTEM on 2026-08-26, and the ESP32's ingest +# guard subscribes to that exact string. +# - ⚠️ IT IS NOT A DUMP OF THE LIVE INSTALL'S ENTITY REGISTRY. That is what +# `--live-entities` is for. This file is about THE BUILD. +# +# Editing this file is how you say "the announcement changed on purpose". Do not +# edit it to make a check pass. +sensor.goodwe_setpoint goodwe_ctl_setpoint goodwe_rs485_controller +sensor.goodwe_grid_power goodwe_ctl_grid goodwe_rs485_controller +sensor.goodwe_battery_power goodwe_ctl_battery goodwe_rs485_controller +sensor.goodwe_battery_soc goodwe_ctl_soc goodwe_rs485_controller +sensor.goodwe_circulating_power goodwe_ctl_circulation goodwe_rs485_controller +sensor.goodwe_maintenance_phase goodwe_ctl_phase goodwe_rs485_controller +sensor.goodwe_controller_status goodwe_ctl_status goodwe_rs485_controller +sensor.goodwe_p1_sample_age goodwe_ctl_p1_age goodwe_rs485_controller diff --git a/goodwe_controller/tools/rehearsal/live-entities-recorded.txt b/goodwe_controller/tools/rehearsal/live-entities-recorded.txt index d991ef4..0a07fd8 100644 --- a/goodwe_controller/tools/rehearsal/live-entities-recorded.txt +++ b/goodwe_controller/tools/rehearsal/live-entities-recorded.txt @@ -1,29 +1,34 @@ -# The entity ids this add-on ANNOUNCES at the shipped roster of one. +# THE ENTITY IDS THE INSTALL HAS — a stand-in for the owner's real export. # -# ⚠️ THIS FILE IS EXTERNAL DATA AND THAT IS ITS ENTIRE PURPOSE. The before/after -# entity diff inside `rehearse_roster.py` cannot see a change to the id -# COMPOSITION, because both sides of that diff are composed by the same build: -# rename the MQTT device and every id moves on both sides equally, and the diff -# reports "nothing orphaned" about a change that orphans everything. That is -# `MUTATION.md` habit 4 - a check whose data cannot reach the failure it names - -# and a list that does NOT move with the code is the only thing that closes it. +# ⚠️ THIS FILE AND `announced-reference.txt` ANSWER DIFFERENT QUESTIONS AND ARE +# NOT INTERCHANGEABLE: # -# PROVENANCE, stated exactly: -# - recorded 2026-09-08 by running this tool against `release/1.0` at -# `meter_source: firmware`, roster of one. It is what the code announces. -# - `sensor.goodwe_p1_sample_age` is additionally recorded in `app/mqtt.py` as -# VERIFIED AGAINST THE LIVE SYSTEM on 2026-08-26, and the ESP32's ingest -# guard subscribes to that exact string. -# - ⚠️ IT IS NOT A DUMP OF THE LIVE INSTALL'S ENTITY REGISTRY. Live HA carries -# roughly 49 `goodwe*` entities (card ARCH-02, read 2026-09-06); the other -# ~41 belong to the ESPHome firmware, to template sensors and to helpers, -# and this add-on announces none of them. THE OWNER STEP in DOCS.md replaces -# this file with the real export, and the tool then separates the ids it -# owns from the ids it does not rather than claiming orphans it cannot -# cause. +# announced-reference.txt what the BUILD announces. Loaded on EVERY run. +# Three columns, because HA keys on `unique_id` and +# on the device identifier as well as on the +# composed entity_id. +# this file (--live-entities) +# what the INSTALL has. One id per line. Orphans +# are graded against it: an id the install has and +# the AFTER shape does not announce is an orphan +# whatever the build's own history says. # -# Editing this file is how you say "the ids changed on purpose". Do not edit it -# to make a check pass. +# ⚠️ OMITTING `--live-entities` IS A REFUSAL (exit 2), NOT A QUIETER RUN. It +# used to be the other way round - an unreadable file refused, a missing flag +# passed with exit 0 and said nothing about the half it had not checked - and +# the owner runs this ONCE, at commissioning, under time pressure. +# `--no-live-entities` is the deliberate opt-out and it prints a standing +# banner in the report header and again on the exit line. +# +# PROVENANCE: these eight are what `release/1.0` announces at +# `meter_source: firmware`, roster of one, recorded 2026-09-08. They stand in +# for the export so the checks run in CI. ⚠️ THE REAL INSTALL CARRIES ROUGHLY 49 +# `goodwe*` ENTITIES (card ARCH-02, read 2026-09-06); the other ~41 belong to +# the ESPHome firmware, to template sensors and to helpers, and this add-on +# announces none of them. The tool separates the ids it owns from the ids it +# does not rather than claiming orphans it cannot cause - which is the half of +# this file the eight-line stand-in cannot exercise, and the reason DOCS.md's +# owner step asks for the real export. sensor.goodwe_battery_power sensor.goodwe_battery_soc sensor.goodwe_circulating_power diff --git a/goodwe_controller/tools/rehearse_roster.py b/goodwe_controller/tools/rehearse_roster.py index a86fdf2..3c8c539 100644 --- a/goodwe_controller/tools/rehearse_roster.py +++ b/goodwe_controller/tools/rehearse_roster.py @@ -267,8 +267,33 @@ def compose(device_name, entity_name) -> str: ANCHOR_ID = "sensor.goodwe_p1_sample_age" -def entity_ids(opts: dict) -> "dict[str, str]": - """`{entity_id: unique_id}` for every row this add-on announces on `opts`. +class Row: + """One announced entity, in the three terms Home Assistant keys on. + + ⚠️ THE UNIQUE_ID AND THE DEVICE IDENTIFIER ARE NOT DECORATION, AND LEAVING + THEM OUT OF THE COMPARISON WAS THIS TOOL'S SECOND HABIT-4. HA's MQTT + integration keys its ENTITY registry on `unique_id` and its DEVICE registry + on the device identifier. Change either one and HA creates a NEW entity + beside the old - while the COMPOSED entity_id is byte-identical, so a + comparison over composed ids alone reports a clean transition about a change + that orphans the whole install. Measured: renaming `mqtt.BASE` and changing + `DEVICE["identifiers"]` both came back exit 0 before this class existed. + """ + + __slots__ = ("entity_id", "unique_id", "device_id") + + def __init__(self, entity_id, unique_id, device_id): + self.entity_id = entity_id + self.unique_id = unique_id + self.device_id = device_id + + def __repr__(self): + return (f"{self.entity_id} (unique_id {self.unique_id}, device " + f"{self.device_id})") + + +def announced_rows(opts: dict) -> "dict[str, Row]": + """`{entity_id: Row}` for every row this add-on announces on `opts`. ⚠️ `discovery_payloads()` IS THE SHIPPED ANNOUNCER and `roster_names()` is the shipped naming rule, so the roster shape decision is not restated here. @@ -278,7 +303,9 @@ def entity_ids(opts: dict) -> "dict[str, str]": omit=() if is_enabled(opts) else ("p1_age",)) out = {} for _topic, cfg in payloads: - out[compose(cfg["device"]["name"], cfg["name"])] = cfg["unique_id"] + eid = compose(cfg["device"]["name"], cfg["name"]) + out[eid] = Row(eid, cfg["unique_id"], + (cfg["device"].get("identifiers") or [""])[0]) return out @@ -301,20 +328,56 @@ def _check_anchor() -> None: "The model is wrong; no orphan claim from it can be believed.") -def load_entity_list(path: str) -> set: - """The entity ids that exist on the install, one per line, `#` comments. +# Shipped beside this file, and LOADED ON EVERY RUN. See `load_reference()`. +DEFAULT_REFERENCE = (Path(__file__).resolve().parent / "rehearsal" + / "announced-reference.txt") - ⚠️ EXTERNAL DATA, AND THE TOOL IS BLIND WITHOUT IT. The before/after diff is - composed twice by the SAME build, so a change to the id composition itself - - a renamed MQTT device, a renamed sensor - moves both sides equally and the - diff reports nothing. `tools/rehearsal/live-entities-recorded.txt` carries - the recorded list and DOCS.md's owner step replaces it with a real export. + +def load_reference(path) -> "dict[str, Row]": + """What this build is SUPPOSED to announce: ` `. + + ⚠️ EXTERNAL DATA, AND THE TOOL IS BLIND WITHOUT IT - which is why it is + loaded unconditionally and a missing one is EXIT 2 rather than a quieter + run. The before/after diff is composed twice by the SAME build, so a change + to the composition itself moves both sides equally and the diff reports + nothing. Only a reference that does not move with the code can see it. + + ⚠️ THREE COLUMNS ARE REQUIRED AND A SHORT LINE IS REFUSED. A two-column + reference would silently stop grading device identifiers, which is exactly + the blind spot this file exists to close - and it would do it without + saying so, which is the failure mode the whole card is about. + """ + rows = {} + for lineno, line in enumerate( + Path(path).read_text(encoding="utf-8").splitlines(), 1): + line = line.split("#", 1)[0].strip() + if not line: + continue + parts = line.split() + if len(parts) != 3: + raise Refused( + f"{path} line {lineno}: expected ` " + f"`, got {len(parts)} field(s). A short " + f"line would silently stop grading what HA keys on.") + rows[parts[0]] = Row(*parts) + if not rows: + raise Refused(f"{path}: no rows in it") + return rows + + +def load_entity_list(path: str) -> set: + """The entity ids that EXIST ON THE INSTALL, one per line, `#` comments. + + This is the owner's export from Home Assistant, and it answers a different + question from `load_reference()`: that one says what the BUILD announces, + this one says what the INSTALL has. An id the install has and the new shape + does not announce is an orphan whatever the build's own history says. """ out = set() for line in Path(path).read_text(encoding="utf-8").splitlines(): line = line.split("#", 1)[0].strip() if line: - out.add(line) + out.add(line.split()[0]) if not out: raise Refused(f"{path}: no entity ids in it") return out @@ -359,7 +422,7 @@ class Snapshot: self.units = [inv.name for inv in controller.inverters] self.data = dict(store.data) self.read = set(store.data.reads) - self.entities = entity_ids(opts) + self.entities = announced_rows(opts) self.latched = {inv.name: bool(inv.sign_wd.tripped) for inv in controller.inverters} self.latch_reason = {inv.name: inv.sign_wd.reason @@ -371,9 +434,11 @@ class Snapshot: self.store_migration = controller.store_migration self.maint_migration = controller.maint_migration self.machine_read = machine_reads(controller, store) - # Keys no machine reads whose value is the default `Store.load()` would - # have supplied. Reported, never a finding: see `diff()`. + # Keys no machine reads, and keys that disappeared, whose value is the + # default `Store.load()` would have supplied. Reported, never a + # finding: see `diff()`. self.stranded_at_default = [] + self.vanished_at_default = [] @property def unit_keys(self): @@ -400,8 +465,15 @@ def machine_reads(controller, store) -> set: Inverter(inv.name, store=store) for m in controller.maints: for key in MAINT_UNIT_KEYS: + # ⚠️ `get()` AND NOT ALSO `get_time()`. Both route through + # `store.data.get(self.key(name))`, so either one records the read + # this probe exists to record - but `get_time()` on `phase`, which + # holds "idle" and not a timestamp, makes `Store.get_time()` log + # "unparseable timestamp" at WARNING. A probe that manufactures + # alarming log lines about a healthy store teaches an operator to + # ignore this tool's stderr, which is where the controller's own + # real warnings appear. m.store.get(key) - m.store.get_time(key) seen = set(store.data.reads) store.data.reads.clear() return seen @@ -442,11 +514,24 @@ def diff(before: Snapshot, after: Snapshot) -> list: findings = [] # -- store keys --------------------------------------------------------- - gone = sorted(set(before.data) - set(after.data)) - for key in gone: + # ⚠️ THE SAME DEFAULT/NON-DEFAULT RULE THE STRANDED CHECK USES, AND FOR THE + # SAME REASON - `MigrationReport.lost`'s, in store.py: a dropped + # `sign_tripped = False` has lost the DEFAULT, which is the statement + # "absent" already makes, so nothing is gone; a dropped `True` has lost a + # LATCH. Applying it to STRANDED and not here was an inconsistency with a + # cost: a store whose flat start wrote the two SAFETY-02 defaults under one + # name and whose roster start writes them under another reported two + # VANISHED findings about a pair of defaults, and CONTROL-C went red on a + # build that was CORRECT. + for key in sorted(set(before.data) - set(after.data)): + value = before.data[key] + bare = key.rsplit("/", 1)[-1] + if value in (None, DEFAULTS.get(bare)): + after.vanished_at_default.append(f"{key} = {value!r}") + continue findings.append(Finding( "VANISHED STORE KEY", - f"{key} = {before.data[key]!r} was in the store before the " + f"{key} = {value!r} was in the store before the " f"transition and is not there afterwards")) # ⭐ THE ONE SAFETY-44 IS ABOUT. A key that is present, correct, verified by @@ -536,38 +621,91 @@ def _latch_finding(key, before, after): return out -def entity_list_findings(recorded: set, before: Snapshot, - after: Snapshot) -> list: - """Grade both snapshots against a list that does NOT move with the code.""" +# ⚠️ THE REFERENCE IS GRADED AT A FIXED, CANONICAL SHAPE AND NOT AT THE +# OPERATOR'S CONFIG. The reference records what `release/1.0` announces at +# `meter_source: firmware`, roster of one. Grading it against the BEFORE config +# instead asks two questions at once and gets a wrong answer to both: a site +# with `meter_source: off` legitimately omits `p1_age` (see `discovery_payloads` +# and `is_enabled`), so the tool reported "the reference records +# sensor.goodwe_p1_sample_age and this build does not announce it" about a +# perfectly correct build. The question here is ONLY "has this build's +# composition drifted", which is a property of the CODE, so it is asked of the +# code at the shape the reference was taken at. +REFERENCE_PROBE_OPTS = {"meter_source": "firmware"} + + +def reference_findings(reference: "dict[str, Row]") -> list: + """Does this BUILD still announce what the reference records? + + ⚠️ RUN UNCONDITIONALLY, AND OVER ALL THREE TERMS HA KEYS ON. The + before/after diff cannot see a change to the composition, because both of + its sides are composed by the same build. This is the only check in the + tool whose reference does not move with the code, so it is the only one + that can - and it is graded on the entity_id, the `unique_id` AND the + device identifier, because a change to either of the latter two orphans an + install while leaving every composed entity_id byte-identical. + """ out = [] - announced_before = set(before.entities) - announced_after = set(after.entities) - for eid in sorted(announced_before - recorded): + built = announced_rows(REFERENCE_PROBE_OPTS) + for eid in sorted(set(built) - set(reference)): out.append(Finding( "COMPOSITION DRIFT", - f"this build announces {eid} in the BEFORE shape, and that id is " - "not on the recorded list of ids the install has. The id " - "composition has changed under the install: Home Assistant keeps " - "the id it composed at first discovery, so what this actually " - "creates is a NEW entity beside a dead one.")) - owned = recorded & announced_before - for eid in sorted(owned - announced_after): + f"this build announces {eid} and the reference does not record " + f"it. The composed entity_id has changed: Home Assistant keeps the " + f"id it composed at FIRST discovery, so what this creates is a NEW " + f"entity beside one that stops updating.")) + for eid in sorted(set(reference) - set(built)): out.append(Finding( - "ORPHANED ENTITY (against the recorded list)", + "COMPOSITION DRIFT", + f"the reference records {eid} and this build does NOT announce it " + f"at the shape the reference was taken at " + f"({REFERENCE_PROBE_OPTS}).")) + for eid in sorted(set(reference) & set(built)): + want, got = reference[eid], built[eid] + if want.unique_id != got.unique_id: + out.append(Finding( + "UNIQUE_ID DRIFT", + f"{eid} is announced with unique_id {got.unique_id!r} where " + f"the reference records {want.unique_id!r}. ⚠️ THE COMPOSED " + f"ENTITY_ID IS UNCHANGED, WHICH IS WHY THIS NEEDS ITS OWN " + f"CHECK: HA's MQTT entity registry keys on `unique_id`, so a " + f"changed one is a NEW entity - and discovery is retained with " + f"no removal payload, so the old config stays on the broker " + f"and the old entity stays in the registry, named by every " + f"automation that referenced it and never updating again.")) + if want.device_id != got.device_id: + out.append(Finding( + "DEVICE IDENTIFIER DRIFT", + f"{eid} is announced on device {got.device_id!r} where the " + f"reference records {want.device_id!r}. HA's device registry " + f"keys on the identifier, so this is a NEW device carrying new " + f"entities, beside the old device and its dead ones.")) + return out + + +def install_findings(installed: set, before: Snapshot, + after: Snapshot) -> list: + """Grade the transition against the ids the INSTALL actually has.""" + out = [] + owned = installed & set(before.entities) + for eid in sorted(owned - set(after.entities)): + out.append(Finding( + "ORPHANED ENTITY (against the install's own entity list)", f"{eid} exists on the install and is announced in the BEFORE " "shape, and the AFTER shape does not announce it.")) return out -def report_entity_list(recorded: set, before: Snapshot, after: Snapshot) -> None: +def report_entity_list(installed: set, before: Snapshot, + after: Snapshot) -> None: announced_before = set(before.entities) - foreign = sorted(recorded - announced_before) + foreign = sorted(installed - announced_before) print() - print(" -- AGAINST THE RECORDED ENTITY LIST ------------------------------") - print(f" {len(recorded)} ids on the list, " - f"{len(recorded & announced_before)} of them announced by this " + print(" -- AGAINST THE INSTALL'S OWN ENTITY LIST -------------------------") + print(f" {len(installed)} ids on the install, " + f"{len(installed & announced_before)} of them announced by this " f"add-on in the BEFORE shape") - print(f" {len(foreign)} on the list that this add-on announces in " + print(f" {len(foreign)} on the install that this add-on announces in " "NEITHER shape - the roster switch cannot orphan these, they belong " "to the") print(" ESPHome firmware, to template sensors or to helpers:") @@ -638,9 +776,19 @@ def _round_trip_findings(completed: str, readvanced: Snapshot) -> list: return out -def report_transition(name, before, after, findings) -> None: +def report_transition(leg_id, name, before, after, findings) -> None: + """One leg of the rehearsal. + + ⚠️ `LEG ` IS A MACHINE-PARSEABLE MARKER AND IT IS LOAD-BEARING. Every + finding is printed twice - once here and once in the closing summary - so + COUNTING a finding's text across the whole report cannot tell ONE red leg + from TWO. `test_rehearse.py` part 5 was written that way and reported `ok` + on a build whose failure was a single leg. The pin now splits the report on + these markers and asks each leg separately. + """ print() print("=" * 78) + print(f" LEG {leg_id}") print(f" {name}") print("=" * 78) print(f" roster before : {before.units}") @@ -677,14 +825,16 @@ def report_transition(name, before, after, findings) -> None: mark = " " if key in after.data else " (absent, defaulted) " print(f" {key}{mark}") - if after.stranded_at_default: + if after.stranded_at_default or after.vanished_at_default: print() - print(" keys no machine reads whose value IS the default, so " - "nothing is lost by") - print(" stranding them - reported rather than hidden, not counted " - "as findings:") + print(" keys whose value IS the default `Store.load()` supplies, so " + "nothing is lost") + print(" by stranding or dropping them - reported rather than " + "hidden, not findings:") for key in after.stranded_at_default: - print(f" {key} = {after.data[key]!r}") + print(f" stranded {key} = {after.data[key]!r}") + for text in after.vanished_at_default: + print(f" vanished {text}") print() print(" -- MAINTENANCE HISTORY (MAINT-05: a second move must not drop it) -") @@ -710,20 +860,31 @@ def report_transition(name, before, after, findings) -> None: print() print(" -- ENTITY IDS ----------------------------------------------------") allids = sorted(set(before.entities) | set(after.entities)) + print(f" {'entity_id':<44} {'unique_id':<30} {'device':<26} state") for eid in allids: + row = after.entities.get(eid) or before.entities[eid] inb, ina = eid in before.entities, eid in after.entities mark = "both" if (inb and ina) else ("ORPHANED" if inb else "new") - print(f" {eid:<48} {mark}") + print(f" {eid:<44} {row.unique_id:<30} {row.device_id:<26} {mark}") print(f" {len(before.entities)} announced before, " f"{len(after.entities)} after") print() - print(" -- FINDINGS ------------------------------------------------------") + print(f" -- FINDINGS in LEG {leg_id} " + + "-" * max(0, 50 - len(str(leg_id)))) if not findings: - print(" none: nothing vanished, nothing stranded, nothing orphaned, " - "no history lost") + print(f" none in LEG {leg_id}: nothing vanished, nothing stranded, " + "nothing orphaned, no history lost") for f in findings: print(f" ⚠️ {f}") + # ⚠️ AN EXPLICIT CLOSE, AND IT IS NOT TIDINESS. Without it a reader that + # splits on the opening markers alone gives every line after the LAST one + # to that leg - including the closing summary, which repeats every finding + # from EVERY leg. The last leg then reads red whenever any leg is red, and + # a per-leg check built on that is the same defect it was written to fix, + # one level down. Measured: with only the opening marker, a build whose + # SAME-STEP leg was clean still reported SAME-STEP disarmed. + print(f" END LEG {leg_id}") def main(argv=None) -> int: @@ -736,20 +897,29 @@ def main(argv=None) -> int: help="the options being switched to (declared roster)") ap.add_argument("--reverse", action="store_true", help="also rehearse the RETREAT: roster -> flat") - ap.add_argument("--live-entities", default=None, - help="a file of entity ids the install actually has, one " - "per line. ⚠️ WITHOUT IT THE TOOL CANNOT SEE A CHANGE " - "TO THE ID COMPOSITION, because both sides of its own " - "diff are composed by the same build. " - "tools/rehearsal/live-entities-recorded.txt is the " - "recorded default; DOCS.md's owner step replaces it " - "with a real export.") ap.add_argument("--round-trip", action="store_true", help="also rehearse RETREAT then RE-ADVANCE, with a " "maintenance cycle completing while retreated. ⚠️ A " "retreat is only a retreat if you can come back from " "it, so the round trip is the leg that decides " "whether the reverse direction is supported.") + ap.add_argument("--reference", default=str(DEFAULT_REFERENCE), + help="what this build is SUPPOSED to announce: " + "` ` per " + "line. Loaded on EVERY run; a missing or malformed " + "one is exit 2, because it is the only reference in " + "this tool that does not move with the code.") + ap.add_argument("--live-entities", default=None, + help="a file of the entity ids the INSTALL actually has, " + "one per line - the owner's export from Home " + "Assistant. Orphans are graded against it.") + ap.add_argument("--no-live-entities", action="store_true", + help="⚠️ RUN WITHOUT THE INSTALL'S ENTITY LIST, ACCEPTING " + "THAT THE ORPHAN CHECK IS NOT GRADED AGAINST " + "REALITY. Required to omit --live-entities; the " + "report then carries a standing banner and the exit " + "line says so, because a forgotten flag must never " + "look like a clean bill of health.") ap.add_argument("--paths", default="both", choices=("staged", "same-step", "both"), help="staged: the box runs the BEFORE config once, then " @@ -759,45 +929,81 @@ def main(argv=None) -> int: "Default: both.") args = ap.parse_args(argv) + # ⚠️ THE ASYMMETRY THAT USED TO BE HERE WAS BACKWARDS AND IT IS THE WHOLE + # REASON THIS BLOCK EXISTS. A `--live-entities` file that could not be READ + # refused with exit 2; the flag simply not being PASSED produced "NO + # FINDINGS" and exit 0, with nothing in the report to say half the orphan + # check had not run - measured, `grep -ci` for the words returned 0. The + # owner runs this ONCE, at commissioning, under time pressure. A forgotten + # flag must not hand back a clean bill of health for the half of the + # transition that orphans entities. So omitting it is now a REFUSAL, and + # accepting the weaker run has to be said out loud. try: + if not args.live_entities and not args.no_live_entities: + raise Refused( + "no --live-entities. The orphan check is graded against the " + "ids the INSTALL has, and without that file it is not graded " + "against reality at all. Export it (DOCS.md, 'Before you " + "commission inverter B'), or pass --no-live-entities to " + "accept the weaker run - which is then stated in the report " + "and on the exit line rather than left to be assumed.") + if args.live_entities and args.no_live_entities: + raise Refused("--live-entities and --no-live-entities are " + "contradictory; pass one.") _check_anchor() + reference = load_reference(args.reference) before_opts = load_options(args.before) after_opts = load_options(args.after) + installed = (load_entity_list(args.live_entities) + if args.live_entities else None) if not Path(args.state).is_file(): raise Refused(f"{args.state}: not a file") - except Refused as err: + except (OSError, Refused) as err: print(f"REFUSED: {err}", file=sys.stderr) return EXIT_COULD_NOT_RUN + degraded = installed is None + print(__doc__.split("EXIT CODES")[0].rstrip()) print() print(f" state file (copied, never written): {args.state}") print(f" before config: {args.before}") print(f" after config: {args.after}") + print(f" reference : {args.reference} ({len(reference)} rows, graded " + f"on entity_id + unique_id + device identifier)") + if degraded: + print() + print(" " + "!" * 74) + print(" !! DEGRADED RUN: --no-live-entities. The ORPHAN CHECK IS NOT " + "GRADED AGAINST") + print(" !! THE INSTALL. This run can still see the composition " + "drifting away from") + print(" !! the reference, and it can see the store migration - but it " + "cannot tell") + print(" !! you that an entity the install HAS stops being announced, " + "because it") + print(" !! has not been told what the install has. ⚠️ A CLEAN RESULT " + "BELOW IS NOT A") + print(" !! CLEAN BILL OF HEALTH FOR THE ENTITIES.") + print(" " + "!" * 74) + else: + print(f" install list : {args.live_entities} ({len(installed)} ids)") findings = [] - recorded = None - try: - if args.live_entities: - recorded = load_entity_list(args.live_entities) - except (OSError, Refused) as err: - print(f"REFUSED: {err}", file=sys.stderr) - return EXIT_COULD_NOT_RUN tmpdir = tempfile.mkdtemp(prefix="rehearse-roster-") try: if args.paths in ("staged", "both"): b = snapshot("staged-before", before_opts, args.state, tmpdir) a = snapshot_over("staged-after", after_opts, b) - f = diff(b, a) - if recorded: - f += entity_list_findings(recorded, b, a) + f = diff(b, a) + reference_findings(reference) + if installed is not None: + f += install_findings(installed, b, a) report_transition( - "STAGED: the box runs the flat config, then restarts on the " - "roster", b, a, f) - if recorded: - report_entity_list(recorded, b, a) - print(" (findings from the recorded list are in the list " - "above)") + "STAGED", + "the box runs the flat config, then restarts on the roster", + b, a, f) + if installed is not None: + report_entity_list(installed, b, a) findings += f if args.paths in ("same-step", "both"): @@ -807,12 +1013,13 @@ def main(argv=None) -> int: # version that knew about namespaces. b2 = snapshot("samestep-before", before_opts, args.state, tmpdir) a2 = snapshot("samestep-after", after_opts, args.state, tmpdir) - f2 = diff(b2, a2) - if recorded: - f2 += entity_list_findings(recorded, b2, a2) + f2 = diff(b2, a2) + reference_findings(reference) + if installed is not None: + f2 += install_findings(installed, b2, a2) report_transition( - "SAME-STEP (SAFETY-44): a pre-DEV-02 flat store meets a " - "declared roster on its FIRST start under this version", + "SAME-STEP", + "SAFETY-44's path: a pre-DEV-02 flat store meets a declared " + "roster on its FIRST start under this version", b2, a2, f2) findings += f2 @@ -820,9 +1027,12 @@ def main(argv=None) -> int: r1 = snapshot("retreat-before", after_opts, args.state, tmpdir) r2 = snapshot_over("retreat-after", before_opts, r1) fr = diff(r1, r2) + if installed is not None: + fr += install_findings(installed, r1, r2) report_transition( - "THE RETREAT: roster -> flat, which is the back-out if " - "commissioning B goes wrong", r1, r2, fr) + "RETREAT", + "roster -> flat, which is the back-out if commissioning B " + "goes wrong", r1, r2, fr) findings += fr if args.round_trip: @@ -839,8 +1049,9 @@ def main(argv=None) -> int: q3 = snapshot_over("roundtrip-readvanced", after_opts, q2) fq = diff(q2, q3) + _round_trip_findings(worked, q3) report_transition( - "THE ROUND TRIP: roster -> flat -> roster, with a maintenance " - f"cycle completing at {worked!r} while retreated", + "ROUND-TRIP", + f"roster -> flat -> roster, with a maintenance cycle " + f"completing at {worked!r} while retreated", q2, q3, fq) findings += fq except Refused as err: @@ -855,11 +1066,17 @@ def main(argv=None) -> int: print(f" {len(findings)} FINDING(S) - this transition is not clean") for f in findings: print(f" ⚠️ {f}") - print("=" * 78) - return EXIT_FINDING - print(" NO FINDINGS in the rehearsed transitions") + else: + print(" NO FINDINGS in the rehearsed transitions") + # ⚠️ ON THE EXIT LINE, NOT ONLY IN THE HEADER. A report is skimmed from the + # bottom. + if degraded: + print(" ⚠️ DEGRADED RUN (--no-live-entities): the orphan check was " + "NOT graded against") + print(" the install's own entity list. This result does not cover " + "orphaned entities.") print("=" * 78) - return EXIT_OK + return EXIT_FINDING if findings else EXIT_OK if __name__ == "__main__":