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)) # ==============================================================================