MAINT-05 fix round: grade the migration against the ROSTER, not against the name it was handed
checks / unit (pull_request) Successful in 4m47s
checks / unit (pull_request) Successful in 4m47s
REJECTED because a survivor undoes AC 5, and it survived for the recurring
reason: two fixture values that must be distinguishable were EQUAL.
THE SURVIVOR. app/main.py:282 mutated from `self.inverters[0].name` to `UNIT_A`
left the full 19-file suite green. `inverter.UNIT_A == "inverter-a"`, and every
two-pack fixture in test_maintenance.py named its first roster entry
"inverter-a" - so the two expressions were the same string in every check that
existed. main.py's own comment made the distinction load-bearing and nothing in
the repository could see whether that comment was true.
THE FAILURE IS REAL. `build_roster()` names a declared roster's entries from the
config and numbers a nameless one POSITIONALLY (`inverter-1`), so "inverter-a"
is not even the fallback. On ["garage","shed"] with a flat last_completed of
2026-08-07 the mutant migrates into unit/inverter-a/, pack 0 reads back None,
and both packs come up believing they have never been recalibrated.
AND IT FAILED SILENTLY, which was the worse half: `_verify_moves()` read back
the keys it had just WRITTEN, under the namespace it had just been HANDED, was
satisfied, and MAINT_SCHEMA_KEY was stamped - so the next start skipped the
migration too. A postcondition that reads back the namespace it was handed
cannot detect being handed the wrong one.
WHAT CHANGED
1. THE POSTCONDITION IS GRADED AGAINST THE ROSTER. `_verify_moves()` takes the
names the MACHINES read and reports a namespace that is not among them as
`misdirected` - a new, differently-named failure from `lost`:
* `lost` is "the store took the write and dropped it" - a storage fault,
with a harmless-default case (a dropped `last_completed: None` loses
nothing, because absent and never-recalibrated are the same statement);
* `misdirected` is "the store did exactly as it was told, by a caller that
was wrong". There is no harmless-default version of it - a write nobody
reads is not information whatever its value - so it fails `ok` alone.
`ok` gates the marker, so a misdirected migration is NOT stamped and the next
start re-runs it. Asymmetric on purpose: a re-run is recoverable (rule 1
makes it a no-op, rule 2 left the flat keys to recover from), a silent skip
is not. Part 9b MEASURES the recovery rather than asserting it.
2. `migrate_maint_keys()` TAKES THE ROSTER IN PLACE OF AN `n_units` COUNT. The
count was `len(self.inverters)` at the only call site, so the roster carries
it, and a count beside the names is a second opinion about how many packs
there are.
3. THE FIXTURES CAN NOW REACH THE FAILURE THEY NAME. Parts 9 and 11 run on
["garage","shed"], each behind a PRECONDITION check that the first name is
not UNIT_A - so an edit that makes them equal again goes red instead of
making the claims vacuous. Part 11's fixture date is relative to `now`, not
a literal, so "history preserved" and "history lost" cannot converge as the
file ages past `interval_days`.
MUTATION LEDGER - all red, purged tree, none at +0 B, full detail on the PR:
M1 main.py inverters[0].name -> UNIT_A -16 B 4 checks red
M2 store.py drop `roster=names` from _verify_moves -14 B 5 checks red
M3 store.py drop the misdirected.append -37 B 5 checks red
M4 store.py ok: drop `and not self.misdirected` -25 B 3 checks red
M5 fixture garage -> inverter-a (the trap itself) +4 B 5 checks red
M6 store.py parts[1] -> parts[-1] +1 B 1 check red
FOUND AND NOT FIXED - `migrate_unit_keys()` HAS THE SAME HOLE AND IT IS A LATCH.
It runs BEFORE `build_roster()` (each Inverter restores its latch in its
constructor), so it is handed the constant UNIT_A with no roster to grade
against. On a declared roster a pre-DEV-02 `sign_tripped: true` lands under
unit/inverter-a/sign_tripped, which no unit reads - and
`_grade_store_migration()`, whose whole job is to re-arm exactly this, is
DEFEATED BY THE KEY THE DEFECT WROTE: its "is anything namespaced latched" test
is satisfied by the misdirected value itself. Measured, with the flat-config
counterfactual, and pinned as a running check (test_maintenance.py part 13)
whose assertions state the WRONG behaviour on purpose. It wants its own card:
the remedy is in inverter.py and SAFETY-02's semantics, and a silent expansion
into a safety latch is how a rejected PR becomes a broken one.
Suite: 19/19 PASS, purged tree, bind-mounted :ro in goodwe-addon-ci.
No hardware was touched; every fixture here is SIMULATED and pack B's 3000 W
remains a marked hypothesis.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
This commit is contained in:
co-authored by
Claude Opus 5
parent
1aef19e995
commit
65cddf35f1
@@ -276,11 +276,24 @@ class Controller:
|
||||
# It targets `self.inverters[0].name` and NOT `UNIT_A`: the flat keys
|
||||
# belonged to whichever pack the one FSM was driving, which is the first
|
||||
# roster entry, and a declared `inverters:` roster names its own entries
|
||||
# (`inverter.build_roster()` says so). Writing `UNIT_A` here would
|
||||
# deposit an install's whole maintenance history under a name that is
|
||||
# not on the roster at all.
|
||||
# (`inverter.build_roster()` says so, and its fallback for a nameless
|
||||
# entry is a POSITIONAL `inverter-1`, so `inverter-a` is not even the
|
||||
# default there). Writing `UNIT_A` here would deposit an install's whole
|
||||
# maintenance history under a name that is not on the roster at all.
|
||||
#
|
||||
# ⚠️ AND THAT SENTENCE USED TO BE A COMMENT NOTHING COULD GRADE, WHICH
|
||||
# IS WHY THE ROSTER IS PASSED RATHER THAN A COUNT. Every two-pack
|
||||
# fixture in the suite named its first entry `inverter-a`, so
|
||||
# `self.inverters[0].name` and `UNIT_A` were the same string in every
|
||||
# check that existed and swapping them left all 19 files green - while
|
||||
# on a roster of `["garage", "shed"]` it silently loses the history and
|
||||
# stamps the marker, so the next start skips the migration too.
|
||||
# `migrate_maint_keys()` now grades the namespace it wrote against the
|
||||
# names the machines read; test_maintenance.py sections 9 and 11 drive
|
||||
# a `["garage", "shed"]` roster, which is what makes the mutation red
|
||||
# instead of invisible.
|
||||
self.maint_migration = migrate_maint_keys(
|
||||
store, self.inverters[0].name, len(self.inverters))
|
||||
store, self.inverters[0].name, [inv.name for inv in self.inverters])
|
||||
_LOG.info("maintenance history: %s", self.maint_migration.explain())
|
||||
if not self.maint_migration.ok:
|
||||
# Not a disarmed guard - history, not a latch - so it does not
|
||||
|
||||
@@ -211,13 +211,23 @@ class MigrationReport:
|
||||
# to shout: an alarm that fires on the harmless case is an alarm an
|
||||
# operator learns to dismiss before the real one arrives.
|
||||
self.lost = []
|
||||
# ⚠️ A DIFFERENT FAILURE FROM `lost`, AND IT HAS TO BE, BECAUSE THE
|
||||
# READ-BACK CANNOT SEE IT. `lost` is "the store took the write and
|
||||
# dropped it". This is "the store took the write, kept it perfectly, and
|
||||
# NOTHING WILL EVER READ IT" - the migration was handed a unit name that
|
||||
# is not on the roster, so every machine looks under a different key and
|
||||
# finds nothing. There is no harmless-default version of this the way
|
||||
# there is for `lost`: a write nobody reads is not information, whatever
|
||||
# its value, so anything in here fails `ok` on its own.
|
||||
self.misdirected = []
|
||||
# Why nothing ran, when nothing ran. Empty means "already at schema".
|
||||
self.skipped = ""
|
||||
|
||||
@property
|
||||
def ok(self) -> bool:
|
||||
"""Nothing that CARRIED STATE failed to land."""
|
||||
return not self.lost
|
||||
"""Nothing that CARRIED STATE failed to land, and it landed WHERE THE
|
||||
MACHINES LOOK."""
|
||||
return not self.lost and not self.misdirected
|
||||
|
||||
def explain(self) -> str:
|
||||
if not self.ran:
|
||||
@@ -238,6 +248,8 @@ class MigrationReport:
|
||||
out += " - PROBLEMS: " + "; ".join(self.problems)
|
||||
if self.lost:
|
||||
out += " - STATE LOST: " + "; ".join(self.lost)
|
||||
if self.misdirected:
|
||||
out += " - WRONG NAMESPACE: " + "; ".join(self.misdirected)
|
||||
return out
|
||||
|
||||
|
||||
@@ -329,7 +341,7 @@ def _move_flat_keys(store, unit: str, keys, rep: MigrationReport) -> None:
|
||||
rep.moved[target] = value
|
||||
|
||||
|
||||
def _verify_moves(store, rep: MigrationReport) -> None:
|
||||
def _verify_moves(store, rep: MigrationReport, roster=None) -> None:
|
||||
"""THE POSTCONDITION, AND IT IS A READ-BACK RATHER THAN AN ASSUMPTION.
|
||||
|
||||
This is the difference between these functions and a two-line copy: the
|
||||
@@ -339,6 +351,34 @@ def _verify_moves(store, rep: MigrationReport) -> None:
|
||||
same `unit_key()` those readers read through. A `False` here is a guard that
|
||||
is off, or a month of coulomb-count history gone, on an install whose state
|
||||
file says otherwise.
|
||||
|
||||
⚠️ AND `roster` IS THE OTHER HALF, WHICH THE READ-BACK ON ITS OWN CANNOT BE.
|
||||
A POSTCONDITION THAT READS BACK THE NAMESPACE IT WAS HANDED CANNOT DETECT
|
||||
BEING HANDED THE WRONG ONE. That is not a hypothetical: MAINT-05 shipped
|
||||
with `migrate_maint_keys()` taking a bare `unit` string, and a call site that
|
||||
passed `inverter.UNIT_A` instead of the first roster entry's name migrated an
|
||||
install's whole maintenance history into `unit/inverter-a/...` on a roster of
|
||||
`["garage", "shed"]`, read all four keys back under that same name, was
|
||||
SATISFIED, and stamped the schema marker - so the next start skipped the
|
||||
migration too and both packs came up believing they had never been
|
||||
recalibrated. Every symptom of a clean migration, and the history gone.
|
||||
`build_roster()` names a declared roster's entries from the config and falls
|
||||
back to a POSITIONAL `inverter-N`, so `inverter-a` is not even the default
|
||||
there; there is no roster on which that write could have been read.
|
||||
|
||||
So the namespace is graded against the one the MACHINES read - the roster -
|
||||
rather than against the argument this function was handed, which is the
|
||||
thing under test. A unit that is not on the roster is `misdirected`, which
|
||||
fails `ok`, which means the caller does NOT stamp the version marker and the
|
||||
next start runs the migration again. A re-run is recoverable, by rule 1; a
|
||||
silent skip is the state store.py's header says must never happen.
|
||||
|
||||
`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.
|
||||
"""
|
||||
for target, value in rep.moved.items():
|
||||
got = getattr(store, "data", {}).get(target, "<<missing>>")
|
||||
@@ -351,8 +391,24 @@ def _verify_moves(store, rep: MigrationReport) -> None:
|
||||
if value != DEFAULTS.get(legacy):
|
||||
rep.lost.append(note)
|
||||
|
||||
if roster is None:
|
||||
return
|
||||
names = [str(n) for n in roster]
|
||||
# `unit_key()` builds `unit/<unit>/<key>`, so the unit is field 1. Split
|
||||
# through the same spelling the writer used rather than re-deriving it.
|
||||
for target in sorted(rep.moved):
|
||||
parts = target.split("/")
|
||||
unit = parts[1] if len(parts) > 2 else ""
|
||||
if unit in names:
|
||||
continue
|
||||
rep.verified = False
|
||||
note = (f"{target} was migrated into namespace {unit!r}, which is NOT "
|
||||
f"on the roster {names!r} - nothing reads that key")
|
||||
rep.problems.append(note)
|
||||
rep.misdirected.append(note)
|
||||
|
||||
def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport:
|
||||
|
||||
def migrate_maint_keys(store, unit: str, roster) -> MigrationReport:
|
||||
"""Move the flat maintenance history into the FIRST pack's namespace.
|
||||
|
||||
MAINT-05. Until this card there was one `Maintenance` and one history, and
|
||||
@@ -363,14 +419,32 @@ def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport:
|
||||
what `Controller.maint_unit` named and what `validate.py` rule 6 graded
|
||||
against.
|
||||
|
||||
⚠️ `n_units <= 1` IS A RETURN, NOT A SPECIAL CASE. At one inverter the flat
|
||||
keys ARE that pack's namespace (§6.2's identity, the same one
|
||||
⚠️ `len(roster) <= 1` IS A RETURN, NOT A SPECIAL CASE. At one inverter the
|
||||
flat keys ARE that pack's namespace (§6.2's identity, the same one
|
||||
`arbiter.actuator_for()` leans on), `Maintenance` is constructed to read
|
||||
them directly, and a migration would add four keys plus a marker to a store
|
||||
that must come out byte-identical. The migration happens on the day the
|
||||
roster grows, which is the day the flat spelling stops being able to answer
|
||||
"which pack".
|
||||
|
||||
⚠️ `roster` IS REQUIRED, AND IT REPLACED AN `n_units: int`. Two reasons, and
|
||||
the second is the one MAINT-05 was rejected over.
|
||||
|
||||
The small one: `n_units` was `len(self.inverters)` at the only call site, so
|
||||
the roster carries it - `len(roster)` - and a count passed beside the names
|
||||
is a second opinion about how many packs there are.
|
||||
|
||||
The load-bearing one: THE NAMES ARE THE POSTCONDITION. `unit` says which
|
||||
pack the flat keys belonged to and that is genuinely a decision - the first
|
||||
roster entry, because that is the pack the one FSM was driving - but a
|
||||
decision the caller can get WRONG, and until this signature existed nothing
|
||||
could tell. `_verify_moves()` read back the namespace it was handed, so a
|
||||
migration into a name no machine reads verified clean and stamped the
|
||||
marker. Handing the roster in means the read-back is graded against the
|
||||
keys the MACHINES read, and a `unit` that is not among these names comes
|
||||
back `misdirected`: not `ok`, marker NOT written, migration runs again next
|
||||
start. See `_verify_moves()` for the install this was measured on.
|
||||
|
||||
THE THREE RULES ARE `migrate_unit_keys()`'s, SHARED IN CODE rather than
|
||||
restated: `_move_flat_keys()` is rule 1 (never overwrite a namespaced value)
|
||||
and rule 2 (the flat keys are left in the file), `_verify_moves()` is the
|
||||
@@ -390,8 +464,9 @@ def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport:
|
||||
"""
|
||||
rep = MigrationReport()
|
||||
data = getattr(store, "data", {})
|
||||
names = [str(n) for n in roster]
|
||||
rep.from_version = int(data.get(MAINT_SCHEMA_KEY, 0) or 0)
|
||||
if n_units <= 1:
|
||||
if len(names) <= 1:
|
||||
rep.skipped = ("one inverter: the flat maintenance keys ARE that pack's "
|
||||
"history and are read in place, so there is nothing to "
|
||||
"move and the store layout does not change")
|
||||
@@ -404,13 +479,19 @@ def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport:
|
||||
|
||||
rep.ran = True
|
||||
_move_flat_keys(store, unit, MAINT_UNIT_KEYS, rep)
|
||||
_verify_moves(store, rep)
|
||||
_verify_moves(store, rep, roster=names)
|
||||
|
||||
if rep.ok:
|
||||
store.set(MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION)
|
||||
else:
|
||||
_LOG.error("per-pack maintenance-history migration LOST STATE and has "
|
||||
"not been marked done: %s", rep.explain())
|
||||
# ⚠️ NOT MARKED DONE, FOR EITHER FAILURE. The same sentence
|
||||
# `migrate_unit_keys()` carries: the one thing worse than a failed
|
||||
# migration is a failed migration a later version skips. Rule 1 makes
|
||||
# the re-run a no-op where the keys did land, so refusing the marker
|
||||
# costs one repeated pass and buys the difference between a migration
|
||||
# that can still be fixed and one the store says already happened.
|
||||
_LOG.error("per-pack maintenance-history migration did NOT land and "
|
||||
"has not been marked done: %s", rep.explain())
|
||||
return rep
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user