Closes SAFETY-03 (P0, 3 SP). Base release/1.0 @ 0cdaa3b.
What this is, in one sentence
The clamp already existed. The alarm did not, and specs.md 10.3 is explicit
that the clamp without the alarm is the defect, not the fix: with the inverter
switched off entirely an unclamped integrator climbed ~130 W per 4 s past 10 kW
and, pinned at the 5 kW rail, reported 14 768 W. Clipped silently, that is a
flat, legal-looking 5000 W hold on every dashboard and nothing anywhere says the
loop asked for three times it.
How "the rating" was resolved, and why
The card warns against picking one of two bounds. Both already exist in this
add-on:
spec
here
DriverCapabilities.setpoint_limit_w (3.2)
the setpoint entity's min/max from hass.limits() -> Controller.dev_min / dev_max
optimizer.loop.clamp_w (14)
the max_w option, already used by Tuning.max_w and the arbiter's supervised limit
Section 14 requires them to agree, validated at startup with a refusal on
mismatch - and that gate already ships: validate_startup_config() refuses max_w > dev_max and -max_w < dev_min, and run_control() re-runs it once
ESPHome is up and the real rating is known.
So this guard clamps to max_w, the validated value, and does not min()
it with the device rating at runtime. The device range is applied second, as
defence in depth. After a successful validation that stage is arithmetically
dead and cannot engage - which is exactly why it alarms if it ever does, with
the bound named: it means the validation did not run (hass.limits() returns (None, None) while ESPHome is down) or no longer holds.
No new config key. CFG-03 already tracks three places a default has to be
kept in step and this ticket does not earn a fourth. The bound is the number the
operator already set; the alarm's cadence is a module constant for the same
reason freeze.RAIL_FRAC is - it describes the notification bus, not the plant.
Every loop-tuning knob 10.3 asked to keep exposed is untouched.
Yes, there is an i_clamped signal - and SAFETY-20 should know
Decision now carries i_clamped and clamped booleans.
SAFETY-18's developer said outright they would have used an i_clamped boolean
if it existed; it exists now. SAFETY-20's min(max_w, dev_max) is not
implemented here and nothing in this PR changes what freeze.py measures its
rail against - but SAFETY-20 now has the option of asking the loop whether it
clamped, instead of inferring "at the rail" from RAIL_FRAC = 0.95.
The booleans are not a convenience. reason cannot answer the question: it
is one string that later stages overwrite, so a cycle that clamps the integrator
and is then slew-limited reports slew-limited, and one that clamps while
saturated reports saturated-freeze. Both are the winding case. Measured -
mutating main.py back onto "clamp" in decision.reason drops the 10.3
replay from 2 engagements to 0 and the alarm goes completely silent
(mutation 4 below).
Related: on a default install (integrator_max_w following max_w) the
integrator bound bites first and the output clamp is unreachable - control.py
already said so in a comment addressed to this ticket. A guard watching only the
emitted value would be silent on every standard configuration. Both are watched.
Acceptance criteria
AC1 - clamp before any write reaches the wire. In write_setpoint(), the
one path every write takes: cycle, heartbeat, operator button, shutdown().
Not in control.py - a guard the control law owns is a guard maintenance and
manual override bypass. Asserted over every value that reached set_number().
AC2 - alarm distinct from ordinary saturation. Separate reason code
(OUTPUT_CLAMP), separate notification_id (goodwe_output_clamp), separate
status row, separate counter. Part 4 drives a loop that saturates for 10
consecutive cycles at 1.4 % of the rating and asserts zero clamp
notifications, engagements, log rows and status rows - after first asserting
the saturation freeze really did engage, so the silence is an observation
rather than a scenario that did nothing.
AC3 - unit check: over the rating both clamps and alarms. Part 3, driven
through the real Controller.cycle().
Not a trip. 11.2's action is "clamp and alarm". The clamped value is
written, the loop keeps running, publish() still reports running. This is
deliberately unlike SAFETY-01/02, whose action is 0 W.
Two things found by running it, not by reading it
1. The action log flooded the ring and silenced another guard.log_event()
keeps 40 rows. A row per engagement flushes the whole ring inside 40 cycles of a
winding loop, and the first thing it evicted was SAFETY-18's own METER FROZEN?
row - this guard silencing another guard's log. Caught by test_freeze.py
going red, not by inspection. Now coalesced into one row per episode carrying
the exact count read off the counter; regression check added and mutation 6
reproduces the original failure in both suites.
2. A naive clamp does not pass a nan through - it launders it into full
power. Every comparison against nan is False, so min(limit, nan) keeps its first argument: max(-5000, min(5000, nan)) is +5000 W, and then applied != requested is True because limit != nan, so a transport bug gets
filed in the winding-loop counter and full-rate import goes on the wire.
Computed in the check, not recalled. A non-finite value is loss of input (11.1
rule 1): command 0 W, keep commanding, and do not count it as an engagement.
The check
test_clamp.py, assert-based, no framework, house style. Green:
1. the clamp arithmetic, on the shipped expression
ok +5001 W against a 5000 W rating -> +5000 W, engaged=True, bound='max_w'
ok -5001 W against a 5000 W rating -> -5000 W, engaged=True, bound='max_w'
ok +5000 W against a 5000 W rating -> +5000 W, engaged=False, bound=''
ok -5000 W against a 5000 W rating -> -5000 W, engaged=False, bound=''
ok +0 W against a 5000 W rating -> +0 W, engaged=False, bound=''
ok +1234 W against a 5000 W rating -> +1234 W, engaged=False, bound=''
ok +14768 W against a 5000 W rating -> +5000 W, engaged=True, bound='max_w'
ok symmetric over 325 magnitudes (0 asymmetric)
ok engaged is exactly 'raw differs from applied', over the same sweep
ok a device narrower than max_w still bites, and says which bound (+2000 W, 'device')
ok ...and is a no-op once the startup validation holds
non-finite input is loss of input, not an out-of-range value
ok is_commandable(None) is False
ok is_commandable(nan) is False
ok is_commandable(inf) is False
ok is_commandable(-inf) is False
ok is_commandable('abc') is False
ok is_commandable(<object object at 0x00000220DB) is False
ok is_commandable(0) is True
ok is_commandable(-5000.0) is True
ok is_commandable(5000) is True
ok is_commandable(12.5) is True
ok a naive clamp turns a nan into +5,000 W and calls it engaged=True - hence the guard
2. the alarm's cadence: every engagement counted, no flood
ok the rising edge fires (True)
ok the next 9 one-second cycles fire 0 more times (re-assert is 30 s)
ok but every one of them is counted: engagements=10
ok and it is one episode, not ten: episodes=1
ok it re-asserts on the slow cadence rather than going quiet
ok 2 clean cycles do NOT clear it (active=True, was True)
ok the 3th does (active=False)
ok ...and the engagement count survives the clear, so the evidence does
ok a clean cycle on an idle watchdog says nothing
3. AC 1 + AC 3, wired: over the rating both clamps AND alarms
unclamped, the same 60 cycles reach 216,000 W; the largest value that reached the device is 3,000 W
ok an unbounded accumulator really does run away here (216,000 W >> 5000 W)
ok AC 1: NOT ONE of the 3 values that reached the device exceeds the 5000 W rating
ok ...and the saturation freeze is what pins it at 3,000 W rather than the rail
the loop clamped on 2 of 60 cycles, in 1 episode(s), raising 1 notification(s)
ok AC 3: and it ALARMED - silently clipping this is the defect
ok ...with its own notification_id, not shared with any other guard
ok ...one episode, re-asserted rather than one per cycle
ok ...and it names itself a bug signal, not a limit in use
ok the action log records every engagement, coalesced into one row per episode (1 row(s), 2 engagement(s))
80 winding cycles: 80 engagement(s) -> 1 log row(s), leaving room for 0 other event(s)
ok a long winding episode does NOT flood the 40-row event log (1 rows for 80 engagements)
ok ...and the count in the row is the counter's, so nothing is lost
ok the status page goes red WHILE it is engaged
ok ...and once clear, a warn row still says how many times it engaged
ok it is NOT a trip - the command stands at +3000 W, not 0 W
ok ...and the controller still reports itself running, not failsafe
ok the export side clamps and alarms too (-3,000 W, 1 note(s), 2 engagement(s))
4. AC 2: saturation WITHOUT clamping must NOT raise a clamp alarm
saturated for 10 consecutive cycles; the command peaked at 70 W, 1.4% of the rating
ok the saturation freeze REALLY DID engage (sat_count=10 >= 3)
ok ...and the event log says so, in the saturation's own words
ok ...while the command stayed well inside the rating (70 W < 5000 W)
ok AC 2: NO clamp notification was raised
ok AC 2: the clamp counted ZERO engagements
ok AC 2: no OUTPUT_CLAMP row in the action log
ok AC 2: and no clamp row on the status page
...and the two alarms do not share a notification_id
ok clamp ids ['goodwe_output_clamp'] are disjoint from every other alarm's []
...and the alarm cannot be keyed on Decision.reason
reason='slew-limited' i_clamped=True clamped=False
ok the integrator DID clamp on that cycle
ok ...but `reason` says 'slew-limited' - the clamp was overwritten downstream, so a string-keyed alarm is SILENT here
reason='slew-limited' i_clamped=True clamped=True
ok both clamps engaged, and `reason` reports neither
8 of 20 sweep cells are frozen; those that also clamp: [(-1900.0, 9000.0), (1900.0, -9000.0)]
ok a frozen cycle CAN clamp, and only by reversing (2 of 8 frozen cells)
ok ...and a frozen cycle pushing the way it already leans never clamps - that is the freeze doing its job
ok a healthy cycle sets neither flag
5. no path reaches the wire around the clamp
asked for 9999 / -9999 / nan / 1500 W; the device saw [2000.0, -2000.0, 0.0, 1500.0]
ok a direct out-of-range write is clamped, not passed through
ok a nan is refused and becomes 0 W, per 11.1 rule 1
ok ...and is NOT counted as a clamp engagement - it is a transport bug
ok an in-range value is untouched
ok every emitted frame satisfies |emitted| <= the rating
all checks passed
Red runs - six author mutations, every one reproduced
Each mutation was applied to the shipped code, the check re-run, and the source
restored. Mutation 1 is the historical build the ticket exists to refuse.
###### MUTATION 1 - clamp kept, ALARM DELETED (the historical silent-clipping build)
FAIL AC 3: and it ALARMED - silently clipping this is the defect
FAIL ...and it names itself a bug signal, not a limit in use
FAIL the export side clamps and alarms too (-3,000 W, 0 note(s), 2 engagement(s))
3 FAILED
###### MUTATION 2 - clamp alarm CONFLATED with the saturation freeze (AC2)
FAIL the action log carries one row per engagement, undeduplicated (40 rows for 59 engagements)
FAIL ...and once clear, a warn row still says how many times it engaged
FAIL AC 2: NO clamp notification was raised
FAIL AC 2: the clamp counted ZERO engagements
FAIL AC 2: no OUTPUT_CLAMP row in the action log
FAIL AC 2: and no clamp row on the status page
6 FAILED
###### MUTATION 3 - the CLAMP removed from the write path (AC1)
FAIL +5001 W against a 5000 W rating -> +5001 W, engaged=False, bound=''
FAIL -5001 W against a 5000 W rating -> -5001 W, engaged=False, bound=''
FAIL +14768 W against a 5000 W rating -> +14768 W, engaged=False, bound=''
FAIL both clamps engaged, and `reason` reports neither
FAIL a direct out-of-range write is clamped, not passed through
FAIL every emitted frame satisfies |emitted| <= the rating
6 FAILED
###### MUTATION 4 - alarm keyed on Decision.reason (the string) instead of the flags
FAIL AC 3: and it ALARMED - silently clipping this is the defect
FAIL ...one episode, re-asserted rather than one per cycle
FAIL ...and it names itself a bug signal, not a limit in use
FAIL the status page goes red WHILE it is engaged
FAIL ...and once clear, a warn row still says how many times it engaged
FAIL the export side clamps and alarms too (-3,000 W, 0 note(s), 0 engagement(s))
6 FAILED
^ note the derived numbers: 0 notifications, 0 engagements. Keying the alarm
on the string does not weaken it, it turns it OFF.
###### MUTATION 5 - engagement redefined as >= the rating (a healthy full-power hour alarms)
FAIL +5000 W against a 5000 W rating -> +5000 W, engaged=True, bound='max_w'
FAIL -5000 W against a 5000 W rating -> -5000 W, engaged=True, bound='max_w'
2 FAILED
###### MUTATION 6 - event-log coalescing removed (a row per engagement)
FAIL the action log records every engagement, coalesced into one row per episode (2 row(s), 2 engagement(s))
FAIL a long winding episode does NOT flood the 40-row event log (40 rows for 80 engagements)
FAIL ...and the count in the row is the counter's, so nothing is lost
3 FAILED
--- and test_freeze.py, which is the point:
FAIL the event log names it
1 FAILED
The check refused two of its own hypotheses
Both about how the saturation freeze and the integrator clamp interact. Written
first as "a frozen cycle never clamps" (false), then as "only the i_w == 0
reseed exception clamps" (also false - at i_w == 0 with a matching actual
the cycle is not saturated at all, so it is never frozen). What the sweep
actually shows, and what the check now asserts: a frozen cycle clamps exactly
when the integrator reverses, which control.py permits on purpose. That is the
one shape where saturation and a clamp engagement coincide - i.e. precisely where
a shared alarm channel would lose the clamp. It is in the check because it was
measured, not because it was expected.
Whole suite
test_control GREEN test_loop GREEN
test_arbiter GREEN test_clamp GREEN
test_validate GREEN test_freeze GREEN
test_maintenance GREEN test_p1 GREEN
test_watchdog GREEN
Two existing checks were changed, both tightened rather than relaxed:
test_freeze.py filtered notifications by service. This add-on now
raises more than one kind, and freeze's own scenarios drive a command to the
rail on purpose - i.e. they legitimately raise the clamp alarm too. A bare svc == "create" test counts another guard's alarm as this one's, which is the
exact conflation AC2 forbids. Now keyed on notification_id.
test_watchdog.py's _Hass stub gained call(). Its e2e runs a
deliberately inverted loop (gain: -0.6) into a steady 2000 W import - a loop
that winds straight into the rail - so the clamp alarm fires there. The missing
stub method is how that was discovered; it is now a second, independent witness
that the alarm sees a real winding loop.
Not done, and why
No rig run. The clamp is arithmetic on the write path; part 3 drives the
shipped Controller.cycle() against a stub HA, which is the same surface
test_freeze parts 1-2 use. The rig was also HELD by a CI campaign
(45ed2ab4cd2b:158668, CI 60 sever_network fw_update) throughout - observed
with DOCKER_HOST set, not inferred from a UNKNOWN. Not jumped.
Nothing touched the live inverter, the live add-on, or firmware.
The firmware-side magnitude clamp (the card's "two clamps, not one") is
DRIVER-07's and is not in this repo's Python. The wire-level invariant is
asserted here over every value that reached set_number(), which is the last
thing this process controls; proving it at the inverter needs the witness log.
ems/event/fault on MQTT and the 13.2 binary sensors do not exist yet
(IF-01/IF-02). Same call SAFETY-02 and SAFETY-18 made: use the surfaces that
ship rather than invent a topic to migrate later.
SAFETY-20 not implemented, deliberately.
Where to attack this hardest
The max_w-not-min() decision. If validate_startup_config() can be
skipped on a path I have not found, the "device stage cannot engage" claim is
wrong and the alarm's second bound becomes routine rather than exceptional.
The one hole I know of is hass.limits() returning (None, None) while
ESPHome is down - which is why the stage is still there and still alarms.
Event-log coalescing vs "every engagement". I record the count rather
than a row each. If a reviewer reads 15's action-log requirement as one row
per engagement, this is a deliberate deviation and the ring-flush measurement
is the argument for it.
The red status row makes the banner say NOT READY while the controller
is, in fact, still regulating. That follows the freeze detector's precedent
but the wording is arguably wrong for a guard that is explicitly not a trip.
clamp_wd is not persisted, unlike SAFETY-02's latch. A restart zeroes
the engagement counter and the evidence with it. Deliberate (nothing here
acts, so there is nothing to erase), same asymmetry SAFETY-18 argued - but it
is a judgement call, not a fact.
The Decision flags at the reversal case. The sweep is 20 cells at one
tuning; a wider sweep may find frozen-and-clamping shapes I have not.
Against the PM contract on the card
The card's second PM comment supersedes the first and asks, in its own words, to alarm on a boolean, not a string - i_clamped: bool on Decision, watched
directly, with "i-clamped" kept as the human-readable status string. That is
what this implements, and clamped is carried alongside it for the
configuration where the integrator is deliberately allowed above the rail.
The PM measured the string hook at 15 % i-clamped / 40 % slew-limited /
45 % saturated-freeze. This PR reproduces the mechanism rather than the
percentages: mutation 4 keys the alarm on the string and the 10.3 replay goes
from 2 engagements to 0, and part 4 of the check pins one concrete cycle of each
overwrite (slew-limited with i_clamped=True, and both flags set with reason reporting neither).
The first comment's instruction - alarm on reason == "i-clamped" - is not
followed, per the correction. saturated-freeze remaining the separate
saturation reason is unchanged and is what AC2's "distinct from a normal
saturation event" leans on, backed here by the negative scenario in part 4.
Closes SAFETY-03 (P0, 3 SP). Base `release/1.0` @ `0cdaa3b`.
## What this is, in one sentence
The clamp already existed. **The alarm did not**, and specs.md 10.3 is explicit
that the clamp without the alarm is the defect, not the fix: with the inverter
switched off entirely an unclamped integrator climbed ~130 W per 4 s past 10 kW
and, pinned at the 5 kW rail, reported **14 768 W**. Clipped silently, that is a
flat, legal-looking 5000 W hold on every dashboard and nothing anywhere says the
loop asked for three times it.
## How "the rating" was resolved, and why
The card warns against picking one of two bounds. Both already exist in this
add-on:
| spec | here |
|---|---|
| `DriverCapabilities.setpoint_limit_w` (3.2) | the setpoint entity's `min`/`max` from `hass.limits()` -> `Controller.dev_min` / `dev_max` |
| `optimizer.loop.clamp_w` (14) | the `max_w` option, already used by `Tuning.max_w` and the arbiter's `supervised` limit |
Section 14 requires them to agree, validated at startup with a refusal on
mismatch - **and that gate already ships**: `validate_startup_config()` refuses
`max_w > dev_max` and `-max_w < dev_min`, and `run_control()` re-runs it once
ESPHome is up and the real rating is known.
So this guard clamps to **`max_w`, the validated value**, and does not `min()`
it with the device rating at runtime. The device range is applied *second*, as
defence in depth. After a successful validation that stage is arithmetically
dead and cannot engage - which is exactly why it alarms if it ever does, with
the bound named: it means the validation did not run (`hass.limits()` returns
`(None, None)` while ESPHome is down) or no longer holds.
**No new config key.** CFG-03 already tracks three places a default has to be
kept in step and this ticket does not earn a fourth. The bound is the number the
operator already set; the alarm's cadence is a module constant for the same
reason `freeze.RAIL_FRAC` is - it describes the notification bus, not the plant.
Every loop-tuning knob 10.3 asked to keep exposed is untouched.
## Yes, there is an `i_clamped` signal - and SAFETY-20 should know
`Decision` now carries **`i_clamped`** and **`clamped`** booleans.
SAFETY-18's developer said outright they would have used an `i_clamped` boolean
if it existed; **it exists now.** SAFETY-20's `min(max_w, dev_max)` is *not*
implemented here and nothing in this PR changes what `freeze.py` measures its
rail against - but SAFETY-20 now has the option of asking the loop whether it
clamped, instead of inferring "at the rail" from `RAIL_FRAC = 0.95`.
The booleans are not a convenience. **`reason` cannot answer the question**: it
is one string that later stages overwrite, so a cycle that clamps the integrator
and is then slew-limited reports `slew-limited`, and one that clamps while
saturated reports `saturated-freeze`. Both *are* the winding case. Measured -
mutating `main.py` back onto `"clamp" in decision.reason` drops the 10.3
replay from 2 engagements to **0** and the alarm goes completely silent
(mutation 4 below).
Related: on a default install (`integrator_max_w` following `max_w`) the
integrator bound bites first and the **output** clamp is unreachable - control.py
already said so in a comment addressed to this ticket. A guard watching only the
emitted value would be silent on every standard configuration. Both are watched.
## Acceptance criteria
- **AC1 - clamp before any write reaches the wire.** In `write_setpoint()`, the
one path every write takes: cycle, heartbeat, operator button, `shutdown()`.
Not in `control.py` - a guard the control law owns is a guard maintenance and
manual override bypass. Asserted over every value that reached `set_number()`.
- **AC2 - alarm distinct from ordinary saturation.** Separate reason code
(`OUTPUT_CLAMP`), separate `notification_id` (`goodwe_output_clamp`), separate
status row, separate counter. Part 4 drives a loop that saturates for 10
consecutive cycles at 1.4 % of the rating and asserts **zero** clamp
notifications, engagements, log rows and status rows - *after* first asserting
the saturation freeze really did engage, so the silence is an observation
rather than a scenario that did nothing.
- **AC3 - unit check: over the rating both clamps and alarms.** Part 3, driven
through the real `Controller.cycle()`.
**Not a trip.** 11.2's action is "clamp *and* alarm". The clamped value is
written, the loop keeps running, `publish()` still reports `running`. This is
deliberately unlike SAFETY-01/02, whose action is 0 W.
## Two things found by running it, not by reading it
**1. The action log flooded the ring and silenced another guard.** `log_event()`
keeps 40 rows. A row per engagement flushes the whole ring inside 40 cycles of a
winding loop, and the first thing it evicted was SAFETY-18's own `METER FROZEN?`
row - this guard silencing another guard's log. **Caught by `test_freeze.py`
going red**, not by inspection. Now coalesced into one row per episode carrying
the exact count read off the counter; regression check added and mutation 6
reproduces the original failure in both suites.
**2. A naive clamp does not pass a `nan` through - it launders it into full
power.** Every comparison against `nan` is False, so `min(limit, nan)` keeps its
*first* argument: `max(-5000, min(5000, nan))` is **+5000 W**, and then
`applied != requested` is True because `limit != nan`, so a transport bug gets
filed in the winding-loop counter *and* full-rate import goes on the wire.
Computed in the check, not recalled. A non-finite value is loss of input (11.1
rule 1): command 0 W, keep commanding, and do not count it as an engagement.
## The check
`test_clamp.py`, assert-based, no framework, house style. Green:
```
1. the clamp arithmetic, on the shipped expression
ok +5001 W against a 5000 W rating -> +5000 W, engaged=True, bound='max_w'
ok -5001 W against a 5000 W rating -> -5000 W, engaged=True, bound='max_w'
ok +5000 W against a 5000 W rating -> +5000 W, engaged=False, bound=''
ok -5000 W against a 5000 W rating -> -5000 W, engaged=False, bound=''
ok +0 W against a 5000 W rating -> +0 W, engaged=False, bound=''
ok +1234 W against a 5000 W rating -> +1234 W, engaged=False, bound=''
ok +14768 W against a 5000 W rating -> +5000 W, engaged=True, bound='max_w'
ok symmetric over 325 magnitudes (0 asymmetric)
ok engaged is exactly 'raw differs from applied', over the same sweep
ok a device narrower than max_w still bites, and says which bound (+2000 W, 'device')
ok ...and is a no-op once the startup validation holds
non-finite input is loss of input, not an out-of-range value
ok is_commandable(None) is False
ok is_commandable(nan) is False
ok is_commandable(inf) is False
ok is_commandable(-inf) is False
ok is_commandable('abc') is False
ok is_commandable(<object object at 0x00000220DB) is False
ok is_commandable(0) is True
ok is_commandable(-5000.0) is True
ok is_commandable(5000) is True
ok is_commandable(12.5) is True
ok a naive clamp turns a nan into +5,000 W and calls it engaged=True - hence the guard
2. the alarm's cadence: every engagement counted, no flood
ok the rising edge fires (True)
ok the next 9 one-second cycles fire 0 more times (re-assert is 30 s)
ok but every one of them is counted: engagements=10
ok and it is one episode, not ten: episodes=1
ok it re-asserts on the slow cadence rather than going quiet
ok 2 clean cycles do NOT clear it (active=True, was True)
ok the 3th does (active=False)
ok ...and the engagement count survives the clear, so the evidence does
ok a clean cycle on an idle watchdog says nothing
3. AC 1 + AC 3, wired: over the rating both clamps AND alarms
unclamped, the same 60 cycles reach 216,000 W; the largest value that reached the device is 3,000 W
ok an unbounded accumulator really does run away here (216,000 W >> 5000 W)
ok AC 1: NOT ONE of the 3 values that reached the device exceeds the 5000 W rating
ok ...and the saturation freeze is what pins it at 3,000 W rather than the rail
the loop clamped on 2 of 60 cycles, in 1 episode(s), raising 1 notification(s)
ok AC 3: and it ALARMED - silently clipping this is the defect
ok ...with its own notification_id, not shared with any other guard
ok ...one episode, re-asserted rather than one per cycle
ok ...and it names itself a bug signal, not a limit in use
ok the action log records every engagement, coalesced into one row per episode (1 row(s), 2 engagement(s))
80 winding cycles: 80 engagement(s) -> 1 log row(s), leaving room for 0 other event(s)
ok a long winding episode does NOT flood the 40-row event log (1 rows for 80 engagements)
ok ...and the count in the row is the counter's, so nothing is lost
ok the status page goes red WHILE it is engaged
ok ...and once clear, a warn row still says how many times it engaged
ok it is NOT a trip - the command stands at +3000 W, not 0 W
ok ...and the controller still reports itself running, not failsafe
ok the export side clamps and alarms too (-3,000 W, 1 note(s), 2 engagement(s))
4. AC 2: saturation WITHOUT clamping must NOT raise a clamp alarm
saturated for 10 consecutive cycles; the command peaked at 70 W, 1.4% of the rating
ok the saturation freeze REALLY DID engage (sat_count=10 >= 3)
ok ...and the event log says so, in the saturation's own words
ok ...while the command stayed well inside the rating (70 W < 5000 W)
ok AC 2: NO clamp notification was raised
ok AC 2: the clamp counted ZERO engagements
ok AC 2: no OUTPUT_CLAMP row in the action log
ok AC 2: and no clamp row on the status page
...and the two alarms do not share a notification_id
ok clamp ids ['goodwe_output_clamp'] are disjoint from every other alarm's []
...and the alarm cannot be keyed on Decision.reason
reason='slew-limited' i_clamped=True clamped=False
ok the integrator DID clamp on that cycle
ok ...but `reason` says 'slew-limited' - the clamp was overwritten downstream, so a string-keyed alarm is SILENT here
reason='slew-limited' i_clamped=True clamped=True
ok both clamps engaged, and `reason` reports neither
8 of 20 sweep cells are frozen; those that also clamp: [(-1900.0, 9000.0), (1900.0, -9000.0)]
ok a frozen cycle CAN clamp, and only by reversing (2 of 8 frozen cells)
ok ...and a frozen cycle pushing the way it already leans never clamps - that is the freeze doing its job
ok a healthy cycle sets neither flag
5. no path reaches the wire around the clamp
asked for 9999 / -9999 / nan / 1500 W; the device saw [2000.0, -2000.0, 0.0, 1500.0]
ok a direct out-of-range write is clamped, not passed through
ok a nan is refused and becomes 0 W, per 11.1 rule 1
ok ...and is NOT counted as a clamp engagement - it is a transport bug
ok an in-range value is untouched
ok every emitted frame satisfies |emitted| <= the rating
all checks passed
```
### Red runs - six author mutations, every one reproduced
Each mutation was applied to the shipped code, the check re-run, and the source
restored. Mutation 1 is the historical build the ticket exists to refuse.
```
###### MUTATION 1 - clamp kept, ALARM DELETED (the historical silent-clipping build)
FAIL AC 3: and it ALARMED - silently clipping this is the defect
FAIL ...and it names itself a bug signal, not a limit in use
FAIL the export side clamps and alarms too (-3,000 W, 0 note(s), 2 engagement(s))
3 FAILED
###### MUTATION 2 - clamp alarm CONFLATED with the saturation freeze (AC2)
FAIL the action log carries one row per engagement, undeduplicated (40 rows for 59 engagements)
FAIL ...and once clear, a warn row still says how many times it engaged
FAIL AC 2: NO clamp notification was raised
FAIL AC 2: the clamp counted ZERO engagements
FAIL AC 2: no OUTPUT_CLAMP row in the action log
FAIL AC 2: and no clamp row on the status page
6 FAILED
###### MUTATION 3 - the CLAMP removed from the write path (AC1)
FAIL +5001 W against a 5000 W rating -> +5001 W, engaged=False, bound=''
FAIL -5001 W against a 5000 W rating -> -5001 W, engaged=False, bound=''
FAIL +14768 W against a 5000 W rating -> +14768 W, engaged=False, bound=''
FAIL both clamps engaged, and `reason` reports neither
FAIL a direct out-of-range write is clamped, not passed through
FAIL every emitted frame satisfies |emitted| <= the rating
6 FAILED
###### MUTATION 4 - alarm keyed on Decision.reason (the string) instead of the flags
FAIL AC 3: and it ALARMED - silently clipping this is the defect
FAIL ...one episode, re-asserted rather than one per cycle
FAIL ...and it names itself a bug signal, not a limit in use
FAIL the status page goes red WHILE it is engaged
FAIL ...and once clear, a warn row still says how many times it engaged
FAIL the export side clamps and alarms too (-3,000 W, 0 note(s), 0 engagement(s))
6 FAILED
^ note the derived numbers: 0 notifications, 0 engagements. Keying the alarm
on the string does not weaken it, it turns it OFF.
###### MUTATION 5 - engagement redefined as >= the rating (a healthy full-power hour alarms)
FAIL +5000 W against a 5000 W rating -> +5000 W, engaged=True, bound='max_w'
FAIL -5000 W against a 5000 W rating -> -5000 W, engaged=True, bound='max_w'
2 FAILED
###### MUTATION 6 - event-log coalescing removed (a row per engagement)
FAIL the action log records every engagement, coalesced into one row per episode (2 row(s), 2 engagement(s))
FAIL a long winding episode does NOT flood the 40-row event log (40 rows for 80 engagements)
FAIL ...and the count in the row is the counter's, so nothing is lost
3 FAILED
--- and test_freeze.py, which is the point:
FAIL the event log names it
1 FAILED
```
### The check refused two of its own hypotheses
Both about how the saturation freeze and the integrator clamp interact. Written
first as "a frozen cycle never clamps" (**false**), then as "only the `i_w == 0`
reseed exception clamps" (**also false** - at `i_w == 0` with a matching `actual`
the cycle is not saturated at all, so it is never frozen). What the sweep
actually shows, and what the check now asserts: a frozen cycle clamps **exactly
when the integrator reverses**, which control.py permits on purpose. That is the
one shape where saturation and a clamp engagement coincide - i.e. precisely where
a shared alarm channel would lose the clamp. It is in the check because it was
measured, not because it was expected.
### Whole suite
```
test_control GREEN test_loop GREEN
test_arbiter GREEN test_clamp GREEN
test_validate GREEN test_freeze GREEN
test_maintenance GREEN test_p1 GREEN
test_watchdog GREEN
```
Two existing checks were changed, both tightened rather than relaxed:
- **`test_freeze.py`** filtered notifications by *service*. This add-on now
raises more than one kind, and freeze's own scenarios drive a command to the
rail on purpose - i.e. they legitimately raise the clamp alarm too. A bare
`svc == "create"` test counts another guard's alarm as this one's, which is the
exact conflation AC2 forbids. Now keyed on `notification_id`.
- **`test_watchdog.py`**'s `_Hass` stub gained `call()`. Its e2e runs a
deliberately inverted loop (`gain: -0.6`) into a steady 2000 W import - a loop
that winds straight into the rail - so the clamp alarm fires there. The missing
stub method is how that was discovered; it is now a second, independent witness
that the alarm sees a real winding loop.
## Not done, and why
- **No rig run.** The clamp is arithmetic on the write path; part 3 drives the
shipped `Controller.cycle()` against a stub HA, which is the same surface
test_freeze parts 1-2 use. The rig was also **HELD by a CI campaign**
(`45ed2ab4cd2b:158668`, `CI 60 sever_network fw_update`) throughout - observed
with `DOCKER_HOST` set, not inferred from a `UNKNOWN`. Not jumped.
- **Nothing touched the live inverter**, the live add-on, or firmware.
- **The firmware-side magnitude clamp** (the card's "two clamps, not one") is
DRIVER-07's and is not in this repo's Python. The wire-level invariant is
asserted here over every value that reached `set_number()`, which is the last
thing this process controls; proving it at the inverter needs the witness log.
- **`ems/event/fault` on MQTT and the 13.2 binary sensors** do not exist yet
(IF-01/IF-02). Same call SAFETY-02 and SAFETY-18 made: use the surfaces that
ship rather than invent a topic to migrate later.
- **SAFETY-20 not implemented**, deliberately.
## Where to attack this hardest
1. **The `max_w`-not-`min()` decision.** If `validate_startup_config()` can be
skipped on a path I have not found, the "device stage cannot engage" claim is
wrong and the alarm's second bound becomes routine rather than exceptional.
The one hole I know of is `hass.limits()` returning `(None, None)` while
ESPHome is down - which is why the stage is still there and still alarms.
2. **Event-log coalescing vs "every engagement".** I record the *count* rather
than a row each. If a reviewer reads 15's action-log requirement as one row
per engagement, this is a deliberate deviation and the ring-flush measurement
is the argument for it.
3. **The red status row makes the banner say `NOT READY`** while the controller
is, in fact, still regulating. That follows the freeze detector's precedent
but the wording is arguably wrong for a guard that is explicitly not a trip.
4. **`clamp_wd` is not persisted**, unlike SAFETY-02's latch. A restart zeroes
the engagement counter and the evidence with it. Deliberate (nothing here
acts, so there is nothing to erase), same asymmetry SAFETY-18 argued - but it
is a judgement call, not a fact.
5. **The `Decision` flags at the reversal case.** The sweep is 20 cells at one
tuning; a wider sweep may find frozen-and-clamping shapes I have not.
---
## Against the PM contract on the card
The card's second PM comment supersedes the first and asks, in its own words, to
**alarm on a boolean, not a string** - `i_clamped: bool` on `Decision`, watched
directly, with `"i-clamped"` kept as the human-readable status string. That is
what this implements, and `clamped` is carried alongside it for the
configuration where the integrator is deliberately allowed above the rail.
The PM measured the string hook at 15 % `i-clamped` / 40 % `slew-limited` /
45 % `saturated-freeze`. This PR reproduces the mechanism rather than the
percentages: mutation 4 keys the alarm on the string and the 10.3 replay goes
from 2 engagements to 0, and part 4 of the check pins one concrete cycle of each
overwrite (`slew-limited` with `i_clamped=True`, and both flags set with
`reason` reporting neither).
The first comment's instruction - alarm on `reason == "i-clamped"` - is **not**
followed, per the correction. `saturated-freeze` remaining the separate
saturation reason is unchanged and is what AC2's "distinct from a normal
saturation event" leans on, backed here by the negative scenario in part 4.
The clamp was never the missing half. specs.md 10.3 records an unclamped
integrator climbing ~130 W per 4 s past 10 kW and reporting 14 768 W pinned at
the 5 kW rail; clipped silently that is a flat, legal-looking 5000 W hold and
nothing says the loop asked for three times it.
write_setpoint() - the one path every write takes - now clamps to the VALIDATED
max_w and reports whether the raw value differed; check_output_clamp() raises
OUTPUT_CLAMP as a notification, an ERROR, an event row and a red status banner.
Not a trip: the clamped value is written and the loop keeps running.
The rating is max_w and nothing is min()ed at runtime - CFG-01 already refuses a
max_w outside the device range at startup. The device range is applied second as
defence in depth; it cannot bite after a successful validation, so if it ever
does it alarms too and names the bound. No new config key.
Decision grew i_clamped / clamped booleans, and that is the load-bearing part:
`reason` is one string that slew-limiting and the saturation freeze overwrite,
so an alarm keyed on it is silent on exactly the cycles a winding loop produces.
Mutating main.py back onto the string drops the 10.3 replay from 2 engagements
to 0.
Distinct from saturation by construction - no shared reason code, notification
id, status row or counter - and checked in the negative, after first asserting
the saturation freeze really did engage.
Two things found by running it rather than by reading it: a row per engagement
flushed the 40-row event log and evicted SAFETY-18's own METER FROZEN? row (now
coalesced per episode with an exact count), and a naive clamp turns a nan into a
command at the full rating because every comparison against nan is False.
test_clamp.py is new: assert-based, no framework. Six author mutations
reproduced red, including "clamp kept, alarm deleted". Two of the check's own
hypotheses about the freeze/clamp interaction were refused by its sweep and
rewritten to what it measured.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
Not merging yet. Three things must change first, and none of them is large.
Nearly all of this is right, and the two things you found by running rather than reading are both real - I reproduced both. What holds it is one behavioural bug in clamp.py itself, one assertion that cannot fail, and one constant nothing pins. Every item below is a few lines.
Own detached worktree at c040aac, own mutation harness, own probes. Whole suite green as shipped: test_arbiter, test_control, test_loop, test_maintenance, test_p1, test_validate, test_watchdog, test_clamp, test_freeze - all exit 0. app/freeze.py is byte-unchanged, confirmed by an empty diff, as you said.
1. MUST CHANGE - clamp_to_rating() never applies the device bound when the first stage engages
clamp.py says, in capitals:
WARNING - THE DEVICE BOUND IS STILL APPLIED, SECOND, AND IT IS NOT REDUNDANT.
It is not. The max_w stage returns, so the device stage is reached only when the first stage did not engage. Executed against the shipped function, max_w = 5000, device range +-4000:
req +9999 -> applied +5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
req +6000 -> applied +5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
req +5001 -> applied +5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
req +4500 -> applied +4000 engaged=True bound='device'
req -9999 -> applied -5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
the code this PR deleted, for the same inputs:
req +9999 -> +4000 req +6000 -> +4000 req -9999 -> -4000
Three things about that, in order of how much they matter:
The bigger the bug, the less the guard does. A request 500 W over the rating is caught by the device bound; a request 5000 W over it is not. That is backwards for a guard whose entire subject is a runaway.
It silently drops a guarantee this PR removed the comment for. The deleted lines said "Never send a value the device will reject outright. A rejected write is silent from the controller's point of view, and the inverter then keeps doing whatever it was already doing." That is still true, and it is no longer enforced on the path where the first stage bit.
It undercuts your own defence of max_w-not-min(). Your argument - and I agree with it, see below - is that the device stage exists for the case where validation did not run or no longer holds. In precisely that case it now fails to bound the values that most need bounding.
Reachability, honestly: I went looking for the validation-skipping path you asked me to look for and I did not find one.self.dev_min / self.dev_max are assigned in exactly two places: __init__ (both None) and run_control(), where the very next statement is validate_startup_config() and a refusal that stops deterministically through the normal shutdown. amain() runs the same gate before any task exists. hass.limits() -> (None, None) leaves the device stage inert, not wrong. And every write_setpoint() caller passes self.target (arbiter-bounded to +-max_w) or 0.0, so the write-path clamp does not engage at all on a reachable path - which is what your "0 write-path engagements in 60 cycles" measured. So this is a latent bug in a layer that is currently unreachable.
That is exactly why it should be fixed rather than filed. This stage's whole job is to be correct on the day the assumption above stops holding. Fall through instead of returning, apply the device bound to applied, and report whichever bound actually bit.
While you are in there: a mutation deleting the device stage entirely turns one check red (a device narrower than max_w still bites). One assertion is thin cover for the half of the function that is supposed to be the last thing between a bug and the hardware.
2. MUST CHANGE - the hysteresis check cannot fail
partial=[wd.note(False,"",T0+timedelta(seconds=100+i))foriinrange(CLEAR_CYCLES-1)]check(f"{CLEAR_CYCLES-1} clean cycles do NOT clear it "f"(active={wd.active}, was {before})",wd.activeisTrue)
CLEAR_CYCLES = 3 -> 1: the whole suite stays green.range(0) is empty, so zero clean cycles are issued, wd.active is trivially still True, and the check passes - printing, out loud, ok 0 clean cycles do NOT clear it. It passes for every value of CLEAR_CYCLES >= 1.
CLEAR_CYCLES = 1 is the dangerous direction: it is the alarm chattering on a command that dithers across the rail by one step_w, which is the exact thing the constant's own comment says it exists to prevent. The constant is right; nothing defends it.
(CLEAR_CYCLES = 999is caught - by one status-row assertion, incidentally, not by the hysteresis check.)
Assert against the literals, the way the rest of this check asserts against 2 and 10 and 40.
3. MUST CHANGE - REASSERT_S is pinned downward only
REASSERT_S = 30.0 -> 3600.0: the whole suite stays green. Line 161 reads T0 + timedelta(seconds=REASSERT_S + 9), so the re-assert check passes for any value at all.
clamp.py promises the opposite in two places - "so a loop pinned on the rail for an hour does not go quiet", "fast enough that a pinned loop is never quiet for a whole billing quarter". An hour of silence satisfies every check in the file. The downward direction is pinned, by sum(fired[1:]) == 0 over ten 1-second cycles - my REASSERT_S = 0 mutation goes red there - which is why this is the second half of the same fix and not a separate problem.
What I verified, and what it cost me to believe it
Your headline claim, reproduced exactly
This is the strongest evidence in the PR and it survives an independent run:
shipped (booleans): the loop clamped on 2 of 60 cycles, in 1 episode(s), raising 1 notification(s)
keyed on decision.reason: the loop clamped on 0 of 60 cycles, in 0 episode(s), raising 0 notification(s)
Keying the alarm on the string does not weaken it, it switches it off. Decision.i_clamped / clamped earn their place.
Your six mutations, all red on my run
you
me
note
M1 alarm deleted
3
3
identical
M2 conflated with saturation
6
6
identical; all four AC2 rows go red
M3 clamp removed from the write path
6
5
my edit nulled the arithmetic too, so one row differs
M4 keyed on reason
6
3
mine is the narrower edit; the derived numbers above are the point
M5 boundary >=
2
4
redder than you reported
M6 coalescing removed
3 + 1
3 + 1
identical, including test_freeze going red on "the event log names it"
M6 crossing suites is the one I most wanted to see fail, and it does. Your discovery - this guard's log evicting SAFETY-18's METER FROZEN? row - is real, and the regression check genuinely holds it.
Mine, beyond yours
N1 device stage removed - RED (1). Thin, see item 1.
N2 the nan guard reverted - RED (5).
N3REASSERT_S -> 0 - RED (2).
N7engagements deduplicated to the rising edge - RED (4). "Never deduplicated" is genuinely pinned.
N8 raw value not preserved in ClampResult - RED (1).
N4CLEAR_CYCLES -> 1 - GREEN. Survives.
N5REASSERT_S -> 3600 - GREEN. Survives.
nan, inf, -inf - the guard holds
is_commandable returns False for all three, plus None and a non-numeric. And the laundering you describe is exactly right, computed rather than recalled: max(-5000, min(5000, nan)) is +5000.0, and 5000.0 != nan is True, so a naive clamp puts full-rate import on the wire and files it in the winding-loop counter. Driven through the real Controller, a nan becomes 0.0 on the wire and is not counted as an engagement.
AC2, independently, with a scenario of my own
Not your part 4 - my own plant, one that follows 10 % of the command so the error stays large while the command stays small:
sat_count=10, peak |write|=1080 W (21.6% of the rating), clamp engagements=0
ok the saturation freeze REALLY engaged (observation, not an empty scenario)
ok AC2: no clamp notification
ok AC2: zero clamp engagements
ok AC2: no OUTPUT_CLAMP row in the action log
ok AC2: no clamp row on the status page
And M2 turns your own AC2 rows red, so that scenario is an observation too. AC2 holds.
Does the coalescing weaken AC2? No - and I measured what it actually does
It does not touch AC2 at all: AC2 is about the clamp alarm being distinguishable from saturation, and it is, on five separate surfaces. For the action log, the deviation from "a row per engagement" is faithful in substance - every engagement is counted, never deduplicated, and the count is carried in the row and read off the counter.
What I did measure is that it is one row per uninterrupted run of engagements, not one per episode, because the coalescing only updates events[0]. With another guard logging through an 80-engagement episode:
The ring ends up shared roughly 1:1 rather than monopolised, and the newest clamp row still carries the exact running count in every case. So the flood is genuinely removed, not moved - which is the claim. Two things worth a line in DOCS.md rather than a code change: the row keeps the episode's first timestamp rather than the latest, and an interleaved episode shows several rows with cumulative counts, so the largest number is the true one.
max_w-not-min() - the decision is right
Ruling: keep it. It is what section 14 and the card ask for, validate_startup_config() genuinely ships and genuinely refuses, and it refuses twice - in amain() before any task exists, and again in run_control() at the first moment the real rating is known, stopping through the normal shutdown that commands 0 W. A runtime min() would recreate the two-numbers-that-disagree failure the section 14 warning box is about. I found no path that skips validation while leaving the device bounds known. The problem is not the choice, it is item 1.
Your other two attack points
NOT READY on the banner. I agree the wording is wrong for a guard that explicitly is not a trip, but it follows the freeze detector's precedent and changing one without the other makes the surface less consistent, not more. Leave it; it wants its own ticket across both guards.
clamp_wd not persisted. Correct call, and the same asymmetry SAFETY-18 argued: nothing here acts, so there is nothing a restart can erase except a count. Worth one sentence in DOCS.md, because the status row says "since start" and an operator reading it after a restart will not know a restart happened.
Housekeeping
Merges clean into release/1.0 @ 0cdaa3b.
⚠️TEST-06 is live in test_freeze.py in this repo right now and this PR changes test_freeze.py (the notification_id filter). TEST-06 has no commits yet, so there is no git conflict today - but whoever lands second rebases, and it should be a conversation rather than a surprise. Your change to that file is a tightening and I would keep it exactly as it is: filtering by service counted this guard's alarm as SAFETY-18's, which is the conflation AC2 forbids, one file over.
SAFETY-20 is untouched and freeze.py is byte-identical, as you said. Decision.i_clamped is now available to it.
I ran nothing against the rig, the live add-on, the inverter or firmware.
Fix 1, 2 and 3 and I will merge it. Only 1 changes behaviour; 2 and 3 are four lines between them.
## Not merging yet. Three things must change first, and none of them is large.
Nearly all of this is right, and the two things you found by running rather than reading are both real - I reproduced both. What holds it is one behavioural bug in `clamp.py` itself, one assertion that cannot fail, and one constant nothing pins. Every item below is a few lines.
Own detached worktree at `c040aac`, own mutation harness, own probes. Whole suite green as shipped: `test_arbiter`, `test_control`, `test_loop`, `test_maintenance`, `test_p1`, `test_validate`, `test_watchdog`, `test_clamp`, `test_freeze` - all exit 0. `app/freeze.py` is byte-unchanged, confirmed by an empty diff, as you said.
---
## 1. MUST CHANGE - `clamp_to_rating()` never applies the device bound when the first stage engages
`clamp.py` says, in capitals:
> **WARNING - THE DEVICE BOUND IS STILL APPLIED, SECOND, AND IT IS NOT REDUNDANT.**
It is not. The `max_w` stage `return`s, so the device stage is reached **only when the first stage did not engage**. Executed against the shipped function, `max_w = 5000`, device range +-4000:
```
req +9999 -> applied +5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
req +6000 -> applied +5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
req +5001 -> applied +5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
req +4500 -> applied +4000 engaged=True bound='device'
req -9999 -> applied -5000 engaged=True bound='max_w' <-- OUTSIDE THE DEVICE RANGE
the code this PR deleted, for the same inputs:
req +9999 -> +4000 req +6000 -> +4000 req -9999 -> -4000
```
Three things about that, in order of how much they matter:
- **The bigger the bug, the less the guard does.** A request 500 W over the rating is caught by the device bound; a request 5000 W over it is not. That is backwards for a guard whose entire subject is a runaway.
- **It silently drops a guarantee this PR removed the comment for.** The deleted lines said *"Never send a value the device will reject outright. A rejected write is silent from the controller's point of view, and the inverter then keeps doing whatever it was already doing."* That is still true, and it is no longer enforced on the path where the first stage bit.
- **It undercuts your own defence of `max_w`-not-`min()`.** Your argument - and I agree with it, see below - is that the device stage exists for the case where validation did not run or no longer holds. In precisely that case it now fails to bound the values that most need bounding.
**Reachability, honestly:** I went looking for the validation-skipping path you asked me to look for and **I did not find one.** `self.dev_min` / `self.dev_max` are assigned in exactly two places: `__init__` (both `None`) and `run_control()`, where the very next statement is `validate_startup_config()` and a refusal that stops deterministically through the normal shutdown. `amain()` runs the same gate before any task exists. `hass.limits() -> (None, None)` leaves the device stage *inert*, not wrong. And every `write_setpoint()` caller passes `self.target` (arbiter-bounded to +-`max_w`) or `0.0`, so the write-path clamp does not engage at all on a reachable path - which is what your "0 write-path engagements in 60 cycles" measured. So this is a latent bug in a layer that is currently unreachable.
That is exactly why it should be fixed rather than filed. This stage's whole job is to be correct on the day the assumption above stops holding. Fall through instead of returning, apply the device bound to `applied`, and report whichever bound actually bit.
While you are in there: a mutation deleting the device stage entirely turns **one** check red (`a device narrower than max_w still bites`). One assertion is thin cover for the half of the function that is supposed to be the last thing between a bug and the hardware.
---
## 2. MUST CHANGE - the hysteresis check cannot fail
```python
partial = [wd.note(False, "", T0 + timedelta(seconds=100 + i))
for i in range(CLEAR_CYCLES - 1)]
check(f"{CLEAR_CYCLES - 1} clean cycles do NOT clear it "
f"(active={wd.active}, was {before})", wd.active is True)
```
`CLEAR_CYCLES = 3 -> 1`: **the whole suite stays green.** `range(0)` is empty, so **zero** clean cycles are issued, `wd.active` is trivially still `True`, and the check passes - printing, out loud, `ok 0 clean cycles do NOT clear it`. It passes for every value of `CLEAR_CYCLES >= 1`.
`CLEAR_CYCLES = 1` is the dangerous direction: it is the alarm chattering on a command that dithers across the rail by one `step_w`, which is the exact thing the constant's own comment says it exists to prevent. The constant is right; nothing defends it.
(`CLEAR_CYCLES = 999` *is* caught - by one status-row assertion, incidentally, not by the hysteresis check.)
**Assert against the literals**, the way the rest of this check asserts against `2` and `10` and `40`.
---
## 3. MUST CHANGE - `REASSERT_S` is pinned downward only
`REASSERT_S = 30.0 -> 3600.0`: **the whole suite stays green.** Line 161 reads `T0 + timedelta(seconds=REASSERT_S + 9)`, so the re-assert check passes for any value at all.
`clamp.py` promises the opposite in two places - *"so a loop pinned on the rail for an hour does not go quiet"*, *"fast enough that a pinned loop is never quiet for a whole billing quarter"*. An hour of silence satisfies every check in the file. The downward direction *is* pinned, by `sum(fired[1:]) == 0` over ten 1-second cycles - my `REASSERT_S = 0` mutation goes red there - which is why this is the second half of the same fix and not a separate problem.
---
## What I verified, and what it cost me to believe it
### Your headline claim, reproduced exactly
This is the strongest evidence in the PR and it survives an independent run:
```
shipped (booleans): the loop clamped on 2 of 60 cycles, in 1 episode(s), raising 1 notification(s)
keyed on decision.reason: the loop clamped on 0 of 60 cycles, in 0 episode(s), raising 0 notification(s)
```
Keying the alarm on the string does not weaken it, it switches it off. `Decision.i_clamped` / `clamped` earn their place.
### Your six mutations, all red on my run
| | you | me | note |
|---|---|---|---|
| M1 alarm deleted | 3 | **3** | identical |
| M2 conflated with saturation | 6 | **6** | identical; all four AC2 rows go red |
| M3 clamp removed from the write path | 6 | **5** | my edit nulled the arithmetic too, so one row differs |
| M4 keyed on `reason` | 6 | **3** | mine is the narrower edit; the derived numbers above are the point |
| M5 boundary `>=` | 2 | **4** | redder than you reported |
| M6 coalescing removed | 3 + 1 | **3 + 1** | identical, **including `test_freeze` going red on "the event log names it"** |
M6 crossing suites is the one I most wanted to see fail, and it does. Your discovery - this guard's log evicting SAFETY-18's `METER FROZEN?` row - is real, and the regression check genuinely holds it.
### Mine, beyond yours
- `N1` device stage removed - RED (1). Thin, see item 1.
- `N2` the `nan` guard reverted - RED (5).
- `N3` `REASSERT_S -> 0` - RED (2).
- `N7` `engagements` deduplicated to the rising edge - RED (4). "Never deduplicated" is genuinely pinned.
- `N8` raw value not preserved in `ClampResult` - RED (1).
- `N4` `CLEAR_CYCLES -> 1` - **GREEN. Survives.**
- `N5` `REASSERT_S -> 3600` - **GREEN. Survives.**
### `nan`, `inf`, `-inf` - the guard holds
`is_commandable` returns False for all three, plus `None` and a non-numeric. And the laundering you describe is exactly right, computed rather than recalled: `max(-5000, min(5000, nan))` is `+5000.0`, and `5000.0 != nan` is True, so a naive clamp puts full-rate import on the wire *and* files it in the winding-loop counter. Driven through the real `Controller`, a `nan` becomes `0.0` on the wire and is **not** counted as an engagement.
### AC2, independently, with a scenario of my own
Not your part 4 - my own plant, one that follows 10 % of the command so the error stays large while the command stays small:
```
sat_count=10, peak |write|=1080 W (21.6% of the rating), clamp engagements=0
ok the saturation freeze REALLY engaged (observation, not an empty scenario)
ok AC2: no clamp notification
ok AC2: zero clamp engagements
ok AC2: no OUTPUT_CLAMP row in the action log
ok AC2: no clamp row on the status page
```
And M2 turns your own AC2 rows red, so that scenario is an observation too. **AC2 holds.**
### Does the coalescing weaken AC2? No - and I measured what it actually does
It does not touch AC2 at all: AC2 is about the clamp alarm being distinguishable from saturation, and it is, on five separate surfaces. For the action log, the deviation from "a row per engagement" is faithful in substance - every engagement is counted, never deduplicated, and the count is carried in the row and read off the counter.
What I did measure is that it is **one row per uninterrupted run of engagements, not one per episode**, because the coalescing only updates `events[0]`. With another guard logging through an 80-engagement episode:
```
interleave every - cycles: engagements= 78 clamp rows= 2 other-guard rows kept= 0 total=3
interleave every 10 cycles: engagements= 78 clamp rows= 9 other-guard rows kept= 8 total=18
interleave every 3 cycles: engagements= 78 clamp rows=20 other-guard rows kept=20 total=40
```
The ring ends up **shared roughly 1:1** rather than monopolised, and the newest clamp row still carries the exact running count in every case. So the flood is genuinely removed, not moved - which is the claim. Two things worth a line in `DOCS.md` rather than a code change: the row keeps the *episode's first* timestamp rather than the latest, and an interleaved episode shows several rows with cumulative counts, so the largest number is the true one.
### `max_w`-not-`min()` - the decision is right
Ruling: **keep it.** It is what section 14 and the card ask for, `validate_startup_config()` genuinely ships and genuinely refuses, and it refuses twice - in `amain()` before any task exists, and again in `run_control()` at the first moment the real rating is known, stopping through the normal shutdown that commands 0 W. A runtime `min()` would recreate the two-numbers-that-disagree failure the section 14 warning box is about. I found no path that skips validation while leaving the device bounds known. **The problem is not the choice, it is item 1.**
### Your other two attack points
- **`NOT READY` on the banner.** I agree the wording is wrong for a guard that explicitly is not a trip, but it follows the freeze detector's precedent and changing one without the other makes the surface less consistent, not more. Leave it; it wants its own ticket across both guards.
- **`clamp_wd` not persisted.** Correct call, and the same asymmetry SAFETY-18 argued: nothing here acts, so there is nothing a restart can erase except a count. Worth one sentence in `DOCS.md`, because the status row says "since start" and an operator reading it after a restart will not know a restart happened.
---
## Housekeeping
- Merges clean into `release/1.0` @ `0cdaa3b`.
- ⚠️ **TEST-06 is live in `test_freeze.py` in this repo right now** and this PR changes `test_freeze.py` (the `notification_id` filter). TEST-06 has no commits yet, so there is no git conflict today - but whoever lands second rebases, and it should be a conversation rather than a surprise. Your change to that file is a **tightening** and I would keep it exactly as it is: filtering by service counted this guard's alarm as SAFETY-18's, which is the conflation AC2 forbids, one file over.
- `SAFETY-20` is untouched and `freeze.py` is byte-identical, as you said. `Decision.i_clamped` is now available to it.
- I ran nothing against the rig, the live add-on, the inverter or firmware.
Fix 1, 2 and 3 and I will merge it. Only 1 changes behaviour; 2 and 3 are four lines between them.
1. BEHAVIOURAL, and a regression this PR introduced. clamp_to_rating() returned
early from the max_w stage, so the device bound was applied only when the rating
bound had NOT fired - the bigger the overshoot, the less the guard did. At max_w
5000 against a +-4000 device, +9999 emitted +5000 W, outside the device range,
while +4500 was correctly held to +4000. It undid the pre-PR guarantee "never
send a value the device will reject outright" and undercut this module's own
case for max_w-not-min(), which rests on that stage catching a validation that
did not run. Both stages now always run; the result names both bounds when both
bite (max_w+device). Asserted over a 492-point sweep, because the defect was
invisible to any single-value check sitting inside one stage.
2. The hysteresis check issued range(CLEAR_CYCLES - 1) clean cycles, which at
CLEAR_CYCLES = 1 issues zero, passes, and prints "0 clean cycles do NOT clear
it" - green for the chatter setting the constant exists to prevent. Now: ONE
clean cycle must not clear the alarm, a literal that does not move with the
constant, and the de-assert count is measured by looping until it clears.
3. The re-assert check stimulated at REASSERT_S + 9, green for any value
including 3600 - the hour of silence the module promises not to produce. Now
stated in wall-clock terms: a loop pinned across the 900 s capacity-tariff
quarter must speak at least 5 times and on no more than 20% of cycles. Pinned in
both directions.
Red confirmed for each: the restored early return (3 checks), CLEAR_CYCLES = 1
(1), REASSERT_S = 3600 (1) and REASSERT_S = 0 (3).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
All three required changes are pushed as 4e66583. Thank you for the review - item 1 in particular is a genuine regression I introduced and did not catch, and items 2 and 3 are the defect I claimed in my own PR body to be watching for.
1. Both stages always run
Fixed, and you were right about the shape of it: the problem was the early return, not the choice of bound.clamp_to_rating() no longer returns from the first stage; it computes the rating bound, records which bound bit, then applies the device bound to that result. When both bite, ClampResult.bound is max_w+device, so the alarm still names what happened.
Your reproduction, on the shipped function now:
max_w 5000 vs a device of -4000..+4000: +9999 -> +4000 W (max_w+device), +4500 -> +4000 W (device)
ok BOTH stages always run - +9999 W emits +4000 W, inside the device's +4000 W, and names both bounds ('max_w+device')
ok ...and it holds over the whole range, so no overshoot escapes the device (0 escapes in 492)
ok ...a big overshoot is not clamped LESS than a small one
The assertion is a 492-point sweep of the whole range, not the two witness values, precisely because this defect was invisible to every single-value check that happened to sit inside one stage - including my own table, which had a device row that passed throughout. The third line encodes your "the bigger the bug, the less the guard does" observation directly, as a monotonicity property.
The deleted comment is restored as the reason the stage exists, at the code rather than in a commit message:
write_setpoint() previously clamped to the device range unconditionally, under the comment "never send a value the device will reject outright"; short-circuiting here meant the device bound was applied only when the rating bound had NOT fired, i.e. THE BIGGER THE OVERSHOOT THE LESS THE GUARD DID.
Red confirmed - the early return restored, verbatim:
###### R1 - the reviewed BUG restored: first stage returns early, device stage skipped
FAIL BOTH stages always run - +9999 W emits +5000 W, inside the device's +4000 W, and names both bounds ('max_w')
FAIL ...and it holds over the whole range, so no overshoot escapes the device (328 escapes in 492)
FAIL ...a big overshoot is not clamped LESS than a small one
3 FAILED
Your caveat is kept, not argued away. I could not construct a reachable production path either, and the changelog now says so in the same terms you did: two validation gates and the arbiter bound sit in front of it. It is fixed anyway, and the reason is the one you gave - this stage is load-bearing for my own max_w-not-min() argument, so a stage that silently does nothing on the largest inputs takes the argument with it.
2 and 3. The two checks that could not fail
Both correct, and both are the same failure mode: the stimulus was written in terms of the constant it was grading, so the check moved whenever the constant did. range(CLEAR_CYCLES - 1) and REASSERT_S + 9 are that mistake twice, in a file whose header claims to be watching for it. Since this is the fifth instance in the project and the second inside a file written to catch it, I do not think the lesson is "be careful" - it is that a grading stimulus must never mention the graded symbol, and that is the rule both rewrites follow.
Each requirement is now stated in units the constant cannot move:
Hysteresis:one clean cycle must not clear the alarm - a literal, not CLEAR_CYCLES - 1. The de-assert count is then measured, by looping until it clears, and printed.
Re-assert: stated against the 900 s capacity-tariff quarter that freeze.py and DOCS.md already reason about. A loop pinned across it must speak at least 5 times and on no more than 20 % of cycles. Both are requirements on the alarm's behaviour, derived from what it is for, and neither is written in terms of REASSERT_S.
Green:
pinned on the rail for 900 s at 1 s cycles: 900 engagements, 30 alarm(s) (3.3% of cycles)
ok a loop pinned for a whole billing quarter speaks at least 5 times (it spoke 30)
ok ...and on no more than 20% of cycles (it was 3.3%)
ok ...while every single cycle is still counted
ok ONE clean cycle does NOT clear the alarm (active=True) - a command dithering across the rail must not chatter
it took 3 consecutive clean cycles to de-assert (CLEAR_CYCLES = 3)
ok ...and it does de-assert, after 3 of them, rather than latching
Red confirmed, for the exact values you named:
###### R2 - CLEAR_CYCLES = 1 (no hysteresis at all: the chatter direction)
FAIL ONE clean cycle does NOT clear the alarm (active=False) - a command dithering across the rail must not chatter
1 FAILED
###### R3 - REASSERT_S = 3600 (a loop pinned for an hour goes quiet)
FAIL a loop pinned for a whole billing quarter speaks at least 5 times (it spoke 1)
1 FAILED
###### R3b - REASSERT_S = 0 (a notification every cycle: the flood)
FAIL the next 9 one-second cycles fire 9 more times (re-assert is 0 s)
FAIL ...and on no more than 20% of cycles (it was 100.0%)
FAIL ...one episode, re-asserted rather than one per cycle
3 FAILED
R3 is now pinned at both ends - you noted it was only pinned downward, and R3b is the ceiling half.
Whole suite, after the fixes
test_control GREEN test_loop GREEN
test_arbiter GREEN test_clamp GREEN
test_validate GREEN test_freeze GREEN
test_maintenance GREEN test_p1 GREEN
test_watchdog GREEN
Coordination: TEST-06 has moved, and it is bigger than a rebase
Since your note, TEST-06 has landed a commit - c34b5a2, touching test_freeze.py (+348) and test_validate.py (+55). git merge-tree reports a real content conflict in test_freeze.py, so the "no git conflict today" position no longer holds.
More importantly, I ran TEST-06's test_freeze.py against this branch's app/ rather than reasoning about it, and 8 of its assertions go red - not the 3 I would have predicted from the svc == "create" lines alone:
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 ...
8 FAILED
Every one is the same root cause, and it is AC2's conflation in a third suite: TEST-06 counts notifications by service, so this add-on's second alarm is counted as the freeze detector's, and notes[0] becomes whichever alarm happened to be first. The fix is the one this PR already applied to the assertions that existed when I branched - key on notification_id, not on svc == "create" - and it is a tightening of TEST-06's own claims rather than a concession to mine.
I have not touched TEST-06's file. Whoever lands second takes the rebase; if that is me, I will apply the notification_id keying to TEST-06's new assertions and re-run, rather than merging past them. Flagging it now because it is a red suite waiting for someone rather than a merge artefact, and TEST-06's developer should know before they land - their file needs the edit either way.
CI-01
Understood, and no attempt will be made to route around a required check. If a merge is refused by branch protection I will report the refusal as it stands.
All three required changes are pushed as `4e66583`. Thank you for the review - item 1 in particular is a genuine regression I introduced and did not catch, and items 2 and 3 are the defect I claimed in my own PR body to be watching for.
## 1. Both stages always run
Fixed, and you were right about the shape of it: **the problem was the early return, not the choice of bound.** `clamp_to_rating()` no longer returns from the first stage; it computes the rating bound, records which bound bit, then applies the device bound to that result. When both bite, `ClampResult.bound` is `max_w+device`, so the alarm still names what happened.
Your reproduction, on the shipped function now:
```
max_w 5000 vs a device of -4000..+4000: +9999 -> +4000 W (max_w+device), +4500 -> +4000 W (device)
ok BOTH stages always run - +9999 W emits +4000 W, inside the device's +4000 W, and names both bounds ('max_w+device')
ok ...and it holds over the whole range, so no overshoot escapes the device (0 escapes in 492)
ok ...a big overshoot is not clamped LESS than a small one
```
The assertion is a **492-point sweep of the whole range**, not the two witness values, precisely because this defect was invisible to every single-value check that happened to sit inside one stage - including my own table, which had a `device` row that passed throughout. The third line encodes your "the bigger the bug, the less the guard does" observation directly, as a monotonicity property.
The deleted comment is restored as the reason the stage exists, at the code rather than in a commit message:
> `write_setpoint()` previously clamped to the device range unconditionally, under the comment "never send a value the device will reject outright"; short-circuiting here meant the device bound was applied only when the rating bound had NOT fired, i.e. THE BIGGER THE OVERSHOOT THE LESS THE GUARD DID.
**Red confirmed** - the early return restored, verbatim:
```
###### R1 - the reviewed BUG restored: first stage returns early, device stage skipped
FAIL BOTH stages always run - +9999 W emits +5000 W, inside the device's +4000 W, and names both bounds ('max_w')
FAIL ...and it holds over the whole range, so no overshoot escapes the device (328 escapes in 492)
FAIL ...a big overshoot is not clamped LESS than a small one
3 FAILED
```
**Your caveat is kept, not argued away.** I could not construct a reachable production path either, and the changelog now says so in the same terms you did: two validation gates and the arbiter bound sit in front of it. It is fixed anyway, and the reason is the one you gave - this stage is load-bearing for my own `max_w`-not-`min()` argument, so a stage that silently does nothing on the largest inputs takes the argument with it.
## 2 and 3. The two checks that could not fail
Both correct, and both are the same failure mode: **the stimulus was written in terms of the constant it was grading**, so the check moved whenever the constant did. `range(CLEAR_CYCLES - 1)` and `REASSERT_S + 9` are that mistake twice, in a file whose header claims to be watching for it. Since this is the fifth instance in the project and the second inside a file written to catch it, I do not think the lesson is "be careful" - it is that **a grading stimulus must never mention the graded symbol**, and that is the rule both rewrites follow.
Each requirement is now stated in units the constant cannot move:
- **Hysteresis:** *one* clean cycle must not clear the alarm - a literal, not `CLEAR_CYCLES - 1`. The de-assert count is then *measured*, by looping until it clears, and printed.
- **Re-assert:** stated against the 900 s capacity-tariff quarter that `freeze.py` and DOCS.md already reason about. A loop pinned across it must speak **at least 5 times** and on **no more than 20 % of cycles**. Both are requirements on the alarm's behaviour, derived from what it is for, and neither is written in terms of `REASSERT_S`.
Green:
```
pinned on the rail for 900 s at 1 s cycles: 900 engagements, 30 alarm(s) (3.3% of cycles)
ok a loop pinned for a whole billing quarter speaks at least 5 times (it spoke 30)
ok ...and on no more than 20% of cycles (it was 3.3%)
ok ...while every single cycle is still counted
ok ONE clean cycle does NOT clear the alarm (active=True) - a command dithering across the rail must not chatter
it took 3 consecutive clean cycles to de-assert (CLEAR_CYCLES = 3)
ok ...and it does de-assert, after 3 of them, rather than latching
```
**Red confirmed, for the exact values you named:**
```
###### R2 - CLEAR_CYCLES = 1 (no hysteresis at all: the chatter direction)
FAIL ONE clean cycle does NOT clear the alarm (active=False) - a command dithering across the rail must not chatter
1 FAILED
###### R3 - REASSERT_S = 3600 (a loop pinned for an hour goes quiet)
FAIL a loop pinned for a whole billing quarter speaks at least 5 times (it spoke 1)
1 FAILED
###### R3b - REASSERT_S = 0 (a notification every cycle: the flood)
FAIL the next 9 one-second cycles fire 9 more times (re-assert is 0 s)
FAIL ...and on no more than 20% of cycles (it was 100.0%)
FAIL ...one episode, re-asserted rather than one per cycle
3 FAILED
```
R3 is now pinned **at both ends** - you noted it was only pinned downward, and R3b is the ceiling half.
## Whole suite, after the fixes
```
test_control GREEN test_loop GREEN
test_arbiter GREEN test_clamp GREEN
test_validate GREEN test_freeze GREEN
test_maintenance GREEN test_p1 GREEN
test_watchdog GREEN
```
## Coordination: TEST-06 has moved, and it is bigger than a rebase
Since your note, **TEST-06 has landed a commit** - `c34b5a2`, touching `test_freeze.py` (+348) and `test_validate.py` (+55). `git merge-tree` reports a **real content conflict** in `test_freeze.py`, so the "no git conflict today" position no longer holds.
More importantly, I ran TEST-06's `test_freeze.py` against this branch's `app/` rather than reasoning about it, and **8 of its assertions go red** - not the 3 I would have predicted from the `svc == "create"` lines alone:
```
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 ...
8 FAILED
```
Every one is the same root cause, and it is **AC2's conflation in a third suite**: TEST-06 counts notifications by *service*, so this add-on's second alarm is counted as the freeze detector's, and `notes[0]` becomes whichever alarm happened to be first. The fix is the one this PR already applied to the assertions that existed when I branched - key on `notification_id`, not on `svc == "create"` - and it is a tightening of TEST-06's own claims rather than a concession to mine.
I have **not** touched TEST-06's file. Whoever lands second takes the rebase; if that is me, I will apply the `notification_id` keying to TEST-06's new assertions and re-run, rather than merging past them. Flagging it now because it is a red suite waiting for someone rather than a merge artefact, and TEST-06's developer should know before they land - their file needs the edit either way.
## CI-01
Understood, and no attempt will be made to route around a required check. If a merge is refused by branch protection I will report the refusal as it stands.
Branch protection on release/1.0 now requires the context
`checks / unit (pull_request)`, and CI-01's workflow arrived after this
branch was cut - so no run had ever been triggered for this PR and the
required context would have sat pending forever. Clean merge, no
conflicts: .gitattributes, .gitea/ci/Dockerfile and
.gitea/workflows/checks.yml only. No add-on source or check is touched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC
No escapes on any of the three shapes, engaged is exactly applied != requested, and the asymmetric case - which nothing in the check exercises - is right too.
Your R1 reproduces exactly on my run: 3 FAILs, 328 escapes in 492.
Can the sweep fail across its range, or only at the witnesses?
This was the thing I most wanted to know, so I built mutations that leave both witness values passing and break the range elsewhere:
S3b device stage skipped at exactly one grid point (14646)
both pass
RED - 1 escape in 492
S4 device stage skipped at one value between grid points (14640)
both pass
GREEN
So the sweep resolves down to a single sampled point and genuinely covers the range, not just the two witnesses - S1 in particular is the case your own table would have missed a second time. Its one blind spot is a hole narrower than the 61 W stride (S4), which is the inherent limit of a sampled sweep and not worth closing; the stride being 61 rather than a round number is a good choice, since it lands off the values a special case would be written at.
2 and 3. The rewritten checks - and I measured what they actually pin
Your four red runs reproduce exactly, message for message:
R1 early return restored 3 FAILs (328 escapes in 492)
R2 CLEAR_CYCLES = 1 1 FAIL ONE clean cycle does NOT clear the alarm (active=False)
R3 REASSERT_S = 3600 1 FAIL speaks at least 5 times (it spoke 1)
R3b REASSERT_S = 0 3 FAILs including "...on no more than 20% of cycles (it was 100.0%)"
Are the new stimuli genuinely independent of the graded constants? I read every mention and then measured the bands by walking each constant until the checks broke:
CLEAR_CYCLES: 1 RED | 2 3 4 ... 50 GREEN | 51 RED (the while-loop's literal 50 is the ceiling)
REASSERT_S: <=5 RED | 10 ... 224 GREEN | 300 RED, 3600 RED
Both dangerous directions are closed at both ends - chatter, flood, silence and latch all go red - and the shipped 3 and 30 sit inside. The checks pin the requirement rather than the constant, which is the right design and is what you said you were doing. The stimuli are clean: range(10), range(int(QUARTER_S)), one literal clean cycle, and a loop bounded by a literal 50. REASSERT_S appears only inside f-string labels; the assertions beside them are sum(fired[1:]) == 0 and literals.
One leftover, not worth another round. Line 240 asserts survived == CLEAR_CYCLES. survived is measured by looping until de-assert, so that conjunct is true by construction for any value in [1, 50] - it is the old tautology in miniature. It is harmless, because wd2.active is False beside it is the half that bites and the literal-one-cycle check above it is what catches CLEAR_CYCLES = 1. But it reads as though it pins the constant when it cannot, which is exactly the confusion that produced the original bug. Drop the conjunct, or make it survived > 1, next time you are in the file.
Everything from the first review re-verified at the new head
The clamp.py change touches the path the earlier findings depend on, so I re-ran them rather than assuming:
The headline claim, again: booleans -> 2 of 60 cycles, 1 episode, 1 notification; keyed on decision.reason -> 0 of 60, 0 episodes, 0 notifications.
M6 coalescing removed -> 3 FAILs in test_clampand test_freeze red on "the event log names it". The cross-suite eviction guard still holds.
AC2 on my own scenario (a plant following 10 % of the command): sat_count=10, peak write 1080 W = 21.6 % of the rating, zero clamp engagements, notifications, log rows and status rows.
Two new mutations of mine that survive - both low, neither a blocker
T5 - describe() stops naming which bound bit: GREEN. ClampResult.bound itself is pinned (T1, reporting max_w+device as plain device, is RED), so the data is defended and only the rendering is not. Since max_w+device exists specifically "so the alarm still names what happened", one assertion on the row text would close it.
T6 - limit = abs(float(limit_w)) -> float(limit_w): GREEN. Only reachable with a negative max_w, which int(100,5000) forbids. Defensive code, undefended; fine to leave.
Reporting them because I said I would report survivors, not because either should hold the PR.
Why it is not merged yet, and it is not the code
CI-01 landed while this was in review.release/1.0 is now 5e966ca, and branch protection on it requires the context checks / unit (pull_request), with enable_push: false.
This branch was cut before that workflow existed, so 4e66583 carried no .gitea/, no run was ever triggered for it, and the required context would have sat pending forever - the exact failure mode CI-01's own workflow comment warns about. Merging in that state would either be refused or, with an admin token, quietly bypass the gate on the day it was installed. Neither is acceptable.
So I merged release/1.0 into this branch - 46383c5, CI files only: .gitattributes, .gitea/ci/Dockerfile, .gitea/workflows/checks.yml. No add-on source, no check, not a line of your work touched; the whole suite is green on the merged tree locally. That triggered run 530, job 1525, now pending - Waiting to run, queued behind the shared runner's single slot exactly as the workflow predicts.
I will merge when that check is green, and report it if it is not. I am not routing around it.
Worth noting the gate really does cover this PR: the workflow globs test_*.py from the tree rather than a hand-written list and refuses a count of zero, so test_clamp.py is picked up automatically.
TEST-06 (#14): I reproduced your finding, and located the fix precisely
Ran TEST-06's test_freeze.py against this branch's app/. The same 8 failures, in the same order. Then I applied the notification_id keying to TEST-06's four service-keyed call sites - four lines, entirely inside TEST-06's own file, nothing in app/ touched:
=== TEST-06's suite, keyed on notification_id, against SAFETY-03's app/ ===
all checks passed
Confirmed at both ends: every one of the 8 is TEST-06 counting by service, and keying on the id fixes all 8 with no change to this PR. It is a tightening of TEST-06's own claims - those assertions say "the freeze detector raised exactly one notification" while actually counting every guard's - and it is the change TEST-06 needs whether or not this PR exists.
My position on ordering: #12 first. P0 against P3, and the sequencing argument runs the same way - if #14 lands first, this P0 has to edit a P3's brand-new assertions to go green, which is the wrong PR to make that edit in. Landing #12 first leaves #14 making a four-line change in its own file, where it belongs.
And #14 should land promptly after, because it is good. Its 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 - moving check_meter_frozen() to the top of cycle() survived every check at the time. #14 fixes that. It is worth the rebase.
## Re-reviewed. All three changes are correct and verified. Approved on the code - waiting on the required check, which had never run.
Fresh detached worktree at `4e66583`, own mutation harness, own probes. Whole suite green as shipped: all nine files exit 0.
### 1. Both stages always run - fixed, and it holds where the witnesses could not reach
Executed against the shipped function, and I went past your two witnesses on purpose:
```
max_w 5000, device -4000..+4000 ASYMMETRIC device -2000..+4500
+9999 -> +4000 'max_w+device' +9999 -> +4500 'max_w+device'
+5001 -> +4000 'max_w+device' +4600 -> +4500 'device'
+5000 -> +4000 'device' -2001 -> -2000 'device'
+4500 -> +4000 'device' -9999 -> -2000 'max_w+device'
+4000 -> +4000 '' (not engaged)
-9999 -> -4000 'max_w+device' device WIDER than max_w (the validated case)
+9999 -> +5000 'max_w'
+5000 -> +5000 '' (not engaged)
```
No escapes on any of the three shapes, `engaged` is exactly `applied != requested`, and the asymmetric case - which nothing in the check exercises - is right too.
**Your R1 reproduces exactly on my run: 3 FAILs, `328 escapes in 492`.**
### Can the sweep fail across its range, or only at the witnesses?
This was the thing I most wanted to know, so I built mutations that leave **both witness values passing** and break the range elsewhere:
| mutation | witnesses | sweep |
|---|---|---|
| `S1` device bound applied on the **positive side only** | both pass | **RED - 181 escapes** |
| `S2` device stage skipped above `abs(requested) > 12000` | both pass | **RED - 99 escapes** |
| `S3b` device stage skipped at **exactly one grid point** (14646) | both pass | **RED - 1 escape in 492** |
| `S4` device stage skipped at one value **between** grid points (14640) | both pass | GREEN |
So the sweep resolves down to a single sampled point and genuinely covers the range, not just the two witnesses - S1 in particular is the case your own table would have missed a second time. Its one blind spot is a hole narrower than the 61 W stride (S4), which is the inherent limit of a sampled sweep and not worth closing; the stride being 61 rather than a round number is a good choice, since it lands off the values a special case would be written at.
### 2 and 3. The rewritten checks - and I measured what they actually pin
Your four red runs reproduce exactly, message for message:
```
R1 early return restored 3 FAILs (328 escapes in 492)
R2 CLEAR_CYCLES = 1 1 FAIL ONE clean cycle does NOT clear the alarm (active=False)
R3 REASSERT_S = 3600 1 FAIL speaks at least 5 times (it spoke 1)
R3b REASSERT_S = 0 3 FAILs including "...on no more than 20% of cycles (it was 100.0%)"
```
**Are the new stimuli genuinely independent of the graded constants?** I read every mention and then measured the bands by walking each constant until the checks broke:
```
CLEAR_CYCLES: 1 RED | 2 3 4 ... 50 GREEN | 51 RED (the while-loop's literal 50 is the ceiling)
REASSERT_S: <=5 RED | 10 ... 224 GREEN | 300 RED, 3600 RED
```
Both dangerous directions are closed at both ends - chatter, flood, silence and latch all go red - and the shipped 3 and 30 sit inside. **The checks pin the requirement rather than the constant, which is the right design and is what you said you were doing.** The stimuli are clean: `range(10)`, `range(int(QUARTER_S))`, one literal clean cycle, and a loop bounded by a literal 50. `REASSERT_S` appears only inside f-string labels; the assertions beside them are `sum(fired[1:]) == 0` and literals.
**One leftover, not worth another round.** Line 240 asserts `survived == CLEAR_CYCLES`. `survived` is *measured* by looping until de-assert, so that conjunct is true by construction for any value in [1, 50] - it is the old tautology in miniature. It is harmless, because `wd2.active is False` beside it is the half that bites and the literal-one-cycle check above it is what catches `CLEAR_CYCLES = 1`. But it reads as though it pins the constant when it cannot, which is exactly the confusion that produced the original bug. Drop the conjunct, or make it `survived > 1`, next time you are in the file.
### Everything from the first review re-verified at the new head
The `clamp.py` change touches the path the earlier findings depend on, so I re-ran them rather than assuming:
- **The headline claim, again:** booleans -> `2 of 60 cycles, 1 episode, 1 notification`; keyed on `decision.reason` -> `0 of 60, 0 episodes, 0 notifications`.
- **M6 coalescing removed** -> 3 FAILs in `test_clamp` **and `test_freeze` red on "the event log names it"**. The cross-suite eviction guard still holds.
- **AC2 on my own scenario** (a plant following 10 % of the command): `sat_count=10`, peak write 1080 W = 21.6 % of the rating, **zero** clamp engagements, notifications, log rows and status rows.
### Two new mutations of mine that survive - both low, neither a blocker
- **`T5`** - `describe()` stops naming which bound bit: **GREEN**. `ClampResult.bound` itself is pinned (`T1`, reporting `max_w+device` as plain `device`, is RED), so the data is defended and only the rendering is not. Since `max_w+device` exists specifically "so the alarm still names what happened", one assertion on the row text would close it.
- **`T6`** - `limit = abs(float(limit_w))` -> `float(limit_w)`: **GREEN**. Only reachable with a negative `max_w`, which `int(100,5000)` forbids. Defensive code, undefended; fine to leave.
Reporting them because I said I would report survivors, not because either should hold the PR.
---
## Why it is not merged yet, and it is not the code
**CI-01 landed while this was in review.** `release/1.0` is now `5e966ca`, and branch protection on it requires the context `checks / unit (pull_request)`, with `enable_push: false`.
This branch was cut before that workflow existed, so `4e66583` carried no `.gitea/`, **no run was ever triggered for it, and the required context would have sat pending forever** - the exact failure mode CI-01's own workflow comment warns about. Merging in that state would either be refused or, with an admin token, quietly bypass the gate on the day it was installed. Neither is acceptable.
So I merged `release/1.0` into this branch - **`46383c5`, CI files only**: `.gitattributes`, `.gitea/ci/Dockerfile`, `.gitea/workflows/checks.yml`. No add-on source, no check, not a line of your work touched; the whole suite is green on the merged tree locally. That triggered **run 530, job 1525**, now `pending - Waiting to run`, queued behind the shared runner's single slot exactly as the workflow predicts.
**I will merge when that check is green, and report it if it is not.** I am not routing around it.
Worth noting the gate really does cover this PR: the workflow globs `test_*.py` from the tree rather than a hand-written list and refuses a count of zero, so `test_clamp.py` is picked up automatically.
---
## TEST-06 (#14): I reproduced your finding, and located the fix precisely
Ran TEST-06's `test_freeze.py` against this branch's `app/`. **The same 8 failures, in the same order.** Then I applied the `notification_id` keying to TEST-06's four service-keyed call sites - four lines, entirely inside TEST-06's own file, nothing in `app/` touched:
```
=== TEST-06's suite, keyed on notification_id, against SAFETY-03's app/ ===
all checks passed
```
Confirmed at both ends: every one of the 8 is TEST-06 counting by service, and keying on the id fixes all 8 with no change to this PR. It is a **tightening of TEST-06's own claims** - those assertions say "the freeze detector raised exactly one notification" while actually counting every guard's - and it is the change TEST-06 needs whether or not this PR exists.
**My position on ordering: #12 first.** P0 against P3, and the sequencing argument runs the same way - if #14 lands first, this P0 has to edit a P3's brand-new assertions to go green, which is the wrong PR to make that edit in. Landing #12 first leaves #14 making a four-line change in its own file, where it belongs.
And #14 should land promptly after, because it is good. Its `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 - moving `check_meter_frozen()` to the top of `cycle()` survived every check at the time. #14 fixes that. It is worth the rebase.
Merge-order note from the TEST-06 review (PR #14). You go first, and you should not have to touch #14's assertions to do it.
I have bounced #14 with required changes. goodwe_controller/test_freeze.py conflicts between the two branches — git merge-tree c34b5a2 4e66583 gives CONFLICT (content) — so one of us rebases either way. Order is #12 first, #14 promptly after, for two reasons beyond P0-before-P3.
The 8 failures you found are real, and they are #14's to fix, not yours. I reproduced them independently by running #14's test_freeze.py against your app/: 8 red, including its AC1 check and the assertion it built to replace a deleted one. The cause is that #14's new notes_of() helper selects notifications by service rather than by notification_id, so your goodwe_output_clamp alarm is counted as the freeze guard's. Nothing in your branch is wrong here.
It is not a one-line fix, which is the other reason you should not be carrying it. I measured two repairs:
two helpers (all_notes_of() by service, notes_of() by id)
3 red
—
Keying everything on the id makes it look fixed but empties the list when the id changes, so seven checks then fail together and one of them can no longer fail on its own. And 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. Whether each of those should now mean "nothing at all" or "nothing from the freeze guard" is a judgement call about #14's own assertions, and #14's author should make it.
So: land #12 on its own merits. When #14 comes back it will rebase onto you and make that call in its own file.
Two things from my review that touch your branch directly.
write_setpoint() now clamps, and a second safety check does not know.check_meter_frozen() grades self.target — pre-clamp intent — while SAFETY-02 eight lines earlier grades standing = self.last_write if self.last_write is not None else self.target, with a comment explaining that the wire value is the right thing to judge. Today they differ only under dev_min/dev_max; once your clamp_to_rating() lands the divergence becomes routine. I have written this up as a SAFETY-20 finding rather than asking anyone to change it here — flagging it so it is not a surprise. Nothing in your PR needs to act on it.
The required check.release/1.0 requires checks / unit (pull_request), and I gather you already unblocked this branch by merging release/1.0 in to trigger a real run. Good — #14 has to do the same and I have told it so.
Nothing here blocks you. Ping me if the rebase surfaces anything you would rather I re-check.
**Merge-order note from the TEST-06 review (PR #14). You go first, and you should not have to touch #14's assertions to do it.**
I have bounced #14 with required changes. `goodwe_controller/test_freeze.py` conflicts between the two branches — `git merge-tree c34b5a2 4e66583` gives `CONFLICT (content)` — so one of us rebases either way. **Order is #12 first, #14 promptly after**, for two reasons beyond P0-before-P3.
**The 8 failures you found are real, and they are #14's to fix, not yours.** I reproduced them independently by running #14's `test_freeze.py` against your `app/`: 8 red, including its AC1 check and the assertion it built to replace a deleted one. The cause is that #14's new `notes_of()` helper selects notifications by *service* rather than by `notification_id`, so your `goodwe_output_clamp` alarm is counted as the freeze guard's. Nothing in your branch is wrong here.
**It is not a one-line fix, which is the other reason you should not be carrying it.** I measured two repairs:
| repair | vs your `app/` | rename `notification_id` in main.py |
|---|---|---|
| #14 as it stands | 8 red | RED 1 |
| key `notes_of()` on the freeze id | 0 red | **RED 7** |
| two helpers (`all_notes_of()` by service, `notes_of()` by id) | **3 red** | — |
Keying everything on the id makes it *look* fixed but empties the list when the id changes, so seven checks then fail together and one of them can no longer fail on its own. And 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. Whether each of those should now mean "nothing at all" or "nothing from the freeze guard" is a judgement call about #14's own assertions, and #14's author should make it.
So: land #12 on its own merits. When #14 comes back it will rebase onto you and make that call in its own file.
**Two things from my review that touch your branch directly.**
1. **`write_setpoint()` now clamps, and a second safety check does not know.** `check_meter_frozen()` grades `self.target` — pre-clamp intent — while SAFETY-02 eight lines earlier grades `standing = self.last_write if self.last_write is not None else self.target`, with a comment explaining that the wire value is the right thing to judge. Today they differ only under `dev_min`/`dev_max`; once your `clamp_to_rating()` lands the divergence becomes routine. I have written this up as a SAFETY-20 finding rather than asking anyone to change it here — flagging it so it is not a surprise. Nothing in your PR needs to act on it.
2. **The required check.** `release/1.0` requires `checks / unit (pull_request)`, and I gather you already unblocked this branch by merging `release/1.0` in to trigger a real run. Good — #14 has to do the same and I have told it so.
Nothing here blocks you. Ping me if the rebase surfaces anything you would rather I re-check.
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 SAFETY-03 (P0, 3 SP). Base
release/1.0@0cdaa3b.What this is, in one sentence
The clamp already existed. The alarm did not, and specs.md 10.3 is explicit
that the clamp without the alarm is the defect, not the fix: with the inverter
switched off entirely an unclamped integrator climbed ~130 W per 4 s past 10 kW
and, pinned at the 5 kW rail, reported 14 768 W. Clipped silently, that is a
flat, legal-looking 5000 W hold on every dashboard and nothing anywhere says the
loop asked for three times it.
How "the rating" was resolved, and why
The card warns against picking one of two bounds. Both already exist in this
add-on:
DriverCapabilities.setpoint_limit_w(3.2)min/maxfromhass.limits()->Controller.dev_min/dev_maxoptimizer.loop.clamp_w(14)max_woption, already used byTuning.max_wand the arbiter'ssupervisedlimitSection 14 requires them to agree, validated at startup with a refusal on
mismatch - and that gate already ships:
validate_startup_config()refusesmax_w > dev_maxand-max_w < dev_min, andrun_control()re-runs it onceESPHome is up and the real rating is known.
So this guard clamps to
max_w, the validated value, and does notmin()it with the device rating at runtime. The device range is applied second, as
defence in depth. After a successful validation that stage is arithmetically
dead and cannot engage - which is exactly why it alarms if it ever does, with
the bound named: it means the validation did not run (
hass.limits()returns(None, None)while ESPHome is down) or no longer holds.No new config key. CFG-03 already tracks three places a default has to be
kept in step and this ticket does not earn a fourth. The bound is the number the
operator already set; the alarm's cadence is a module constant for the same
reason
freeze.RAIL_FRACis - it describes the notification bus, not the plant.Every loop-tuning knob 10.3 asked to keep exposed is untouched.
Yes, there is an
i_clampedsignal - and SAFETY-20 should knowDecisionnow carriesi_clampedandclampedbooleans.SAFETY-18's developer said outright they would have used an
i_clampedbooleanif it existed; it exists now. SAFETY-20's
min(max_w, dev_max)is notimplemented here and nothing in this PR changes what
freeze.pymeasures itsrail against - but SAFETY-20 now has the option of asking the loop whether it
clamped, instead of inferring "at the rail" from
RAIL_FRAC = 0.95.The booleans are not a convenience.
reasoncannot answer the question: itis one string that later stages overwrite, so a cycle that clamps the integrator
and is then slew-limited reports
slew-limited, and one that clamps whilesaturated reports
saturated-freeze. Both are the winding case. Measured -mutating
main.pyback onto"clamp" in decision.reasondrops the 10.3replay from 2 engagements to 0 and the alarm goes completely silent
(mutation 4 below).
Related: on a default install (
integrator_max_wfollowingmax_w) theintegrator bound bites first and the output clamp is unreachable - control.py
already said so in a comment addressed to this ticket. A guard watching only the
emitted value would be silent on every standard configuration. Both are watched.
Acceptance criteria
write_setpoint(), theone path every write takes: cycle, heartbeat, operator button,
shutdown().Not in
control.py- a guard the control law owns is a guard maintenance andmanual override bypass. Asserted over every value that reached
set_number().(
OUTPUT_CLAMP), separatenotification_id(goodwe_output_clamp), separatestatus row, separate counter. Part 4 drives a loop that saturates for 10
consecutive cycles at 1.4 % of the rating and asserts zero clamp
notifications, engagements, log rows and status rows - after first asserting
the saturation freeze really did engage, so the silence is an observation
rather than a scenario that did nothing.
through the real
Controller.cycle().Not a trip. 11.2's action is "clamp and alarm". The clamped value is
written, the loop keeps running,
publish()still reportsrunning. This isdeliberately unlike SAFETY-01/02, whose action is 0 W.
Two things found by running it, not by reading it
1. The action log flooded the ring and silenced another guard.
log_event()keeps 40 rows. A row per engagement flushes the whole ring inside 40 cycles of a
winding loop, and the first thing it evicted was SAFETY-18's own
METER FROZEN?row - this guard silencing another guard's log. Caught by
test_freeze.pygoing red, not by inspection. Now coalesced into one row per episode carrying
the exact count read off the counter; regression check added and mutation 6
reproduces the original failure in both suites.
2. A naive clamp does not pass a
nanthrough - it launders it into fullpower. Every comparison against
nanis False, somin(limit, nan)keeps itsfirst argument:
max(-5000, min(5000, nan))is +5000 W, and thenapplied != requestedis True becauselimit != nan, so a transport bug getsfiled in the winding-loop counter and full-rate import goes on the wire.
Computed in the check, not recalled. A non-finite value is loss of input (11.1
rule 1): command 0 W, keep commanding, and do not count it as an engagement.
The check
test_clamp.py, assert-based, no framework, house style. Green:Red runs - six author mutations, every one reproduced
Each mutation was applied to the shipped code, the check re-run, and the source
restored. Mutation 1 is the historical build the ticket exists to refuse.
The check refused two of its own hypotheses
Both about how the saturation freeze and the integrator clamp interact. Written
first as "a frozen cycle never clamps" (false), then as "only the
i_w == 0reseed exception clamps" (also false - at
i_w == 0with a matchingactualthe cycle is not saturated at all, so it is never frozen). What the sweep
actually shows, and what the check now asserts: a frozen cycle clamps exactly
when the integrator reverses, which control.py permits on purpose. That is the
one shape where saturation and a clamp engagement coincide - i.e. precisely where
a shared alarm channel would lose the clamp. It is in the check because it was
measured, not because it was expected.
Whole suite
Two existing checks were changed, both tightened rather than relaxed:
test_freeze.pyfiltered notifications by service. This add-on nowraises more than one kind, and freeze's own scenarios drive a command to the
rail on purpose - i.e. they legitimately raise the clamp alarm too. A bare
svc == "create"test counts another guard's alarm as this one's, which is theexact conflation AC2 forbids. Now keyed on
notification_id.test_watchdog.py's_Hassstub gainedcall(). Its e2e runs adeliberately inverted loop (
gain: -0.6) into a steady 2000 W import - a loopthat winds straight into the rail - so the clamp alarm fires there. The missing
stub method is how that was discovered; it is now a second, independent witness
that the alarm sees a real winding loop.
Not done, and why
shipped
Controller.cycle()against a stub HA, which is the same surfacetest_freeze parts 1-2 use. The rig was also HELD by a CI campaign
(
45ed2ab4cd2b:158668,CI 60 sever_network fw_update) throughout - observedwith
DOCKER_HOSTset, not inferred from aUNKNOWN. Not jumped.DRIVER-07's and is not in this repo's Python. The wire-level invariant is
asserted here over every value that reached
set_number(), which is the lastthing this process controls; proving it at the inverter needs the witness log.
ems/event/faulton MQTT and the 13.2 binary sensors do not exist yet(IF-01/IF-02). Same call SAFETY-02 and SAFETY-18 made: use the surfaces that
ship rather than invent a topic to migrate later.
Where to attack this hardest
max_w-not-min()decision. Ifvalidate_startup_config()can beskipped on a path I have not found, the "device stage cannot engage" claim is
wrong and the alarm's second bound becomes routine rather than exceptional.
The one hole I know of is
hass.limits()returning(None, None)whileESPHome is down - which is why the stage is still there and still alarms.
than a row each. If a reviewer reads 15's action-log requirement as one row
per engagement, this is a deliberate deviation and the ring-flush measurement
is the argument for it.
NOT READYwhile the controlleris, in fact, still regulating. That follows the freeze detector's precedent
but the wording is arguably wrong for a guard that is explicitly not a trip.
clamp_wdis not persisted, unlike SAFETY-02's latch. A restart zeroesthe engagement counter and the evidence with it. Deliberate (nothing here
acts, so there is nothing to erase), same asymmetry SAFETY-18 argued - but it
is a judgement call, not a fact.
Decisionflags at the reversal case. The sweep is 20 cells at onetuning; a wider sweep may find frozen-and-clamping shapes I have not.
Against the PM contract on the card
The card's second PM comment supersedes the first and asks, in its own words, to
alarm on a boolean, not a string -
i_clamped: boolonDecision, watcheddirectly, with
"i-clamped"kept as the human-readable status string. That iswhat this implements, and
clampedis carried alongside it for theconfiguration where the integrator is deliberately allowed above the rail.
The PM measured the string hook at 15 %
i-clamped/ 40 %slew-limited/45 %
saturated-freeze. This PR reproduces the mechanism rather than thepercentages: mutation 4 keys the alarm on the string and the 10.3 replay goes
from 2 engagements to 0, and part 4 of the check pins one concrete cycle of each
overwrite (
slew-limitedwithi_clamped=True, and both flags set withreasonreporting neither).The first comment's instruction - alarm on
reason == "i-clamped"- is notfollowed, per the correction.
saturated-freezeremaining the separatesaturation reason is unchanged and is what AC2's "distinct from a normal
saturation event" leans on, backed here by the negative scenario in part 4.
Not merging yet. Three things must change first, and none of them is large.
Nearly all of this is right, and the two things you found by running rather than reading are both real - I reproduced both. What holds it is one behavioural bug in
clamp.pyitself, one assertion that cannot fail, and one constant nothing pins. Every item below is a few lines.Own detached worktree at
c040aac, own mutation harness, own probes. Whole suite green as shipped:test_arbiter,test_control,test_loop,test_maintenance,test_p1,test_validate,test_watchdog,test_clamp,test_freeze- all exit 0.app/freeze.pyis byte-unchanged, confirmed by an empty diff, as you said.1. MUST CHANGE -
clamp_to_rating()never applies the device bound when the first stage engagesclamp.pysays, in capitals:It is not. The
max_wstagereturns, so the device stage is reached only when the first stage did not engage. Executed against the shipped function,max_w = 5000, device range +-4000:Three things about that, in order of how much they matter:
max_w-not-min(). Your argument - and I agree with it, see below - is that the device stage exists for the case where validation did not run or no longer holds. In precisely that case it now fails to bound the values that most need bounding.Reachability, honestly: I went looking for the validation-skipping path you asked me to look for and I did not find one.
self.dev_min/self.dev_maxare assigned in exactly two places:__init__(bothNone) andrun_control(), where the very next statement isvalidate_startup_config()and a refusal that stops deterministically through the normal shutdown.amain()runs the same gate before any task exists.hass.limits() -> (None, None)leaves the device stage inert, not wrong. And everywrite_setpoint()caller passesself.target(arbiter-bounded to +-max_w) or0.0, so the write-path clamp does not engage at all on a reachable path - which is what your "0 write-path engagements in 60 cycles" measured. So this is a latent bug in a layer that is currently unreachable.That is exactly why it should be fixed rather than filed. This stage's whole job is to be correct on the day the assumption above stops holding. Fall through instead of returning, apply the device bound to
applied, and report whichever bound actually bit.While you are in there: a mutation deleting the device stage entirely turns one check red (
a device narrower than max_w still bites). One assertion is thin cover for the half of the function that is supposed to be the last thing between a bug and the hardware.2. MUST CHANGE - the hysteresis check cannot fail
CLEAR_CYCLES = 3 -> 1: the whole suite stays green.range(0)is empty, so zero clean cycles are issued,wd.activeis trivially stillTrue, and the check passes - printing, out loud,ok 0 clean cycles do NOT clear it. It passes for every value ofCLEAR_CYCLES >= 1.CLEAR_CYCLES = 1is the dangerous direction: it is the alarm chattering on a command that dithers across the rail by onestep_w, which is the exact thing the constant's own comment says it exists to prevent. The constant is right; nothing defends it.(
CLEAR_CYCLES = 999is caught - by one status-row assertion, incidentally, not by the hysteresis check.)Assert against the literals, the way the rest of this check asserts against
2and10and40.3. MUST CHANGE -
REASSERT_Sis pinned downward onlyREASSERT_S = 30.0 -> 3600.0: the whole suite stays green. Line 161 readsT0 + timedelta(seconds=REASSERT_S + 9), so the re-assert check passes for any value at all.clamp.pypromises the opposite in two places - "so a loop pinned on the rail for an hour does not go quiet", "fast enough that a pinned loop is never quiet for a whole billing quarter". An hour of silence satisfies every check in the file. The downward direction is pinned, bysum(fired[1:]) == 0over ten 1-second cycles - myREASSERT_S = 0mutation goes red there - which is why this is the second half of the same fix and not a separate problem.What I verified, and what it cost me to believe it
Your headline claim, reproduced exactly
This is the strongest evidence in the PR and it survives an independent run:
Keying the alarm on the string does not weaken it, it switches it off.
Decision.i_clamped/clampedearn their place.Your six mutations, all red on my run
reason>=test_freezegoing red on "the event log names it"M6 crossing suites is the one I most wanted to see fail, and it does. Your discovery - this guard's log evicting SAFETY-18's
METER FROZEN?row - is real, and the regression check genuinely holds it.Mine, beyond yours
N1device stage removed - RED (1). Thin, see item 1.N2thenanguard reverted - RED (5).N3REASSERT_S -> 0- RED (2).N7engagementsdeduplicated to the rising edge - RED (4). "Never deduplicated" is genuinely pinned.N8raw value not preserved inClampResult- RED (1).N4CLEAR_CYCLES -> 1- GREEN. Survives.N5REASSERT_S -> 3600- GREEN. Survives.nan,inf,-inf- the guard holdsis_commandablereturns False for all three, plusNoneand a non-numeric. And the laundering you describe is exactly right, computed rather than recalled:max(-5000, min(5000, nan))is+5000.0, and5000.0 != nanis True, so a naive clamp puts full-rate import on the wire and files it in the winding-loop counter. Driven through the realController, ananbecomes0.0on the wire and is not counted as an engagement.AC2, independently, with a scenario of my own
Not your part 4 - my own plant, one that follows 10 % of the command so the error stays large while the command stays small:
And M2 turns your own AC2 rows red, so that scenario is an observation too. AC2 holds.
Does the coalescing weaken AC2? No - and I measured what it actually does
It does not touch AC2 at all: AC2 is about the clamp alarm being distinguishable from saturation, and it is, on five separate surfaces. For the action log, the deviation from "a row per engagement" is faithful in substance - every engagement is counted, never deduplicated, and the count is carried in the row and read off the counter.
What I did measure is that it is one row per uninterrupted run of engagements, not one per episode, because the coalescing only updates
events[0]. With another guard logging through an 80-engagement episode:The ring ends up shared roughly 1:1 rather than monopolised, and the newest clamp row still carries the exact running count in every case. So the flood is genuinely removed, not moved - which is the claim. Two things worth a line in
DOCS.mdrather than a code change: the row keeps the episode's first timestamp rather than the latest, and an interleaved episode shows several rows with cumulative counts, so the largest number is the true one.max_w-not-min()- the decision is rightRuling: keep it. It is what section 14 and the card ask for,
validate_startup_config()genuinely ships and genuinely refuses, and it refuses twice - inamain()before any task exists, and again inrun_control()at the first moment the real rating is known, stopping through the normal shutdown that commands 0 W. A runtimemin()would recreate the two-numbers-that-disagree failure the section 14 warning box is about. I found no path that skips validation while leaving the device bounds known. The problem is not the choice, it is item 1.Your other two attack points
NOT READYon the banner. I agree the wording is wrong for a guard that explicitly is not a trip, but it follows the freeze detector's precedent and changing one without the other makes the surface less consistent, not more. Leave it; it wants its own ticket across both guards.clamp_wdnot persisted. Correct call, and the same asymmetry SAFETY-18 argued: nothing here acts, so there is nothing a restart can erase except a count. Worth one sentence inDOCS.md, because the status row says "since start" and an operator reading it after a restart will not know a restart happened.Housekeeping
release/1.0@0cdaa3b.test_freeze.pyin this repo right now and this PR changestest_freeze.py(thenotification_idfilter). TEST-06 has no commits yet, so there is no git conflict today - but whoever lands second rebases, and it should be a conversation rather than a surprise. Your change to that file is a tightening and I would keep it exactly as it is: filtering by service counted this guard's alarm as SAFETY-18's, which is the conflation AC2 forbids, one file over.SAFETY-20is untouched andfreeze.pyis byte-identical, as you said.Decision.i_clampedis now available to it.Fix 1, 2 and 3 and I will merge it. Only 1 changes behaviour; 2 and 3 are four lines between them.
All three required changes are pushed as
4e66583. Thank you for the review - item 1 in particular is a genuine regression I introduced and did not catch, and items 2 and 3 are the defect I claimed in my own PR body to be watching for.1. Both stages always run
Fixed, and you were right about the shape of it: the problem was the early return, not the choice of bound.
clamp_to_rating()no longer returns from the first stage; it computes the rating bound, records which bound bit, then applies the device bound to that result. When both bite,ClampResult.boundismax_w+device, so the alarm still names what happened.Your reproduction, on the shipped function now:
The assertion is a 492-point sweep of the whole range, not the two witness values, precisely because this defect was invisible to every single-value check that happened to sit inside one stage - including my own table, which had a
devicerow that passed throughout. The third line encodes your "the bigger the bug, the less the guard does" observation directly, as a monotonicity property.The deleted comment is restored as the reason the stage exists, at the code rather than in a commit message:
Red confirmed - the early return restored, verbatim:
Your caveat is kept, not argued away. I could not construct a reachable production path either, and the changelog now says so in the same terms you did: two validation gates and the arbiter bound sit in front of it. It is fixed anyway, and the reason is the one you gave - this stage is load-bearing for my own
max_w-not-min()argument, so a stage that silently does nothing on the largest inputs takes the argument with it.2 and 3. The two checks that could not fail
Both correct, and both are the same failure mode: the stimulus was written in terms of the constant it was grading, so the check moved whenever the constant did.
range(CLEAR_CYCLES - 1)andREASSERT_S + 9are that mistake twice, in a file whose header claims to be watching for it. Since this is the fifth instance in the project and the second inside a file written to catch it, I do not think the lesson is "be careful" - it is that a grading stimulus must never mention the graded symbol, and that is the rule both rewrites follow.Each requirement is now stated in units the constant cannot move:
CLEAR_CYCLES - 1. The de-assert count is then measured, by looping until it clears, and printed.freeze.pyand DOCS.md already reason about. A loop pinned across it must speak at least 5 times and on no more than 20 % of cycles. Both are requirements on the alarm's behaviour, derived from what it is for, and neither is written in terms ofREASSERT_S.Green:
Red confirmed, for the exact values you named:
R3 is now pinned at both ends - you noted it was only pinned downward, and R3b is the ceiling half.
Whole suite, after the fixes
Coordination: TEST-06 has moved, and it is bigger than a rebase
Since your note, TEST-06 has landed a commit -
c34b5a2, touchingtest_freeze.py(+348) andtest_validate.py(+55).git merge-treereports a real content conflict intest_freeze.py, so the "no git conflict today" position no longer holds.More importantly, I ran TEST-06's
test_freeze.pyagainst this branch'sapp/rather than reasoning about it, and 8 of its assertions go red - not the 3 I would have predicted from thesvc == "create"lines alone:Every one is the same root cause, and it is AC2's conflation in a third suite: TEST-06 counts notifications by service, so this add-on's second alarm is counted as the freeze detector's, and
notes[0]becomes whichever alarm happened to be first. The fix is the one this PR already applied to the assertions that existed when I branched - key onnotification_id, not onsvc == "create"- and it is a tightening of TEST-06's own claims rather than a concession to mine.I have not touched TEST-06's file. Whoever lands second takes the rebase; if that is me, I will apply the
notification_idkeying to TEST-06's new assertions and re-run, rather than merging past them. Flagging it now because it is a red suite waiting for someone rather than a merge artefact, and TEST-06's developer should know before they land - their file needs the edit either way.CI-01
Understood, and no attempt will be made to route around a required check. If a merge is refused by branch protection I will report the refusal as it stands.
Re-reviewed. All three changes are correct and verified. Approved on the code - waiting on the required check, which had never run.
Fresh detached worktree at
4e66583, own mutation harness, own probes. Whole suite green as shipped: all nine files exit 0.1. Both stages always run - fixed, and it holds where the witnesses could not reach
Executed against the shipped function, and I went past your two witnesses on purpose:
No escapes on any of the three shapes,
engagedis exactlyapplied != requested, and the asymmetric case - which nothing in the check exercises - is right too.Your R1 reproduces exactly on my run: 3 FAILs,
328 escapes in 492.Can the sweep fail across its range, or only at the witnesses?
This was the thing I most wanted to know, so I built mutations that leave both witness values passing and break the range elsewhere:
S1device bound applied on the positive side onlyS2device stage skipped aboveabs(requested) > 12000S3bdevice stage skipped at exactly one grid point (14646)S4device stage skipped at one value between grid points (14640)So the sweep resolves down to a single sampled point and genuinely covers the range, not just the two witnesses - S1 in particular is the case your own table would have missed a second time. Its one blind spot is a hole narrower than the 61 W stride (S4), which is the inherent limit of a sampled sweep and not worth closing; the stride being 61 rather than a round number is a good choice, since it lands off the values a special case would be written at.
2 and 3. The rewritten checks - and I measured what they actually pin
Your four red runs reproduce exactly, message for message:
Are the new stimuli genuinely independent of the graded constants? I read every mention and then measured the bands by walking each constant until the checks broke:
Both dangerous directions are closed at both ends - chatter, flood, silence and latch all go red - and the shipped 3 and 30 sit inside. The checks pin the requirement rather than the constant, which is the right design and is what you said you were doing. The stimuli are clean:
range(10),range(int(QUARTER_S)), one literal clean cycle, and a loop bounded by a literal 50.REASSERT_Sappears only inside f-string labels; the assertions beside them aresum(fired[1:]) == 0and literals.One leftover, not worth another round. Line 240 asserts
survived == CLEAR_CYCLES.survivedis measured by looping until de-assert, so that conjunct is true by construction for any value in [1, 50] - it is the old tautology in miniature. It is harmless, becausewd2.active is Falsebeside it is the half that bites and the literal-one-cycle check above it is what catchesCLEAR_CYCLES = 1. But it reads as though it pins the constant when it cannot, which is exactly the confusion that produced the original bug. Drop the conjunct, or make itsurvived > 1, next time you are in the file.Everything from the first review re-verified at the new head
The
clamp.pychange touches the path the earlier findings depend on, so I re-ran them rather than assuming:2 of 60 cycles, 1 episode, 1 notification; keyed ondecision.reason->0 of 60, 0 episodes, 0 notifications.test_clampandtest_freezered on "the event log names it". The cross-suite eviction guard still holds.sat_count=10, peak write 1080 W = 21.6 % of the rating, zero clamp engagements, notifications, log rows and status rows.Two new mutations of mine that survive - both low, neither a blocker
T5-describe()stops naming which bound bit: GREEN.ClampResult.bounditself is pinned (T1, reportingmax_w+deviceas plaindevice, is RED), so the data is defended and only the rendering is not. Sincemax_w+deviceexists specifically "so the alarm still names what happened", one assertion on the row text would close it.T6-limit = abs(float(limit_w))->float(limit_w): GREEN. Only reachable with a negativemax_w, whichint(100,5000)forbids. Defensive code, undefended; fine to leave.Reporting them because I said I would report survivors, not because either should hold the PR.
Why it is not merged yet, and it is not the code
CI-01 landed while this was in review.
release/1.0is now5e966ca, and branch protection on it requires the contextchecks / unit (pull_request), withenable_push: false.This branch was cut before that workflow existed, so
4e66583carried no.gitea/, no run was ever triggered for it, and the required context would have sat pending forever - the exact failure mode CI-01's own workflow comment warns about. Merging in that state would either be refused or, with an admin token, quietly bypass the gate on the day it was installed. Neither is acceptable.So I merged
release/1.0into this branch -46383c5, CI files only:.gitattributes,.gitea/ci/Dockerfile,.gitea/workflows/checks.yml. No add-on source, no check, not a line of your work touched; the whole suite is green on the merged tree locally. That triggered run 530, job 1525, nowpending - Waiting to run, queued behind the shared runner's single slot exactly as the workflow predicts.I will merge when that check is green, and report it if it is not. I am not routing around it.
Worth noting the gate really does cover this PR: the workflow globs
test_*.pyfrom the tree rather than a hand-written list and refuses a count of zero, sotest_clamp.pyis picked up automatically.TEST-06 (#14): I reproduced your finding, and located the fix precisely
Ran TEST-06's
test_freeze.pyagainst this branch'sapp/. The same 8 failures, in the same order. Then I applied thenotification_idkeying to TEST-06's four service-keyed call sites - four lines, entirely inside TEST-06's own file, nothing inapp/touched:Confirmed at both ends: every one of the 8 is TEST-06 counting by service, and keying on the id fixes all 8 with no change to this PR. It is a tightening of TEST-06's own claims - those assertions say "the freeze detector raised exactly one notification" while actually counting every guard's - and it is the change TEST-06 needs whether or not this PR exists.
My position on ordering: #12 first. P0 against P3, and the sequencing argument runs the same way - if #14 lands first, this P0 has to edit a P3's brand-new assertions to go green, which is the wrong PR to make that edit in. Landing #12 first leaves #14 making a four-line change in its own file, where it belongs.
And #14 should land promptly after, because it is good. Its
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 - movingcheck_meter_frozen()to the top ofcycle()survived every check at the time. #14 fixes that. It is worth the rebase.Merge-order note from the TEST-06 review (PR #14). You go first, and you should not have to touch #14's assertions to do it.
I have bounced #14 with required changes.
goodwe_controller/test_freeze.pyconflicts between the two branches —git merge-tree c34b5a2 4e66583givesCONFLICT (content)— so one of us rebases either way. Order is #12 first, #14 promptly after, for two reasons beyond P0-before-P3.The 8 failures you found are real, and they are #14's to fix, not yours. I reproduced them independently by running #14's
test_freeze.pyagainst yourapp/: 8 red, including its AC1 check and the assertion it built to replace a deleted one. The cause is that #14's newnotes_of()helper selects notifications by service rather than bynotification_id, so yourgoodwe_output_clampalarm is counted as the freeze guard's. Nothing in your branch is wrong here.It is not a one-line fix, which is the other reason you should not be carrying it. I measured two repairs:
app/notification_idin main.pynotes_of()on the freeze idall_notes_of()by service,notes_of()by id)Keying everything on the id makes it look fixed but empties the list when the id changes, so seven checks then fail together and one of them can no longer fail on its own. And 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. Whether each of those should now mean "nothing at all" or "nothing from the freeze guard" is a judgement call about #14's own assertions, and #14's author should make it.So: land #12 on its own merits. When #14 comes back it will rebase onto you and make that call in its own file.
Two things from my review that touch your branch directly.
write_setpoint()now clamps, and a second safety check does not know.check_meter_frozen()gradesself.target— pre-clamp intent — while SAFETY-02 eight lines earlier gradesstanding = self.last_write if self.last_write is not None else self.target, with a comment explaining that the wire value is the right thing to judge. Today they differ only underdev_min/dev_max; once yourclamp_to_rating()lands the divergence becomes routine. I have written this up as a SAFETY-20 finding rather than asking anyone to change it here — flagging it so it is not a surprise. Nothing in your PR needs to act on it.The required check.
release/1.0requireschecks / unit (pull_request), and I gather you already unblocked this branch by mergingrelease/1.0in to trigger a real run. Good — #14 has to do the same and I have told it so.Nothing here blocks you. Ping me if the rebase surfaces anything you would rather I re-check.