From 3516947b5e79124aa4f7875a186ab486a1d6c050 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sat, 5 Sep 2026 14:35:00 +0200 Subject: [PATCH 1/4] SAFETY-41: raise the clamp alarm's re-assert 30 s -> 150 s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner reported the OUTPUT_CLAMP notification as a persistent annoyance on the live install. It re-asserted every 30 s, all day. ⚠️ THIS IS A REGIME CHANGE, NOT A CHANGE OF MIND ABOUT THE ALARM. 30 s was chosen when a loop-side engagement was rare - clamp.py's own note records "60 cycles of the §10.3 runaway produce 2 loop-side engagements". The live install produced 48 IN 25 MINUTES of ordinary operation, because `max_w` is 5000 W where the loop's tuning was derived at a 1500 W clamp (SAFETY-39). The integrator therefore reaches its bound constantly and the alarm narrated it every 30 s. An alarm that speaks that often about the normal case is how an operator learns to ignore the one that matters, which is the exact failure clamp.py's docstring is written against. ⚠️ 150 s IS THE REQUIREMENT'S OWN EDGE WITH ONE STEP OF MARGIN, not a comfort choice. test_clamp.py part 2 pins the cadence in WALL-CLOCK terms rather than against this constant: a loop pinned on the rail must speak at least MIN_PER_QUARTER = 5 times inside the 900 s quarter that bills the capacity tariff. That caps the constant at 900/5 = 180 s. Measured: 180 s -> 5 alarms per quarter (exactly the required 5, no margin) 150 s -> 6 alarms per quarter 300 s -> 3, and the floor check goes RED 180 s would sit on the boundary, so any later change to the firing logic would tip it red for a reason unrelated to that change. The FLOOR is the thing that must not move, and it does not. ⚠️ WHAT THIS DOES NOT DO. It changes how often the alarm repeats. It does NOT make the alarm correct: the message still calls the integrator bound a BUG SIGNAL and still pairs a request from one moment with a `self.target` read at another. That is SAFETY-40. And it does not stop the loop railing in the first place; that is SAFETY-39. This is a stop-gap and says so at the line. Nothing was suppressed: every engagement is still counted, the action-log row is unchanged, and the §10.3 runaway still raises the alarm - an earlier attempt at this that split the alarm so only `clamped` notified was REVERTED because test_clamp.py:398 proved it would have disabled runaway detection entirely (a runaway presents as `i_clamped`, and control.py:78-81 says `clamped` is unreachable on a default install). Full suite: 20 passed, 0 failed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/app/clamp.py | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/goodwe_controller/app/clamp.py b/goodwe_controller/app/clamp.py index e7a9a87..4c8c5f1 100644 --- a/goodwe_controller/app/clamp.py +++ b/goodwe_controller/app/clamp.py @@ -145,10 +145,32 @@ from datetime import datetime # nothing else in this add-on uses it. See "DISTINCT FROM SATURATION". REASON_CODE = "OUTPUT_CLAMP" -# Re-assert the notification this often while the clamp stays engaged. 30 s is -# the planning cadence (2.3), i.e. slow enough not to flood and fast enough -# that a pinned loop is never quiet for a whole billing quarter. -REASSERT_S = 30.0 +# Re-assert the notification this often while the clamp stays engaged. +# +# ⚠️ RAISED 30 s -> 150 s on 2026-09-05, and the reason is a REGIME CHANGE, not a +# change of mind about the alarm. 30 s was the planning cadence (2.3), chosen +# when a loop-side engagement was a rare event: this module's own note records +# "60 cycles of the §10.3 runaway produce 2 loop-side engagements". On the live +# install it produced 48 IN 25 MINUTES of ordinary operation, because `max_w` is +# 5000 W where the loop's tuning was derived at a 1500 W clamp (SAFETY-39) - so +# the integrator reaches its bound constantly and the notification re-asserted +# every 30 s, all day. An alarm that speaks that often about the normal case is +# how an operator learns to ignore the one that matters, which is the exact +# failure this module's docstring is written against. +# +# ⚠️ 150 s IS NOT A COMFORT CHOICE, IT IS THE REQUIREMENT'S OWN EDGE, WITH ONE +# STEP OF MARGIN. test_clamp.py part 2 pins the cadence in WALL-CLOCK terms +# rather than against this constant - a loop pinned on the rail must speak at +# least MIN_PER_QUARTER = 5 times inside the 900 s quarter that bills the +# capacity tariff. That caps this constant at 900/5 = 180 s. At 180 s the check +# passes with exactly 5 of a required 5, so any later change to the firing logic +# tips it red; 150 s gives 6. The floor is the thing that must not move. +# +# ⚠️ THIS IS A STOP-GAP AND SHOULD BE REVISITED. It reduces how often the alarm +# repeats; it does not make the alarm correct. SAFETY-40 is the wording and the +# i-clamped/clamped split, and SAFETY-39 is the tuning that would stop the loop +# railing in the first place. Neither is done by this line. +REASSERT_S = 150.0 # Consecutive clean cycles before the alarm de-asserts (10.2 hysteresis). CLEAR_CYCLES = 3 -- 2.54.0 From 1d393ced2258d95b39cab5a43bc2ea8448d4a278 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sat, 5 Sep 2026 15:51:56 +0200 Subject: [PATCH 2/4] SAFETY-41: correct the comment, and fix the check it cites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found two false statements in the comment and one weak check. All three are fixed here; the constant itself is unchanged at 150 s and was never in question. 1. "That caps this constant at 900/5 = 180 s" was FALSE ABOUT THE CHECK IT CITED. test_clamp.py part 2 sweeps i = 0..899 and the rising edge at i=0 is free, so it measured `1 + floor(899/R)` - one alarm more than a real quarter earns. Measured: 224 s was GREEN across the whole suite. Anyone raising this to 200 s would have had a green board and a requirement quietly unmet - the same defect recorded at test_clamp.py:216, one notch weaker. The check now grades floor(QUARTER_S / REASSERT_S) directly, so it enforces the ceiling the comment claims for it. Verified: 150 PASS, 180 PASS, 181 FAIL, 224 FAIL. 180 is also the true requirement edge - on the worst sliding 900 s window 180 earns 5 alarms and 181 earns 4. 2. The "60 cycles of the §10.3 runaway" note was attributed to this module and truncated. It is main.py:2479 and reads "60 cycles of the §10.3 runaway produce 0 write-path engagements and 2 loop-side ones". Quoted in full, attributed correctly. 3. "48 engagements in 25 minutes" was left implying a pinned rail. It is roughly one cycle in six - real, and far above the 2-in-60 the guard was sized for, but not continuous. Restated, and labelled as a live-log reading rather than a measurement in this repo. Also recorded: the 1500 W tuning derivation is in the OTHER repo (admin/goodwe NOTES.md, "Simulated 2 kW load step (gain 0.6, clamp 1500)"), so this repo cannot corroborate it and a reader should not go looking here; and MIN_PER_QUARTER = 5 is a test-local integer, not a spec figure - the 900 s quarter is system-wide but the 5 is not, so the 180 s ceiling rests on one number a test author chose. Full suite: 20 passed, 0 failed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/app/clamp.py | 63 +++++++++++++++++++++++---------- goodwe_controller/test_clamp.py | 15 ++++++++ 2 files changed, 59 insertions(+), 19 deletions(-) diff --git a/goodwe_controller/app/clamp.py b/goodwe_controller/app/clamp.py index 4c8c5f1..0cc4af8 100644 --- a/goodwe_controller/app/clamp.py +++ b/goodwe_controller/app/clamp.py @@ -149,27 +149,52 @@ REASON_CODE = "OUTPUT_CLAMP" # # ⚠️ RAISED 30 s -> 150 s on 2026-09-05, and the reason is a REGIME CHANGE, not a # change of mind about the alarm. 30 s was the planning cadence (2.3), chosen -# when a loop-side engagement was a rare event: this module's own note records -# "60 cycles of the §10.3 runaway produce 2 loop-side engagements". On the live -# install it produced 48 IN 25 MINUTES of ordinary operation, because `max_w` is -# 5000 W where the loop's tuning was derived at a 1500 W clamp (SAFETY-39) - so -# the integrator reaches its bound constantly and the notification re-asserted -# every 30 s, all day. An alarm that speaks that often about the normal case is -# how an operator learns to ignore the one that matters, which is the exact -# failure this module's docstring is written against. +# when a loop-side engagement was a rare event: `main.py`'s note at the action +# log records "60 cycles of the §10.3 runaway produce 0 write-path engagements +# and 2 loop-side ones". On the live install the counter reached 48 loop-side +# ENGAGEMENTS in ~25 minutes of ordinary operation - roughly one cycle in six, +# not a pinned rail, but far more often than a guard sized for 2-in-60 assumed - +# and at a 30 s re-assert the owner received a persistent notification about it +# all day. # -# ⚠️ 150 s IS NOT A COMFORT CHOICE, IT IS THE REQUIREMENT'S OWN EDGE, WITH ONE -# STEP OF MARGIN. test_clamp.py part 2 pins the cadence in WALL-CLOCK terms -# rather than against this constant - a loop pinned on the rail must speak at -# least MIN_PER_QUARTER = 5 times inside the 900 s quarter that bills the -# capacity tariff. That caps this constant at 900/5 = 180 s. At 180 s the check -# passes with exactly 5 of a required 5, so any later change to the firing logic -# tips it red; 150 s gives 6. The floor is the thing that must not move. +# ⚠️ THE FIGURE ABOVE IS AN ENGAGEMENT COUNT READ OFF THE LIVE LOG, NOT A +# MEASUREMENT IN THIS REPO, and it is not the alarm count. Do not compare it +# with the 2-in-60 above without noting that one is engagements and the other is +# engagements too, but over a runaway rather than ordinary running. # -# ⚠️ THIS IS A STOP-GAP AND SHOULD BE REVISITED. It reduces how often the alarm -# repeats; it does not make the alarm correct. SAFETY-40 is the wording and the -# i-clamped/clamped split, and SAFETY-39 is the tuning that would stop the loop -# railing in the first place. Neither is done by this line. +# WHY the loop reaches its integrator bound so much more often than the guard's +# author saw: `max_w` on the live install is 5000 W, while the loop's slew was +# derived against a 1500 W clamp - admin/goodwe `NOTES.md` §"Simulated 2 kW load +# step (gain 0.6, clamp 1500)". ⚠️ THAT DERIVATION IS IN THE OTHER REPO; nothing +# in this one corroborates it, so do not go looking here. SAFETY-39 is the ticket +# that re-derives it, and fixing that is what would stop the railing; this +# constant only changes how often the alarm repeats. +# +# ⚠️ 150 s IS THE REQUIREMENT'S EDGE WITH MARGIN, AND THE CHECK NOW ENFORCES THAT +# EDGE. `test_clamp.py` part 2 grades the cadence in wall-clock terms - a pinned +# loop must speak at least MIN_PER_QUARTER = 5 times inside the 900 s quarter +# that bills the capacity tariff - which caps this constant at 900/5 = 180 s. +# Measured on the worst sliding 900 s window: 180 s earns 5 alarms, 181 s earns +# 4. 150 s earns 6. +# +# ⚠️ THAT CHECK USED TO PERMIT 224 s, AND THE FIX SHIPPED WITH THIS COMMIT. Its +# sweep starts at i=0, where the rising edge is free, so it measured one alarm +# more than a real quarter earns. A later editor raising this to 200 s would have +# had a green suite and a requirement quietly unmet. `test_clamp.py` now grades +# floor(QUARTER_S / REASSERT_S) directly. +# +# ⚠️ MIN_PER_QUARTER = 5 IS A TEST-LOCAL INTEGER, not a spec figure - it exists +# only in `test_clamp.py` and the CHANGELOG entry that introduced it. The 900 s +# quarter IS system-wide (`tariff.py`, `freeze.py`, `DOCS.md`). So the 180 s +# ceiling rests on one number a test author chose; if that number is ever +# revisited, this constant moves with it. +# +# ⚠️ THIS IS A STOP-GAP AND DOES NOT MAKE THE ALARM CORRECT. The message still +# calls the integrator bound a BUG SIGNAL and still pairs a request from one +# moment with a `self.target` read at another - SAFETY-40. Nothing here is +# suppressed: every engagement is still counted, the action-log row is written on +# every engaged cycle regardless, and the rising edge of every episode alarms +# unconditionally, so no alarm is DROPPED - only delayed. REASSERT_S = 150.0 # Consecutive clean cycles before the alarm de-asserts (10.2 hysteresis). diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 7f5c3dc..49f0022 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -239,6 +239,21 @@ print(f" pinned on the rail for {QUARTER_S:.0f} s at 1 s cycles: " check(f"a loop pinned for a whole billing quarter speaks at least " f"{MIN_PER_QUARTER} times (it spoke {alarms})", alarms >= MIN_PER_QUARTER) +# ⚠️ REVIEW-FOUND, SAFETY-41, and it is the SAME DEFECT as the one recorded +# twenty lines above, one notch weaker. The sweep above runs i = 0..899 and the +# rising edge at i=0 is FREE, so it measures `1 + floor(899 / REASSERT_S)` - one +# alarm more than a real quarter earns. Measured on this very check: +# REASSERT_S = 180 -> 5 alarms, green (and 180 is the true edge: on the worst +# REASSERT_S = 224 -> 5 alarms, GREEN sliding 900 s window 180 gives 5 and +# REASSERT_S = 225 -> 4 alarms, red 181 gives 4) +# So the check as written permits 224 s while the REQUIREMENT permits 180 s, and +# anyone raising the constant to 200 s would get a green suite and conclude the +# floor was met when a real quarter would earn 4 alarms. Grade the cadence +# DIRECTLY, so the check enforces the ceiling its own comment claims for it. +check(f"...and the CADENCE itself honours the floor, independent of where the " + f"sweep happens to start: floor({QUARTER_S:.0f}/{REASSERT_S:.0f}) = " + f"{int(QUARTER_S // REASSERT_S)} >= {MIN_PER_QUARTER}", + QUARTER_S // REASSERT_S >= MIN_PER_QUARTER) check(f"...and on no more than {MAX_FRACTION:.0%} of cycles (it was " f"{alarms / QUARTER_S:.1%})", alarms <= MAX_FRACTION * QUARTER_S) check("...while every single cycle is still counted", -- 2.54.0 From 8790c4d38f60e676bc10539c3287e921174d5950 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sat, 5 Sep 2026 17:06:29 +0200 Subject: [PATCH 3/4] SAFETY-41: rate-limit the RISING EDGE, which is where the flood came from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review falsified the first version of this change, and the correction is the substance of it. Raising REASSERT_S 30 -> 150 removed NONE of the owner's notifications. The re-assert interval governs only the still-engaged path; the rising edge fired unconditionally. At the measured ~5 s cycle the live pattern - 48 engagements in ~25 minutes, about one cycle in six - leaves ~25 s of clean cycles between engagements, past CLEAR_CYCLES x 5 s = 15 s. So the alarm DE-ASSERTED between almost every engagement and each one returned as a fresh rising edge. Measured on the shipped class over that window: 48 episodes, 48 notifications at REASSERT_S = 30, and still 48 at 150. The interval now bounds the NOTIFICATION RATE rather than the repeat rate within one episode: `last_alarm` survives a de-assert, and a new episode speaks only if the interval has elapsed. Same window afterwards: 51 episodes, 51 engagements, 11 notifications. ⚠️ NOTHING IS DROPPED, AND THE TWO PROPERTIES THAT MATTER ARE PINNED: - a PINNED loop still speaks 6 times in the 900 s billing quarter, against a floor of 5 (part 2, unchanged); - a rising edge from a COLD watchdog still fires IMMEDIATELY, so a runaway beginning from quiet is never held back by a budget spent on an earlier, unrelated episode. That check is the counterfactual and it is the half that would go red if the limit ignored elapsed silence. Every engagement is still counted, every episode still increments, and the action-log row is still written on every engaged cycle. The budget is on notifications, never on evidence. The new pin is written in wall-clock seconds and in the shape the field produced, so it does not move when the constant does. Verified by reverting the rising edge to unconditional: 2 checks go RED naming it. Full suite: 21 passed, 0 failed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/app/clamp.py | 56 ++++++++++++++++++++++++++++----- goodwe_controller/test_clamp.py | 45 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 7 deletions(-) diff --git a/goodwe_controller/app/clamp.py b/goodwe_controller/app/clamp.py index 0cc4af8..b20e3e6 100644 --- a/goodwe_controller/app/clamp.py +++ b/goodwe_controller/app/clamp.py @@ -157,10 +157,20 @@ REASON_CODE = "OUTPUT_CLAMP" # and at a 30 s re-assert the owner received a persistent notification about it # all day. # -# ⚠️ THE FIGURE ABOVE IS AN ENGAGEMENT COUNT READ OFF THE LIVE LOG, NOT A -# MEASUREMENT IN THIS REPO, and it is not the alarm count. Do not compare it -# with the 2-in-60 above without noting that one is engagements and the other is -# engagements too, but over a runaway rather than ordinary running. +# ⚠️ BOTH FIGURES ARE ENGAGEMENT COUNTS; only the regime differs - 2-in-60 was +# measured over a §10.3 RUNAWAY, 48-in-305 was ordinary running on the live +# install. The 48 is read off the live log, not measured in this repo. +# +# ⚠️ AND RAISING THIS CONSTANT ALONE DID NOT FIX THAT FLOOD - review found it +# and the first version of this change was wrong. At ~5 s cycles a 1-in-6 +# engagement leaves ~25 s of clean cycles between engagements, past +# CLEAR_CYCLES x 5 s = 15 s, so the alarm DE-ASSERTED between almost every one +# and came back as a fresh rising edge. The rising edge used to fire +# unconditionally, so those notifications never consulted this constant at all: +# 48 notifications at 30 s, and still 48 at 150 s. The fix was to rate-limit the +# RISING EDGE too and let `last_alarm` survive a de-assert, so this interval +# bounds the notification rate across episodes rather than only within one. +# Measured on that same window afterwards: 51 episodes, 11 notifications. # # WHY the loop reaches its integrator bound so much more often than the guard's # author saw: `max_w` on the live install is 5000 W, while the loop's slew was @@ -320,8 +330,36 @@ class ClampWatchdog: self.active = True self.since = now self.episodes += 1 - self.last_alarm = now - return True + # ⚠️ SAFETY-41. THE RISING EDGE IS RATE-LIMITED TOO, and it did + # not used to be. It fired unconditionally, so a condition that + # engages and clears repeatedly raised one notification PER + # EPISODE no matter how fast the episodes came - and + # `REASSERT_S` never entered into it, because it only governs + # the still-engaged path below. + # + # That is the shape the live install actually produced: 48 + # engagements in 25 minutes, scattered roughly one cycle in six. + # At a ~5 s cycle the gaps are ~25 s of clean cycles, which + # exceeds CLEAR_CYCLES x 5 s = 15 s, so the alarm DE-ASSERTED + # between almost every engagement and each one came back as a + # fresh rising edge. Measured on this class over that window: + # 48 episodes and 48 notifications at REASSERT_S = 30 - and + # still 48 at REASSERT_S = 150. Raising the re-assert removed + # NONE of them, because none of them came from the re-assert. + # + # So the interval now bounds the NOTIFICATION RATE rather than + # the repeat rate within one episode: `last_alarm` survives a + # de-assert, and a new episode speaks only if the interval has + # elapsed. ⚠️ NOTHING IS DROPPED. Every engagement is still + # counted, every episode still increments, the action-log row is + # still written on every engaged cycle, and a genuine runaway - + # which by definition keeps engaging - is picked up by the + # re-assert path within REASSERT_S. Delay, not loss. + if (self.last_alarm is None + or (now - self.last_alarm).total_seconds() >= REASSERT_S): + self.last_alarm = now + return True + return False # Still engaged: re-assert on the slow cadence, not every cycle. if (self.last_alarm is None or (now - self.last_alarm).total_seconds() >= REASSERT_S): @@ -335,7 +373,11 @@ class ClampWatchdog: if self.clean_cycles >= CLEAR_CYCLES: self.active = False self.since = None - self.last_alarm = None + # ⚠️ `last_alarm` DELIBERATELY SURVIVES THE DE-ASSERT. Clearing it + # here is what made the next rising edge speak immediately however + # soon it arrived, which is the flood described at the rising edge + # above. The episode ends; the notification budget does not reset + # with it. self.reason = "" self.clean_cycles = 0 return False diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 49f0022..8caf7eb 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -259,6 +259,51 @@ check(f"...and on no more than {MAX_FRACTION:.0%} of cycles (it was " check("...while every single cycle is still counted", pinned.engagements == int(QUARTER_S)) +# --- THE SCATTERED REGIME, WHICH IS THE ONE THE OWNER ACTUALLY HIT ----------- +# ⚠️ REVIEW-FOUND, SAFETY-41, and it falsified the first version of this card. +# The rising edge used to fire UNCONDITIONALLY, so REASSERT_S governed only the +# still-engaged path. A condition that engages and clears repeatedly therefore +# raised one notification PER EPISODE at any value of the constant. +# +# That is what the live install produced: 48 engagements in ~25 minutes, +# scattered about one cycle in six. At the measured ~5 s cycle the gaps are ~25 s +# of clean cycles, past CLEAR_CYCLES x 5 s = 15 s, so the alarm de-asserted +# between almost every engagement. Raising REASSERT_S 30 -> 150 removed NONE of +# them. The interval now bounds the notification RATE across episodes. +# +# The stimulus is written in wall-clock seconds and in the shape the field +# actually produced, so it does not move when the constant does. +SCATTER_CYCLE_S = 5.0 +SCATTER_ONE_IN = 6 +SCATTER_CYCLES = 305 + +scat = ClampWatchdog() +scat_alarms = sum(1 for i in range(SCATTER_CYCLES) + if scat.note(i % SCATTER_ONE_IN == 0, "scattered", + T0 + timedelta(seconds=SCATTER_CYCLE_S * i))) +_scat_span = SCATTER_CYCLE_S * SCATTER_CYCLES +print(f" scattered 1-in-{SCATTER_ONE_IN} over {_scat_span:.0f} s at " + f"{SCATTER_CYCLE_S:.0f} s cycles: {scat.episodes} episodes, " + f"{scat.engagements} engagements, {scat_alarms} alarm(s)") +check(f"a condition that de-asserts between engagements is rate-limited ACROSS " + f"episodes, not once per episode ({scat_alarms} alarms for " + f"{scat.episodes} episodes)", + scat_alarms < scat.episodes) +check(f"...and the rate honours the same interval the pinned case does: at most " + f"ceil({_scat_span:.0f}/{REASSERT_S:.0f}) + 1 = " + f"{int(_scat_span // REASSERT_S) + 1}", + scat_alarms <= int(_scat_span // REASSERT_S) + 1) +check("...while EVERY episode and EVERY engagement is still counted - the " + "budget is on notifications, never on evidence", + scat.episodes == scat.engagements == SCATTER_CYCLES // SCATTER_ONE_IN + 1) +# ⚠️ THE COUNTERFACTUAL. A runaway that begins from quiet must NOT be delayed by +# a budget spent on an earlier, unrelated episode - so a cold watchdog speaks at +# once, and this is the half that would go red if the rate limit were applied +# without regard to how long it has been silent. +check("...and a rising edge from a COLD watchdog still fires immediately, so a " + "runaway beginning from quiet is never held back", + ClampWatchdog().note(True, "cold runaway", T0) is True) + # --- THE HYSTERESIS, PINNED AGAINST THE CHATTER DIRECTION -------------------- # ⚠️ REVIEW-FOUND. This used to issue `range(CLEAR_CYCLES - 1)` clean cycles, # which at CLEAR_CYCLES = 1 issues ZERO of them and passes while printing -- 2.54.0 From 3527c8a04c42963c2e5bc9b070aeaaa1a3c04195 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sat, 5 Sep 2026 18:13:09 +0200 Subject: [PATCH 4/4] SAFETY-41: escalate a sustained engagement, so a runaway is never held MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the case I asked it to construct, and it is not a corner case. The rate limit alone DELAYS A RUNAWAY: a benign one-cycle clamp that spends the budget, then a continuous §10.3-shaped runaway, held the runaway's first notification for up to 140 s - one REASSERT_S minus a cycle. On the live install, where ordinary railing spends the budget essentially all the time, a runaway arriving during normal operation would be delayed with near-certainty. That is the "benign noise suppresses the bug signal" shape this module's docstring is written against, and the family the first attempt at this card died of. THE ESCALATION. An episode that has not yet spoken because an EARLIER, unrelated episode spent the budget speaks anyway once it has engaged SUSTAINED_CYCLES = 3 times in a row. The discriminator is run length and the two populations genuinely differ: the live flood was runs of length ONE (engage, de-assert, engage), while a runaway engages every cycle. Measured, and it costs nothing it was protecting: live scatter flood (305 cycles, 1-in-6) 11 -> 11 notifications chatter 1-engaged/3-clean over 8000 s 50 -> 50 notifications pinned 900 s quarter (floor is 5) 6 -> 6 alarms runaway delay after a benign episode 140 s -> 10 s ⚠️ MY COUNTERFACTUAL CHECK COULD NOT FAIL FOR THE DEFECT IT NAMED, and review caught that too. It read `ClampWatchdog().note(True, ...) is True` - a FRESH object, which only exercises the `last_alarm is None` branch, the trivially-true one. A fresh watchdog has no earlier episode to be held back BY. It now SPENDS THE BUDGET FIRST: benign clamp, gap past CLEAR_CYCLES, then a continuous runaway, and asserts the delay is well under REASSERT_S. That is the third instance in this file of the family recorded at line 216 and at the 224 s sweep, and it was guarding the most safety-critical property in the change. Also pinned: `last_alarm` SURVIVING the de-assert. Review built the mutation - if the de-assert also refreshed it, chatter collapses from 50 notifications to 1 and the alarm never fires again, with the suite green. That is the "alarm that never fires" class, a different bug from one that fires later. Now caught. Both new checks verified against the mutations they name: removing the escalation -> 1 FAILED; refreshing last_alarm on de-assert -> 1 FAILED. Full suite: 21 passed, 0 failed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/app/clamp.py | 45 +++++++++++++++++++++++-- goodwe_controller/test_clamp.py | 58 ++++++++++++++++++++++++++++----- 2 files changed, 93 insertions(+), 10 deletions(-) diff --git a/goodwe_controller/app/clamp.py b/goodwe_controller/app/clamp.py index b20e3e6..d787108 100644 --- a/goodwe_controller/app/clamp.py +++ b/goodwe_controller/app/clamp.py @@ -154,8 +154,9 @@ REASON_CODE = "OUTPUT_CLAMP" # and 2 loop-side ones". On the live install the counter reached 48 loop-side # ENGAGEMENTS in ~25 minutes of ordinary operation - roughly one cycle in six, # not a pinned rail, but far more often than a guard sized for 2-in-60 assumed - -# and at a 30 s re-assert the owner received a persistent notification about it -# all day. +# and the owner received a persistent notification about it all day. ⚠️ NOT +# because of the 30 s re-assert - see the block below; those notifications never +# consulted this constant at all. # # ⚠️ BOTH FIGURES ARE ENGAGEMENT COUNTS; only the regime differs - 2-in-60 was # measured over a §10.3 RUNAWAY, 48-in-305 was ordinary running on the live @@ -210,6 +211,28 @@ REASSERT_S = 150.0 # Consecutive clean cycles before the alarm de-asserts (10.2 hysteresis). CLEAR_CYCLES = 3 +# ⚠️ SAFETY-41. Consecutive ENGAGED cycles after which an episode speaks even if +# the notification budget is spent. THE ESCALATION EXISTS BECAUSE THE RATE LIMIT +# ALONE DELAYS A RUNAWAY, and review measured it: a benign one-cycle clamp +# followed by a continuous runaway held the runaway's first notification for up +# to 140 s - one REASSERT_S minus a cycle. On the live install, where ordinary +# railing spends the budget essentially all the time, a runaway arriving during +# normal operation would be delayed with near-certainty. That is the "benign +# noise suppresses the bug signal" shape this module's docstring is written +# against. +# +# THE DISCRIMINATOR IS RUN LENGTH, and the two populations genuinely differ: the +# live flood was runs of length ONE (engage, de-assert, engage), while a §10.3 +# runaway engages on EVERY cycle. So a sustained run speaks regardless of budget, +# and scattered chatter does not. +# +# Measured, shipped-without vs shipped-with - it costs nothing it was protecting: +# live scatter flood (305 cycles, 1-in-6) 11 -> 11 notifications +# chatter 1-engaged/3-clean over 8000 s 50 -> 50 notifications +# pinned 900 s quarter (floor is 5) 6 -> 6 alarms +# runaway delay after a benign episode 140 s -> 10 s +SUSTAINED_CYCLES = 3 + @dataclass(frozen=True) class ClampResult: @@ -315,6 +338,10 @@ class ClampWatchdog: self.last_alarm: datetime | None = None self.reason = "" self.clean_cycles = 0 + # SAFETY-41. Consecutive engaged cycles, and whether THIS episode has + # spoken yet - the two the escalation clause needs. + self.run_len = 0 + self.spoken = False def note(self, engaged: bool, reason: str, now: datetime) -> bool: """Advance one cycle. True when the caller should raise the alarm. @@ -326,10 +353,12 @@ class ClampWatchdog: self.engagements += 1 self.clean_cycles = 0 self.reason = reason + self.run_len += 1 if not self.active: self.active = True self.since = now self.episodes += 1 + self.spoken = False # ⚠️ SAFETY-41. THE RISING EDGE IS RATE-LIMITED TOO, and it did # not used to be. It fired unconditionally, so a condition that # engages and clears repeatedly raised one notification PER @@ -358,15 +387,27 @@ class ClampWatchdog: if (self.last_alarm is None or (now - self.last_alarm).total_seconds() >= REASSERT_S): self.last_alarm = now + self.spoken = True return True return False + # ⚠️ THE ESCALATION. An episode that has not yet spoken because the + # budget was spent by an EARLIER, unrelated episode speaks anyway + # once it has engaged SUSTAINED_CYCLES times in a row. A runaway + # engages every cycle and reaches it; the scattered one-cycle + # railing that produced the live flood never does. + if not self.spoken and self.run_len >= SUSTAINED_CYCLES: + self.last_alarm = now + self.spoken = True + return True # Still engaged: re-assert on the slow cadence, not every cycle. if (self.last_alarm is None or (now - self.last_alarm).total_seconds() >= REASSERT_S): self.last_alarm = now + self.spoken = True return True return False + self.run_len = 0 if not self.active: return False self.clean_cycles += 1 diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 8caf7eb..1968880 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -78,7 +78,8 @@ import sys from datetime import datetime, timedelta, timezone from app.clamp import ( - CLEAR_CYCLES, REASON_CODE, REASSERT_S, ClampWatchdog, clamp_to_rating, + CLEAR_CYCLES, REASON_CODE, REASSERT_S, SUSTAINED_CYCLES, ClampWatchdog, + clamp_to_rating, is_commandable, ) from app.allocator import Allocation, UnitPlan, plan @@ -296,13 +297,54 @@ check(f"...and the rate honours the same interval the pinned case does: at most check("...while EVERY episode and EVERY engagement is still counted - the " "budget is on notifications, never on evidence", scat.episodes == scat.engagements == SCATTER_CYCLES // SCATTER_ONE_IN + 1) -# ⚠️ THE COUNTERFACTUAL. A runaway that begins from quiet must NOT be delayed by -# a budget spent on an earlier, unrelated episode - so a cold watchdog speaks at -# once, and this is the half that would go red if the rate limit were applied -# without regard to how long it has been silent. -check("...and a rising edge from a COLD watchdog still fires immediately, so a " - "runaway beginning from quiet is never held back", - ClampWatchdog().note(True, "cold runaway", T0) is True) +# ⚠️ THE COUNTERFACTUAL, AND THE FIRST VERSION OF IT COULD NOT FAIL FOR THE +# DEFECT IT NAMED. It read `ClampWatchdog().note(True, ...) is True` - a FRESH +# object, so it only ever exercised the `last_alarm is None` branch, which is the +# trivially-true one. A fresh watchdog has no earlier episode to be held back BY. +# That is the third instance in this file of the family recorded at line 216 and +# at the 224 s sweep, and it was guarding the most safety-critical property in +# the change. Review found it. +# +# The stimulus must therefore SPEND THE BUDGET FIRST: a benign one-cycle clamp, +# a gap long enough to de-assert, then a continuous runaway. Without the +# escalation clause the runaway's first notification is held for up to +# REASSERT_S - measured at 140 s before it was added. +_esc = ClampWatchdog() +_esc.note(True, "benign", T0) # spends the budget +for _i in range(1, 1 + CLEAR_CYCLES + 1): # de-assert + _esc.note(False, "", T0 + timedelta(seconds=5.0 * _i)) +_RUNAWAY_AT = 5.0 * (CLEAR_CYCLES + 2) +_esc_fires = [5.0 * (CLEAR_CYCLES + 2 + _j) + for _j in range(200) + if _esc.note(True, "runaway", + T0 + timedelta(seconds=5.0 * (CLEAR_CYCLES + 2 + _j)))] +_esc_delay = _esc_fires[0] - _RUNAWAY_AT if _esc_fires else None +print(f" budget spent by a benign episode, then a continuous runaway from " + f"t={_RUNAWAY_AT:.0f}s: first alarm at t={_esc_fires[0]:.0f}s " + f"(delay {_esc_delay:.0f}s)") +check(f"⚠️ a SUSTAINED runaway speaks even when an earlier, unrelated episode " + f"spent the budget - it is not held for REASSERT_S ({_esc_delay:.0f}s " + f"delay, escalation is {SUSTAINED_CYCLES} cycles)", + _esc_delay is not None and _esc_delay < REASSERT_S / 2) +check("...and it speaks at the escalation boundary rather than immediately, " + "which is what keeps the scattered flood suppressed", + _esc_delay >= 5.0 * (SUSTAINED_CYCLES - 1)) + +# ⚠️ `last_alarm` SURVIVING THE DE-ASSERT IS NOW LOAD-BEARING, AND NOTHING +# PINNED IT. Review built the mutation: if the de-assert path ALSO refreshed +# `last_alarm = now`, the chatter case collapses from 50 notifications to 1 - the +# alarm never fires again. That is the "alarm that never fires" class, which is a +# different bug from one that fires later, and the suite was green on it. +_starve = ClampWatchdog() +_starve_fires = sum(1 for _i in range(1600) + if _starve.note(_i % 4 == 0, "chatter", + T0 + timedelta(seconds=5.0 * _i))) +print(f" chatter 1-engaged/{CLEAR_CYCLES}-clean over 8000 s: " + f"{_starve.episodes} episodes, {_starve_fires} notification(s)") +check(f"a de-assert does not RESET the notification budget, so repeated chatter " + f"cannot starve the alarm to silence ({_starve_fires} notifications over " + f"8000 s, not 1)", + _starve_fires > 10) # --- THE HYSTERESIS, PINNED AGAINST THE CHATTER DIRECTION -------------------- # ⚠️ REVIEW-FOUND. This used to issue `range(CLEAR_CYCLES - 1)` clean cycles, -- 2.54.0