Two survivors from TARIFF-01's review, both closed here 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; they are struck with evidence rather than re-implemented.
Item 1 - record_block()'s policy argument was unobservable
Mutating PeakStore.record_block()'s w = billed_w(block, policy) to w = billed_w(block, POLICY_NET) (+4 B) leaves all 17 suites green. Measured three times: 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 (17 PASS, rc=0).
The product code is correct.tariff.py:363 is def record_block(self, block, policy: str = POLICY_NET) and it does forward the argument. What was missing is a check that could tell that apart from a hardcoded call.
The cause 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, and 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. Section 7 grades the function on an unbalanced fixture and says nothing about its caller.
What I did. 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, read back from both tables (peak() and recompute_month(), so both writes in that one 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 rather than over vals, because it is the banked ones this triple is made of.
Direction, which is why it 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, 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, and I want to be clear about that: any use of either alias still produces an Attribute the _names half catches, so this was never a live exposure. The defect is that 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. AC 2 allowed either fixing the check or narrowing the sentence; I fixed the check, because the fix is smaller than the honest narrowing would have been.
_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 now grade the grader itself, and both halves 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 rather than 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 nogetattr default, under 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, the under-guarding direction for SAFETY-07's floor. Restoring a default would re-open a money guard, and I did not.
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 - which demonstrates TARIFF-01's "double counting is answered structurally, not by care" claim rather than contradicting it. Untouched.
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.
The control, and the point of the ticket: A1 graded by the shippedtest_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, is now measured a third time, and is now closed.
MUTATION.md's survey row for this survivor is updated from "re-verified - still a survivor" to closed, the same way TEST-16 updated the SAFETY-26 row.
What I am unsure about, and what a reviewer should look hardest at
A PRE-EXISTING, CLOCK-DEPENDENT RED IN test_peakguard.py - unrelated to this PR, but I hit it and then chased it down. This wants its own card.
CI attempt 1 on this sha failed, on test_peakguard.py, on a check this PR does not touch. Attempt 2 on the identical sha passed. This PR changes test_tariff.py, CHANGELOG.md and MUTATION.md only - no product code, no peakguard fixture - so I did not accept "flake" and reproduced it deterministically instead.
On a PRISTINE origin/release/1.0 tree (git archive origin/release/1.0, nothing of mine in it), under libfaketime in the goodwe-addon-ci image:
wall clock, seconds into the 900 s quarter
remaining
result
5, 60, 180, 330, 480, 630, 780, 800, 830
895 .. 70 s
PASS
850
50 s
5 RED
860
40 s
5 RED
870
30 s
4 RED
895
5 s
PASS
So test_peakguard.py is red whenever the wall clock lands in roughly the last 40-60 seconds of a quarter - about 5-6 % of runs, which is the right order for "seen once in nine local runs and once in two CI runs". The set of red checks varies with the offset, which is what a saturation boundary looks like rather than one bad assertion.
The CI timestamp confirms it. Attempt 1's failure was stamped 06:44:59 UTC; the quarter boundary is 06:45:00. The failing fixture sits at the end of a 49 s suite, so it ran within the last minute of that quarter - inside the window above.
Where it lives.wired_over_limit() (test_peakguard.py:1007) derives its load backwards from remaining = QUARTER_S - (now - blk) precisely so the answer does not depend on the minute it ran - its own docstring says "a check that saturated or not depending on the minute it ran would be a coin toss wearing a verdict". The intent is right and the last minute is not covered by it; its four-attempt retry loop only re-rolls on a boundary crossing, not on a small remaining.
I did not fix it here. It is another suite, it is not in TEST-13's scope, and test_peakguard.py part 10 is live territory for SAFETY-25 / TEST-18 - a blind edit from this branch would collide. Flagging it for a card instead, with the repro above ready to use. Reviewer: if CI on this PR goes red on test_peakguard.py, that is this bug and not this change - re-run it.
_FORBIDDEN_MODULES is a second list beside _FORBIDDEN. The name grader's set already contains "arbiter"; the module grader needs clamp and control too, which are not command-side names. I kept them separate rather than deriving one from the other, because merging them would make the name grader start refusing the string "control" in a variable name. Worth a second opinion.
Segment matching is the judgement call. It refuses app.arbiter and permits controller. A future module genuinely named e.g. arbiter_shim would slip past. I chose the false-negative over refusing goodwe_controller, and the eight innocent spellings pin that choice so it is at least visible.
My first negative control was wrong and is fixed. I initially claimed arbitrage was the substring trap that mattered; it is not - "arbiter" is not a substring of "arbitrage", and mutation C2 stayed green and caught me. The controls are now clamps / arbiters / controller, which really are traps, and C2 goes red on them. Flagging this because it is exactly the class of thing this ticket is about.
Coordination with the adjacent open cards
Read before editing, as asked:
TEST-18 flags the two-selector split in section 10. No overlap - all of item 1 lands in section 7, and item 2 rewrites only the _imports block at the very end of section 10, below the _peak_rows selector and touching neither it nor peak_rows. The _peak_rows lambda and the "quarter(s) banked" substring are unchanged.
TARIFF-05 proposes making all three peak-row states selectable by one substring. That is product text in main.py's checks(); nothing here touches it, and if TARIFF-05 lands first this PR still applies cleanly.
## What was wrong
Two survivors from TARIFF-01's review, both closed here 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; they are struck with evidence rather than re-implemented.
### Item 1 - `record_block()`'s policy argument was unobservable
Mutating `PeakStore.record_block()`'s `w = billed_w(block, policy)` to `w = billed_w(block, POLICY_NET)` (**+4 B**) leaves **all 17 suites green**. Measured three times: 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 (17 PASS, rc=0).
**The product code is correct.** `tariff.py:363` is `def record_block(self, block, policy: str = POLICY_NET)` and it does forward the argument. What was missing is a check that could tell that apart from a hardcoded call.
The cause 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, and **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. Section 7 grades the *function* on an unbalanced fixture and says nothing about its *caller*.
**What I did.** 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, read back from **both** tables (`peak()` and `recompute_month()`, so both writes in that one 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 rather than over `vals`, because it is the banked ones this triple is made of.
**Direction, which is why it 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`, 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, and I want to be clear about that**: any *use* of either alias still produces an `Attribute` the `_names` half catches, so this was never a live exposure. The defect is that 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. AC 2 allowed either fixing the check or narrowing the sentence; I fixed the check, because the fix is smaller than the honest narrowing would have been.
`_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 now grade the grader itself, and **both** halves 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 rather than 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, under 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**, the under-guarding direction for SAFETY-07's floor. Restoring a default would re-open a money guard, and I did not.
**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 - which demonstrates TARIFF-01's "double counting is answered structurally, not by care" claim rather than contradicting it. Untouched.
## 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.
| run | delta | result |
|---|---|---|
| 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**. Shipped suite on the same tree: **GREEN** - the hole |
| **B2** add `import app.arbiter` to `app/tariff.py` | +19 B | **1 RED**. Shipped suite on the same tree: **GREEN** - the hole |
| **B3** add `from .control import compute` (the form the OLD grader did catch - regression control) | - | **1 RED** |
| **C1** delete the `ast.Import` branch from `_imported_modules()` | - | **1 RED**, naming the two spellings it missed |
| **C2** segment match -> substring match (`any(f in m for f in ...)`) | - | **1 RED**, naming `clamps` / `arbiters` / `controller` as falsely flagged |
**The control, and the point of the 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, is now measured a third time, and is now closed.
`MUTATION.md`'s survey row for this survivor is updated from "re-verified - still a survivor" to closed, the same way TEST-16 updated the SAFETY-26 row.
## What I am unsure about, and what a reviewer should look hardest at
1. **A PRE-EXISTING, CLOCK-DEPENDENT RED IN `test_peakguard.py` - unrelated to this PR, but I hit it and then chased it down. This wants its own card.**
CI attempt 1 on this sha failed, on `test_peakguard.py`, on a check this PR does not touch. Attempt 2 on the **identical sha** passed. This PR changes `test_tariff.py`, `CHANGELOG.md` and `MUTATION.md` only - no product code, no peakguard fixture - so I did not accept "flake" and reproduced it deterministically instead.
**On a PRISTINE `origin/release/1.0` tree** (`git archive origin/release/1.0`, nothing of mine in it), under `libfaketime` in the `goodwe-addon-ci` image:
| wall clock, seconds into the 900 s quarter | remaining | result |
|---|---|---|
| 5, 60, 180, 330, 480, 630, 780, 800, 830 | 895 .. 70 s | **PASS** |
| **850** | 50 s | **5 RED** |
| **860** | 40 s | **5 RED** |
| 870 | 30 s | **4 RED** |
| 895 | 5 s | PASS |
So `test_peakguard.py` is red whenever the wall clock lands in roughly the **last 40-60 seconds of a quarter** - about 5-6 % of runs, which is the right order for "seen once in nine local runs and once in two CI runs". The *set* of red checks varies with the offset, which is what a saturation boundary looks like rather than one bad assertion.
**The CI timestamp confirms it.** Attempt 1's failure was stamped `06:44:59 UTC`; the quarter boundary is `06:45:00`. The failing fixture sits at the end of a 49 s suite, so it ran within the last minute of that quarter - inside the window above.
**Where it lives.** `wired_over_limit()` (`test_peakguard.py:1007`) derives its load backwards from `remaining = QUARTER_S - (now - blk)` precisely so the answer does not depend on the minute it ran - its own docstring says "*a check that saturated or not depending on the minute it ran would be a coin toss wearing a verdict*". The intent is right and the last minute is not covered by it; its four-attempt retry loop only re-rolls on a *boundary crossing*, not on a small `remaining`.
**I did not fix it here.** It is another suite, it is not in TEST-13's scope, and `test_peakguard.py` part 10 is live territory for SAFETY-25 / TEST-18 - a blind edit from this branch would collide. Flagging it for a card instead, with the repro above ready to use. **Reviewer: if CI on this PR goes red on `test_peakguard.py`, that is this bug and not this change - re-run it.**
2. **`_FORBIDDEN_MODULES` is a second list beside `_FORBIDDEN`.** The name grader's set already contains `"arbiter"`; the module grader needs `clamp` and `control` too, which are not command-side *names*. I kept them separate rather than deriving one from the other, because merging them would make the name grader start refusing the string `"control"` in a variable name. Worth a second opinion.
3. **Segment matching is the judgement call.** It refuses `app.arbiter` and permits `controller`. A future module genuinely named e.g. `arbiter_shim` would slip past. I chose the false-negative over refusing `goodwe_controller`, and the eight innocent spellings pin that choice so it is at least visible.
4. **My first negative control was wrong and is fixed.** I initially claimed `arbitrage` was the substring trap that mattered; it is not - `"arbiter"` is not a substring of `"arbitrage"`, and mutation C2 stayed green and caught me. The controls are now `clamps` / `arbiters` / `controller`, which really are traps, and C2 goes red on them. Flagging this because it is exactly the class of thing this ticket is about.
## Coordination with the adjacent open cards
Read before editing, as asked:
* **TEST-18** flags the two-selector split in section 10. **No overlap** - all of item 1 lands in **section 7**, and item 2 rewrites only the `_imports` block at the very end of section 10, below the `_peak_rows` selector and touching neither it nor `peak_rows`. The `_peak_rows` lambda and the `"quarter(s) banked"` substring are unchanged.
* **TARIFF-05** proposes making all three peak-row states selectable by one substring. That is product text in `main.py`'s `checks()`; **nothing here touches it**, and if TARIFF-05 lands first this PR still applies cleanly.
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) <noreply@anthropic.com>
admin
merged commit 1c59cfeaf5 into release/1.02026-09-01 09:16:38 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
What was wrong
Two survivors from TARIFF-01's review, both closed here in
test_tariff.pyonly.app/is untouched and no shipped behaviour changes. Two further items on the card were already closed in shipped code; they are struck with evidence rather than re-implemented.Item 1 -
record_block()'s policy argument was unobservableMutating
PeakStore.record_block()'sw = billed_w(block, policy)tow = billed_w(block, POLICY_NET)(+4 B) leaves all 17 suites green. Measured three times: 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 (17 PASS, rc=0).The product code is correct.
tariff.py:363isdef record_block(self, block, policy: str = POLICY_NET)and it does forward the argument. What was missing is a check that could tell that apart from a hardcoded call.The cause 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 toblock.offtake_avg_wwheneverper_phase_offtake_avg_wis absent, and 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. Section 7 grades the function on an unbalanced fixture and says nothing about its caller.What I did. Section 7's own
(3000, 500, -200) Wblock - whose three reductions are 3300 / 3500 / 3000 W, asserted distinct three lines above - is now round-tripped through the store under each policy, read back from both tables (peak()andrecompute_month(), so both writes in that one 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 rather than overvals, because it is the banked ones this triple is made of.Direction, which is why it 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 fromproject().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 _abis anImportFromwhose.moduleis None (the dots are in.level, the module name is in.names), so it collected""and matched nothing;import app.arbiteris anast.Importnode the walk filtered out entirely.Both were added to
app/tariff.pyand the shipped suite stayed green, with the_nameshalf not firing either (an alias import creates noNamenode).Low severity, and I want to be clear about that: any use of either alias still produces an
Attributethe_nameshalf catches, so this was never a live exposure. The defect is that 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. AC 2 allowed either fixing the check or narrowing the sentence; I fixed the check, because the fix is smaller than the honest narrowing would have been._imported_modules()now walksImportas well asImportFromand joinsfrom . import Xonto its base._forbidden_imports()matches per dotted segment, not by substring:app.arbitermust not hide behind a package prefix, andclamps/arbiters/controllermust not be falsely flagged -controlleris not hypothetical, this package isgoodwe_controller. Five evasion spellings and eight innocent ones now grade the grader itself, and both halves are shown non-vacuous againstmain.py, which carries all three import forms and really does import all three modules.Items 3 and 4 - already closed, struck rather than re-implemented
app/tariff.py:266-267isdef is_observed(self): return self.state == OBSERVED- already false forNO_RECORD. Nothing to do.app/tariff.py:672-680readsheld = max(averager.held_net_w, 0.0)with nogetattrdefault, under a comment naming TEST-13 item 4. Fixed by SAFETY-07 in616b62d;held_net_wis a documented public property onQuarterAverager(p1.py:290), so a rename raisesAttributeErroron the first cycle instead of crediting 0 W held. A projection crediting 0 W held reads low, the under-guarding direction for SAFETY-07's floor. Restoring a default would re-open a money guard, and I did not.The recorded non-finding stays a non-finding. Removing
clear_partial()is still green and that is a PASS.quarter_startis the PRIMARY KEY and the upsert carriesON CONFLICT ... WHERE excluded.offtake_avg_w > quarter_blocks.offtake_avg_w, so a replay is a structural no-op - which demonstrates TARIFF-01's "double counting is answered structurally, not by care" claim rather than contradicting it. Untouched.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 thegoodwe-addon-ciimage against a:romount.billed_w(block, policy)->billed_w(block, POLICY_NET)sumandmaxround-trips, both tiers, and the distinctness pinbilled_w(block, policy)->billed_w(block, POLICY_MAX)netandsumround-trips, both tiers, the default, and the pinfrom . import arbiter as _abtoapp/tariff.pyimport app.arbitertoapp/tariff.pyfrom .control import compute(the form the OLD grader did catch - regression control)ast.Importbranch from_imported_modules()any(f in m for f in ...))clamps/arbiters/controlleras falsely flaggedThe control, and the point of the ticket: A1 graded by the shipped
test_tariff.pyis green, and A1 graded by all 17 shipped suites on a clean uncontended run is 17 PASS, rc=0. The survivor is real, is now measured a third time, and is now closed.MUTATION.md's survey row for this survivor is updated from "re-verified - still a survivor" to closed, the same way TEST-16 updated the SAFETY-26 row.What I am unsure about, and what a reviewer should look hardest at
A PRE-EXISTING, CLOCK-DEPENDENT RED IN
test_peakguard.py- unrelated to this PR, but I hit it and then chased it down. This wants its own card.CI attempt 1 on this sha failed, on
test_peakguard.py, on a check this PR does not touch. Attempt 2 on the identical sha passed. This PR changestest_tariff.py,CHANGELOG.mdandMUTATION.mdonly - no product code, no peakguard fixture - so I did not accept "flake" and reproduced it deterministically instead.On a PRISTINE
origin/release/1.0tree (git archive origin/release/1.0, nothing of mine in it), underlibfaketimein thegoodwe-addon-ciimage:So
test_peakguard.pyis red whenever the wall clock lands in roughly the last 40-60 seconds of a quarter - about 5-6 % of runs, which is the right order for "seen once in nine local runs and once in two CI runs". The set of red checks varies with the offset, which is what a saturation boundary looks like rather than one bad assertion.The CI timestamp confirms it. Attempt 1's failure was stamped
06:44:59 UTC; the quarter boundary is06:45:00. The failing fixture sits at the end of a 49 s suite, so it ran within the last minute of that quarter - inside the window above.Where it lives.
wired_over_limit()(test_peakguard.py:1007) derives its load backwards fromremaining = QUARTER_S - (now - blk)precisely so the answer does not depend on the minute it ran - its own docstring says "a check that saturated or not depending on the minute it ran would be a coin toss wearing a verdict". The intent is right and the last minute is not covered by it; its four-attempt retry loop only re-rolls on a boundary crossing, not on a smallremaining.I did not fix it here. It is another suite, it is not in TEST-13's scope, and
test_peakguard.pypart 10 is live territory for SAFETY-25 / TEST-18 - a blind edit from this branch would collide. Flagging it for a card instead, with the repro above ready to use. Reviewer: if CI on this PR goes red ontest_peakguard.py, that is this bug and not this change - re-run it._FORBIDDEN_MODULESis a second list beside_FORBIDDEN. The name grader's set already contains"arbiter"; the module grader needsclampandcontroltoo, which are not command-side names. I kept them separate rather than deriving one from the other, because merging them would make the name grader start refusing the string"control"in a variable name. Worth a second opinion.Segment matching is the judgement call. It refuses
app.arbiterand permitscontroller. A future module genuinely named e.g.arbiter_shimwould slip past. I chose the false-negative over refusinggoodwe_controller, and the eight innocent spellings pin that choice so it is at least visible.My first negative control was wrong and is fixed. I initially claimed
arbitragewas the substring trap that mattered; it is not -"arbiter"is not a substring of"arbitrage", and mutation C2 stayed green and caught me. The controls are nowclamps/arbiters/controller, which really are traps, and C2 goes red on them. Flagging this because it is exactly the class of thing this ticket is about.Coordination with the adjacent open cards
Read before editing, as asked:
_importsblock at the very end of section 10, below the_peak_rowsselector and touching neither it norpeak_rows. The_peak_rowslambda and the"quarter(s) banked"substring are unchanged.main.py'schecks(); nothing here touches it, and if TARIFF-05 lands first this PR still applies cleanly.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) <noreply@anthropic.com>