From acbdf41248325fb839c452639776e344970d2537 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Tue, 1 Sep 2026 08:40:09 +0200 Subject: [PATCH] TEST-13: grade record_block()'s policy argument, and the two import spellings SAFETY-20's check could not see Two survivors from TARIFF-01's review, closed in test_tariff.py only. app/ is untouched and no shipped behaviour changes. Two further items on the card were already closed in shipped code and are struck rather than re-implemented. ITEM 1 - the per-phase reduction's PLUMBING was unobservable. Mutating PeakStore.record_block()'s `billed_w(block, policy)` to `billed_w(block, POLICY_NET)` (+4 B) leaves all 17 suites green. Measured three times now: by TARIFF-01's reviewer, re-run through TEST-15's purge harness this session, and re-run again here on a clean uncontended tree. The product code is correct - tariff.py:363 really is `def record_block(self, block, policy: str = POLICY_NET)` and it really does forward the argument. What was missing is any check that could tell that apart from a hardcoded call, and the reason is the third instance of one shape: A STIMULUS EVALUATED WHERE THE QUANTITY IT GRADES IS DEGENERATE, after WITNESS-02's span normalisation and TARIFF-01's own DST window. billed_w() short-circuits to block.offtake_avg_w whenever per_phase_offtake_avg_w is absent. Every block reaching a store anywhere in the suite was single-phase or built as _CB(..., None) - so NET, SUM and MAX collapsed onto one number and the argument could not be observed at the call site. Section 7 grades the FUNCTION on an unbalanced fixture and says nothing about its CALLER. So section 7's own (3000, 500, -200) W block - whose three reductions are 3300 / 3500 / 3000 W, asserted distinct three lines above - is now round-tripped THROUGH the store under each policy and read back from both tables (peak() and recompute_month(), so both writes in that single transaction are graded). record_block() with no policy is checked to bank the NET figure. An anti-degeneracy pin is stated over the three BANKED numbers, not over vals, because it is the banked ones this triple is made of. Direction, which is why this is worth plumbing-testing: NET is the SMALLEST of the three here, because the exporting phase nets off. A store that quietly billed NET while asked for MAX would UNDER-record the month's peak, and an under-recorded peak is maximum headroom for SAFETY-07 - the same under-guarding direction as the getattr(..., 0.0) default SAFETY-07 already removed from project(). ITEM 2 - the SAFETY-20 import grader claimed more than it checked. It collected `m.module for m in walk(tree) if isinstance(m, ImportFrom)` and matched a literal list of six spellings. That is blind to two of Python's three import forms: * `from . import arbiter as _ab` is an ImportFrom whose .module is None - the dots are in .level and the module name is in .names - so it collected "" and matched nothing; * `import app.arbiter` is an ast.Import node the walk filtered out entirely. Both were added to app/tariff.py and the shipped suite stayed GREEN, with the _names half not firing either (an alias import creates no Name node). Low severity: any USE of either alias still produces an Attribute the _names half catches. But the printed sentence "it imports nothing from arbiter, clamp or control" was false for two forms out of three, and a check that overclaims is worse than one that claims less, because the next reader stops looking. _imported_modules() now walks Import as well as ImportFrom and joins `from . import X` onto its base. _forbidden_imports() matches per dotted SEGMENT, not by substring: `app.arbiter` must not hide behind a package prefix, and `clamps` / `arbiters` / `controller` must not be falsely flagged - `controller` is not hypothetical, this package is goodwe_controller. Five evasion spellings and eight innocent ones grade the grader itself, and both the name half and the import half are shown non-vacuous against main.py, which carries all three import forms and really does import all three modules. ITEMS 3 AND 4 - ALREADY CLOSED. STRUCK, NOT RE-IMPLEMENTED. Item 3: app/tariff.py:266-267 is `def is_observed(self): return self.state == OBSERVED`, already false for NO_RECORD. Nothing to do. Item 4: app/tariff.py:672-680 reads `held = max(averager.held_net_w, 0.0)` with NO getattr default, and carries a comment naming TEST-13 item 4. Fixed by SAFETY-07 in 616b62d; held_net_w is a documented public property on QuarterAverager (p1.py:290), so a rename raises AttributeError on the first cycle instead of crediting 0 W held. A projection crediting 0 W held reads LOW, which is the under-guarding direction for SAFETY-07's floor. Restoring a default would re-open a money guard. And the recorded NON-finding stays a non-finding: removing clear_partial() is still green and that is a PASS. quarter_start is the PRIMARY KEY and the upsert carries `ON CONFLICT ... WHERE excluded.offtake_avg_w > quarter_blocks.offtake_avg_w`, so a replay is a structural no-op. That demonstrates TARIFF-01's "double counting is answered structurally, not by care" claim rather than contradicting it. EVIDENCE - all through tools/mutation_run.sh (recursive __pycache__ purge, python3 -B, tree re-read from disk, mutated line grepped off disk, byte delta recorded), in the goodwe-addon-ci image against a :ro mount. baseline, 17 suites, before any edit ............................ 17 PASS baseline, 17 suites, final tree ................................. 17 PASS A1 billed_w(block, policy) -> billed_w(block, POLICY_NET) +4 B 5 RED the 'sum' and 'max' round-trips, both tiers, and the distinctness pin A2 billed_w(block, policy) -> billed_w(block, POLICY_MAX) +4 B 6 RED the 'net' and 'sum' round-trips, both tiers, the default, and the pin B1 add `from . import arbiter as _ab` to app/tariff.py +29 B 1 RED the import sentence. SHIPPED suite on the same tree: GREEN - the hole. B2 add `import app.arbiter` to app/tariff.py +19 B 1 RED the import sentence. SHIPPED suite on the same tree: GREEN - the hole. B3 add `from .control import compute` (the form the OLD grader DID catch, as a regression control) 1 RED C1 delete the ast.Import branch from _imported_modules() 1 RED names the two spellings it MISSED, so the grader-grader is live C2 segment match -> substring match (`any(f in m for f in ...)`) 1 RED names clamps / arbiters / controller as falsely flagged CONTROL, and the point of the whole ticket: A1 graded by the SHIPPED test_tariff.py is green, and A1 graded by all 17 SHIPPED suites on a clean uncontended run is 17 PASS, rc=0. The survivor is real and it is now closed. ONE THING THE REVIEWER SHOULD KNOW. During the first (contended - two docker containers at once) run of that 17-suite survivor sweep, test_peakguard.py reported FAIL once. It has passed 8 times since on the identical tree, including three concurrent runs and one clean uncontended full sweep, and its tail was lost to a grep so I cannot say which check it was. I believe it is a timing-sensitive check under CPU contention and NOT related to this change - this commit touches no product code and no peakguard fixture - but I could not reproduce it, so I am recording it rather than asserting it away. Co-Authored-By: Claude Opus 5 (1M context) --- goodwe_controller/CHANGELOG.md | 52 ++++++++++ goodwe_controller/MUTATION.md | 2 +- goodwe_controller/test_tariff.py | 169 ++++++++++++++++++++++++++++--- 3 files changed, 208 insertions(+), 15 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 70038c1..e804246 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,58 @@ ## Unreleased +**`PeakStore.record_block()` now has its POLICY ARGUMENT graded, and the +SAFETY-20 import check now looks at all three ways of writing an import +(TEST-13).** Two survivors from TARIFF-01's review, both closed in +`test_tariff.py` only - `app/` is untouched and no shipped behaviour changed. + +* **The plumbing of the per-phase reduction was unobservable.** Mutating + `record_block()`'s `billed_w(block, policy)` to `billed_w(block, POLICY_NET)` + left all 17 suites green (+4 B; recorded by TARIFF-01's reviewer, re-run + through the purge harness for TEST-15, and re-run again here). The cause is + the third instance of the same shape as WITNESS-02's span normalisation and + TARIFF-01's own DST window: **a stimulus evaluated where the quantity it + grades is degenerate.** `billed_w()` short-circuits to `block.offtake_avg_w` + whenever `per_phase_offtake_avg_w` is absent, and every block that reached a + store anywhere in the suite was single-phase or built with + `per_phase_offtake_avg_w=None` - so NET, SUM and MAX collapsed onto one + number and the argument could not be seen. Section 7 already owns an + UNBALANCED fixture, `(3000, 500, -200) W`, whose three reductions are 3300 / + 3500 / 3000 W; that block is now round-tripped **through the store** under + each policy and read back from both tables. NET is the smallest of the three + here, so a store that quietly billed NET while asked for MAX would + under-record the month's peak - maximum headroom for SAFETY-07, the same + under-guarding direction as the `getattr(..., 0.0)` default SAFETY-07 already + removed from `project()`. +* **The import half of the SAFETY-20 grader made a claim it did not check.** It + collected `ImportFrom.module` only, which is blind to two of Python's three + import spellings: `from . import arbiter as _ab` is an `ImportFrom` whose + `.module` is **None** (the module name is in `.names`), and `import + app.arbiter` is an `ast.Import` node the walk never visited. Both were added + to `app/tariff.py` and the shipped suite stayed **green**. Never a live + exposure - the `_names` half still catches any USE of either alias - but the + printed sentence "it imports nothing from arbiter, clamp or control" was + false for two forms out of three, which is worse than claiming less. The + grader now walks `Import` as well, joins `from . import X` onto its base, and + matches per dotted SEGMENT so `app.arbiter` cannot hide behind a package + prefix while `clamps` / `arbiters` / `controller` are not falsely flagged. + Both graders are also now shown non-vacuous against `main.py`, which really + does import all three modules. +* **Two items on the card were already closed and are struck, not + re-implemented.** `PeakReading.is_observed` is `self.state == OBSERVED` + (`app/tariff.py:266`), already false for `NO_RECORD`; and `project()`'s + `held` read is `max(averager.held_net_w, 0.0)` with **no** `getattr` default, + fixed by SAFETY-07 in 616b62d with a comment naming TEST-13 item 4. Restoring + a default there would re-open a money guard. Removing `clear_partial()` also + stays green and that remains a **PASS**: the `quarter_start` PRIMARY KEY plus + `ON CONFLICT ... WHERE excluded > existing` makes a replay a structural + no-op, which is TARIFF-01's claim rather than a hole in it. + +Verified through `tools/mutation_run.sh` (purge, `python3 -B`, tree re-read from +disk, mutated line grepped off disk, byte delta recorded): 7 mutations, all red +on the named checks, plus green baselines before and after and a control run +showing the shipped suite passing every one of them. + **The two `arbitrage()` rungs SAFETY-26's AC4 sweep found pinned from ONE SIDE ONLY are now pinned from both - and, the actual finding, are pinned so they stop MASKING EACH OTHER (TEST-16).** `spread == 0.0` weakened to `<= 0.001` survived diff --git a/goodwe_controller/MUTATION.md b/goodwe_controller/MUTATION.md index 01209ab..2722727 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -172,7 +172,7 @@ commit and PR bodies. | **SAFETY-07** wiring mutations B1/B2/B3 and `test_peakguard.py` parts 7 and 10 (2d570d8, e7c95bf) | **not at risk** | those mutations are **executed in-process**, monkeypatched onto the imported module. No source file is edited, so no `.pyc` is consulted. | | every **"all red, no survivors"** claim in the window — TARIFF-02 (24/0), TARIFF-03 (11), MAINT-04 (10), SAFETY-18 (6+4), SAFETY-03 (6), TEL-04 (10), TEL-05 (6) | **not at risk** | direction. Stale bytecode turns a red into a green survivor, never the reverse. A red is evidence that *something* changed under the check. | | **TEST-06**'s deleted assertion, "no new notification while parked at 0 W" (c34b5a2) | **not at risk / superseded** | it was recorded as undecidable and then *falsified* — `test_freeze.py` records the one-line mutation that takes the file red 9 times. The stronger later claim is a red one. | -| **TARIFF-01** reviewer survivor: `record_block()` ignoring its `policy` argument (0dbd485) | **re-verified — still a survivor** | re-run here through `tools/mutation_run.sh` against the 15 suites that were on the record, all green with `billed_w(block, policy)` mutated to `billed_w(block, POLICY_NET)`. The recorded diagnosis holds: every block reaching the store is single-phase, where NET, SUM and MAX collapse. | +| **TARIFF-01** reviewer survivor: `record_block()` ignoring its `policy` argument (0dbd485) | **re-verified — still a survivor** | re-run here through `tools/mutation_run.sh` against the 15 suites that were on the record, all green with `billed_w(block, policy)` mutated to `billed_w(block, POLICY_NET)`. The recorded diagnosis holds: every block reaching the store is single-phase, where NET, SUM and MAX collapse. **Now CLOSED by TEST-13** (`test_tariff.py` section 7), which round-trips section 7's own unbalanced `(3000, 500, -200) W` block THROUGH the store under each policy - three distinct banked figures, so a hardcoded policy takes at least two of them red. Third instance of "a stimulus evaluated where the quantity it grades is degenerate", after WITNESS-02's span normalisation and TARIFF-01's own DST window. | | **SAFETY-25** 8 mutations against `test_peakguard.py` part 10, then 9 more after review, all red | **not at risk** | each run against a tree copied fresh with `__pycache__` excluded and mounted `:ro`, so no `.pyc` existed to go stale - the same structural immunity recorded for CI above. Byte deltas recorded on the PR; none is `+0 B`. All are **reds**, which is the direction this defect cannot fabricate. ⚠️ The second round exists because the first round's mutations were all aimed at DISARMING the guard, and the claim part 10 actually made was about the guard being FIXED - the reviewer built the fix and the suite stayed green. Bytecode was never the issue there; an unaimed battery was. | | **SAFETY-03** reviewer survivor: `abs()` dropped from `clamp_to_rating`'s limit (6aabb83) | **re-verified — still a survivor** | re-run the same way, all 15 green. Also as recorded: unreachable under the `int(100, 5000)` schema. | | **TARIFF-01**'s other three survivors, **SAFETY-03**'s `describe()` survivor, **TARIFF-02**'s M3/M5/M7, **SAFETY-18**'s three | **still unverified** | recorded before the purge discipline, with no byte delta on the record, so a `+0 B` same-second revert cannot be excluded. All are survivors, which is the direction this defect fabricates. TARIFF-02's and SAFETY-18's were **closed by later commits whose claims are reds**, so what is unverified is the *diagnosis*, not the current state of the code. | diff --git a/goodwe_controller/test_tariff.py b/goodwe_controller/test_tariff.py index d905d1b..a071163 100644 --- a/goodwe_controller/test_tariff.py +++ b/goodwe_controller/test_tariff.py @@ -681,6 +681,65 @@ check(f"...and on a BALANCED load NET and SUM coincide ({b_vals[POLICY_NET]:.0f} f"so a balanced fixture could not have told them apart", near(b_vals[POLICY_NET], b_vals[POLICY_SUM], 1e-6)) +# --- TEST-13 item 1: the POLICY ARGUMENT'S PLUMBING, not just billed_w() ------ +# `PeakStore.record_block(block, policy)` really does call `billed_w(block, +# policy)` - the product code is correct and is not what this adds. What was +# missing is that NO CHECK COULD TELL that call apart from a hardcoded +# `billed_w(block, POLICY_NET)`. Mutating it to exactly that left all 17 suites +# green, twice measured (TARIFF-01's reviewer, re-run through the purge harness +# for TEST-15 and again here). +# +# The reason is this section's own subject, one level up. `billed_w()` +# short-circuits to `block.offtake_avg_w` whenever `per_phase_offtake_avg_w` is +# absent, and EVERY block that reached a store anywhere in this file was +# single-phase or built with `per_phase_offtake_avg_w=None` - so all three +# policies collapsed onto one number and the argument was unobservable at the +# call site. The checks above grade the FUNCTION on an unbalanced fixture; they +# say nothing about whether its CALLER forwards what it was handed. That is the +# same shape as WITNESS-02's span normalisation and TARIFF-01's own DST window: +# a stimulus evaluated where the quantity it grades is degenerate. +# +# So the stimulus here is `blk` - the unbalanced three-phase block from the top +# of this section, whose three reductions were asserted three lines up to be +# three DIFFERENT numbers. A `record_block()` that ignores its argument stores +# one policy's figure under all three names and at least two of these +# round-trips go red. +# +# Which direction the defect runs in, because that is what makes it worth +# plumbing-testing at all: NET is the SMALLEST of the three on this fixture (the +# exporting phase nets off against the other two). A store that silently billed +# NET while its caller asked for MAX would under-record the month's peak, and an +# under-recorded peak is maximum headroom for SAFETY-07 - the same +# under-guarding direction as item 4's `getattr(..., 0.0)` default. +_banked = {} +for _pol in (POLICY_NET, POLICY_SUM, POLICY_MAX): + _st = fresh_store(f"policy-{_pol}") + _st.record_block(blk, _pol) + _read = _st.peak(month=month_key(blk.start)) + _derived = _st.recompute_month(month_key(blk.start)) + _banked[_pol] = _read.peak_w + check(f"record_block(block, {_pol!r}) banks {vals[_pol]:.0f} W - the store " + f"applies the reduction it was HANDED, not one of its own", + _read.is_observed and near(_read.peak_w, vals[_pol])) + check(f"...and re-deriving from the 15-min tier gives the same {_pol!r} " + f"figure, so BOTH writes in that one transaction used that reduction", + _derived.is_observed and near(_derived.peak_w, vals[_pol])) + +_st_def = fresh_store("policy-default") +_st_def.record_block(blk) +check(f"...and record_block() with NO policy argument banks the NET figure " + f"({vals[POLICY_NET]:.0f} W), matching billed_w()'s own shipped default", + near(_st_def.peak(month=month_key(blk.start)).peak_w, vals[POLICY_NET])) +# The anti-degeneracy pin, stated over the BANKED numbers rather than over +# `vals`, because it is the banked ones this triple is made of. If a future +# fixture change ever made two of these coincide, the triple above would start +# passing under a hardcoded policy and would be silently back where it started. +print(f" banked through the store: " + + ", ".join(f"{k}={v:.0f} W" for k, v in _banked.items())) +check("the three BANKED figures are three distinct numbers, so this triple is " + "not itself degenerate - it can only pass if the argument is read", + len({round(v, 6) for v in _banked.values() if v is not None}) == 3) + # ============================================================================== print() @@ -976,24 +1035,106 @@ _names |= {n.name for n in _ast.walk(_tree) _FORBIDDEN = {"setpoint", "write_setpoint", "target", "Decision", "i_clamped", "clamped", "Claim", "resolve", "P_PEAK", "arbiter"} _found = sorted(_names & _FORBIDDEN) -_imports = sorted(m.module or "" for m in _ast.walk(_tree) - if isinstance(m, _ast.ImportFrom)) + +# ⚠️ THE IMPORT HALF WALKS `Import` TOO, AND READS THE IMPORTED NAMES - TEST-13 +# item 2. It used to be `m.module for m in walk(_tree) if isinstance(m, +# ImportFrom)`, matched against a literal list of six spellings, and that is +# blind to TWO OF PYTHON'S THREE import forms: +# +# * `from . import arbiter as _ab` is an `ImportFrom` whose `.module` is +# **None** - the dots live in `.level` and the module name lives in +# `.names` - so the old grader collected `""` and matched nothing; +# * `import app.arbiter` is an `ast.Import` node, a different class the walk +# filtered out entirely and therefore never looked at. +# +# Neither was a live exposure: the `_names` half above catches any USE of either +# alias, because reaching anything through it produces an `Attribute` whose +# `.attr` is a forbidden name. The defect was in the SENTENCE. A check that +# prints "it imports nothing from arbiter, clamp or control" while verifying +# that for one spelling out of three is worse than one that claims less, because +# the next reader stops looking. So: collect every module any import statement +# names, in all three forms, and match per dotted SEGMENT - `app.arbiter` and +# `.arbiter` must not be able to hide behind a package prefix either. +_FORBIDDEN_MODULES = {"arbiter", "clamp", "control"} + + +def _imported_modules(tree): + """Every module path an import statement in `tree` names, all three forms.""" + out = set() + for node in _ast.walk(tree): + if isinstance(node, _ast.Import): + # `import app.arbiter`, `import app.arbiter as x` - the whole + # dotted path is the module. + out |= {a.name for a in node.names} + elif isinstance(node, _ast.ImportFrom): + base = node.module or "" # None for `from . import arbiter` + if base: + out.add(base) # `from .arbiter import resolve` + # ...and in the `module is None` form the MODULE is in .names, so + # the names have to be joined on whether or not there is a base. + out |= {f"{base}.{a.name}" if base else a.name for a in node.names} + return out + + +def _forbidden_imports(mods): + # Segment-wise, not substring: `arbitrage` contains no forbidden segment and + # must not be flagged, while `app.arbiter` must be. + return sorted(m for m in mods + if _FORBIDDEN_MODULES & {p for p in m.split(".") if p}) + + +_imports = sorted(_imported_modules(_tree)) print(f" tariff.py evaluates {len(_names)} distinct names and imports " - f"from {_imports or ['(stdlib only)']}") + f"{sorted({m.split('.')[0] for m in _imports}) or ['(nothing)']}") check(f"none of them is a command-side name {sorted(_FORBIDDEN)} - the utility " f"bills what FLOWED, so this reads the meter and nothing else" + (f"; found {_found}" if _found else ""), not _found) -check("...and it imports nothing from arbiter, clamp or control: it files no " - "claim, applies no clamp, and commands nothing - SAFETY-07's job", - not [m for m in _imports if m in ("arbiter", "clamp", "control", - ".arbiter", ".clamp", ".control")]) -# ...and the guard is not vacuous: the same grader run over main.py, which DOES -# command, must light up. -_main = {n.attr for n in _ast.walk( - _ast.parse((HERE / "app" / "main.py").read_text(encoding="utf-8"))) - if isinstance(n, _ast.Attribute)} -check(f"the same grader over main.py finds {sorted(_main & _FORBIDDEN)}, so it " - f"is capable of failing", bool(_main & _FORBIDDEN)) +check("...and it imports nothing from arbiter, clamp or control in ANY of the " + "three import spellings: it files no claim, applies no clamp, and " + "commands nothing - SAFETY-07's job", + not _forbidden_imports(_imports)) + +# The import grader graded. Every one of these is a real way to reach the +# command side from this module, and the first two are the ones the previous +# version let through; the last two it already caught and must keep catching. +_EVASIONS = ["from .arbiter import resolve", + "from . import arbiter as _ab", + "import app.arbiter", + "from app.control import compute", + "import app.clamp as _c"] +_missed = [e for e in _EVASIONS + if not _forbidden_imports(_imported_modules(_ast.parse(e)))] +check(f"...and that import grader is not spelling-blind: all {len(_EVASIONS)} " + f"ways of importing the command side are caught" + + (f"; MISSED {_missed}" if _missed else ""), not _missed) +# ...and it is not simply flagging everything, which would make the line above +# free. The last three are the reason the match is per SEGMENT and not `f in m`: +# `clamps`, `arbiters` and `controller` all CONTAIN a forbidden word and none of +# them is the command side. `controller` is not hypothetical - this package is +# `goodwe_controller`, and a substring grader pointed at a path rather than a +# module name would refuse the whole repository. +_INNOCENT = ["import json", "from datetime import datetime, timezone", + "from . import arbitrage", "from .price import Uncertainty", + "from zoneinfo import ZoneInfo", + "import clamps", "from .arbiters import x", + "from . import controller"] +_false = [i for i in _INNOCENT + if _forbidden_imports(_imported_modules(_ast.parse(i)))] +check("...and it does not fire on innocent imports whose names merely CONTAIN a " + "forbidden word (`clamps`, `arbiters`, `controller`) - a substring match " + "would have" + + (f"; FALSELY FLAGGED {_false}" if _false else ""), not _false) + +# ...and neither half is vacuous: the same two graders run over main.py, which +# DOES command, must both light up. main.py carries all three import forms. +_main_tree = _ast.parse((HERE / "app" / "main.py").read_text(encoding="utf-8")) +_main = {n.attr for n in _ast.walk(_main_tree) if isinstance(n, _ast.Attribute)} +_main_imports = _forbidden_imports(_imported_modules(_main_tree)) +check(f"the same NAME grader over main.py finds {sorted(_main & _FORBIDDEN)}, " + f"so it is capable of failing", bool(_main & _FORBIDDEN)) +check(f"...and the same IMPORT grader over main.py finds {_main_imports}, so " + f"it is capable of failing on a real module and not only on a fixture", + bool(_main_imports)) # ============================================================================== -- 2.54.0