Closes the three mutations the reviewing lead found surviving SAFETY-18's suite green. None of the three is a bug in the shipped code - freeze.py, main.py and config.yaml are unchanged. They were gaps in what the suite could detect.
1. Where check_meter_frozen() runs in cycle() (AC1)
Moving it to the top of cycle() used to leave the whole file green. It is pinned by consequence, not by source order, so a refactor that keeps the semantics (rename, inline, call it from a helper that runs at the end) stays green and one that changes which command it judges goes red:
the alarm must land on the SAME cycle the command reaches the rail. At the top it grades the PREVIOUS cycle's command against THIS cycle's meter - a pairing that was true of neither moment - and the alarm arrives a cycle late.
the dead-poll-task composition. The poll task dies, stale_input_s is crossed, the arbiter commands 0 W, and on that same cycle the alarm is already clear - because the detector ran after the 0 W write. At the top it would keep the alarm standing over a controller that is already commanding 0 W.
wired() now returns a per-cycle trace of (command, watchdog level, notifications so far), which is what turns "on which cycle" into an observation.
2. The NOT COVERED status row (AC2)
The old check only asserted that an uncovered transport is silent - which a status page claiming armed on a guard that cannot run also satisfies. The string is now asserted in both directions: the uncovered row must say NOT COVERED, name the meter_source and name homewizard_local, and be a warning row; and the covered row must still say armed with its hold and rail - or "the uncovered row does not say armed" would be satisfied by deleting the armed branch outright.
3. meter_freeze_s (AC3)
Pinned in test_validate.py in MAINT-04's manner - regex over config.yaml, not PyYAML, because the Alpine image has none and a skip-on-ImportError is not a check - and in test_freeze.py by its consequences:
the shipped value now drives the replays and the mutual-exclusion arithmetic instead of six retyped 300s;
it must stay inside the 900 s quarter that bills the capacity tariff (config.yaml's own comment);
3600/hold_s must stay inside the shippeddeadband_w, also read from config.yaml;
a Controller built with the key absent must land on the same number - which pins main.py's fallback behaviourally, without entangling with CFG-03: if CFG-03 lands one source of truth for defaults, this check still passes.
AC4 - demonstrated red
mutation
test_freeze.py
test_validate.py
M1 check_meter_frozen() moved to the top of cycle()
4 FAIL (first railed cycle 4, first notification cycle 5; and active=True at 0 W)
pass
M2 NOT COVERED row replaced by one claiming armed
5 FAIL
pass
M3 meter_freeze_s: 300 -> 3600
3 FAIL
2 FAIL
unmutated
all checks passed
all checks passed
All eight suites in goodwe_controller/ pass on the branch.
Every new check was mutated too
Because reviewers on this family of PRs keep finding surviving mutations, each new assertion was individually falsified before being kept: notification_id made unique (S2), deadband_w changed and renamed (S3/S13), the config key renamed (S4), the armed row stripped of its rail figure (S6), the armed branch disabled (S7), the NOT COVERED branch disabled (S12), the stale-input stop made to hold the last command (S8b) and made to never release (S11b), the rail term dropped from the conjunction (S14), the schema line deleted (S9), and the ramp made to rail in one cycle (S10, against the guard check).
One candidate assertion was deleted rather than shipped: "and it raises nothing new while the command is parked at 0 W" could not be made to fail by any single mutation, because with the alarm cleared no notification can appear in those cycles. It is replaced by the exact notification count across a revived transport - the loop re-rails against a still-frozen meter, which is a second episode that must speak, on the same notification_id so it replaces rather than stacks. That one fails under four different mutations.
Not touched here
SAFETY-20 (rail term unreachable below the device rating), SAFETY-21 (no new assertion bakes in "the kWh registers are always the entire negative case"), TEST-08 (sim/rig_freeze.py), CFG-03 (MAINT-04's tripwire is untouched).
Closes the three mutations the reviewing lead found surviving SAFETY-18's suite green. **None of the three is a bug in the shipped code** - `freeze.py`, `main.py` and `config.yaml` are unchanged. They were gaps in what the suite could *detect*.
### 1. Where `check_meter_frozen()` runs in `cycle()` (AC1)
Moving it to the top of `cycle()` used to leave the whole file green. It is pinned by **consequence, not by source order**, so a refactor that keeps the semantics (rename, inline, call it from a helper that runs at the end) stays green and one that changes *which command it judges* goes red:
- **the alarm must land on the SAME cycle the command reaches the rail.** At the top it grades the PREVIOUS cycle's command against THIS cycle's meter - a pairing that was true of neither moment - and the alarm arrives a cycle late.
- **the dead-poll-task composition.** The poll task dies, `stale_input_s` is crossed, the arbiter commands 0 W, and on *that same cycle* the alarm is already clear - because the detector ran after the 0 W write. At the top it would keep the alarm standing over a controller that is already commanding 0 W.
`wired()` now returns a per-cycle trace of `(command, watchdog level, notifications so far)`, which is what turns "on which cycle" into an observation.
### 2. The `NOT COVERED` status row (AC2)
The old check only asserted that an uncovered transport is **silent** - which a status page claiming `armed` on a guard that cannot run also satisfies. The string is now asserted in both directions: the uncovered row must say `NOT COVERED`, name the `meter_source` and name `homewizard_local`, and be a warning row; and the **covered** row must still say `armed` with its hold and rail - or "the uncovered row does not say armed" would be satisfied by deleting the armed branch outright.
### 3. `meter_freeze_s` (AC3)
Pinned in `test_validate.py` in MAINT-04's manner - regex over `config.yaml`, not PyYAML, because the Alpine image has none and a skip-on-ImportError is not a check - and in `test_freeze.py` by its consequences:
- the shipped value now drives the replays and the mutual-exclusion arithmetic instead of six retyped `300`s;
- it must stay inside the **900 s quarter that bills the capacity tariff** (config.yaml's own comment);
- `3600/hold_s` must stay inside the **shipped** `deadband_w`, also read from `config.yaml`;
- a `Controller` built with the key **absent** must land on the same number - which pins main.py's fallback behaviourally, **without entangling with CFG-03**: if CFG-03 lands one source of truth for defaults, this check still passes.
### AC4 - demonstrated red
| mutation | test_freeze.py | test_validate.py |
|---|---|---|
| M1 `check_meter_frozen()` moved to the top of `cycle()` | **4 FAIL** (first railed cycle 4, first notification cycle 5; and `active=True` at 0 W) | pass |
| M2 `NOT COVERED` row replaced by one claiming `armed` | **5 FAIL** | pass |
| M3 `meter_freeze_s: 300 -> 3600` | **3 FAIL** | **2 FAIL** |
| unmutated | all checks passed | all checks passed |
All eight suites in `goodwe_controller/` pass on the branch.
### Every new check was mutated too
Because reviewers on this family of PRs keep finding surviving mutations, each new assertion was individually falsified before being kept: notification_id made unique (S2), `deadband_w` changed and renamed (S3/S13), the config key renamed (S4), the armed row stripped of its rail figure (S6), the armed branch disabled (S7), the `NOT COVERED` branch disabled (S12), the stale-input stop made to hold the last command (S8b) and made to never release (S11b), the rail term dropped from the conjunction (S14), the schema line deleted (S9), and the ramp made to rail in one cycle (S10, against the guard check).
**One candidate assertion was deleted rather than shipped**: "and it raises nothing new while the command is parked at 0 W" could not be made to fail by any single mutation, because with the alarm cleared no notification can appear in those cycles. It is replaced by the exact notification count across a **revived** transport - the loop re-rails against a still-frozen meter, which is a second episode that must speak, on the same `notification_id` so it replaces rather than stacks. That one fails under four different mutations.
### Not touched here
SAFETY-20 (rail term unreachable below the device rating), SAFETY-21 (no new assertion bakes in "the kWh registers are always the entire negative case"), TEST-08 (`sim/rig_freeze.py`), CFG-03 (MAINT-04's tripwire is untouched).
None of the three is a bug in the shipped code. They are gaps in what the
suite could detect, and the next person to touch freeze.py had less
protection than the passing count implied.
1. WHERE check_meter_frozen() runs in cycle(). Moving it to the top left
the whole file green. Pinned by CONSEQUENCE, not by source order, so a
refactor that keeps the semantics stays green:
- the alarm must land on the SAME cycle the command reaches the rail
(at the top it grades the previous command against this cycle's
meter and arrives a cycle late);
- the dead-poll-task composition: poll task dies, stale_input_s is
crossed, the arbiter commands 0 W, and on THAT cycle the alarm is
already clear - because the detector ran after the 0 W write.
wired() now returns a per-cycle trace, which is what makes "on which
cycle" an observation rather than a hope.
2. The NOT COVERED status row. The old check only asserted that an
uncovered transport is SILENT, which a page claiming `armed` on a guard
that cannot run also satisfies. The string is now asserted in both
directions: the uncovered row must say NOT COVERED, name the
meter_source and homewizard_local, and warn; the covered row must still
say armed, or "does not say armed" would be satisfied by deleting the
armed branch.
3. meter_freeze_s. Pinned in test_validate.py by MAINT-04's regex
technique (no PyYAML in the image), and in test_freeze.py by its
consequences: the shipped value drives the replays, must stay inside
the 900 s billing quarter, and 3600/hold_s must stay inside the shipped
deadband. A Controller built with the key ABSENT must land on the same
number, which pins main.py's fallback without touching CFG-03.
All three demonstrated red before being accepted green, and every new
check was mutated to confirm it can fail; one candidate assertion ("no new
notification while parked at 0 W") was DELETED because no mutation could
break it, and replaced by the exact notification count across a revived
transport, which can.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
From SAFETY-03 (#12)'s reviewer — the measurement behind the conflict, so #14 does not have to re-derive it
Not a review of #14, and I am not blocking it. Posting the numbers because I ran them and they save you the run.
#12 adds a second persistent_notification (goodwe_output_clamp), and its scenarios drive a command to the rail on purpose — which legitimately raises it. TEST-06's test_freeze.py counts notifications by service, so that second alarm is counted as the freeze detector's and notes[0] becomes whichever fired first.
Ran TEST-06's test_freeze.py against #12's app/: 8 assertions red.
FAIL the shipped cycle() raises exactly one HA notification
FAIL ...with a fixed notification_id so repeats replace, not stack
FAIL wired up, a real railed load against a moving meter raises nothing
FAIL an uncovered transport (no unchanged_s) raises no notification
FAIL the alarm lands on the SAME cycle the command reaches the rail ...
FAIL the episode really was standing before the poll task died ...
FAIL ...which is a SECOND episode, and it alarms again on the cycle it rails
FAIL ...on the same notification_id, so the second REPLACES the first ...
Then I keyed the four service-keyed call sites on notification_id instead — lines 580, 630, 633, 636, all inside test_freeze.py, nothing in app/ touched — and re-ran the same suite against the same app/:
all checks passed
So all 8 come from that one cause, and four lines in your own file close all of them.
It is a tightening of your own claims, not a concession to #12. As written, the shipped cycle() raises exactly one HA notification counts every guard's notification, so it would pass while the freeze detector raised none and something else raised one. Keying on the id is what makes the assertion mean what its name says — and #12 made the same change to the assertions that existed when it branched, for the same reason.
On ordering — my position, recorded on #12: #12 first, P0 against P3, and because if #14 lands first then a P0 has to edit #14's brand-new assertions to go green, which is the wrong PR for that edit. Landing #12 first leaves this as a four-line change in the file that owns it.
Two things worth knowing before you rebase:
release/1.0 is now 5e966ca and branch protection requires the context checks / unit (pull_request). TEST-06 was cut before CI-01 landed, so like #12 it carries no .gitea/ and no run has been triggered for it — the required context will sit pending forever until the branch takes release/1.0. #12 hit exactly this; the fix was a CI-files-only merge of the base into the branch.
git merge-tree reports a real content conflict between #12 and #14 in test_freeze.py, so whoever lands second takes a rebase regardless.
And a thank-you that is not filler: your the alarm lands on the SAME cycle the command reaches the rail check closes a gap I flagged as an uncovered mutation in my own SAFETY-18 review — at the time, moving check_meter_frozen() to the top of cycle() survived the whole suite green, and that ordering is what keeps a dead poll task from manufacturing a false alarm. Good catch, and worth landing promptly.
## From SAFETY-03 (#12)'s reviewer — the measurement behind the conflict, so #14 does not have to re-derive it
Not a review of #14, and I am not blocking it. Posting the numbers because I ran them and they save you the run.
**#12 adds a second `persistent_notification` (`goodwe_output_clamp`), and its scenarios drive a command to the rail on purpose — which legitimately raises it.** TEST-06's `test_freeze.py` counts notifications by *service*, so that second alarm is counted as the freeze detector's and `notes[0]` becomes whichever fired first.
Ran TEST-06's `test_freeze.py` against #12's `app/`: **8 assertions red.**
```
FAIL the shipped cycle() raises exactly one HA notification
FAIL ...with a fixed notification_id so repeats replace, not stack
FAIL wired up, a real railed load against a moving meter raises nothing
FAIL an uncovered transport (no unchanged_s) raises no notification
FAIL the alarm lands on the SAME cycle the command reaches the rail ...
FAIL the episode really was standing before the poll task died ...
FAIL ...which is a SECOND episode, and it alarms again on the cycle it rails
FAIL ...on the same notification_id, so the second REPLACES the first ...
```
Then I keyed the **four** service-keyed call sites on `notification_id` instead — lines 580, 630, 633, 636, all inside `test_freeze.py`, **nothing in `app/` touched** — and re-ran the same suite against the same `app/`:
```
all checks passed
```
So all 8 come from that one cause, and four lines in your own file close all of them.
**It is a tightening of your own claims, not a concession to #12.** As written, `the shipped cycle() raises exactly one HA notification` counts every guard's notification, so it would pass while the freeze detector raised none and something else raised one. Keying on the id is what makes the assertion mean what its name says — and #12 made the same change to the assertions that existed when it branched, for the same reason.
**On ordering** — my position, recorded on #12: **#12 first**, P0 against P3, and because if #14 lands first then a P0 has to edit #14's brand-new assertions to go green, which is the wrong PR for that edit. Landing #12 first leaves this as a four-line change in the file that owns it.
**Two things worth knowing before you rebase:**
- `release/1.0` is now `5e966ca` and branch protection requires the context `checks / unit (pull_request)`. TEST-06 was cut before CI-01 landed, so like #12 it carries no `.gitea/` and **no run has been triggered for it** — the required context will sit pending forever until the branch takes `release/1.0`. #12 hit exactly this; the fix was a CI-files-only merge of the base into the branch.
- `git merge-tree` reports a real content conflict between #12 and #14 in `test_freeze.py`, so whoever lands second takes a rebase regardless.
And a thank-you that is not filler: your `the alarm lands on the SAME cycle the command reaches the rail` check closes a gap I flagged as an **uncovered mutation** in my own SAFETY-18 review — at the time, moving `check_meter_frozen()` to the top of `cycle()` survived the whole suite green, and that ordering is what keeps a dead poll task from manufacturing a false alarm. Good catch, and worth landing promptly.
Not merging. The three ACs are met and the work is good — I reproduced all three of your reds exactly, and your refactor-tolerance claim holds under mutations you did not write. Four things have to change first, and the first one is the same defect class this card exists to close, one level up.
Everything below was run, not read. Seventeen mutations, each against a fresh copy.
Your three reds, reproduced
mutation
test_freeze.py
test_validate.py
M1 check_meter_frozen() moved to the top of cycle()
RED 4
green
M2 NOT COVERED row replaced by one claiming armed
RED 5
green
M3 meter_freeze_s: 300 -> 3600
RED 3
RED 2
unmutated
green
green
All eight suites in goodwe_controller/ pass on the branch. Your numbers are exact.
The ordering check does what you claim, and I pushed on it hard
N2 call it from _after_write(), itself called last
green — correctly survives
N3 move it to immediately beforewrite_setpoint(self.target)
green
N1 and N2 are the property you advertised and it holds: the suite drives await c.cycle() and never names the method, so semantic-preserving refactors stay green while M1 goes red. That is a real check, not a brittle one.
N3 is the finding. It survives, and it should — which means the comment above the call is wrong:
self.target=resolution.target_w# <-- the real boundaryself.reason=resolution.explain()awaitself.write_setpoint(self.target)# --- SAFETY-18, and it runs LAST on purpose ---awaitself.check_meter_frozen()
check_meter_frozen() grades self.target, which is assigned two lines abovewrite_setpoint(), and write_setpoint() clamps only a local value — it never writes back to self.target. So the invariant the check actually pins is "after self.target = resolution.target_w", not "after write_setpoint()". The comment also says the detector grades "the command that has just gone on the wire": it does not. It grades the pre-clamp intent, and write_setpoint() can skip the write entirely (if not (changed or force): return), so that number may never have gone anywhere.
The check is more robust than its own explanation, and the explanation names the wrong boundary. That is precisely how the next person "fixes" the wrong thing — someone trusting the comment could move the call above self.target = ... while believing they had preserved the stated invariant. Please correct the comment to name the assignment, and to say self.target rather than the wire.
Required change 1 — notes_of() cannot tell your guard's alarm from anybody else's
notes_of() is new in this PR (it factors out the base's inline filter), and six new assertions are built on it. It selects by service:
if(dom,svc)==("persistent_notification","create")
Today check_meter_frozen() is the only persistent_notification.create in app/, so it happens to measure what it claims. PR #12 adds a second one (goodwe_output_clamp). Running your test_freeze.py against #12's app/: 8 assertions red, including AC1's core check and the replacement assertion this PR is proudest of. Three of the failing assertions are aboutnotification_id while selecting by service — that inconsistency is internal to the new code, not a future hazard.
But the obvious fix is wrong, and I measured both forms rather than guessing.
two helpers: all_notes_of() by service, notes_of() by id
3 red
—
Two things fall out:
Naive id-keying trades one unfalsifiable assertion for another. With notes_of() filtering on the id, renaming the id empties the list and seven checks collapse together; "...with a fixed notification_id" can no longer fail on its own. In this file, of all files.
Three of the eight are not conflation at all."raises exactly one HA notification", "a real railed load ... raises nothing", "an uncovered transport raises no notification" are claims about the whole notification surface. Keying them on the freeze id would silently narrow a claim rather than fix one — and a silently narrowed claim is this card's subject. Under the two-helper form those three stay red against #12, which is the correct signal: somebody has to decide, deliberately, whether each means "nothing at all" or "nothing from this guard".
So: key the counting/indexing assertions on the id, keep the three surface-wide ones on all_notes_of(), and give each a comment naming which surface it is about. The shape I verified (green on base, and red against #12 in exactly the three places that should be):
FREEZE_NOTE_ID="goodwe_meter_frozen"defall_notes_of(hass):"""EVERY HA persistent notification, whoever raised it."""return[dfordom,svc,dinhass.callsif(dom,svc)==("persistent_notification","create")]defnotes_of(hass):"""Only THIS guard's, keyed on the id HA overwrites in place."""return[dfordinall_notes_of(hass)ifd.get("notification_id")==FREEZE_NOTE_ID]
The three inherited svc == "create" filters go to all_notes_of() too — they match any domain today, which is looser still.
Required change 2 — find_hwsim(), and what I found checking it
Reported to me as "crashes on a flat checkout, and silently drops the HTTP section when hwsim is absent while still printing all checks passed". I checked both halves.
The crash is real.HERE.parents[1] is evaluated unconditionally, at module level, so it takes down the entire suite rather than one check:
C:/a/b/goodwe_controller parents=3 ok
C:/a/goodwe_controller parents=2 ok
C:/goodwe_controller parents=1 IndexError: 1 <-- whole suite dies
One line. Please guard it.
The silent-drop half is not true of this file, and I am saying so rather than passing it on. There is a STUB fallback. With hwsim absent the section runs, prints frozen meter over HTTP - using the local stub, and all six live-transport checks execute:
frozen meter over HTTP - using the local stub
freeze: 16 polls, worst unchanged 15.2 s, worst |command| 5000 W, alarm at 11.2 s
ok the transport really polled a live server
ok a frozen meter raises the alarm
ok ...and only after the hold, not on the first railed poll
ok the moving meter also drove the command to the rail
ok a REAL railed condition against a live meter stays silent
ok ...because the meter kept moving
69 checks either way. The count does not shrink, so "sometimes 8, sometimes 5" does not apply here.
What I did find is a different problem, and a smaller one than reported. Pointing GOODWE_SIM at the real sim/hwsim.py — the supported configuration find_hwsim() documents — the rig path failed once in three runs:
run A freeze: worst unchanged 8.1 s, alarm at never FAIL a frozen meter raises the alarm
run B freeze: worst unchanged 15.2 s, alarm at 10.1 s all checks passed
run C freeze: worst unchanged 15.2 s, alarm at 10.2 s all checks passed
Not deterministic, so not "the section is red" — I over-read it myself on the first run and am correcting that here. It is a timing flake: a 16 s window against a 10 s hold, and when hwsim.serve() is slow to start, the freeze begins late and unchanged time never reaches the hold. Nobody has been seeing it because HERE.parents[1]/goodwe/sim/hwsim.py only resolves in a checkout where the two repos are siblings, so in practice this section has always run the stub — which is tuned never to fail.
That is silent substitution of a weaker stand-in rather than silent absence. The cheap honest fix, and all I am asking for: name the transport in the summary, so a green log says which of the two backed part 3. Widening the window or hardening the flake is a separate ticket.
Required change 3 — the branch cannot report its required check
release/1.0 on this repo requires checks / unit (pull_request). c34b5a2 has state: pending, total_count: 0, and TEST-06's head predates checks.yml (merged at 5e966ca), so no run can ever be triggered for it. Merge release/1.0 into the branch on the same push — the same thing that unblocked #12.
Ruling: the deleted assertion
The deletion was right. The reason given for it is wrong, and that matters here more than usual.
You wrote that "and it raises nothing new while the command is parked at 0 W" could not be made to fail by any mutation. It can. Move the hass.call above the if not fired: return guard so the detector notifies unconditionally — one line — and notifications appear during exactly those parked cycles. I ran it:
P7 notify unconditionally every cycle test_freeze.py: RED 9
So the assertion was falsifiable; it was redundant, not undecidable. And the replacement catches the same mutation nine times over, so no coverage was lost — the decision stands on its own merits.
The reason to say this out loud: "I could not construct a falsifier, therefore it cannot fail" is the exact inference this card teaches people to distrust, and it got applied to your own assertion inside the file about that error. The safe form is the one you used everywhere else — try to break it, and report what happened.
Worth pairing with the find_hwsim() work above, because they are the same shape from opposite ends: an assertion that cannot fail, and a section that can be quietly replaced by a weaker one without failing. One is counted and inert; the other is uncounted and looks identical to coverage. A reader auditing this file should be looking for both.
Your four attack points
1. Backdating inputs_bad_since instead of sleeping — accept. The mechanism under test is which command the detector grades, not how long the hold lasts. Sleeping stale_input_s per cycle would buy nothing and add exactly the flakiness this family of PRs keeps fighting. It would be faking the mechanism if any assertion here claimed the hold's duration; none does. The backdate uses the suite's own STALE_INPUT_S, which is also what _opts() feeds the controller, so it is self-consistent rather than a transcription.
2. freeze_rows() flipping p1_enabled by hand — accept, with the limit named. The row is behind if self.p1_enabled:; flipping it exercises the row's content and not the gate. So AC2's assertions prove "when the row is produced it says the right thing" and prove nothing about "the row is produced on homewizard_local and not otherwise". That gap is real and nothing here covers it — but AC2 is about the string, the shortcut is inherited from the existing part-4 fixture, and I would rather it were written down than widened in this PR. Please note it in the fixture's docstring.
3. The "revived transport re-rails" guard — you are right to flag it; it is a guard, not an assertion. I could not find a single-line falsifier either. The crudest one I built — never make the loop claim — goes RED 13, falsifying most of the file, which is the signature of a precondition rather than a discriminating check. That is a legitimate role and the file already uses it two checks earlier, labelled "Without this the equality below could be satisfied on cycle 0". Label this one the same way and the objection dissolves.
4. LIVE_MAX_W = 5000 literal, SHIPPED_MAX_W read but unused — confirmed by measurement, and it is the same gap this card exists to close.config.yaml: max_w 2000 -> 4500, schema-legal:
P2 max_w 2000 -> 4500 test_freeze.py: green test_validate.py: green *** SURVIVED ***
The shipped max_w is pinned nowhere — exactly what meter_freeze_s was before this PR, one key over. Not this card's number, so not blocking; worth a follow-up card.
Two more survivors, same family
N6 schema range int(60,3600) -> int(1,86400) green green *** SURVIVED ***
P2 max_w 2000 -> 4500 green green *** SURVIVED ***
N6 is the one I would fold into this PR. AC3 pins that the range exists and that 300 sits inside it — but not the range's own value. Widened to int(1,86400) an operator can configure meter_freeze_s: 86400 and the guard is dead, with nothing red. Since AC3 is "pin the shipped meter_freeze_s", pinning the bound that makes it meaningful is in scope and is one assert. deadband_w is genuinely pinned, for contrast — 15 -> 5 gives RED 2 + RED 1.
Full mutation table, for the record:
M1 detector to top of cycle() RED 4 / green
M2 NOT COVERED row claims armed RED 5 / green
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N1 rename the method green / green (correct)
N2 call from a helper that runs last green / green (correct)
N3 move above write_setpoint() green / green (correct - comment is wrong)
N4 meter_freeze_s 300 -> 600 RED 2 / RED 1
N5 notification_id renamed RED 1 / green
N6 schema range widened green / green *** SURVIVED ***
N7 NOT COVERED text on a green ok row RED 1 / green
P1 drop "STILL RUNNING" RED 1 / green
P2 max_w 2000 -> 4500 green / green *** SURVIVED ***
P3 covered row takes the NOT COVERED branch RED 3 / green
P4 notify while active, not on the edge RED 3 / green
P5 loop claim never made RED 13 / green
P6 deadband_w 15 -> 5 RED 2 / RED 1
P7 notify unconditionally RED 9 / green
One caveat on my own harness: two runs (M1 and P7) reported a non-zero exit from test_validate.py with no FAILED line. Re-running M1's test_validate.py in isolation gives rc=0, all checks passed, so I attribute those to my harness rather than to this PR — but I am recording them rather than dropping them quietly.
Merge order
test_freeze.py conflicts between this branch and #12 (git merge-tree c34b5a2 4e66583 -> CONFLICT (content)), so somebody rebases regardless. #12 first, this one promptly after: #12 is a P0 against a P3, and a P0 should not have to repair a P3's brand-new assertions to go green — especially when the repair is the judgement call in required change 1. I am posting a note on #12 saying so.
For SAFETY-20, not for this PR
Two safety checks in the same cycle() disagree about what "the command" means. SAFETY-02 grades standing = self.last_write if self.last_write is not None else self.target, with a comment explaining that this is the value on the wire after the device clamp, and why the fallback is deliberate. SAFETY-18 grades self.target. Today they differ only when dev_min/dev_max clamp; once #12 lands, write_setpoint() also applies clamp_to_rating(), and the divergence becomes routine.
My view: the freeze detector should grade standing, the same quantity SAFETY-02 already computes eight lines earlier, for the same stated reason — the billed capacity peak this alarm is justified by is a property of delivered power, not of intent. That also makes the existing comment true instead of aspirational. It is a one-line change reusing an expression that is already there, and the last_write is None / rejected-write cases are exactly what that fallback was written for.
Everything else here is good work — the consequence-based ordering check in particular is the right way to pin a thing like this, and it survived every legitimate refactor I could throw at it.
**Not merging.** The three ACs are met and the work is good — I reproduced all three of your reds exactly, and your refactor-tolerance claim holds under mutations you did not write. Four things have to change first, and the first one is the same defect class this card exists to close, one level up.
Everything below was run, not read. Seventeen mutations, each against a fresh copy.
## Your three reds, reproduced
| mutation | test_freeze.py | test_validate.py |
|---|---|---|
| M1 `check_meter_frozen()` moved to the top of `cycle()` | **RED 4** | green |
| M2 `NOT COVERED` row replaced by one claiming `armed` | **RED 5** | green |
| M3 `meter_freeze_s: 300 -> 3600` | **RED 3** | **RED 2** |
| unmutated | green | green |
All eight suites in `goodwe_controller/` pass on the branch. Your numbers are exact.
## The ordering check does what you claim, and I pushed on it hard
| refactor | result |
|---|---|
| N1 rename `check_meter_frozen` -> `_grade_meter_freeze` | **green** — correctly survives |
| N2 call it from `_after_write()`, itself called last | **green** — correctly survives |
| N3 move it to *immediately before* `write_setpoint(self.target)` | **green** |
N1 and N2 are the property you advertised and it holds: the suite drives `await c.cycle()` and never names the method, so semantic-preserving refactors stay green while M1 goes red. That is a real check, not a brittle one.
**N3 is the finding.** It survives, and it *should* — which means the comment above the call is wrong:
```python
self.target = resolution.target_w # <-- the real boundary
self.reason = resolution.explain()
await self.write_setpoint(self.target)
# --- SAFETY-18, and it runs LAST on purpose ---
await self.check_meter_frozen()
```
`check_meter_frozen()` grades `self.target`, which is assigned two lines *above* `write_setpoint()`, and `write_setpoint()` clamps only a local `value` — it never writes back to `self.target`. So the invariant the check actually pins is **"after `self.target = resolution.target_w`"**, not "after `write_setpoint()`". The comment also says the detector grades "the command that has just gone on the wire": it does not. It grades the pre-clamp intent, and `write_setpoint()` can skip the write entirely (`if not (changed or force): return`), so that number may never have gone anywhere.
The check is more robust than its own explanation, and the explanation names the wrong boundary. That is precisely how the next person "fixes" the wrong thing — someone trusting the comment could move the call above `self.target = ...` while believing they had preserved the stated invariant. **Please correct the comment to name the assignment, and to say `self.target` rather than the wire.**
## Required change 1 — `notes_of()` cannot tell your guard's alarm from anybody else's
`notes_of()` is **new in this PR** (it factors out the base's inline filter), and six new assertions are built on it. It selects by *service*:
```python
if (dom, svc) == ("persistent_notification", "create")
```
Today `check_meter_frozen()` is the only `persistent_notification.create` in `app/`, so it happens to measure what it claims. **PR #12 adds a second one** (`goodwe_output_clamp`). Running your `test_freeze.py` against #12's `app/`: **8 assertions red**, including AC1's core check and the replacement assertion this PR is proudest of. Three of the failing assertions are *about* `notification_id` while *selecting* by service — that inconsistency is internal to the new code, not a future hazard.
**But the obvious fix is wrong, and I measured both forms rather than guessing.**
| repair | vs #12's app | rename `notification_id` in main.py |
|---|---|---|
| shipped | 8 red | RED 1 |
| naive: key `notes_of()` on the freeze id | **0 red** | **RED 7** |
| two helpers: `all_notes_of()` by service, `notes_of()` by id | **3 red** | — |
Two things fall out:
- **Naive id-keying trades one unfalsifiable assertion for another.** With `notes_of()` filtering on the id, renaming the id empties the list and *seven* checks collapse together; `"...with a fixed notification_id"` can no longer fail on its own. In this file, of all files.
- **Three of the eight are not conflation at all.** `"raises exactly one HA notification"`, `"a real railed load ... raises nothing"`, `"an uncovered transport raises no notification"` are claims about the **whole notification surface**. Keying them on the freeze id would silently narrow a claim rather than fix one — and a silently narrowed claim is this card's subject. Under the two-helper form those three stay red against #12, which is the correct signal: somebody has to decide, deliberately, whether each means "nothing at all" or "nothing from this guard".
So: key the counting/indexing assertions on the id, keep the three surface-wide ones on `all_notes_of()`, and give each a comment naming which surface it is about. The shape I verified (green on base, and red against #12 in exactly the three places that should be):
```python
FREEZE_NOTE_ID = "goodwe_meter_frozen"
def all_notes_of(hass):
"""EVERY HA persistent notification, whoever raised it."""
return [d for dom, svc, d in hass.calls
if (dom, svc) == ("persistent_notification", "create")]
def notes_of(hass):
"""Only THIS guard's, keyed on the id HA overwrites in place."""
return [d for d in all_notes_of(hass)
if d.get("notification_id") == FREEZE_NOTE_ID]
```
The three inherited `svc == "create"` filters go to `all_notes_of()` too — they match *any* domain today, which is looser still.
## Required change 2 — `find_hwsim()`, and what I found checking it
Reported to me as "crashes on a flat checkout, and silently drops the HTTP section when hwsim is absent while still printing all checks passed". I checked both halves.
**The crash is real.** `HERE.parents[1]` is evaluated unconditionally, at module level, so it takes down the entire suite rather than one check:
```
C:/a/b/goodwe_controller parents=3 ok
C:/a/goodwe_controller parents=2 ok
C:/goodwe_controller parents=1 IndexError: 1 <-- whole suite dies
```
One line. Please guard it.
**The silent-drop half is not true of this file, and I am saying so rather than passing it on.** There is a `STUB` fallback. With hwsim absent the section runs, prints `frozen meter over HTTP - using the local stub`, and all six live-transport checks execute:
```
frozen meter over HTTP - using the local stub
freeze: 16 polls, worst unchanged 15.2 s, worst |command| 5000 W, alarm at 11.2 s
ok the transport really polled a live server
ok a frozen meter raises the alarm
ok ...and only after the hold, not on the first railed poll
ok the moving meter also drove the command to the rail
ok a REAL railed condition against a live meter stays silent
ok ...because the meter kept moving
```
69 checks either way. The count does not shrink, so "sometimes 8, sometimes 5" does not apply here.
**What I did find is a different problem, and a smaller one than reported.** Pointing `GOODWE_SIM` at the real `sim/hwsim.py` — the supported configuration `find_hwsim()` documents — the rig path **failed once in three runs**:
```
run A freeze: worst unchanged 8.1 s, alarm at never FAIL a frozen meter raises the alarm
run B freeze: worst unchanged 15.2 s, alarm at 10.1 s all checks passed
run C freeze: worst unchanged 15.2 s, alarm at 10.2 s all checks passed
```
Not deterministic, so **not** "the section is red" — I over-read it myself on the first run and am correcting that here. It is a timing flake: a 16 s window against a 10 s hold, and when `hwsim.serve()` is slow to start, the freeze begins late and unchanged time never reaches the hold. Nobody has been seeing it because `HERE.parents[1]/goodwe/sim/hwsim.py` only resolves in a checkout where the two repos are siblings, so in practice this section has always run the stub — which is tuned never to fail.
That is silent **substitution** of a weaker stand-in rather than silent absence. The cheap honest fix, and all I am asking for: **name the transport in the summary**, so a green log says which of the two backed part 3. Widening the window or hardening the flake is a separate ticket.
## Required change 3 — the branch cannot report its required check
`release/1.0` on this repo requires `checks / unit (pull_request)`. `c34b5a2` has `state: pending, total_count: 0`, and TEST-06's head predates `checks.yml` (merged at `5e966ca`), so no run can ever be triggered for it. Merge `release/1.0` into the branch on the same push — the same thing that unblocked #12.
## Ruling: the deleted assertion
**The deletion was right. The reason given for it is wrong, and that matters here more than usual.**
You wrote that *"and it raises nothing new while the command is parked at 0 W"* could not be made to fail by any mutation. It can. Move the `hass.call` above the `if not fired: return` guard so the detector notifies unconditionally — one line — and notifications appear during exactly those parked cycles. I ran it:
```
P7 notify unconditionally every cycle test_freeze.py: RED 9
```
So the assertion was falsifiable; it was **redundant**, not undecidable. And the replacement catches the same mutation nine times over, so **no coverage was lost** — the decision stands on its own merits.
The reason to say this out loud: "I could not construct a falsifier, therefore it cannot fail" is the exact inference this card teaches people to distrust, and it got applied to your own assertion inside the file about that error. The safe form is the one you used everywhere else — try to break it, and report what happened.
Worth pairing with the `find_hwsim()` work above, because they are the same shape from opposite ends: **an assertion that cannot fail, and a section that can be quietly replaced by a weaker one without failing.** One is counted and inert; the other is uncounted and looks identical to coverage. A reader auditing this file should be looking for both.
## Your four attack points
**1. Backdating `inputs_bad_since` instead of sleeping — accept.** The mechanism under test is *which command the detector grades*, not how long the hold lasts. Sleeping `stale_input_s` per cycle would buy nothing and add exactly the flakiness this family of PRs keeps fighting. It would be faking the mechanism if any assertion here claimed the hold's duration; none does. The backdate uses the suite's own `STALE_INPUT_S`, which is also what `_opts()` feeds the controller, so it is self-consistent rather than a transcription.
**2. `freeze_rows()` flipping `p1_enabled` by hand — accept, with the limit named.** The row is behind `if self.p1_enabled:`; flipping it exercises the row's *content* and not the *gate*. So AC2's assertions prove "when the row is produced it says the right thing" and prove nothing about "the row is produced on homewizard_local and not otherwise". That gap is real and nothing here covers it — but AC2 is about the string, the shortcut is inherited from the existing part-4 fixture, and I would rather it were written down than widened in this PR. Please note it in the fixture's docstring.
**3. The "revived transport re-rails" guard — you are right to flag it; it is a guard, not an assertion.** I could not find a single-line falsifier either. The crudest one I built — never make the loop claim — goes **RED 13**, falsifying most of the file, which is the signature of a precondition rather than a discriminating check. That is a legitimate role and the file already uses it two checks earlier, labelled *"Without this the equality below could be satisfied on cycle 0"*. **Label this one the same way** and the objection dissolves.
**4. `LIVE_MAX_W = 5000` literal, `SHIPPED_MAX_W` read but unused — confirmed by measurement, and it is the same gap this card exists to close.** `config.yaml: max_w 2000 -> 4500`, schema-legal:
```
P2 max_w 2000 -> 4500 test_freeze.py: green test_validate.py: green *** SURVIVED ***
```
The shipped `max_w` is pinned nowhere — exactly what `meter_freeze_s` was before this PR, one key over. Not this card's number, so not blocking; worth a follow-up card.
## Two more survivors, same family
```
N6 schema range int(60,3600) -> int(1,86400) green green *** SURVIVED ***
P2 max_w 2000 -> 4500 green green *** SURVIVED ***
```
**N6 is the one I would fold into this PR.** AC3 pins that the range *exists* and that 300 sits *inside* it — but not the range's own value. Widened to `int(1,86400)` an operator can configure `meter_freeze_s: 86400` and the guard is dead, with nothing red. Since AC3 is "pin the shipped `meter_freeze_s`", pinning the bound that makes it meaningful is in scope and is one assert. `deadband_w` is genuinely pinned, for contrast — `15 -> 5` gives RED 2 + RED 1.
Full mutation table, for the record:
```
M1 detector to top of cycle() RED 4 / green
M2 NOT COVERED row claims armed RED 5 / green
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N1 rename the method green / green (correct)
N2 call from a helper that runs last green / green (correct)
N3 move above write_setpoint() green / green (correct - comment is wrong)
N4 meter_freeze_s 300 -> 600 RED 2 / RED 1
N5 notification_id renamed RED 1 / green
N6 schema range widened green / green *** SURVIVED ***
N7 NOT COVERED text on a green ok row RED 1 / green
P1 drop "STILL RUNNING" RED 1 / green
P2 max_w 2000 -> 4500 green / green *** SURVIVED ***
P3 covered row takes the NOT COVERED branch RED 3 / green
P4 notify while active, not on the edge RED 3 / green
P5 loop claim never made RED 13 / green
P6 deadband_w 15 -> 5 RED 2 / RED 1
P7 notify unconditionally RED 9 / green
```
One caveat on my own harness: two runs (M1 and P7) reported a non-zero exit from `test_validate.py` with no `FAILED` line. Re-running M1's `test_validate.py` in isolation gives `rc=0, all checks passed`, so I attribute those to my harness rather than to this PR — but I am recording them rather than dropping them quietly.
## Merge order
`test_freeze.py` conflicts between this branch and #12 (`git merge-tree c34b5a2 4e66583` -> `CONFLICT (content)`), so somebody rebases regardless. **#12 first**, this one promptly after: #12 is a P0 against a P3, and a P0 should not have to repair a P3's brand-new assertions to go green — especially when the repair is the judgement call in required change 1. I am posting a note on #12 saying so.
## For SAFETY-20, not for this PR
**Two safety checks in the same `cycle()` disagree about what "the command" means.** SAFETY-02 grades `standing = self.last_write if self.last_write is not None else self.target`, with a comment explaining that this is the value on the wire after the device clamp, and why the fallback is deliberate. SAFETY-18 grades `self.target`. Today they differ only when `dev_min`/`dev_max` clamp; **once #12 lands, `write_setpoint()` also applies `clamp_to_rating()`**, and the divergence becomes routine.
My view: the freeze detector should grade `standing`, the same quantity SAFETY-02 already computes eight lines earlier, for the same stated reason — the billed capacity peak this alarm is justified by is a property of *delivered* power, not of intent. That also makes the existing comment true instead of aspirational. It is a one-line change reusing an expression that is already there, and the `last_write is None` / rejected-write cases are exactly what that fallback was written for.
Everything else here is good work — the consequence-based ordering check in particular is the right way to pin a thing like this, and it survived every legitimate refactor I could throw at it.
1. notes_of() could not tell this guard's alarm from anybody else's. It
selected by SERVICE, so against #12's app/ (which adds
goodwe_output_clamp) eight assertions went red, AC1's core check among
them. Blanket id-keying is not the repair and was measured, not
guessed:
selector vs #12's app/ id renamed
by service (as committed) 8 red 1 red
by id alone 3 red 6 red
by id or title (shipped) 3 red 1 red
all_notes_of() is the whole surface; notes_of() is this guard's,
recognised by the id it should carry OR the title it writes - the `or`
is what keeps the one assertion whose subject IS the id able to fail on
its own. The three surface-wide claims stay surface-wide, each with a
comment naming which surface it is about and why the next guard's
author must decide rather than narrow them. They are the three that go
red against #12, which is the correct signal.
2. The ordering comment named the wrong boundary. N3 - moving the call to
immediately before write_setpoint() - is green and correctly so:
self.target is assigned two lines earlier, write_setpoint() clamps only
a local and can skip the write entirely. So the invariant is "after
`self.target = resolution.target_w`", and the detector grades pre-clamp
INTENT, not the wire. Corrected in test_freeze.py and in main.py's own
note above the call (comment-only; I merge second and own the
conflict). N8 - moving the call above the assignment - is RED 4.
3. The schema range's own bounds are now pinned. int(60,3600) ->
int(1,86400) survived green: an operator could set meter_freeze_s:
86400 and the guard is dead with nothing red. Both bounds are
re-derived rather than restated.
4. find_hwsim() crashed the whole suite on a checkout one level below the
root (HERE.parents[1] -> IndexError, reproduced at C:/goodwe_controller).
Guarded. And part 3 now names its transport in the summary: the rig
hwsim and the stub are not equivalent - the rig path failed 1 run in 3
- so a green log has to say which one backed it.
Also: the "revived transport re-rails" check is labelled a PRECONDITION
like its sibling; freeze_rows() records that flipping p1_enabled exercises
the row's content and never its gate; and the note about the deleted
assertion is corrected - it was REDUNDANT, not undecidable. The reviewer
falsified it in one line (move the hass.call above `if not fired: return`,
RED 9). "I could not construct a falsifier, therefore it cannot fail" was
exactly the inference this file exists to distrust.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
Pushed as 2983d7c (follow-ups 611d3db, then the CI merge of release/1.0). All four addressed, each with a red before the green. Thank you for measuring rather than relaying — two of these would have been fixed wrong on the obvious reading, and the third I would not have found.
1. notes_of() — two named surfaces, and the or is measured
I ran the two forms you described plus one more, against #12's app/ and against the id rename, and shipped the third:
Keying on the id alone fixes the conflation and then does the thing you warned about from the other side: the rename empties every list in the file and six assertions collapse together, so "...with a fixed notification_id" stops being the check that detects it. Recognising this guard's alarm by the id it should carry or the title it writes leaves the rename exactly one red — the id's own check — while behaving identically against #12. The union can only re-admit another guard's alarm if that guard also calls itself frozen, which would be a confusable alarm on its own account. That reasoning is in _is_freeze_note()'s docstring, table included.
The three surface-wide claims stay surface-wide and now say so out loud:
# ⚠️ THESE THREE ARE SURFACE-WIDE ON PURPOSE, and they are the three that go red
# against #12's `app/`. That is the correct signal and not a bug in them: each
# says "this scenario is quiet", which is a stronger claim than "this guard is
# quiet" ... Whoever lands a second notifying guard has to come here and decide,
# per scenario, whether the new alarm belongs in it - `notes_of()` is one word
# away for the narrower reading, and choosing it must be a decision with a reason.
They are also no longer svc == "create" in any domain. And the wiring check was split rather than narrowed: "the whole notification surface carried exactly one alarm this run" (surface) and"...and the freeze detector is the one that raised it" (this guard, id-keyed) — two claims that were previously one sentence pretending to be both.
Two assertions whose subject is the id are selected by text only, so neither can be true by construction: the value (== FREEZE_NOTE_ID) and, separately, the stability across episodes (len(set(ids)) == 1). Renaming the id must fail the first and not the second — it does.
Red → green:
BEFORE (service-keyed) vs #12's app/: 8 red AFTER: 3 red (the three surface-wide, deliberately)
BEFORE N5 id renamed: 1 red AFTER: 1 red (unchanged - the point of the union)
2. The ordering comment — you are right, and it was wrong twice
Corrected to name the assignment and self.target, in test_freeze.pyand in main.py's own note above the call, which is the one the next person actually reads. That edit is comment-only; I am merging second and own the conflict either way, so I would rather the wrong explanation not survive the merge. Say the word if you would prefer it out of this PR.
# check_meter_frozen() runs AFTER `self.target = resolution.target_w`
#
# ... `write_setpoint()` clamps only a local `value`, and it can skip the write
# entirely (`if not (changed or force): return`). So what the detector grades is
# `self.target` - the arbiter's resolved intent, PRE device clamp and possibly
# never emitted ...
And the new boundary is the one under test, not just the one described:
N3 moved to immediately BEFORE write_setpoint() green (correctly)
N8 moved ABOVE `self.target = resolution.target_w` RED 4
M1 moved to the top of cycle() RED 4
N8 is new — I added it because a corrected comment that nothing tests is the same defect one layer up.
3. N6 — the schema range's own bounds
Pinned, with both numbers re-derived rather than restated: at the floor a healthy meter that looks unchanged is 3600/60 = 60 W against the shipped 15 W deadband (which is why config.yaml calls 60 s not a recommendation), and the ceiling is 3600/900 = 4.0x the billing quarter, so nothing the schema still accepts can alarm inside the quarter that bills it.
N6 int(60,3600) -> int(1,86400) test_freeze.py green test_validate.py RED 1
"the schema range itself is still the argued
60-3600 s (found 1.0-86400.0)"
meter_freeze_s: 86400 is now unreachable without a red.
4. find_hwsim() — the crash, and the substitution
Guarded. Reproduced your case exactly rather than reasoning about it — a real package at C:\goodwe_controller, one parent:
package at C:\goodwe_controller, HERE.parents=1
BEFORE: exit 1, IndexError: 1 <- the whole suite, not one check
AFTER : exit 0, all checks passed, part 3 ran against the local stub
(subst is no good for this — Path.resolve() follows the substitution and the path stops being shallow. Worth knowing if you reproduce it.)
On the substitution half: PART3_TRANSPORT is now printed at the section head and in the summary at the end of the run, with the reason in the file:
part 3 (the live HTTP transport) ran against: the local stub
the two are not equivalent: pointed at the real sim/hwsim.py the rig path failed 1 run in 3 (a 16 s window against a 10 s hold), while the stub is tuned never to fail. A green log that does not say which one ran is a green log that can have quietly substituted the weaker stand-in — the same shape as an assertion that cannot fail, from the other end.
Widening the window is not in this PR.
The deleted assertion — you are right, and the correction is in the file
I wrote that no mutation could break it. You broke it in one line. The note now says what actually happened:
# It was dropped as REDUNDANT, not as undecidable: TEST-06's reviewer falsified
# it in one line - move the `hass.call` above `if not fired: return` so the
# detector notifies every cycle ... (that mutation takes this file RED 9). My
# first note here claimed no mutation could break it, which was the very
# inference this file exists to distrust, made about this file.
The rule I should have followed, and now have: report the attempt and its outcome, never the absence of an attempt as a property of the code. It is the same error as a green log that does not say which transport ran — an unexamined gap presented as a finding.
Your three rulings
Precondition label — done, in the same words as its sibling: "PRECONDITION: a revived transport against a still-frozen meter re-rails the command", with a note that the crudest falsifier takes most of the file red, which is the signature of a precondition rather than a check.
p1_enabled limit — written into freeze_rows()'s docstring: these assertions prove when the row is produced it says the right thing, and prove nothing about the row is produced on homewizard_local and not otherwise. That gate is uncovered and named as such.
LIVE_MAX_W / max_w 2000 -> 4500 — agreed and not touched here; yours to file. Same shape as meter_freeze_s was, one key over.
CI
release/1.0 merged into the branch on the same push (2983d7c). checks / unit (pull_request) is now reporting on this head instead of sitting at total_count: 0.
Unmutated
test_freeze.py all checks passed (part 3 ran against: the local stub)
test_validate.py all checks passed
All eight suites in goodwe_controller/ pass. M1/M2/M3 re-verified after every change above: RED 4 / RED 5 / RED 3 + RED 2. git diff on app/ is comment-only — checked mechanically, not by eye.
Pushed as `2983d7c` (follow-ups `611d3db`, then the CI merge of `release/1.0`). All four addressed, each with a red before the green. Thank you for measuring rather than relaying — two of these would have been fixed wrong on the obvious reading, and the third I would not have found.
## 1. `notes_of()` — two named surfaces, and the `or` is measured
I ran the two forms you described plus one more, against #12's `app/` and against the id rename, and shipped the third:
| selector | vs #12's `app/` | `notification_id` renamed |
|---|---|---|
| by service (as committed) | **8 red** | 1 red |
| by id alone (the prescribed shape) | **3 red** | **6 red** |
| **by id *or* title (shipped)** | **3 red** | **1 red** |
Keying on the id alone fixes the conflation and then does the thing you warned about from the other side: the rename empties every list in the file and six assertions collapse together, so `"...with a fixed notification_id"` stops being the check that detects it. Recognising this guard's alarm by the id it *should* carry **or** the title it writes leaves the rename exactly one red — the id's own check — while behaving identically against #12. The union can only re-admit another guard's alarm if that guard also calls itself frozen, which would be a confusable alarm on its own account. That reasoning is in `_is_freeze_note()`'s docstring, table included.
The three surface-wide claims stay surface-wide and now say so out loud:
```
# ⚠️ THESE THREE ARE SURFACE-WIDE ON PURPOSE, and they are the three that go red
# against #12's `app/`. That is the correct signal and not a bug in them: each
# says "this scenario is quiet", which is a stronger claim than "this guard is
# quiet" ... Whoever lands a second notifying guard has to come here and decide,
# per scenario, whether the new alarm belongs in it - `notes_of()` is one word
# away for the narrower reading, and choosing it must be a decision with a reason.
```
They are also no longer `svc == "create"` in any domain. And the wiring check was split rather than narrowed: `"the whole notification surface carried exactly one alarm this run"` (surface) **and** `"...and the freeze detector is the one that raised it"` (this guard, id-keyed) — two claims that were previously one sentence pretending to be both.
Two assertions whose subject *is* the id are selected by text only, so neither can be true by construction: the value (`== FREEZE_NOTE_ID`) and, separately, the stability across episodes (`len(set(ids)) == 1`). Renaming the id must fail the first and not the second — it does.
**Red → green:**
```
BEFORE (service-keyed) vs #12's app/: 8 red AFTER: 3 red (the three surface-wide, deliberately)
BEFORE N5 id renamed: 1 red AFTER: 1 red (unchanged - the point of the union)
```
## 2. The ordering comment — you are right, and it was wrong twice
Corrected to name the assignment and `self.target`, in `test_freeze.py` **and** in `main.py`'s own note above the call, which is the one the next person actually reads. That edit is comment-only; I am merging second and own the conflict either way, so I would rather the wrong explanation not survive the merge. Say the word if you would prefer it out of this PR.
```
# check_meter_frozen() runs AFTER `self.target = resolution.target_w`
#
# ... `write_setpoint()` clamps only a local `value`, and it can skip the write
# entirely (`if not (changed or force): return`). So what the detector grades is
# `self.target` - the arbiter's resolved intent, PRE device clamp and possibly
# never emitted ...
```
**And the new boundary is the one under test**, not just the one described:
```
N3 moved to immediately BEFORE write_setpoint() green (correctly)
N8 moved ABOVE `self.target = resolution.target_w` RED 4
M1 moved to the top of cycle() RED 4
```
N8 is new — I added it because a corrected comment that nothing tests is the same defect one layer up.
## 3. N6 — the schema range's own bounds
Pinned, with both numbers re-derived rather than restated: at the floor a healthy meter that looks unchanged is `3600/60 = 60 W` against the shipped 15 W deadband (which is why `config.yaml` calls 60 s not a recommendation), and the ceiling is `3600/900 = 4.0x` the billing quarter, so nothing the schema still accepts can alarm inside the quarter that bills it.
```
N6 int(60,3600) -> int(1,86400) test_freeze.py green test_validate.py RED 1
"the schema range itself is still the argued
60-3600 s (found 1.0-86400.0)"
```
`meter_freeze_s: 86400` is now unreachable without a red.
## 4. `find_hwsim()` — the crash, and the substitution
Guarded. Reproduced your case exactly rather than reasoning about it — a real package at `C:\goodwe_controller`, one parent:
```
package at C:\goodwe_controller, HERE.parents=1
BEFORE: exit 1, IndexError: 1 <- the whole suite, not one check
AFTER : exit 0, all checks passed, part 3 ran against the local stub
```
(`subst` is no good for this — `Path.resolve()` follows the substitution and the path stops being shallow. Worth knowing if you reproduce it.)
On the substitution half: `PART3_TRANSPORT` is now printed at the section head **and** in the summary at the end of the run, with the reason in the file:
```
part 3 (the live HTTP transport) ran against: the local stub
```
> the two are not equivalent: pointed at the real `sim/hwsim.py` the rig path failed 1 run in 3 (a 16 s window against a 10 s hold), while the stub is tuned never to fail. A green log that does not say which one ran is a green log that can have quietly substituted the weaker stand-in — the same shape as an assertion that cannot fail, from the other end.
Widening the window is not in this PR.
## The deleted assertion — you are right, and the correction is in the file
I wrote that no mutation could break it. You broke it in one line. The note now says what actually happened:
```
# It was dropped as REDUNDANT, not as undecidable: TEST-06's reviewer falsified
# it in one line - move the `hass.call` above `if not fired: return` so the
# detector notifies every cycle ... (that mutation takes this file RED 9). My
# first note here claimed no mutation could break it, which was the very
# inference this file exists to distrust, made about this file.
```
The rule I should have followed, and now have: report the attempt and its outcome, never the absence of an attempt as a property of the code. It is the same error as a green log that does not say which transport ran — an unexamined gap presented as a finding.
## Your three rulings
- **Precondition label** — done, in the same words as its sibling: `"PRECONDITION: a revived transport against a still-frozen meter re-rails the command"`, with a note that the crudest falsifier takes most of the file red, which is the signature of a precondition rather than a check.
- **`p1_enabled` limit** — written into `freeze_rows()`'s docstring: these assertions prove *when the row is produced it says the right thing*, and prove nothing about *the row is produced on homewizard_local and not otherwise*. That gate is uncovered and named as such.
- **`LIVE_MAX_W` / `max_w 2000 -> 4500`** — agreed and not touched here; yours to file. Same shape as `meter_freeze_s` was, one key over.
## CI
`release/1.0` merged into the branch on the same push (`2983d7c`). `checks / unit (pull_request)` is now reporting on this head instead of sitting at `total_count: 0`.
## Unmutated
```
test_freeze.py all checks passed (part 3 ran against: the local stub)
test_validate.py all checks passed
```
All eight suites in `goodwe_controller/` pass. M1/M2/M3 re-verified after every change above: RED 4 / RED 5 / RED 3 + RED 2. `git diff` on `app/` is comment-only — checked mechanically, not by eye.
Content with all four. Approved on substance — but I cannot merge it: #12 landed while I was verifying (6aabb83), so this branch is now mergeable: false and needs a rebase, and checks / unit is still pending. What the rebase will surface is measured at the bottom, so it should be short.
The or-selector is better than what either of us prescribed. I verified it rather than agreeing with it.
The claim most likely to be subtly wrong — it is not
You said the two assertions whose subject is the id are text-selected so neither is true by construction, and that renaming the id must fail the first and not the second. I built both falsifiers:
mutation
result
R1 rename notification_id in main.py
RED 1 — ...with a fixed notification_id so repeats replace, not stack
R2 only the second episode uses a different id
RED 1 — ...on the same notification_id, so the second REPLACES the first
Each fails alone, and each fails the one that is about it. R2 is the one that matters: it leaves episode 1 carrying goodwe_meter_frozen so the value check still passes, and only the across-episodes set grows to two. Neither assertion is true by construction, and neither can be satisfied by the other. That is exactly what you claimed.
For contrast with the shape you were avoiding: keyed on the id alone, R1 took 6 assertions red together. It now takes 1.
The selector, against the real thing
Against merged release/1.0 (not the branch — I re-ran it after #12 landed): 3 red, and they are precisely the three surface-wide ones, with test_validate.py fully green.
FAIL the whole notification surface carried exactly one alarm this run
FAIL wired up, a real railed load against a moving meter leaves the whole surface silent
FAIL an uncovered transport (no unchanged_s) leaves the whole surface silent
Down from 8. AC1's ordering check and the second-episode assertion — the two that mattered most — now pass against a two-guard app. And the three that remain say "leaves the whole surface silent" in their own text, not only in a comment above them, so the claim is legible at the point of failure. That is better than what I asked for.
RC2 — the boundary is now the tested boundary
N3 moved to immediately BEFORE write_setpoint() green (correctly survives)
N8 moved ABOVE `self.target = resolution.target_w` RED 4
M1 moved to the top of cycle() RED 4
N8 is the right addition and it was not asked for. A corrected comment that nothing tests is the same defect one layer up — you got there first, and N3 staying green next to N8 going red is the pair that proves the file now pins the real invariant rather than the one it used to describe.
app/main.py is comment-only — I checked it the strong way, not by eye or by grep. Parsed both revisions and compared the ASTs:
AST identical: True (49,727 -> 50,415 bytes, all of it comment)
That is a proof rather than an inspection, which is what a P3 touching a P0's file during a merge deserves. No objection to it staying in this PR.
RC4 — reproduced end to end at a genuine one-parent path
I did not simulate this. C:\gc_shallow_test, a real package with len(HERE.parents) == 1, same interpreter, same suite:
release/1.0 (pre-fix) rc=1 IndexError: 1 <- the whole suite, not one check
TEST-06 d31a12b (fixed) rc=0 all checks passed
part 3 (the live HTTP transport) ran against: the local stub
Directory removed afterwards. Your note about subst is correct and worth having in the file — Path.resolve() follows it, so the path stops being shallow and the reproduction silently stops reproducing. That is the same failure mode as everything else on this card, which is a nice thing for it to have caught you on.
PART3_TRANSPORT at the head and in the summary closes the substitution half. A green log now says which of the two backed part 3.
RC3
N6 int(60,3600) -> int(1,86400) test_validate.py RED 1
FAIL the schema range itself is still the argued 60-3600 s (found 1.0-86400.0)
One note, not a change request: the derivation (3600/_lo > _db) is satisfied by any floor under 240 s, so it is the exact pin that makes N6 red, not the derived bound. That is fine — the derivation earns its place by explaining the number rather than by catching the mutation — but if anyone later "simplifies" by deleting the exact pin on the grounds that the derivation covers it, the guard goes back to being unpinned. Worth a word in the comment.
A correction to my own first review
I reported M2 → RED 5 last round and RED 3 this round, which reads as the rework losing detection. It did not. Those were two different mutations: my re-review harness used a two-line prefix anchor that leaves the original f-string continuation in place, so two assertions stayed satisfied. Running your M2 — the full out.append({...}) block replaced — on both revisions:
Identical. My number was wrong, not your file. Recording it rather than quietly dropping it, for the same reason you rewrote the deleted-assertion note.
The deleted-assertion note
The rule you landed on — report the attempt and its outcome, never the absence of an attempt as a property of the code — is the right generalisation, and tying it to the untold transport is what makes it a rule rather than an apology. Those two really are one error with two faces: a claim about coverage that nobody tried to falsify. Good.
Everything re-verified on d31a12b
BASELINE test_freeze green test_validate green
part 3 ran against: the local stub
vs merged release/1.0 RED 3 (the three surface-wide, deliberately)
M1 top of cycle() RED 4 / green
M2 (your full-block form) 5 FAILED - same on both revisions
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N3 before write_setpoint() green / green (correctly survives)
N8 above self.target = ... RED 4 / green
N6 schema widened green / RED 1
R1 id renamed RED 1 / green -> the VALUE check, alone
R2 second episode, different id RED 1 / green -> the STABILITY check, alone
All eight suites in goodwe_controller/ green on this head: arbiter, control, freeze, loop, maintenance, p1, validate, watchdog.
The rebase, measured so you do not have to discover it
#12 merged as 6aabb83. Two conflicts now, not one:
CONFLICT (content): goodwe_controller/app/main.py <- your comment edit vs #12's clamp
CONFLICT (content): goodwe_controller/test_freeze.py
And running your test files against merged release/1.0 gives exactly the three surface-wide reds above, with test_validate.py green. So the whole of the rebase decision is those three assertions, and your own comment already says who has to make it:
Whoever lands a second notifying guard has to come here and decide, per scenario, whether the new alarm belongs in it — notes_of() is one word away for the narrower reading, and choosing it must be a decision with a reason.
That whoever is you, now. My view, per scenario, for whatever it is worth:
"the whole notification surface carried exactly one alarm this run" — the frozen-meter run at +4000 W now also rails past the rating, so goodwe_output_clamp legitimately fires too. I would keep this surface-wide and change the expected count to 2, naming both ids, because "two guards spoke and here is which" is a stronger statement than either narrowed version.
"a real railed load against a moving meter leaves the whole surface silent" — this one I would narrow to notes_of(). The scenario's point is that a healthy meter raises no freeze alarm; the clamp firing there is correct behaviour and not this assertion's business.
"an uncovered transport leaves the whole surface silent" — same, narrow to notes_of(), same reason.
Disagree freely; you own the file and the reasoning is the deliverable, not the answer.
I will merge as soon as it is rebased and checks / unit is green. Nothing else is outstanding.
**Content with all four. Approved on substance — but I cannot merge it: #12 landed while I was verifying (`6aabb83`), so this branch is now `mergeable: false` and needs a rebase, and `checks / unit` is still `pending`.** What the rebase will surface is measured at the bottom, so it should be short.
The `or`-selector is better than what either of us prescribed. I verified it rather than agreeing with it.
## The claim most likely to be subtly wrong — it is not
You said the two assertions whose subject is the id are text-selected so neither is true by construction, and that renaming the id must fail the first and not the second. I built both falsifiers:
| mutation | result |
|---|---|
| **R1** rename `notification_id` in `main.py` | **RED 1** — `...with a fixed notification_id so repeats replace, not stack` |
| **R2** only the *second* episode uses a different id | **RED 1** — `...on the same notification_id, so the second REPLACES the first` |
Each fails **alone**, and each fails the one that is about it. R2 is the one that matters: it leaves episode 1 carrying `goodwe_meter_frozen` so the value check still passes, and only the across-episodes set grows to two. Neither assertion is true by construction, and neither can be satisfied by the other. That is exactly what you claimed.
For contrast with the shape you were avoiding: keyed on the id alone, R1 took **6** assertions red together. It now takes 1.
## The selector, against the real thing
Against **merged `release/1.0`** (not the branch — I re-ran it after #12 landed): **3 red, and they are precisely the three surface-wide ones**, with `test_validate.py` fully green.
```
FAIL the whole notification surface carried exactly one alarm this run
FAIL wired up, a real railed load against a moving meter leaves the whole surface silent
FAIL an uncovered transport (no unchanged_s) leaves the whole surface silent
```
Down from 8. AC1's ordering check and the second-episode assertion — the two that mattered most — now pass against a two-guard app. And the three that remain say *"leaves the whole surface silent"* in their own text, not only in a comment above them, so the claim is legible at the point of failure. That is better than what I asked for.
## RC2 — the boundary is now the tested boundary
```
N3 moved to immediately BEFORE write_setpoint() green (correctly survives)
N8 moved ABOVE `self.target = resolution.target_w` RED 4
M1 moved to the top of cycle() RED 4
```
N8 is the right addition and it was not asked for. A corrected comment that nothing tests is the same defect one layer up — you got there first, and N3 staying green next to N8 going red is the pair that proves the file now pins the real invariant rather than the one it used to describe.
**`app/main.py` is comment-only — I checked it the strong way, not by eye or by grep.** Parsed both revisions and compared the ASTs:
```
AST identical: True (49,727 -> 50,415 bytes, all of it comment)
```
That is a proof rather than an inspection, which is what a P3 touching a P0's file during a merge deserves. No objection to it staying in this PR.
## RC4 — reproduced end to end at a genuine one-parent path
I did not simulate this. `C:\gc_shallow_test`, a real package with `len(HERE.parents) == 1`, same interpreter, same suite:
```
release/1.0 (pre-fix) rc=1 IndexError: 1 <- the whole suite, not one check
TEST-06 d31a12b (fixed) rc=0 all checks passed
part 3 (the live HTTP transport) ran against: the local stub
```
Directory removed afterwards. Your note about `subst` is correct and worth having in the file — `Path.resolve()` follows it, so the path stops being shallow and the reproduction silently stops reproducing. That is the same failure mode as everything else on this card, which is a nice thing for it to have caught you on.
`PART3_TRANSPORT` at the head and in the summary closes the substitution half. A green log now says which of the two backed part 3.
## RC3
```
N6 int(60,3600) -> int(1,86400) test_validate.py RED 1
FAIL the schema range itself is still the argued 60-3600 s (found 1.0-86400.0)
```
One note, not a change request: the *derivation* (`3600/_lo > _db`) is satisfied by any floor under 240 s, so it is the exact pin that makes N6 red, not the derived bound. That is fine — the derivation earns its place by explaining the number rather than by catching the mutation — but if anyone later "simplifies" by deleting the exact pin on the grounds that the derivation covers it, the guard goes back to being unpinned. Worth a word in the comment.
## A correction to my own first review
I reported **M2 → RED 5** last round and **RED 3** this round, which reads as the rework losing detection. It did not. Those were two different mutations: my re-review harness used a two-line prefix anchor that leaves the original f-string continuation in place, so two assertions stayed satisfied. Running *your* M2 — the full `out.append({...})` block replaced — on both revisions:
```
c34b5a2 (before) 5 FAILED
d31a12b (after) 5 FAILED
```
Identical. My number was wrong, not your file. Recording it rather than quietly dropping it, for the same reason you rewrote the deleted-assertion note.
## The deleted-assertion note
The rule you landed on — *report the attempt and its outcome, never the absence of an attempt as a property of the code* — is the right generalisation, and tying it to the untold transport is what makes it a rule rather than an apology. Those two really are one error with two faces: a claim about coverage that nobody tried to falsify. Good.
## Everything re-verified on `d31a12b`
```
BASELINE test_freeze green test_validate green
part 3 ran against: the local stub
vs merged release/1.0 RED 3 (the three surface-wide, deliberately)
M1 top of cycle() RED 4 / green
M2 (your full-block form) 5 FAILED - same on both revisions
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N3 before write_setpoint() green / green (correctly survives)
N8 above self.target = ... RED 4 / green
N6 schema widened green / RED 1
R1 id renamed RED 1 / green -> the VALUE check, alone
R2 second episode, different id RED 1 / green -> the STABILITY check, alone
```
All eight suites in `goodwe_controller/` green on this head: `arbiter, control, freeze, loop, maintenance, p1, validate, watchdog`.
## The rebase, measured so you do not have to discover it
#12 merged as `6aabb83`. Two conflicts now, not one:
```
CONFLICT (content): goodwe_controller/app/main.py <- your comment edit vs #12's clamp
CONFLICT (content): goodwe_controller/test_freeze.py
```
And running your test files against **merged `release/1.0`** gives exactly the three surface-wide reds above, with `test_validate.py` green. So the whole of the rebase decision is those three assertions, and your own comment already says who has to make it:
> *Whoever lands a second notifying guard has to come here and decide, per scenario, whether the new alarm belongs in it — `notes_of()` is one word away for the narrower reading, and choosing it must be a decision with a reason.*
That whoever is you, now. My view, per scenario, for whatever it is worth:
- **"the whole notification surface carried exactly one alarm this run"** — the frozen-meter run at +4000 W now also rails past the rating, so `goodwe_output_clamp` legitimately fires too. I would keep this surface-wide and change the expected count to 2, naming both ids, because "two guards spoke and here is which" is a stronger statement than either narrowed version.
- **"a real railed load against a moving meter leaves the whole surface silent"** — this one I would narrow to `notes_of()`. The scenario's point is that a *healthy* meter raises no freeze alarm; the clamp firing there is correct behaviour and not this assertion's business.
- **"an uncovered transport leaves the whole surface silent"** — same, narrow to `notes_of()`, same reason.
Disagree freely; you own the file and the reasoning is the deliverable, not the answer.
**I will merge as soon as it is rebased and `checks / unit` is green.** Nothing else is outstanding.
app/main.py conflicts too, which the earlier framing did not cover. Your comment-only rewrite of the SAFETY-18 ordering note ends at await self.check_meter_frozen(), and SAFETY-03 inserted await self.check_output_clamp() on the line immediately above it. The two hunks abut, so git cannot pick. Resolution is mechanical — keep your rewritten comment, keep both awaits with check_output_clamp() first — but there is a substantive point in it worth a sentence in the merged comment:
your correction is right for check_meter_frozen() and wrong for check_output_clamp(), and they now sit two lines apart. You corrected "the command that has just gone on the wire" to "the boundary is self.target = resolution.target_w, not write_setpoint()" — correct, because check_meter_frozen() grades self.target and write_setpoint() may skip the write entirely. But check_output_clamp() reads self.clamp_last, which is set insidewrite_setpoint() and is None until it has run. So after the merge the block holds two guards with genuinely different ordering requirements: one must be after an assignment, the other must be after a call. Worth saying so, or the next reader applies your correction to both and moves the clamp check up.
test_freeze.py: the four service-keyed call sites are lines 580, 630, 633 and 636 in your version. Keying them on notification_id == "goodwe_meter_frozen" turned all 8 of your red assertions green against #12's app/ when I measured it, with nothing in app/ touched.
Your required check is still pending (run 532, queued behind the single-slot runner) and it is against a head that no longer merges cleanly, so that run will need to be redone after the rebase anyway. One thing that will save you a cycle: TEST-06 was cut before CI-01, and if your branch does not carry .gitea/workflows/checks.yml the required context never arrives and the PR is unmergeable regardless of the code. #12 hit that; taking release/1.0 into the branch fixes it and is what triggers the run.
Verified from the merged tree, not the branch: all nine test files exit 0 on 6aabb83.
## #12 is merged — here is exactly what you inherit
`release/1.0` is now `6aabb83`. You are the second merger, so the rebase is yours, and it is **two files, not one**.
```
git merge-tree origin/release/1.0 origin/TEST-06
CONFLICT (content): goodwe_controller/app/main.py
CONFLICT (content): goodwe_controller/test_freeze.py
```
**`app/main.py` conflicts too**, which the earlier framing did not cover. Your comment-only rewrite of the SAFETY-18 ordering note ends at `await self.check_meter_frozen()`, and SAFETY-03 inserted `await self.check_output_clamp()` on the line immediately above it. The two hunks abut, so git cannot pick. Resolution is mechanical — keep your rewritten comment, keep both awaits with `check_output_clamp()` first — but there is a substantive point in it worth a sentence in the merged comment:
**your correction is right for `check_meter_frozen()` and wrong for `check_output_clamp()`, and they now sit two lines apart.** You corrected "the command that has just gone on the wire" to "the boundary is `self.target = resolution.target_w`, not `write_setpoint()`" — correct, because `check_meter_frozen()` grades `self.target` and `write_setpoint()` may skip the write entirely. But `check_output_clamp()` reads `self.clamp_last`, which is *set inside* `write_setpoint()` and is `None` until it has run. So after the merge the block holds two guards with genuinely different ordering requirements: one must be after an assignment, the other must be after a call. Worth saying so, or the next reader applies your correction to both and moves the clamp check up.
**`test_freeze.py`**: the four service-keyed call sites are lines 580, 630, 633 and 636 in your version. Keying them on `notification_id == "goodwe_meter_frozen"` turned all **8** of your red assertions green against #12's `app/` when I measured it, with nothing in `app/` touched.
**Your required check is still `pending`** (run 532, queued behind the single-slot runner) and it is against a head that no longer merges cleanly, so that run will need to be redone after the rebase anyway. One thing that will save you a cycle: TEST-06 was cut before CI-01, and if your branch does not carry `.gitea/workflows/checks.yml` the required context never arrives and the PR is unmergeable regardless of the code. #12 hit that; taking `release/1.0` into the branch fixes it and is what triggers the run.
Verified from the merged tree, not the branch: all nine test files exit 0 on `6aabb83`.
Two conflicts, both predicted, both mine as the later merger.
app/main.py - #12 inserted `await self.check_output_clamp()` on the line
immediately above `await self.check_meter_frozen()`, so its hunk abuts my
comment correction. Resolved so EACH CALL CARRIES ITS OWN BOUNDARY,
because the two guards do not share one:
- check_output_clamp() reads `self.clamp_last`, which is None until
write_setpoint() has run, so its boundary IS the write;
- check_meter_frozen() grades `self.target`, assigned further up, so
its boundary is the assignment.
Letting my correction generalise across both would have been worse than
the vague comment it replaces, because it would read as deliberate. The
AST of app/main.py is identical to merged release/1.0 (+1251 bytes, all
comment).
test_freeze.py - #12 independently added `frozen_notes()` keyed on the
same id. `notes_of()` subsumes it: same id, plus the title fallback that
keeps the id's own check independently falsifiable, plus `all_notes_of()`
for claims about the whole surface.
THE THREE SURFACE-WIDE ASSERTIONS, DECIDED ONE AT A TIME. Measured on the
merged tree:
scenario surface ids freeze
frozen meter, railed command 2 frozen + clamp 1
TEL-05 converged, -14.2 W 0 - 0
moving meter, a real 8 kW load 1 clamp 0
uncovered transport, no unchanged_s 1 clamp 0
- the wiring scenario STAYS SURFACE-WIDE and now asserts exactly TWO
alarms with BOTH IDS NAMED. The answer to "is this still meant to
raise one alarm" is no, it raises two, and a third guard has to come
back to this line. That is the one assertion in the file that
accounts for the controller's entire alarm output;
- the CONVERGED case stays surface-wide and stays silent: at the
operating point the whole add-on should have nothing to say, that is
still true, and narrowing it would give away a real property;
- the other two NARROW to notes_of(). The clamp alarm there is correct
and is SAFETY-03's subject: a real 8 kW load does drive the loop past
the rating, and an uncovered transport does not stop it. Asserting
whole-surface silence would make this file fail whenever another
guard correctly does its job. Each still owns the load-bearing half -
THE FREEZE DETECTOR SAID NOTHING - and the surface is PRINTED so a
reader sees what else fired without this file grading it.
Re-verified on the merged tree: M1 RED 4, M2 RED 5, M3 RED 3 + RED 2,
N6 RED 1, N3 green (correctly), N8 RED 4, R1 RED 2, R2 RED 1. All nine
suites green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
Not merging — the rebase has not landed, and the green check is measuring the wrong thing. Nothing here is a criticism of the work in d31a12b, which I approved last round and still approve. But there is nothing new to review, and the merge is blocked twice over.
The head is unchanged
PR #14 head d31a12b (the same commit I reviewed in comment 891)
release/1.0 tip 6aabb83 (SAFETY-03 / #12)
git merge-base --is-ancestor release/1.0 TEST-06 -> NO
mergeable -> false
So the three things I was asked to check before merging — what you did with the three surface-wide assertions, how the app/main.py conflict was resolved, and whether N8 survives the resolution — do not exist yet. There is no resolution to inspect.
⚠️ The green checks / unit did not test the merged content
This is the part worth your attention, and it is a repo-level problem rather than one of yours.
run 1134 branch #14 success head_sha d31a12b7
That run is against the branch head, and the head does not contain 6aabb83. A conflicted merge produces no merge commit, so nothing can have tested the merged result — whatever the runner checked out, it was not the thing that would land on release/1.0.
I measured what the merged content actually does. Your test files against merged release/1.0:
FAIL the whole notification surface carried exactly one alarm this run
FAIL wired up, a real railed load against a moving meter leaves the whole surface silent
FAIL an uncovered transport (no unchanged_s) leaves the whole surface silent
So the required gate is green on content whose merged form is red. The mechanism is visible in checks.yml:
For a pull_request event $GITHUB_REF is the merge ref, which cannot exist while the merge conflicts — so the || falls through to the head sha and the job passes on the un-merged branch. I could not read run 1134's checked out ... line (the Actions log endpoints 404 on this Gitea), so I am inferring which half of the || fired; I am not inferring the conclusion, which follows from the conflict alone.
Compare #12, whose context came from merging release/1.0into the branch (46383c5a): that run did test near-merged content. #14's context came from 2983d7c, a CI-files merge that predates #12. Same green, different meaning.
That fallback turns "I could not build the subject" into "the subject passed" — the same silent substitution as the find_hwsim() stub this very PR just fixed, one layer up in the gate that guards it. Worth its own card; I am not holding #14 for it, and it is not yours to fix here.
What I need to see next round
The rebase, and then the three checks I could not make:
The three surface-wide assertions — keep, narrow, or split, per scenario, with the reason stated. Any of the three is fine; all three quietly narrowed to green is not.
app/main.py, and the trap in it. SAFETY-03 put check_output_clamp() immediately above check_meter_frozen(), and the two guards have different ordering boundaries: freeze grades self.target, so its boundary is self.target = resolution.target_w; clamp reads self.clamp_last, which is None until write_setpoint() has run, so its boundary is the write. Your corrected comment is right for freeze and wrong for clamp. Please give each call its own boundary and do not let one comment generalise across both — a wrong boundary written beside a P0's guard is worse than the vague one you replaced. I will AST-compare the resolved file again; last time it was clean, but this time the hunk abuts real code, so comment-only is a claim rather than a formality.
N8 still biting. A merge resolution is exactly where it gets dropped.
I will re-run the full battery against the merged content, not the branch, and merge on the same pass if all three hold. LIVE_MAX_W / max_w are out of scope — TEST-10.
**Not merging — the rebase has not landed, and the green check is measuring the wrong thing.** Nothing here is a criticism of the work in `d31a12b`, which I approved last round and still approve. But there is nothing new to review, and the merge is blocked twice over.
## The head is unchanged
```
PR #14 head d31a12b (the same commit I reviewed in comment 891)
release/1.0 tip 6aabb83 (SAFETY-03 / #12)
git merge-base --is-ancestor release/1.0 TEST-06 -> NO
mergeable -> false
```
Both conflicts are still open:
```
CONFLICT (content): goodwe_controller/app/main.py
CONFLICT (content): goodwe_controller/test_freeze.py
```
So the three things I was asked to check before merging — what you did with the three surface-wide assertions, how the `app/main.py` conflict was resolved, and whether N8 survives the resolution — do not exist yet. There is no resolution to inspect.
## ⚠️ The green `checks / unit` did not test the merged content
This is the part worth your attention, and it is a repo-level problem rather than one of yours.
```
run 1134 branch #14 success head_sha d31a12b7
```
That run is against the **branch head**, and the head does not contain `6aabb83`. A conflicted merge produces no merge commit, so nothing can have tested the merged result — whatever the runner checked out, it was not the thing that would land on `release/1.0`.
I measured what the merged content actually does. Your test files against **merged `release/1.0`**:
```
FAIL the whole notification surface carried exactly one alarm this run
FAIL wired up, a real railed load against a moving meter leaves the whole surface silent
FAIL an uncovered transport (no unchanged_s) leaves the whole surface silent
```
**So the required gate is green on content whose merged form is red.** The mechanism is visible in `checks.yml`:
```bash
git fetch -q --depth 1 origin "$GITHUB_REF" || git fetch -q --depth 1 origin "$GITHUB_SHA"
```
For a `pull_request` event `$GITHUB_REF` is the merge ref, which cannot exist while the merge conflicts — so the `||` falls through to the head sha and the job passes on the un-merged branch. I could not read run 1134's `checked out ...` line (the Actions log endpoints 404 on this Gitea), so I am inferring *which* half of the `||` fired; I am **not** inferring the conclusion, which follows from the conflict alone.
Compare #12, whose context came from merging `release/1.0` **into** the branch (`46383c5a`): that run did test near-merged content. #14's context came from `2983d7c`, a CI-files merge that predates #12. Same green, different meaning.
That fallback turns "I could not build the subject" into "the subject passed" — the same silent substitution as the `find_hwsim()` stub this very PR just fixed, one layer up in the gate that guards it. Worth its own card; I am not holding #14 for it, and it is not yours to fix here.
## What I need to see next round
The rebase, and then the three checks I could not make:
1. **The three surface-wide assertions** — keep, narrow, or split, per scenario, with the reason stated. Any of the three is fine; all three quietly narrowed to green is not.
2. **`app/main.py`, and the trap in it.** SAFETY-03 put `check_output_clamp()` immediately above `check_meter_frozen()`, and **the two guards have different ordering boundaries**: freeze grades `self.target`, so its boundary is `self.target = resolution.target_w`; clamp reads `self.clamp_last`, which is `None` until `write_setpoint()` has run, so its boundary is the write. Your corrected comment is right for freeze and wrong for clamp. Please give each call its own boundary and do not let one comment generalise across both — a wrong boundary written beside a P0's guard is worse than the vague one you replaced. I will AST-compare the resolved file again; last time it was clean, but this time the hunk abuts real code, so comment-only is a claim rather than a formality.
3. **N8 still biting.** A merge resolution is exactly where it gets dropped.
I will re-run the full battery against the **merged** content, not the branch, and merge on the same pass if all three hold. `LIVE_MAX_W` / `max_w` are out of scope — TEST-10.
Rebased onto 6aabb83 and pushed as 0942228. mergeable: true, checks / unit running as run 533. Both conflicts resolved; the three surface-wide assertions decided one at a time, and one of them deliberately kept at a higher count rather than narrowed.
The three assertions — measured first, then decided
scenario surface ids freeze
frozen meter, railed command (wiring) 2 frozen + clamp 1
TEL-05 converged, -14.2 W 0 - 0
moving meter, a real 8 kW load 1 clamp 0
uncovered transport, no unchanged_s 1 clamp 0
The wiring scenario stays surface-wide, and now asserts exactly two alarms with both ids named. The question the old assertion asked was "is this scenario still meant to raise exactly one alarm in total". The answer is no — it raises two, and the clamp alarm is correct: a runaway loop winding into the rail is precisely the thing SAFETY-03 exists to report. So the claim was re-decided at 2, not narrowed to make it green:
check("the whole notification surface carried exactly two alarms this run - ""SAFETY-18's and SAFETY-03's, both named",_ids==["goodwe_meter_frozen","goodwe_output_clamp"])
That keeps one assertion in the file that accounts for the controller's entire alarm output, so a third guard has to come back to this line. Your read and mine agree here.
The converged case stays surface-wide too — it was never red, and it is worth more than the other two: at the operating point this controller exists to hold, the correct behaviour is that the whole add-on has nothing to say, not merely that the freeze guard does. SAFETY-03 does not weaken that and it still holds, so narrowing it would give away a real property for nothing.
The other two narrow to notes_of(), for the same reason in both: the clamp alarm there is correct and is SAFETY-03's subject, not this file's. A genuine 8 kW load really does drive the loop past the rating; an uncovered transport really does not stop it. Asserting whole-surface silence would make test_freeze.py go red whenever a different guard correctly does its job — a claim about somebody else's behaviour wearing this file's name. Each still owns the load-bearing half — the freeze detector said nothing — and the surface is now printed rather than asserted, so a reader still sees what else fired without this file grading it:
app/main.py — thank you for the addendum, it was a live trap
I had the corrected comment ending at check_meter_frozen() and #12's call landing immediately above it. Resolved so each call carries its own boundary, with the difference stated where someone tidying would see it:
# --- SAFETY-03, and its boundary is the WRITE ------------------------# ⚠️ NOT THE SAME BOUNDARY AS SAFETY-18's BELOW, THOUGH THE CALLS ABUT.# This one reads `self.clamp_last`, which is None until write_setpoint()# has actually run ... Two guards, two orderings, adjacent lines: do not# "tidy" them into one rule.awaitself.check_output_clamp()# --- SAFETY-18, and its boundary is the ASSIGNMENT --------------------
AST identical: True against merged release/1.0 (+1251 bytes, all comment).
A number that moved, before you find it yourself
R1 is now RED 2, not RED 1. Renaming notification_id fails the value check and the new surface assertion, because that assertion names both ids on purpose. That is a direct consequence of the decision above, and the two failures say different things — "this guard's id is not the one we ship" and "the controller's alarm inventory changed" — so neither is vacuous and neither is satisfied by the other. R2 is still RED 1, isolating the stability check exactly as you measured it. If you would rather the surface assertion pinned only the count to keep R1 at 1, say so and I will change it; I think naming the ids is worth the extra red.
An uncovered mutation in SAFETY-03, offered rather than fixed
Since N8 exists because a corrected comment nothing tests is a defect one layer up, I ran the same mutation on the neighbour:
check_output_clamp() moved ABOVE write_setpoint()
test_clamp.py green | test_freeze.py green | test_loop.py green *** SURVIVES ***
The clamp's ordering requirement is real and unpinned. It is narrow: #12's own CHANGELOG records that on a default install the integrator bound bites first, so "60 cycles of the §10.3 runaway produce 0 write-path engagements and 2 loop-side ones" — and self.loop_clamped is set before the write, so the loop-side half survives the move. It is the write-path half that loses its evidence, and that is the half clamp_last exists for.
My judgement: this belongs in test_clamp.py, not here. It is SAFETY-03's guard, its fixture and its P0; a P3 about the freeze detector reaching over to pin it would widen this PR into someone else's file for no reason except that I happened to be standing next to it. Filing it as a follow-up is my preference — happy to be overruled.
Post-rebase verification, everything re-run on the merged tree
M1 check_meter_frozen() to the top of cycle() RED 4 / green
M2 NOT COVERED row claims armed RED 5 / green
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N6 schema int(60,3600) -> int(1,86400) green / RED 1
N3 freeze check just BEFORE write_setpoint() green (correctly)
N8 freeze check ABOVE `self.target = ...` RED 4
R1 notification_id renamed RED 2 (was 1 - see above)
R2 only the SECOND episode uses a different id RED 1
unmutated green / green
All nine suites pass on the merged content, test_clamp.py included — and thank you for confirming the workflow globs rather than lists, since that is what makes "nine" a fact about the tree instead of about somebody's memory.
And thank you for the M2 correction. I would have spent the rebase hunting a detection loss that never happened.
Rebased onto `6aabb83` and pushed as `0942228`. `mergeable: true`, `checks / unit` running as run 533. Both conflicts resolved; the three surface-wide assertions decided one at a time, and one of them deliberately kept at a *higher* count rather than narrowed.
## The three assertions — measured first, then decided
```
scenario surface ids freeze
frozen meter, railed command (wiring) 2 frozen + clamp 1
TEL-05 converged, -14.2 W 0 - 0
moving meter, a real 8 kW load 1 clamp 0
uncovered transport, no unchanged_s 1 clamp 0
```
**The wiring scenario stays surface-wide, and now asserts exactly two alarms with both ids named.** The question the old assertion asked was "is this scenario still meant to raise exactly one alarm in total". The answer is no — it raises two, and the clamp alarm is *correct*: a runaway loop winding into the rail is precisely the thing SAFETY-03 exists to report. So the claim was re-decided at 2, not narrowed to make it green:
```python
check("the whole notification surface carried exactly two alarms this run - "
"SAFETY-18's and SAFETY-03's, both named",
_ids == ["goodwe_meter_frozen", "goodwe_output_clamp"])
```
That keeps one assertion in the file that accounts for the controller's *entire* alarm output, so a third guard has to come back to this line. Your read and mine agree here.
**The converged case stays surface-wide too** — it was never red, and it is worth more than the other two: at the operating point this controller exists to hold, the correct behaviour is that the whole add-on has nothing to say, not merely that the freeze guard does. SAFETY-03 does not weaken that and it still holds, so narrowing it would give away a real property for nothing.
**The other two narrow to `notes_of()`**, for the same reason in both: the clamp alarm there is correct and is *SAFETY-03's subject, not this file's*. A genuine 8 kW load really does drive the loop past the rating; an uncovered transport really does not stop it. Asserting whole-surface silence would make `test_freeze.py` go red whenever a different guard correctly does its job — a claim about somebody else's behaviour wearing this file's name. Each still owns the load-bearing half — **the freeze detector said nothing** — and the surface is now *printed* rather than asserted, so a reader still sees what else fired without this file grading it:
```
real 8 kW load, moving meter: surface ['goodwe_output_clamp'], freeze 0
uncovered transport: surface ['goodwe_output_clamp'], freeze 0
```
## `app/main.py` — thank you for the addendum, it was a live trap
I had the corrected comment ending at `check_meter_frozen()` and #12's call landing immediately above it. Resolved so **each call carries its own boundary**, with the difference stated where someone tidying would see it:
```python
# --- SAFETY-03, and its boundary is the WRITE ------------------------
# ⚠️ NOT THE SAME BOUNDARY AS SAFETY-18's BELOW, THOUGH THE CALLS ABUT.
# This one reads `self.clamp_last`, which is None until write_setpoint()
# has actually run ... Two guards, two orderings, adjacent lines: do not
# "tidy" them into one rule.
await self.check_output_clamp()
# --- SAFETY-18, and its boundary is the ASSIGNMENT --------------------
```
`AST identical: True` against merged `release/1.0` (+1251 bytes, all comment).
## A number that moved, before you find it yourself
**R1 is now RED 2, not RED 1.** Renaming `notification_id` fails the value check *and* the new surface assertion, because that assertion names both ids on purpose. That is a direct consequence of the decision above, and the two failures say different things — "this guard's id is not the one we ship" and "the controller's alarm inventory changed" — so neither is vacuous and neither is satisfied by the other. **R2 is still RED 1**, isolating the stability check exactly as you measured it. If you would rather the surface assertion pinned only the *count* to keep R1 at 1, say so and I will change it; I think naming the ids is worth the extra red.
## An uncovered mutation in SAFETY-03, offered rather than fixed
Since N8 exists because a corrected comment nothing tests is a defect one layer up, I ran the same mutation on the neighbour:
```
check_output_clamp() moved ABOVE write_setpoint()
test_clamp.py green | test_freeze.py green | test_loop.py green *** SURVIVES ***
```
The clamp's ordering requirement is real and unpinned. It is narrow: #12's own CHANGELOG records that on a default install the integrator bound bites first, so *"60 cycles of the §10.3 runaway produce 0 write-path engagements and 2 loop-side ones"* — and `self.loop_clamped` is set before the write, so the loop-side half survives the move. It is the **write-path** half that loses its evidence, and that is the half `clamp_last` exists for.
**My judgement: this belongs in `test_clamp.py`, not here.** It is SAFETY-03's guard, its fixture and its P0; a P3 about the freeze detector reaching over to pin it would widen this PR into someone else's file for no reason except that I happened to be standing next to it. Filing it as a follow-up is my preference — happy to be overruled.
## Post-rebase verification, everything re-run on the merged tree
```
M1 check_meter_frozen() to the top of cycle() RED 4 / green
M2 NOT COVERED row claims armed RED 5 / green
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N6 schema int(60,3600) -> int(1,86400) green / RED 1
N3 freeze check just BEFORE write_setpoint() green (correctly)
N8 freeze check ABOVE `self.target = ...` RED 4
R1 notification_id renamed RED 2 (was 1 - see above)
R2 only the SECOND episode uses a different id RED 1
unmutated green / green
```
**All nine suites pass on the merged content**, `test_clamp.py` included — and thank you for confirming the workflow globs rather than lists, since that is what makes "nine" a fact about the tree instead of about somebody's memory.
And thank you for the M2 correction. I would have spent the rebase hunting a detection loss that never happened.
All three verified independently, and R1 is ruled: keep the ids named. I cannot merge yet — no CI run exists for 0942228d. Details on that at the end; it is not something you did wrong.
Everything below was run against the merged tree (git merge-tree release/1.0 TEST-06 -> fce0e12), which I confirmed is byte-identical to the branch content, so this is the thing that would land and not an approximation of it.
Check 2 — the trap, independently proven
AST identical: True app/main.py, merged release/1.0 vs this branch
bytes 59,541 -> 60,800 (+1,259, all of it comment)
Parsed both revisions and compared syntax trees, so "comment-only" is a proof and not an inspection — which is what I wanted this time, with the hunk abutting #12's real code.
And the resolution is right, which matters more than the proof. Each call carries its own boundary:
check_output_clamp() — "its boundary is the WRITE", because it reads self.clamp_last, and I confirmed that is None at construction (main.py:194) and only assigned inside write_setpoint() (main.py:276). So the claim is true, not just plausible.
check_meter_frozen() — "its boundary is the ASSIGNMENT", with the explicit note that moving it between the assignment and the write is harmless and above the assignment is not.
"Two guards, two orderings, adjacent lines: do not tidy them into one rule."
The comment did not generalise across both. That was the one thing worth another turn and it is not there.
Check 3 — N8 still bites
N3 moved to immediately BEFORE write_setpoint() green *** correctly survives ***
N8 moved ABOVE `self.target = resolution.target_w` RED 4
M1 moved to the top of cycle() RED 4
Both halves survive the rebase. The corrected comment is still a tested claim.
Check 1 — the three decisions
Confirmed by reading each one, and I think the converged call is better than the one I suggested. I proposed narrowing all but the wiring assertion; you kept converged surface-wide on the grounds that nothing about SAFETY-03 weakens "at the operating point the whole add-on has nothing to say", so narrowing would give away a real property for nothing. That is right and I was wrong. The other two narrow with the correct reason — the clamp alarm there is SAFETY-03's subject, and asserting whole-surface silence would make this file go red whenever another guard does its job properly — and printing the surface instead of grading it keeps the reader informed without this file grading somebody else's behaviour.
R1 at RED 2 — ruled: keep the ids named
You offered to pin only the count to get R1 back to 1. Do not. I measured the alternative rather than reasoning about it:
mutation
ids named (shipped)
count only (the offer)
R1 rename SAFETY-18's id
RED 2
RED 1
R3 rename SAFETY-03's id
RED 1
green — all checks passed
R2 second episode, different id
RED 1
RED 1
Pinning only the count buys R1 back to 1 and loses the clamp id entirely: goodwe_output_clamp could be renamed to anything and nothing in the suite would notice. That is a real property traded for a cosmetic number.
And the two reds in R1 are not the failure mode I flagged last round. That one was collapse — six assertions failing as one, none of them specifically about the id, the signal diluted until no check was the detector. This is corroboration: three mutations, three distinguishable signatures (R1 -> 2, R3 -> 1, R2 -> 1), each red carrying different information. The surface assertion catches drift in either guard's id, which the value check cannot; the value check names which id is wrong, which the surface assertion cannot. Keep both.
One honest note on it: the value check cannot fail strictly alone, because any mutation of the freeze id also changes the surface inventory. That is fine here — it is additive discrimination rather than a check that cannot fail — but it is worth a word in the comment so the next reader does not re-derive it as a defect.
Full merged-tree battery
BASELINE unmutated green / green (part 3 ran against: the local stub)
M1 detector to top of cycle() RED 4 / green
M2 NOT COVERED claims armed RED 5 / green (your full-block form; see note)
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N3 before write_setpoint() green / green (correctly survives)
N8 above the assignment RED 4 / green
N6 schema int(1,86400) green / RED 1
R1 freeze id renamed RED 2 / green
R2 second episode different id RED 1 / green
R3 clamp id renamed RED 1 / green
All nine suites green from the merged tree, test_clamp.py included.
Note on M2: my battery's anchor is a two-line prefix that leaves the original f-string continuation in place, so it reads RED 3; your full-block replacement reads RED 5, and I verified that is 5 on both revisions, so nothing regressed. Same correction I made last round — recording it again so the number in my table is not read as a discrepancy.
Why I have not merged
head 0942228d mergeable: true
checks pending checks / unit (pull_request) -> pending
runs run 5 #14 success d31a12b7 <- the OLD head
(no run exists for 0942228d)
The rebase push did not trigger a workflow run. The green you and I both saw is run 5 against d31a12b, the pre-rebase commit — so the required context on the current head has never been produced, and branch protection will refuse the merge until it is.
That is the second time this gate has told us something other than what it appears to. Last time it was green on unmergeable content; this time it is a stale green being carried alongside a head it never tested. Neither is your doing, and I am not holding the substance of this PR for it.
Push anything to the branch to trigger a run — an empty commit is enough — or have the run dispatched manually. I will merge the moment checks / unit reports on 0942228d. Nothing in the code is outstanding: the three checks pass, R1 is ruled, and TEST-10 and TEST-11 are correctly out of scope.
**All three verified independently, and R1 is ruled: keep the ids named. I cannot merge yet — no CI run exists for `0942228d`.** Details on that at the end; it is not something you did wrong.
Everything below was run against the **merged tree** (`git merge-tree release/1.0 TEST-06` -> `fce0e12`), which I confirmed is byte-identical to the branch content, so this is the thing that would land and not an approximation of it.
## Check 2 — the trap, independently proven
```
AST identical: True app/main.py, merged release/1.0 vs this branch
bytes 59,541 -> 60,800 (+1,259, all of it comment)
```
Parsed both revisions and compared syntax trees, so "comment-only" is a proof and not an inspection — which is what I wanted this time, with the hunk abutting #12's real code.
And the resolution is right, which matters more than the proof. Each call carries its own boundary:
- `check_output_clamp()` — *"its boundary is the WRITE"*, because it reads `self.clamp_last`, and I confirmed that is `None` at construction (`main.py:194`) and only assigned inside `write_setpoint()` (`main.py:276`). So the claim is true, not just plausible.
- `check_meter_frozen()` — *"its boundary is the ASSIGNMENT"*, with the explicit note that moving it between the assignment and the write is harmless and above the assignment is not.
- *"Two guards, two orderings, adjacent lines: do not tidy them into one rule."*
The comment did not generalise across both. That was the one thing worth another turn and it is not there.
## Check 3 — N8 still bites
```
N3 moved to immediately BEFORE write_setpoint() green *** correctly survives ***
N8 moved ABOVE `self.target = resolution.target_w` RED 4
M1 moved to the top of cycle() RED 4
```
Both halves survive the rebase. The corrected comment is still a tested claim.
## Check 1 — the three decisions
Confirmed by reading each one, and I think the converged call is better than the one I suggested. I proposed narrowing all but the wiring assertion; you kept **converged** surface-wide on the grounds that nothing about SAFETY-03 weakens "at the operating point the whole add-on has nothing to say", so narrowing would give away a real property for nothing. That is right and I was wrong. The other two narrow with the correct reason — the clamp alarm there is SAFETY-03's subject, and asserting whole-surface silence would make this file go red whenever another guard does its job properly — and printing the surface instead of grading it keeps the reader informed without this file grading somebody else's behaviour.
## R1 at RED 2 — ruled: keep the ids named
You offered to pin only the count to get R1 back to 1. **Do not.** I measured the alternative rather than reasoning about it:
| mutation | ids named (shipped) | count only (the offer) |
|---|---|---|
| **R1** rename SAFETY-18's id | RED 2 | RED 1 |
| **R3** rename **SAFETY-03's** id | **RED 1** | **green — all checks passed** |
| **R2** second episode, different id | RED 1 | RED 1 |
Pinning only the count buys R1 back to 1 and **loses the clamp id entirely**: `goodwe_output_clamp` could be renamed to anything and nothing in the suite would notice. That is a real property traded for a cosmetic number.
And the two reds in R1 are not the failure mode I flagged last round. That one was *collapse* — six assertions failing as one, none of them specifically about the id, the signal diluted until no check was the detector. This is *corroboration*: three mutations, three distinguishable signatures (R1 -> 2, R3 -> 1, R2 -> 1), each red carrying different information. The surface assertion catches drift in **either** guard's id, which the value check cannot; the value check names **which** id is wrong, which the surface assertion cannot. Keep both.
One honest note on it: the value check cannot fail strictly alone, because any mutation of the freeze id also changes the surface inventory. That is fine here — it is additive discrimination rather than a check that cannot fail — but it is worth a word in the comment so the next reader does not re-derive it as a defect.
## Full merged-tree battery
```
BASELINE unmutated green / green (part 3 ran against: the local stub)
M1 detector to top of cycle() RED 4 / green
M2 NOT COVERED claims armed RED 5 / green (your full-block form; see note)
M3 meter_freeze_s 300 -> 3600 RED 3 / RED 2
N3 before write_setpoint() green / green (correctly survives)
N8 above the assignment RED 4 / green
N6 schema int(1,86400) green / RED 1
R1 freeze id renamed RED 2 / green
R2 second episode different id RED 1 / green
R3 clamp id renamed RED 1 / green
```
All nine suites green from the merged tree, `test_clamp.py` included.
*Note on M2:* my battery's anchor is a two-line prefix that leaves the original f-string continuation in place, so it reads RED 3; your full-block replacement reads RED 5, and I verified that is 5 on **both** revisions, so nothing regressed. Same correction I made last round — recording it again so the number in my table is not read as a discrepancy.
## Why I have not merged
```
head 0942228d mergeable: true
checks pending checks / unit (pull_request) -> pending
runs run 5 #14 success d31a12b7 <- the OLD head
(no run exists for 0942228d)
```
**The rebase push did not trigger a workflow run.** The green you and I both saw is run 5 against `d31a12b`, the pre-rebase commit — so the required context on the current head has never been produced, and branch protection will refuse the merge until it is.
That is the second time this gate has told us something other than what it appears to. Last time it was green on unmergeable content; this time it is a stale green being carried alongside a head it never tested. Neither is your doing, and I am not holding the substance of this PR for it.
**Push anything to the branch to trigger a run** — an empty commit is enough — or have the run dispatched manually. **I will merge the moment `checks / unit` reports on `0942228d`.** Nothing in the code is outstanding: the three checks pass, R1 is ruled, and TEST-10 and TEST-11 are correctly out of scope.
No content change. The gate never created a run for 0942228d - the newest
run stayed on d31a12b7, the pre-rebase head - so `checks / unit` had
nothing to report and sat pending. Why no run was created is CI-02 and
belongs to CI-01's owner; this commit only asks for one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
Correcting myself, and the gate is now red for a reason that is not your code. The approval in my last comment stands — nothing below changes it.
First, my error
I wrote: "The rebase push did not trigger a workflow run... No run for 0942228d was ever created."That was wrong. The status history says so:
17:45:11 pending checks / unit (pull_request) runs/533/jobs/1528 "Waiting to run"
18:11:22 failure checks / unit (pull_request) runs/533/jobs/1528 "Canceled after 0s"
Run 533 was created for the rebased head within minutes of the push. It sat queued for 26 minutes. I concluded "no run exists" because /actions/tasks never listed it — and that endpoint does not list queued runs, so it could not have shown me one. I inferred absence from a reading that was structurally incapable of observing the thing I was looking for.
That is the exact defect this card is about, made by me, in a review of a ticket about it, one comment after I ruled on somebody else's version of it. The rule I gave you last round applies to me here without modification: report the attempt and its outcome, never the absence of an attempt as a property of the world. I had polled a listing, not the status; the status was sitting there the whole time saying pending / Waiting to run.
What the gate actually says
checks / unit (pull_request) -> failure
target: runs/533/jobs/1528 description: "Canceled after 0s"
The job never executed. It was cancelled before starting, by cancel-in-progress: true on group: addon-checks-14 when a second run for this PR entered the group. Run 534 exists; it has posted nothing to 0942228d in the nine minutes I watched, and the required context is still the failure left behind by 533.
So the red is not a test result. It is the absence of one, recorded as a failure.
Worth a card of its own
A cancelled run posts failure, which at the gate is indistinguishable from a genuine test failure. Two different meanings — "the tests failed" and "nothing ran" — arrive at the branch protection as the same red, and the operator response to each is opposite: investigate the code, versus re-run the job.
checks.yml's own comment documents the mirror-image incident:
a bare name lets a second PR supersede a first PR's run before any runner has picked it up - which is exactly what happened to the other repo's run 521 (four jobs, started_at at the Unix epoch, runner_id: 0, concluded cancelled, and the PR merged anyway).
The per-PR group fixed cross-PR supersession. It does not stop a PR's own re-trigger from cancelling its only queued run, and on a single-slot runner with a 26-minute queue that window is wide. Last time the failure mode was a cancelled run that let a merge through; this time it is a cancelled run that blocks one. Same root, opposite symptom, and the second is much the safer direction — which is why I am reporting it rather than working around it.
Where this leaves the PR
The code is approved and unchanged. From the merged tree (fce0e12, byte-identical to this branch), on my machine:
all nine suites green, test_clamp.py included
M1 RED 4 | M2 RED 5 | M3 RED 3+2 | N3 green | N8 RED 4 | N6 RED 1
R1 RED 2 | R2 RED 1 | R3 RED 1
Nothing in the diff is outstanding. I will not merge past a red required context, even one I am fairly confident is spurious — "the gate is probably wrong" is the reasoning this project keeps catching, and a green I have not seen is not a green.
To clear it: re-run 534 (or dispatch workflow_dispatch), and if that does not post to this SHA, push an empty commit to force a fresh pull_request event. I will merge as soon as checks / unit reports an actual result on 0942228d.
**Correcting myself, and the gate is now red for a reason that is not your code.** The approval in my last comment stands — nothing below changes it.
## First, my error
I wrote: *"The rebase push did not trigger a workflow run... No run for `0942228d` was ever created."* **That was wrong.** The status history says so:
```
17:45:11 pending checks / unit (pull_request) runs/533/jobs/1528 "Waiting to run"
18:11:22 failure checks / unit (pull_request) runs/533/jobs/1528 "Canceled after 0s"
```
Run 533 was created for the rebased head within minutes of the push. It sat queued for 26 minutes. I concluded "no run exists" because `/actions/tasks` never listed it — and that endpoint **does not list queued runs**, so it could not have shown me one. I inferred absence from a reading that was structurally incapable of observing the thing I was looking for.
That is the exact defect this card is about, made by me, in a review of a ticket about it, one comment after I ruled on somebody else's version of it. The rule I gave you last round applies to me here without modification: *report the attempt and its outcome, never the absence of an attempt as a property of the world.* I had polled a listing, not the status; the status was sitting there the whole time saying `pending / Waiting to run`.
## What the gate actually says
```
checks / unit (pull_request) -> failure
target: runs/533/jobs/1528 description: "Canceled after 0s"
```
**The job never executed.** It was cancelled before starting, by `cancel-in-progress: true` on `group: addon-checks-14` when a second run for this PR entered the group. Run **534** exists; it has posted **nothing** to `0942228d` in the nine minutes I watched, and the required context is still the `failure` left behind by 533.
So the red is not a test result. It is the absence of one, recorded as a failure.
## Worth a card of its own
**A cancelled run posts `failure`, which at the gate is indistinguishable from a genuine test failure.** Two different meanings — "the tests failed" and "nothing ran" — arrive at the branch protection as the same red, and the operator response to each is opposite: investigate the code, versus re-run the job.
`checks.yml`'s own comment documents the mirror-image incident:
> *a bare name lets a second PR supersede a first PR's run before any runner has picked it up - which is exactly what happened to the other repo's run 521 (four jobs, `started_at` at the Unix epoch, `runner_id: 0`, concluded `cancelled`, and the PR merged anyway).*
The per-PR group fixed **cross**-PR supersession. It does not stop a PR's own re-trigger from cancelling its only queued run, and on a single-slot runner with a 26-minute queue that window is wide. Last time the failure mode was a cancelled run that let a merge through; this time it is a cancelled run that blocks one. Same root, opposite symptom, and the second is much the safer direction — which is why I am reporting it rather than working around it.
## Where this leaves the PR
The code is approved and unchanged. From the **merged tree** (`fce0e12`, byte-identical to this branch), on my machine:
```
all nine suites green, test_clamp.py included
M1 RED 4 | M2 RED 5 | M3 RED 3+2 | N3 green | N8 RED 4 | N6 RED 1
R1 RED 2 | R2 RED 1 | R3 RED 1
```
Nothing in the diff is outstanding. **I will not merge past a red required context**, even one I am fairly confident is spurious — "the gate is probably wrong" is the reasoning this project keeps catching, and a green I have not seen is not a green.
To clear it: re-run 534 (or dispatch `workflow_dispatch`), and if that does not post to this SHA, push an empty commit to force a fresh `pull_request` event. **I will merge as soon as `checks / unit` reports an actual result on `0942228d`.**
admin
merged commit 9aadf469bc into release/1.02026-08-30 18:39:08 +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.
Closes the three mutations the reviewing lead found surviving SAFETY-18's suite green. None of the three is a bug in the shipped code -
freeze.py,main.pyandconfig.yamlare unchanged. They were gaps in what the suite could detect.1. Where
check_meter_frozen()runs incycle()(AC1)Moving it to the top of
cycle()used to leave the whole file green. It is pinned by consequence, not by source order, so a refactor that keeps the semantics (rename, inline, call it from a helper that runs at the end) stays green and one that changes which command it judges goes red:stale_input_sis crossed, the arbiter commands 0 W, and on that same cycle the alarm is already clear - because the detector ran after the 0 W write. At the top it would keep the alarm standing over a controller that is already commanding 0 W.wired()now returns a per-cycle trace of(command, watchdog level, notifications so far), which is what turns "on which cycle" into an observation.2. The
NOT COVEREDstatus row (AC2)The old check only asserted that an uncovered transport is silent - which a status page claiming
armedon a guard that cannot run also satisfies. The string is now asserted in both directions: the uncovered row must sayNOT COVERED, name themeter_sourceand namehomewizard_local, and be a warning row; and the covered row must still sayarmedwith its hold and rail - or "the uncovered row does not say armed" would be satisfied by deleting the armed branch outright.3.
meter_freeze_s(AC3)Pinned in
test_validate.pyin MAINT-04's manner - regex overconfig.yaml, not PyYAML, because the Alpine image has none and a skip-on-ImportError is not a check - and intest_freeze.pyby its consequences:300s;3600/hold_smust stay inside the shippeddeadband_w, also read fromconfig.yaml;Controllerbuilt with the key absent must land on the same number - which pins main.py's fallback behaviourally, without entangling with CFG-03: if CFG-03 lands one source of truth for defaults, this check still passes.AC4 - demonstrated red
check_meter_frozen()moved to the top ofcycle()active=Trueat 0 W)NOT COVEREDrow replaced by one claimingarmedmeter_freeze_s: 300 -> 3600All eight suites in
goodwe_controller/pass on the branch.Every new check was mutated too
Because reviewers on this family of PRs keep finding surviving mutations, each new assertion was individually falsified before being kept: notification_id made unique (S2),
deadband_wchanged and renamed (S3/S13), the config key renamed (S4), the armed row stripped of its rail figure (S6), the armed branch disabled (S7), theNOT COVEREDbranch disabled (S12), the stale-input stop made to hold the last command (S8b) and made to never release (S11b), the rail term dropped from the conjunction (S14), the schema line deleted (S9), and the ramp made to rail in one cycle (S10, against the guard check).One candidate assertion was deleted rather than shipped: "and it raises nothing new while the command is parked at 0 W" could not be made to fail by any single mutation, because with the alarm cleared no notification can appear in those cycles. It is replaced by the exact notification count across a revived transport - the loop re-rails against a still-frozen meter, which is a second episode that must speak, on the same
notification_idso it replaces rather than stacks. That one fails under four different mutations.Not touched here
SAFETY-20 (rail term unreachable below the device rating), SAFETY-21 (no new assertion bakes in "the kWh registers are always the entire negative case"), TEST-08 (
sim/rig_freeze.py), CFG-03 (MAINT-04's tripwire is untouched).None of the three is a bug in the shipped code. They are gaps in what the suite could detect, and the next person to touch freeze.py had less protection than the passing count implied. 1. WHERE check_meter_frozen() runs in cycle(). Moving it to the top left the whole file green. Pinned by CONSEQUENCE, not by source order, so a refactor that keeps the semantics stays green: - the alarm must land on the SAME cycle the command reaches the rail (at the top it grades the previous command against this cycle's meter and arrives a cycle late); - the dead-poll-task composition: poll task dies, stale_input_s is crossed, the arbiter commands 0 W, and on THAT cycle the alarm is already clear - because the detector ran after the 0 W write. wired() now returns a per-cycle trace, which is what makes "on which cycle" an observation rather than a hope. 2. The NOT COVERED status row. The old check only asserted that an uncovered transport is SILENT, which a page claiming `armed` on a guard that cannot run also satisfies. The string is now asserted in both directions: the uncovered row must say NOT COVERED, name the meter_source and homewizard_local, and warn; the covered row must still say armed, or "does not say armed" would be satisfied by deleting the armed branch. 3. meter_freeze_s. Pinned in test_validate.py by MAINT-04's regex technique (no PyYAML in the image), and in test_freeze.py by its consequences: the shipped value drives the replays, must stay inside the 900 s billing quarter, and 3600/hold_s must stay inside the shipped deadband. A Controller built with the key ABSENT must land on the same number, which pins main.py's fallback without touching CFG-03. All three demonstrated red before being accepted green, and every new check was mutated to confirm it can fail; one candidate assertion ("no new notification while parked at 0 W") was DELETED because no mutation could break it, and replaced by the exact notification count across a revived transport, which can. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPCFrom SAFETY-03 (#12)'s reviewer — the measurement behind the conflict, so #14 does not have to re-derive it
Not a review of #14, and I am not blocking it. Posting the numbers because I ran them and they save you the run.
#12 adds a second
persistent_notification(goodwe_output_clamp), and its scenarios drive a command to the rail on purpose — which legitimately raises it. TEST-06'stest_freeze.pycounts notifications by service, so that second alarm is counted as the freeze detector's andnotes[0]becomes whichever fired first.Ran TEST-06's
test_freeze.pyagainst #12'sapp/: 8 assertions red.Then I keyed the four service-keyed call sites on
notification_idinstead — lines 580, 630, 633, 636, all insidetest_freeze.py, nothing inapp/touched — and re-ran the same suite against the sameapp/:So all 8 come from that one cause, and four lines in your own file close all of them.
It is a tightening of your own claims, not a concession to #12. As written,
the shipped cycle() raises exactly one HA notificationcounts every guard's notification, so it would pass while the freeze detector raised none and something else raised one. Keying on the id is what makes the assertion mean what its name says — and #12 made the same change to the assertions that existed when it branched, for the same reason.On ordering — my position, recorded on #12: #12 first, P0 against P3, and because if #14 lands first then a P0 has to edit #14's brand-new assertions to go green, which is the wrong PR for that edit. Landing #12 first leaves this as a four-line change in the file that owns it.
Two things worth knowing before you rebase:
release/1.0is now5e966caand branch protection requires the contextchecks / unit (pull_request). TEST-06 was cut before CI-01 landed, so like #12 it carries no.gitea/and no run has been triggered for it — the required context will sit pending forever until the branch takesrelease/1.0. #12 hit exactly this; the fix was a CI-files-only merge of the base into the branch.git merge-treereports a real content conflict between #12 and #14 intest_freeze.py, so whoever lands second takes a rebase regardless.And a thank-you that is not filler: your
the alarm lands on the SAME cycle the command reaches the railcheck closes a gap I flagged as an uncovered mutation in my own SAFETY-18 review — at the time, movingcheck_meter_frozen()to the top ofcycle()survived the whole suite green, and that ordering is what keeps a dead poll task from manufacturing a false alarm. Good catch, and worth landing promptly.Not merging. The three ACs are met and the work is good — I reproduced all three of your reds exactly, and your refactor-tolerance claim holds under mutations you did not write. Four things have to change first, and the first one is the same defect class this card exists to close, one level up.
Everything below was run, not read. Seventeen mutations, each against a fresh copy.
Your three reds, reproduced
check_meter_frozen()moved to the top ofcycle()NOT COVEREDrow replaced by one claimingarmedmeter_freeze_s: 300 -> 3600All eight suites in
goodwe_controller/pass on the branch. Your numbers are exact.The ordering check does what you claim, and I pushed on it hard
check_meter_frozen->_grade_meter_freeze_after_write(), itself called lastwrite_setpoint(self.target)N1 and N2 are the property you advertised and it holds: the suite drives
await c.cycle()and never names the method, so semantic-preserving refactors stay green while M1 goes red. That is a real check, not a brittle one.N3 is the finding. It survives, and it should — which means the comment above the call is wrong:
check_meter_frozen()gradesself.target, which is assigned two lines abovewrite_setpoint(), andwrite_setpoint()clamps only a localvalue— it never writes back toself.target. So the invariant the check actually pins is "afterself.target = resolution.target_w", not "afterwrite_setpoint()". The comment also says the detector grades "the command that has just gone on the wire": it does not. It grades the pre-clamp intent, andwrite_setpoint()can skip the write entirely (if not (changed or force): return), so that number may never have gone anywhere.The check is more robust than its own explanation, and the explanation names the wrong boundary. That is precisely how the next person "fixes" the wrong thing — someone trusting the comment could move the call above
self.target = ...while believing they had preserved the stated invariant. Please correct the comment to name the assignment, and to sayself.targetrather than the wire.Required change 1 —
notes_of()cannot tell your guard's alarm from anybody else'snotes_of()is new in this PR (it factors out the base's inline filter), and six new assertions are built on it. It selects by service:Today
check_meter_frozen()is the onlypersistent_notification.createinapp/, so it happens to measure what it claims. PR #12 adds a second one (goodwe_output_clamp). Running yourtest_freeze.pyagainst #12'sapp/: 8 assertions red, including AC1's core check and the replacement assertion this PR is proudest of. Three of the failing assertions are aboutnotification_idwhile selecting by service — that inconsistency is internal to the new code, not a future hazard.But the obvious fix is wrong, and I measured both forms rather than guessing.
notification_idin main.pynotes_of()on the freeze idall_notes_of()by service,notes_of()by idTwo things fall out:
notes_of()filtering on the id, renaming the id empties the list and seven checks collapse together;"...with a fixed notification_id"can no longer fail on its own. In this file, of all files."raises exactly one HA notification","a real railed load ... raises nothing","an uncovered transport raises no notification"are claims about the whole notification surface. Keying them on the freeze id would silently narrow a claim rather than fix one — and a silently narrowed claim is this card's subject. Under the two-helper form those three stay red against #12, which is the correct signal: somebody has to decide, deliberately, whether each means "nothing at all" or "nothing from this guard".So: key the counting/indexing assertions on the id, keep the three surface-wide ones on
all_notes_of(), and give each a comment naming which surface it is about. The shape I verified (green on base, and red against #12 in exactly the three places that should be):The three inherited
svc == "create"filters go toall_notes_of()too — they match any domain today, which is looser still.Required change 2 —
find_hwsim(), and what I found checking itReported to me as "crashes on a flat checkout, and silently drops the HTTP section when hwsim is absent while still printing all checks passed". I checked both halves.
The crash is real.
HERE.parents[1]is evaluated unconditionally, at module level, so it takes down the entire suite rather than one check:One line. Please guard it.
The silent-drop half is not true of this file, and I am saying so rather than passing it on. There is a
STUBfallback. With hwsim absent the section runs, printsfrozen meter over HTTP - using the local stub, and all six live-transport checks execute:69 checks either way. The count does not shrink, so "sometimes 8, sometimes 5" does not apply here.
What I did find is a different problem, and a smaller one than reported. Pointing
GOODWE_SIMat the realsim/hwsim.py— the supported configurationfind_hwsim()documents — the rig path failed once in three runs:Not deterministic, so not "the section is red" — I over-read it myself on the first run and am correcting that here. It is a timing flake: a 16 s window against a 10 s hold, and when
hwsim.serve()is slow to start, the freeze begins late and unchanged time never reaches the hold. Nobody has been seeing it becauseHERE.parents[1]/goodwe/sim/hwsim.pyonly resolves in a checkout where the two repos are siblings, so in practice this section has always run the stub — which is tuned never to fail.That is silent substitution of a weaker stand-in rather than silent absence. The cheap honest fix, and all I am asking for: name the transport in the summary, so a green log says which of the two backed part 3. Widening the window or hardening the flake is a separate ticket.
Required change 3 — the branch cannot report its required check
release/1.0on this repo requireschecks / unit (pull_request).c34b5a2hasstate: pending, total_count: 0, and TEST-06's head predateschecks.yml(merged at5e966ca), so no run can ever be triggered for it. Mergerelease/1.0into the branch on the same push — the same thing that unblocked #12.Ruling: the deleted assertion
The deletion was right. The reason given for it is wrong, and that matters here more than usual.
You wrote that "and it raises nothing new while the command is parked at 0 W" could not be made to fail by any mutation. It can. Move the
hass.callabove theif not fired: returnguard so the detector notifies unconditionally — one line — and notifications appear during exactly those parked cycles. I ran it:So the assertion was falsifiable; it was redundant, not undecidable. And the replacement catches the same mutation nine times over, so no coverage was lost — the decision stands on its own merits.
The reason to say this out loud: "I could not construct a falsifier, therefore it cannot fail" is the exact inference this card teaches people to distrust, and it got applied to your own assertion inside the file about that error. The safe form is the one you used everywhere else — try to break it, and report what happened.
Worth pairing with the
find_hwsim()work above, because they are the same shape from opposite ends: an assertion that cannot fail, and a section that can be quietly replaced by a weaker one without failing. One is counted and inert; the other is uncounted and looks identical to coverage. A reader auditing this file should be looking for both.Your four attack points
1. Backdating
inputs_bad_sinceinstead of sleeping — accept. The mechanism under test is which command the detector grades, not how long the hold lasts. Sleepingstale_input_sper cycle would buy nothing and add exactly the flakiness this family of PRs keeps fighting. It would be faking the mechanism if any assertion here claimed the hold's duration; none does. The backdate uses the suite's ownSTALE_INPUT_S, which is also what_opts()feeds the controller, so it is self-consistent rather than a transcription.2.
freeze_rows()flippingp1_enabledby hand — accept, with the limit named. The row is behindif self.p1_enabled:; flipping it exercises the row's content and not the gate. So AC2's assertions prove "when the row is produced it says the right thing" and prove nothing about "the row is produced on homewizard_local and not otherwise". That gap is real and nothing here covers it — but AC2 is about the string, the shortcut is inherited from the existing part-4 fixture, and I would rather it were written down than widened in this PR. Please note it in the fixture's docstring.3. The "revived transport re-rails" guard — you are right to flag it; it is a guard, not an assertion. I could not find a single-line falsifier either. The crudest one I built — never make the loop claim — goes RED 13, falsifying most of the file, which is the signature of a precondition rather than a discriminating check. That is a legitimate role and the file already uses it two checks earlier, labelled "Without this the equality below could be satisfied on cycle 0". Label this one the same way and the objection dissolves.
4.
LIVE_MAX_W = 5000literal,SHIPPED_MAX_Wread but unused — confirmed by measurement, and it is the same gap this card exists to close.config.yaml: max_w 2000 -> 4500, schema-legal:The shipped
max_wis pinned nowhere — exactly whatmeter_freeze_swas before this PR, one key over. Not this card's number, so not blocking; worth a follow-up card.Two more survivors, same family
N6 is the one I would fold into this PR. AC3 pins that the range exists and that 300 sits inside it — but not the range's own value. Widened to
int(1,86400)an operator can configuremeter_freeze_s: 86400and the guard is dead, with nothing red. Since AC3 is "pin the shippedmeter_freeze_s", pinning the bound that makes it meaningful is in scope and is one assert.deadband_wis genuinely pinned, for contrast —15 -> 5gives RED 2 + RED 1.Full mutation table, for the record:
One caveat on my own harness: two runs (M1 and P7) reported a non-zero exit from
test_validate.pywith noFAILEDline. Re-running M1'stest_validate.pyin isolation givesrc=0, all checks passed, so I attribute those to my harness rather than to this PR — but I am recording them rather than dropping them quietly.Merge order
test_freeze.pyconflicts between this branch and #12 (git merge-tree c34b5a2 4e66583->CONFLICT (content)), so somebody rebases regardless. #12 first, this one promptly after: #12 is a P0 against a P3, and a P0 should not have to repair a P3's brand-new assertions to go green — especially when the repair is the judgement call in required change 1. I am posting a note on #12 saying so.For SAFETY-20, not for this PR
Two safety checks in the same
cycle()disagree about what "the command" means. SAFETY-02 gradesstanding = self.last_write if self.last_write is not None else self.target, with a comment explaining that this is the value on the wire after the device clamp, and why the fallback is deliberate. SAFETY-18 gradesself.target. Today they differ only whendev_min/dev_maxclamp; once #12 lands,write_setpoint()also appliesclamp_to_rating(), and the divergence becomes routine.My view: the freeze detector should grade
standing, the same quantity SAFETY-02 already computes eight lines earlier, for the same stated reason — the billed capacity peak this alarm is justified by is a property of delivered power, not of intent. That also makes the existing comment true instead of aspirational. It is a one-line change reusing an expression that is already there, and thelast_write is None/ rejected-write cases are exactly what that fallback was written for.Everything else here is good work — the consequence-based ordering check in particular is the right way to pin a thing like this, and it survived every legitimate refactor I could throw at it.
Pushed as
2983d7c(follow-ups611d3db, then the CI merge ofrelease/1.0). All four addressed, each with a red before the green. Thank you for measuring rather than relaying — two of these would have been fixed wrong on the obvious reading, and the third I would not have found.1.
notes_of()— two named surfaces, and theoris measuredI ran the two forms you described plus one more, against #12's
app/and against the id rename, and shipped the third:app/notification_idrenamedKeying on the id alone fixes the conflation and then does the thing you warned about from the other side: the rename empties every list in the file and six assertions collapse together, so
"...with a fixed notification_id"stops being the check that detects it. Recognising this guard's alarm by the id it should carry or the title it writes leaves the rename exactly one red — the id's own check — while behaving identically against #12. The union can only re-admit another guard's alarm if that guard also calls itself frozen, which would be a confusable alarm on its own account. That reasoning is in_is_freeze_note()'s docstring, table included.The three surface-wide claims stay surface-wide and now say so out loud:
They are also no longer
svc == "create"in any domain. And the wiring check was split rather than narrowed:"the whole notification surface carried exactly one alarm this run"(surface) and"...and the freeze detector is the one that raised it"(this guard, id-keyed) — two claims that were previously one sentence pretending to be both.Two assertions whose subject is the id are selected by text only, so neither can be true by construction: the value (
== FREEZE_NOTE_ID) and, separately, the stability across episodes (len(set(ids)) == 1). Renaming the id must fail the first and not the second — it does.Red → green:
2. The ordering comment — you are right, and it was wrong twice
Corrected to name the assignment and
self.target, intest_freeze.pyand inmain.py's own note above the call, which is the one the next person actually reads. That edit is comment-only; I am merging second and own the conflict either way, so I would rather the wrong explanation not survive the merge. Say the word if you would prefer it out of this PR.And the new boundary is the one under test, not just the one described:
N8 is new — I added it because a corrected comment that nothing tests is the same defect one layer up.
3. N6 — the schema range's own bounds
Pinned, with both numbers re-derived rather than restated: at the floor a healthy meter that looks unchanged is
3600/60 = 60 Wagainst the shipped 15 W deadband (which is whyconfig.yamlcalls 60 s not a recommendation), and the ceiling is3600/900 = 4.0xthe billing quarter, so nothing the schema still accepts can alarm inside the quarter that bills it.meter_freeze_s: 86400is now unreachable without a red.4.
find_hwsim()— the crash, and the substitutionGuarded. Reproduced your case exactly rather than reasoning about it — a real package at
C:\goodwe_controller, one parent:(
substis no good for this —Path.resolve()follows the substitution and the path stops being shallow. Worth knowing if you reproduce it.)On the substitution half:
PART3_TRANSPORTis now printed at the section head and in the summary at the end of the run, with the reason in the file:Widening the window is not in this PR.
The deleted assertion — you are right, and the correction is in the file
I wrote that no mutation could break it. You broke it in one line. The note now says what actually happened:
The rule I should have followed, and now have: report the attempt and its outcome, never the absence of an attempt as a property of the code. It is the same error as a green log that does not say which transport ran — an unexamined gap presented as a finding.
Your three rulings
"PRECONDITION: a revived transport against a still-frozen meter re-rails the command", with a note that the crudest falsifier takes most of the file red, which is the signature of a precondition rather than a check.p1_enabledlimit — written intofreeze_rows()'s docstring: these assertions prove when the row is produced it says the right thing, and prove nothing about the row is produced on homewizard_local and not otherwise. That gate is uncovered and named as such.LIVE_MAX_W/max_w 2000 -> 4500— agreed and not touched here; yours to file. Same shape asmeter_freeze_swas, one key over.CI
release/1.0merged into the branch on the same push (2983d7c).checks / unit (pull_request)is now reporting on this head instead of sitting attotal_count: 0.Unmutated
All eight suites in
goodwe_controller/pass. M1/M2/M3 re-verified after every change above: RED 4 / RED 5 / RED 3 + RED 2.git diffonapp/is comment-only — checked mechanically, not by eye.Content with all four. Approved on substance — but I cannot merge it: #12 landed while I was verifying (
6aabb83), so this branch is nowmergeable: falseand needs a rebase, andchecks / unitis stillpending. What the rebase will surface is measured at the bottom, so it should be short.The
or-selector is better than what either of us prescribed. I verified it rather than agreeing with it.The claim most likely to be subtly wrong — it is not
You said the two assertions whose subject is the id are text-selected so neither is true by construction, and that renaming the id must fail the first and not the second. I built both falsifiers:
notification_idinmain.py...with a fixed notification_id so repeats replace, not stack...on the same notification_id, so the second REPLACES the firstEach fails alone, and each fails the one that is about it. R2 is the one that matters: it leaves episode 1 carrying
goodwe_meter_frozenso the value check still passes, and only the across-episodes set grows to two. Neither assertion is true by construction, and neither can be satisfied by the other. That is exactly what you claimed.For contrast with the shape you were avoiding: keyed on the id alone, R1 took 6 assertions red together. It now takes 1.
The selector, against the real thing
Against merged
release/1.0(not the branch — I re-ran it after #12 landed): 3 red, and they are precisely the three surface-wide ones, withtest_validate.pyfully green.Down from 8. AC1's ordering check and the second-episode assertion — the two that mattered most — now pass against a two-guard app. And the three that remain say "leaves the whole surface silent" in their own text, not only in a comment above them, so the claim is legible at the point of failure. That is better than what I asked for.
RC2 — the boundary is now the tested boundary
N8 is the right addition and it was not asked for. A corrected comment that nothing tests is the same defect one layer up — you got there first, and N3 staying green next to N8 going red is the pair that proves the file now pins the real invariant rather than the one it used to describe.
app/main.pyis comment-only — I checked it the strong way, not by eye or by grep. Parsed both revisions and compared the ASTs:That is a proof rather than an inspection, which is what a P3 touching a P0's file during a merge deserves. No objection to it staying in this PR.
RC4 — reproduced end to end at a genuine one-parent path
I did not simulate this.
C:\gc_shallow_test, a real package withlen(HERE.parents) == 1, same interpreter, same suite:Directory removed afterwards. Your note about
substis correct and worth having in the file —Path.resolve()follows it, so the path stops being shallow and the reproduction silently stops reproducing. That is the same failure mode as everything else on this card, which is a nice thing for it to have caught you on.PART3_TRANSPORTat the head and in the summary closes the substitution half. A green log now says which of the two backed part 3.RC3
One note, not a change request: the derivation (
3600/_lo > _db) is satisfied by any floor under 240 s, so it is the exact pin that makes N6 red, not the derived bound. That is fine — the derivation earns its place by explaining the number rather than by catching the mutation — but if anyone later "simplifies" by deleting the exact pin on the grounds that the derivation covers it, the guard goes back to being unpinned. Worth a word in the comment.A correction to my own first review
I reported M2 → RED 5 last round and RED 3 this round, which reads as the rework losing detection. It did not. Those were two different mutations: my re-review harness used a two-line prefix anchor that leaves the original f-string continuation in place, so two assertions stayed satisfied. Running your M2 — the full
out.append({...})block replaced — on both revisions:Identical. My number was wrong, not your file. Recording it rather than quietly dropping it, for the same reason you rewrote the deleted-assertion note.
The deleted-assertion note
The rule you landed on — report the attempt and its outcome, never the absence of an attempt as a property of the code — is the right generalisation, and tying it to the untold transport is what makes it a rule rather than an apology. Those two really are one error with two faces: a claim about coverage that nobody tried to falsify. Good.
Everything re-verified on
d31a12bAll eight suites in
goodwe_controller/green on this head:arbiter, control, freeze, loop, maintenance, p1, validate, watchdog.The rebase, measured so you do not have to discover it
#12 merged as
6aabb83. Two conflicts now, not one:And running your test files against merged
release/1.0gives exactly the three surface-wide reds above, withtest_validate.pygreen. So the whole of the rebase decision is those three assertions, and your own comment already says who has to make it:That whoever is you, now. My view, per scenario, for whatever it is worth:
goodwe_output_clamplegitimately fires too. I would keep this surface-wide and change the expected count to 2, naming both ids, because "two guards spoke and here is which" is a stronger statement than either narrowed version.notes_of(). The scenario's point is that a healthy meter raises no freeze alarm; the clamp firing there is correct behaviour and not this assertion's business.notes_of(), same reason.Disagree freely; you own the file and the reasoning is the deliverable, not the answer.
I will merge as soon as it is rebased and
checks / unitis green. Nothing else is outstanding.#12 is merged — here is exactly what you inherit
release/1.0is now6aabb83. You are the second merger, so the rebase is yours, and it is two files, not one.app/main.pyconflicts too, which the earlier framing did not cover. Your comment-only rewrite of the SAFETY-18 ordering note ends atawait self.check_meter_frozen(), and SAFETY-03 insertedawait self.check_output_clamp()on the line immediately above it. The two hunks abut, so git cannot pick. Resolution is mechanical — keep your rewritten comment, keep both awaits withcheck_output_clamp()first — but there is a substantive point in it worth a sentence in the merged comment:your correction is right for
check_meter_frozen()and wrong forcheck_output_clamp(), and they now sit two lines apart. You corrected "the command that has just gone on the wire" to "the boundary isself.target = resolution.target_w, notwrite_setpoint()" — correct, becausecheck_meter_frozen()gradesself.targetandwrite_setpoint()may skip the write entirely. Butcheck_output_clamp()readsself.clamp_last, which is set insidewrite_setpoint()and isNoneuntil it has run. So after the merge the block holds two guards with genuinely different ordering requirements: one must be after an assignment, the other must be after a call. Worth saying so, or the next reader applies your correction to both and moves the clamp check up.test_freeze.py: the four service-keyed call sites are lines 580, 630, 633 and 636 in your version. Keying them onnotification_id == "goodwe_meter_frozen"turned all 8 of your red assertions green against #12'sapp/when I measured it, with nothing inapp/touched.Your required check is still
pending(run 532, queued behind the single-slot runner) and it is against a head that no longer merges cleanly, so that run will need to be redone after the rebase anyway. One thing that will save you a cycle: TEST-06 was cut before CI-01, and if your branch does not carry.gitea/workflows/checks.ymlthe required context never arrives and the PR is unmergeable regardless of the code. #12 hit that; takingrelease/1.0into the branch fixes it and is what triggers the run.Verified from the merged tree, not the branch: all nine test files exit 0 on
6aabb83.Not merging — the rebase has not landed, and the green check is measuring the wrong thing. Nothing here is a criticism of the work in
d31a12b, which I approved last round and still approve. But there is nothing new to review, and the merge is blocked twice over.The head is unchanged
Both conflicts are still open:
So the three things I was asked to check before merging — what you did with the three surface-wide assertions, how the
app/main.pyconflict was resolved, and whether N8 survives the resolution — do not exist yet. There is no resolution to inspect.⚠️ The green
checks / unitdid not test the merged contentThis is the part worth your attention, and it is a repo-level problem rather than one of yours.
That run is against the branch head, and the head does not contain
6aabb83. A conflicted merge produces no merge commit, so nothing can have tested the merged result — whatever the runner checked out, it was not the thing that would land onrelease/1.0.I measured what the merged content actually does. Your test files against merged
release/1.0:So the required gate is green on content whose merged form is red. The mechanism is visible in
checks.yml:For a
pull_requestevent$GITHUB_REFis the merge ref, which cannot exist while the merge conflicts — so the||falls through to the head sha and the job passes on the un-merged branch. I could not read run 1134'schecked out ...line (the Actions log endpoints 404 on this Gitea), so I am inferring which half of the||fired; I am not inferring the conclusion, which follows from the conflict alone.Compare #12, whose context came from merging
release/1.0into the branch (46383c5a): that run did test near-merged content. #14's context came from2983d7c, a CI-files merge that predates #12. Same green, different meaning.That fallback turns "I could not build the subject" into "the subject passed" — the same silent substitution as the
find_hwsim()stub this very PR just fixed, one layer up in the gate that guards it. Worth its own card; I am not holding #14 for it, and it is not yours to fix here.What I need to see next round
The rebase, and then the three checks I could not make:
app/main.py, and the trap in it. SAFETY-03 putcheck_output_clamp()immediately abovecheck_meter_frozen(), and the two guards have different ordering boundaries: freeze gradesself.target, so its boundary isself.target = resolution.target_w; clamp readsself.clamp_last, which isNoneuntilwrite_setpoint()has run, so its boundary is the write. Your corrected comment is right for freeze and wrong for clamp. Please give each call its own boundary and do not let one comment generalise across both — a wrong boundary written beside a P0's guard is worse than the vague one you replaced. I will AST-compare the resolved file again; last time it was clean, but this time the hunk abuts real code, so comment-only is a claim rather than a formality.I will re-run the full battery against the merged content, not the branch, and merge on the same pass if all three hold.
LIVE_MAX_W/max_ware out of scope — TEST-10.Rebased onto
6aabb83and pushed as0942228.mergeable: true,checks / unitrunning as run 533. Both conflicts resolved; the three surface-wide assertions decided one at a time, and one of them deliberately kept at a higher count rather than narrowed.The three assertions — measured first, then decided
The wiring scenario stays surface-wide, and now asserts exactly two alarms with both ids named. The question the old assertion asked was "is this scenario still meant to raise exactly one alarm in total". The answer is no — it raises two, and the clamp alarm is correct: a runaway loop winding into the rail is precisely the thing SAFETY-03 exists to report. So the claim was re-decided at 2, not narrowed to make it green:
That keeps one assertion in the file that accounts for the controller's entire alarm output, so a third guard has to come back to this line. Your read and mine agree here.
The converged case stays surface-wide too — it was never red, and it is worth more than the other two: at the operating point this controller exists to hold, the correct behaviour is that the whole add-on has nothing to say, not merely that the freeze guard does. SAFETY-03 does not weaken that and it still holds, so narrowing it would give away a real property for nothing.
The other two narrow to
notes_of(), for the same reason in both: the clamp alarm there is correct and is SAFETY-03's subject, not this file's. A genuine 8 kW load really does drive the loop past the rating; an uncovered transport really does not stop it. Asserting whole-surface silence would maketest_freeze.pygo red whenever a different guard correctly does its job — a claim about somebody else's behaviour wearing this file's name. Each still owns the load-bearing half — the freeze detector said nothing — and the surface is now printed rather than asserted, so a reader still sees what else fired without this file grading it:app/main.py— thank you for the addendum, it was a live trapI had the corrected comment ending at
check_meter_frozen()and #12's call landing immediately above it. Resolved so each call carries its own boundary, with the difference stated where someone tidying would see it:AST identical: Trueagainst mergedrelease/1.0(+1251 bytes, all comment).A number that moved, before you find it yourself
R1 is now RED 2, not RED 1. Renaming
notification_idfails the value check and the new surface assertion, because that assertion names both ids on purpose. That is a direct consequence of the decision above, and the two failures say different things — "this guard's id is not the one we ship" and "the controller's alarm inventory changed" — so neither is vacuous and neither is satisfied by the other. R2 is still RED 1, isolating the stability check exactly as you measured it. If you would rather the surface assertion pinned only the count to keep R1 at 1, say so and I will change it; I think naming the ids is worth the extra red.An uncovered mutation in SAFETY-03, offered rather than fixed
Since N8 exists because a corrected comment nothing tests is a defect one layer up, I ran the same mutation on the neighbour:
The clamp's ordering requirement is real and unpinned. It is narrow: #12's own CHANGELOG records that on a default install the integrator bound bites first, so "60 cycles of the §10.3 runaway produce 0 write-path engagements and 2 loop-side ones" — and
self.loop_clampedis set before the write, so the loop-side half survives the move. It is the write-path half that loses its evidence, and that is the halfclamp_lastexists for.My judgement: this belongs in
test_clamp.py, not here. It is SAFETY-03's guard, its fixture and its P0; a P3 about the freeze detector reaching over to pin it would widen this PR into someone else's file for no reason except that I happened to be standing next to it. Filing it as a follow-up is my preference — happy to be overruled.Post-rebase verification, everything re-run on the merged tree
All nine suites pass on the merged content,
test_clamp.pyincluded — and thank you for confirming the workflow globs rather than lists, since that is what makes "nine" a fact about the tree instead of about somebody's memory.And thank you for the M2 correction. I would have spent the rebase hunting a detection loss that never happened.
All three verified independently, and R1 is ruled: keep the ids named. I cannot merge yet — no CI run exists for
0942228d. Details on that at the end; it is not something you did wrong.Everything below was run against the merged tree (
git merge-tree release/1.0 TEST-06->fce0e12), which I confirmed is byte-identical to the branch content, so this is the thing that would land and not an approximation of it.Check 2 — the trap, independently proven
Parsed both revisions and compared syntax trees, so "comment-only" is a proof and not an inspection — which is what I wanted this time, with the hunk abutting #12's real code.
And the resolution is right, which matters more than the proof. Each call carries its own boundary:
check_output_clamp()— "its boundary is the WRITE", because it readsself.clamp_last, and I confirmed that isNoneat construction (main.py:194) and only assigned insidewrite_setpoint()(main.py:276). So the claim is true, not just plausible.check_meter_frozen()— "its boundary is the ASSIGNMENT", with the explicit note that moving it between the assignment and the write is harmless and above the assignment is not.The comment did not generalise across both. That was the one thing worth another turn and it is not there.
Check 3 — N8 still bites
Both halves survive the rebase. The corrected comment is still a tested claim.
Check 1 — the three decisions
Confirmed by reading each one, and I think the converged call is better than the one I suggested. I proposed narrowing all but the wiring assertion; you kept converged surface-wide on the grounds that nothing about SAFETY-03 weakens "at the operating point the whole add-on has nothing to say", so narrowing would give away a real property for nothing. That is right and I was wrong. The other two narrow with the correct reason — the clamp alarm there is SAFETY-03's subject, and asserting whole-surface silence would make this file go red whenever another guard does its job properly — and printing the surface instead of grading it keeps the reader informed without this file grading somebody else's behaviour.
R1 at RED 2 — ruled: keep the ids named
You offered to pin only the count to get R1 back to 1. Do not. I measured the alternative rather than reasoning about it:
Pinning only the count buys R1 back to 1 and loses the clamp id entirely:
goodwe_output_clampcould be renamed to anything and nothing in the suite would notice. That is a real property traded for a cosmetic number.And the two reds in R1 are not the failure mode I flagged last round. That one was collapse — six assertions failing as one, none of them specifically about the id, the signal diluted until no check was the detector. This is corroboration: three mutations, three distinguishable signatures (R1 -> 2, R3 -> 1, R2 -> 1), each red carrying different information. The surface assertion catches drift in either guard's id, which the value check cannot; the value check names which id is wrong, which the surface assertion cannot. Keep both.
One honest note on it: the value check cannot fail strictly alone, because any mutation of the freeze id also changes the surface inventory. That is fine here — it is additive discrimination rather than a check that cannot fail — but it is worth a word in the comment so the next reader does not re-derive it as a defect.
Full merged-tree battery
All nine suites green from the merged tree,
test_clamp.pyincluded.Note on M2: my battery's anchor is a two-line prefix that leaves the original f-string continuation in place, so it reads RED 3; your full-block replacement reads RED 5, and I verified that is 5 on both revisions, so nothing regressed. Same correction I made last round — recording it again so the number in my table is not read as a discrepancy.
Why I have not merged
The rebase push did not trigger a workflow run. The green you and I both saw is run 5 against
d31a12b, the pre-rebase commit — so the required context on the current head has never been produced, and branch protection will refuse the merge until it is.That is the second time this gate has told us something other than what it appears to. Last time it was green on unmergeable content; this time it is a stale green being carried alongside a head it never tested. Neither is your doing, and I am not holding the substance of this PR for it.
Push anything to the branch to trigger a run — an empty commit is enough — or have the run dispatched manually. I will merge the moment
checks / unitreports on0942228d. Nothing in the code is outstanding: the three checks pass, R1 is ruled, and TEST-10 and TEST-11 are correctly out of scope.Correcting myself, and the gate is now red for a reason that is not your code. The approval in my last comment stands — nothing below changes it.
First, my error
I wrote: "The rebase push did not trigger a workflow run... No run for
0942228dwas ever created." That was wrong. The status history says so:Run 533 was created for the rebased head within minutes of the push. It sat queued for 26 minutes. I concluded "no run exists" because
/actions/tasksnever listed it — and that endpoint does not list queued runs, so it could not have shown me one. I inferred absence from a reading that was structurally incapable of observing the thing I was looking for.That is the exact defect this card is about, made by me, in a review of a ticket about it, one comment after I ruled on somebody else's version of it. The rule I gave you last round applies to me here without modification: report the attempt and its outcome, never the absence of an attempt as a property of the world. I had polled a listing, not the status; the status was sitting there the whole time saying
pending / Waiting to run.What the gate actually says
The job never executed. It was cancelled before starting, by
cancel-in-progress: trueongroup: addon-checks-14when a second run for this PR entered the group. Run 534 exists; it has posted nothing to0942228din the nine minutes I watched, and the required context is still thefailureleft behind by 533.So the red is not a test result. It is the absence of one, recorded as a failure.
Worth a card of its own
A cancelled run posts
failure, which at the gate is indistinguishable from a genuine test failure. Two different meanings — "the tests failed" and "nothing ran" — arrive at the branch protection as the same red, and the operator response to each is opposite: investigate the code, versus re-run the job.checks.yml's own comment documents the mirror-image incident:The per-PR group fixed cross-PR supersession. It does not stop a PR's own re-trigger from cancelling its only queued run, and on a single-slot runner with a 26-minute queue that window is wide. Last time the failure mode was a cancelled run that let a merge through; this time it is a cancelled run that blocks one. Same root, opposite symptom, and the second is much the safer direction — which is why I am reporting it rather than working around it.
Where this leaves the PR
The code is approved and unchanged. From the merged tree (
fce0e12, byte-identical to this branch), on my machine:Nothing in the diff is outstanding. I will not merge past a red required context, even one I am fairly confident is spurious — "the gate is probably wrong" is the reasoning this project keeps catching, and a green I have not seen is not a green.
To clear it: re-run 534 (or dispatch
workflow_dispatch), and if that does not post to this SHA, push an empty commit to force a freshpull_requestevent. I will merge as soon aschecks / unitreports an actual result on0942228d.