admin/goodwe-addon
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

CFG-03: give maintenance option defaults one source of truth
checks / unit (pull_request) Successful in 3m44s
Details

Browse Source 
Option defaults for the maintenance cycle were declared in three places that
could disagree, and MAINT-04 proved they already did. config.yaml shipped
`maintenance_charge_w: 2000`; main.py carried TWO `opts.get(..., 2500)`
fallbacks; MaintConfig's dataclass carried a third `2500.0`. So an options dict
with the key ABSENT was ACCEPTED by validate.py's rule 6 - which read 2000 -
and then built by main.py at 2500, against a 2000 W ceiling:

    options dict with maintenance_charge_w ABSENT, max_w=2000
      validate_startup_config  ->  []  (accepted, no problems)
      MaintConfig().charge_w   ->  2500.0
      arbiter.resolve()        ->  0.0 W, contradiction=True

That is the bug rule 6 exists to refuse, passing rule 6. It is unreachable on a
Supervisor install, which always supplies every key from config.yaml, and fully
reachable from sim/inject.py and from the test suite - which is where anyone
debugging maintenance actually works.

MAINT-04 shipped a TRIPWIRE and said so: test_validate.py pinned all three
literals and asserted the consequence through the real arbiter, so the
divergence could not silently widen or be half-closed. Its PASSING CONDITION
WAS THE BUG. Both it and validate.py's comment said to delete each other when
the divergence was closed. It is closed, so both are deleted.

THE DIVERGENCE WAS WIDER THAN ONE KEY. `maintenance_discharge_w` had the
identical three-way split (config.yaml 2000, MaintConfig 2500) and the card
named only the charge side. Closing one and leaving its twin would have
satisfied the numbers and left three sources of truth one line down, which is
the state that produced the bug. Both are closed, and so is the whole family:
all eight `maintenance_*` keys go through the same mechanism.

WHAT THE SINGLE SOURCE IS. config.yaml's `options:` block - it is what the
Supervisor reads and what the operator edits, so it wins by definition. There
is now exactly one in-code copy, `maintenance.MAINT_OPTION_DEFAULTS`, which is
TARIFF-02's `PRICE_OPTION_DEFAULTS` and SAFETY-24's `PEAK_OPTION_DEFAULTS`
pattern applied to the family that named the card - validate.py's rule 8 comment
already called that "CFG-03 read as a rule rather than as a complaint", so this
makes the rule hold where it was first broken rather than inventing a fourth
mechanism for one option family.

A MIRROR RATHER THAN A RUNTIME READ, and the reason is mechanical: the
Dockerfile copies `app/` and `run.sh` into the image and NOT config.yaml. The
running add-on cannot open the file it is configured by, so there is nothing to
read at 03:00 on a client's Pi, and a YAML-subset parser that could fail at
import is a new way for the add-on not to start. What makes the copy safe is
that test_maintenance.py pins every entry by READING the shipped file - drift is
a CI failure, not a comment.

EVERY OTHER SITE NOW DERIVES FROM IT OR HAS CEASED TO EXIST:

  * MaintConfig's eight dataclass defaults are indexed out of the mirror. No
    numeric literal survives in the class that held the third opinion.
  * `MaintConfig.from_options()` is the ONLY place in the add-on that reads a
    `maintenance_*` option. main.py's eight inline `opts.get(key, <literal>)`
    calls are gone: moving the read next to the mirror means the fallback cannot
    be anything but the mirror, because there is no literal in sight of it to
    disagree with. It uses `key in opts` rather than truthiness, so a shipped
    `false` or `0` is honoured instead of being quietly replaced by the default
    - this card's own bug in a different costume.
  * main.py's SECOND fallback site - the cheap-window charge floor at the
    P_MAINT_SHAPE claim - reads `self.maint.cfg.charge_w` instead of re-opening
    the options dict with its own literal. The Controller had already parsed
    this once in __init__; asking the object that owns it cannot drift from it,
    and the rate the shaping asks for can no longer be a different number from
    the rate the state machine drains at on the same install.
  * validate.py's rule 6 and its `maintenance_discharge_w` warning take their
    fallbacks from the mirror.

WHERE THE RANGE LIVES, STATED AND CHECKED. config.yaml's `schema:` block carries
`maintenance_charge_w: int(500,5000)` - a fourth declaration site of a different
kind. It is a BOUND, not a default, so it is not this bug; but leaving the
question open is how a fifth site arrives. The decision recorded in config.yaml
and in maintenance.py is: bounds live in the schema block ONLY, because the
Supervisor is what enforces them and the Supervisor reads only that file, so a
Python copy of `int(500,5000)` would be a place to disagree and nothing else.
test_maintenance.py checks both halves of that: every mirrored key must have a
schema entry - without one the Supervisor drops the option and the add-on
silently runs on the mirror instead of the operator's value - and every shipped
default must lie INSIDE its own range, since a default the schema rejects is a
config that cannot be re-entered once it is edited. What the schema grammar
cannot express is a relation BETWEEN keys (`maintenance_charge_w` against
`max_w`), which is exactly why rule 6 is written in Python and why it does not
belong in the mirror either.

THE REPLACEMENT CHECKS, AND HOW THEY WERE VERIFIED. test_validate.py's new
section exercises the same four consumers the defect ran through, the same way:
the validator ACCEPTS a dict with every maintenance key missing; a REAL
Controller built from that same dict holds 2000 W (through main.py, not through
a regex over its source); `MaintConfig()` answers 2000 W with no dict at all;
and the real arbiter emits `-2000 W, limited by supervised(max_w)` instead of
`0 W, contradiction=True`. test_maintenance.py pins the mirror to config.yaml in
both directions - equal values, and full coverage of every `maintenance_*` key
the file ships, so a ninth key cannot arrive unmirrored - and asserts the
consumers move as a RELATION rather than against numbers, reading MaintConfig's
defaults back out of `dataclasses.fields()`.

Each new check was verified by BREAKING the thing it watches and observing red,
this project's rule:

  1. config.yaml -> 1500, mirror left at 2000:  test_maintenance 1 FAILED,
     test_validate 4 FAILED.
  2. config.yaml AND the mirror -> 1500: green again, and every consumer moved
     together - MaintConfig() 1500 W, the Controller 1500 W, and the arbiter
     `-1500 W, limited by maintenance(cheap-window charge)` where it had said
     `-2000 W, limited by supervised(max_w)`. That is AC4: change the single
     source and watch the consumers follow.
  3. `charge_w: float = 2500.0` retyped into the dataclass: 2 FAILED in each of
     the two files.
  4. `opts.get("maintenance_charge_w", 2500)` restored in main.py: 1 FAILED,
     naming the file and the literal it found.
  5. `maintenance_hold_min` deleted from config.yaml's options block: 3 FAILED.

Full suite 17/17 in goodwe-addon-ci, twice, once under parallel load.

Stale prose that asserted the now-closed divergence as current is corrected in
app/watchdog.py, test_watchdog.py and test_peakguard.py - watchdog.py's example
still uses 1500 W, but because the point of the example is a charge strictly
inside the ceiling, not because 2500 is what ships.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
glenn schrooyen
2026-09-01 06:16:54 +02:00
co-authored by Claude Opus 5
parent b7267eb51a
commit d5b1ef19b3
10 changed files with 495 additions and 139 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/test_validate.py
+99 -72
View File
@@ -17,6 +17,7 @@ Read MUTATION.md before you report a survivor.
import asyncio
import sys
from app.maintenance import MAINT_OPTION_DEFAULTS, MaintConfig
from app.validate import (
ACTUATOR_WRITE_KEYS, FIRMWARE_INGEST_VETO_S, ConfigRefused, refuse,
validate_startup_config,
@@ -34,10 +35,16 @@ def check(name, cond):
def base(**over):
"""A configuration that must start. Everything else is this, spoilt."""
# ⚠️ The maintenance rates are named EXPLICITLY, at the values config.yaml
# ships (MAINT-04). They are not decoration: with the key absent, rule 6
# falls back to main.py's 2500 W, which against this 2000 W ceiling is a
# genuine refusal - so a base() without them would be a config that does
# not start, and every check below would be measuring the wrong thing.
# ships (MAINT-04). Under MAINT-04 that was load-bearing: with the key
# absent, rule 6 fell back to a 2500 W literal that against this 2000 W
# ceiling is a genuine refusal, so a base() without them was a config that
# does not start and every check below would have been measuring the wrong
# thing. CFG-03 closed that - the fallback is now config.yaml's own 2000 -
# so they are here to say out loud what this fixture asks for rather than to
# rescue it. The CFG-03 section at the foot of this file is what asserts
# that absent and present are now THE SAME NUMBER; do not delete these two
# keys, because a base() that agrees with the default by accident is a
# fixture that cannot tell you when the default moves.
o = {"setpoint_entity": "number.goodwe_setpoint", "max_w": 2000,
"soc_entity": "sensor.soc", "batt_entity": "sensor.batt",
"meter_source": "off",
@@ -379,74 +386,6 @@ check("...and a ceiling already past the billing quarter, so no value the "
"schema still accepts can alarm inside the quarter that bills it",
_hi is not None and _hi > _QUARTER_S)
# --------------------------------------------------------------------------- #
print("the KNOWN cross-file default divergence, pinned rather than commented")
# ⚠️ WHAT THIS GUARDS, AND WHY IT ASSERTS THE BUG RATHER THAN THE FIX.
# config.yaml and validate.py now default maintenance_charge_w to 2000, while
# main.py's two `opts.get(..., 2500)` fallbacks and MaintConfig's dataclass
# default still say 2500. So an options dict with the key ABSENT is ACCEPTED by
# rule 6 at 2000 and then built by main.py at 2500 - which is a contradiction:
#
# validate_startup_config -> [] (accepted)
# MaintConfig().charge_w -> 2500.0
# arbiter -> 0.0 W, contradiction=True
#
# That is the bug rule 6 exists to catch, passing rule 6. It is unreachable on a
# Supervisor install (every key is always supplied from config.yaml) but fully
# reachable from sim/inject.py and from tests - which is exactly where anyone
# debugging maintenance works.
#
# The fix is ONE SOURCE OF TRUTH for option defaults. That is a bigger change
# than this ticket and it lands in main.py, which another ticket is live in - so
# the divergence is RECORDED here instead of closed. The check therefore passes
# on the mismatch and FAILS when either side moves, in EITHER direction:
#
# - config.yaml moves -> FAIL: re-decide what the fallback should be
# - main.py is fixed to match -> FAIL: the divergence is gone, DELETE this
# check and the ⚠️ block in validate.py
#
# A comment cannot do that, and "a comment where a check belonged" is this
# repo's most repeated defect.
_APP = os.path.join(os.path.dirname(os.path.abspath(__file__)), "app")
def _read(*parts):
with open(os.path.join(*parts), encoding="utf-8") as fh:
return fh.read()
# Every literal fallback for this key in main.py, whatever the call shape.
_main_falls = [float(m) for m in re.findall(
r'\.get\(\s*"maintenance_charge_w"\s*,\s*(-?\d+(?:\.\d+)?)\s*\)',
_read(_APP, "main.py"))]
# ...and MaintConfig's own dataclass default, which is the one that bites when
# main.py is bypassed entirely (a Maintenance built straight from defaults).
_dc_falls = [float(m) for m in re.findall(
r"^\s*charge_w:\s*float\s*=\s*(-?\d+(?:\.\d+)?)\s*$",
_read(_APP, "maintenance.py"), re.M)]
check(f"main.py's fallbacks are still found and still 2500 {_main_falls}",
len(_main_falls) == 2 and set(_main_falls) == {2500.0})
check(f"MaintConfig's default is still found and still 2500 {_dc_falls}",
len(_dc_falls) == 1 and set(_dc_falls) == {2500.0})
check(f"...and config.yaml still ships 2000, so the divergence is UNCHANGED "
f"(shipped {_shipped['maintenance_charge_w']:.0f}, code 2500)",
_shipped["maintenance_charge_w"] == 2000.0)
# ⚠️ And the consequence itself, through the REAL code rather than through the
# numbers above - so that if someone "fixes" this by changing a literal without
# understanding it, the thing that goes red is the behaviour, not the transcript.
from app.maintenance import MaintConfig # noqa: E402
_absent = {k: v for k, v in base().items() if k != "maintenance_charge_w"}
_built = MaintConfig().charge_w
check("with the key ABSENT the validator accepts...",
validate_startup_config(_absent, -5000, 5000) == [])
check(f"...while the code would build {_built:.0f} W against a "
f"{_absent['max_w']:.0f} W ceiling...", _built > _absent["max_w"])
_r = _charge(_absent["max_w"], _built)
check("...which the arbiter resolves to 0 W as a contradiction - the hole, "
"asserted so it cannot widen unnoticed",
_r.target_w == 0.0 and _r.contradiction)
# --------------------------------------------------------------------------- #
print("max_w must be a positive magnitude")
# Schema-blocked on a real install, but validate_startup_config() is also
@@ -607,6 +546,94 @@ try:
except TypeError:
check("a Controller with no stop Event is rejected at construction", True)
# --------------------------------------------------------------------------- #
print("CFG-03: the key ABSENT is the SAME NUMBER everywhere, through real code")
# ⚠️ WHAT THIS REPLACES, AND WHY IT IS THE OPPOSITE ASSERTION. MAINT-04 left a
# TRIPWIRE here whose PASSING CONDITION WAS THE BUG: it asserted that
# config.yaml said 2000 while main.py's two `opts.get(..., 2500)` fallbacks and
# MaintConfig's dataclass default said 2500, and it went red if EITHER side
# moved - including when somebody closed the divergence. That check has been
# DELETED, as it and validate.py's ⚠️ block both instructed, and this is what
# stands in its place: the same four consumers, exercised the same way, now
# agreeing.
#
# The four are the four the defect ran through. The validator ACCEPTS a dict
# with the key missing; main.py BUILDS a MaintConfig from that same dict; the
# dataclass answers on its own with no dict at all; and the arbiter is what
# turns their disagreement into a number an operator can see. Any three of them
# agreeing was never the property that mattered.
_absent = {k: v for k, v in base().items()
if not k.startswith("maintenance_")}
_ship_charge = _shipped["maintenance_charge_w"]
_ship_drain = _shipped["maintenance_discharge_w"]
check("the validator still ACCEPTS a dict with every maintenance key missing - "
"the fixture is the reachable one, not a refusal in disguise",
validate_startup_config(_absent, -5000, 5000) == [])
# 1. the mirror against the shipped file. Read, not transcribed: a transcription
# is the thing that drifted.
check(f"the ONE in-code mirror equals what config.yaml ships "
f"(charge {MAINT_OPTION_DEFAULTS['maintenance_charge_w']}, "
f"shipped {_ship_charge:.0f})",
float(MAINT_OPTION_DEFAULTS["maintenance_charge_w"]) == _ship_charge
and float(MAINT_OPTION_DEFAULTS["maintenance_discharge_w"]) == _ship_drain)
# 2. MaintConfig on its own - the one that bites when main.py is bypassed
# entirely, which is sim/inject.py and most of this suite.
check(f"MaintConfig() answers the shipped number, not a third one "
f"({MaintConfig().charge_w:.0f} W)",
MaintConfig().charge_w == _ship_charge
and MaintConfig().discharge_w == _ship_drain)
# 3. main.py's own path, through a real Controller rather than through a regex
# over its source. A source scan can only say the literal is gone; this says
# the object an operator's install actually runs on holds the right number.
_ctl_absent = Controller(_absent, None, _Store2(), _Pub2(), stop=asyncio.Event())
print(f" a Controller built from a dict with NO maintenance key: "
f"charge_w={_ctl_absent.maint.cfg.charge_w:.0f} W, "
f"discharge_w={_ctl_absent.maint.cfg.discharge_w:.0f} W")
check("main.py builds the shipped number from a dict that does not contain it",
_ctl_absent.maint.cfg.charge_w == _ship_charge
and _ctl_absent.maint.cfg.discharge_w == _ship_drain)
# ...and it is the SAME number when the operator supplies it explicitly, which
# is what "the default is not a special case" means.
_ctl_named = Controller(base(), None, _Store2(), _Pub2(), stop=asyncio.Event())
check("...and an explicit key builds the identical config, so absent and "
"present are not two code paths",
_ctl_named.maint.cfg == _ctl_absent.maint.cfg)
# 4. the arbiter - the consequence itself. Under MAINT-04 this pair was
# (2000, 2500) and resolved to 0 W with contradiction=True; that was the
# ACCEPTED-then-contradicted hole. The rates come out of the built config,
# not out of a literal, so if the single source moves this moves with it.
_r_ok = _charge(float(_absent["max_w"]), _ctl_absent.maint.cfg.charge_w)
print(f" arbiter with that config: {_r_ok.explain()}")
check("the arbiter CHARGES instead of contradicting itself - the hole MAINT-04 "
"pinned open is closed at the only place it was ever reachable",
_r_ok.target_w == -_ship_charge and not _r_ok.contradiction)
_r_drain = _drain(float(_absent["max_w"]), _ctl_absent.maint.cfg.discharge_w)
check("...and the drain SET is not clamped either, so the twin key the card "
"did not name is closed too",
_r_drain.target_w == _ship_drain and not _r_drain.contradiction)
# 5. nothing typed a number behind the mirror's back. Rule 6's fallback and
# main.py's are the two sites the divergence lived at, so they are the two
# sites a scan is worth running over.
_srcs = {name: open(os.path.join(os.path.dirname(os.path.abspath(__file__)),
"app", name), encoding="utf-8").read()
for name in ("main.py", "validate.py", "maintenance.py")}
_literals = {name: re.findall(
r'"(maintenance_[a-z_]+)"\s*,\s*(-?\d+(?:\.\d+)?)\s*[,)]', src)
for name, src in _srcs.items()}
check(f"no numeric fallback for any maintenance_* key survives in main.py, "
f"validate.py or maintenance.py {_literals}",
not any(_literals.values()))
check("...and MaintConfig's dataclass defaults carry no numeric literal either "
"- they are indexed out of the mirror",
not re.search(r"^\s*(charge_w|discharge_w):\s*float\s*=\s*-?\d",
_srcs["maintenance.py"], re.M))
# --------------------------------------------------------------------------- #
print()
if fails: