Repository navigation
Harness: no-write guard and stale-container preflight; retire live held-write scenario - #21
Merged
Merged
Conversation
Issue #18: a 7-day soak never forced a write into its post-write cooldown at the same moment MQTT was down, so #12's hold-then-wake branch had no real-world exercise. scenario_forced_held_write blocks MQTT via an allow-list inversion (resolve the REST host inside the container, drop all outbound :443, re-allow just that host) instead of a peer-address or blanket-port block - REST and MQTT share :443, so a blanket block would also stop the forced-REST write from landing, and a peer-address block doesn't hold against AWS IoT's rotating reconnect pool. The second write runs on a background thread (its own service call blocks inside HA until the write resolves, so it can't run on the main thread) while the scenario polls for "held behind cooldown" before unblocking, then confirms "woken early by MQTT reconnect" landed before the cooldown deadline. Chasing the first real run down turned up a second problem: the dev container had been running since before #12/#9 landed, and config- entry reloads don't re-import Python modules, so it was silently executing pre-fix code - a false negative that looked exactly like a production bug. assert_mounted_code_is_loaded() now runs before any scenario and fails loudly if the container's StartedAt predates the newest mtime under its mounted custom_components/exo_pool/*.py. Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…nd item 10) Lets the outage-reconnect harness derive its matching regexes from these constants instead of hand-copied literals, so they can never silently drift from what api.py actually logs. No change to arguments or emitted text - verified against the full custom_components suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
Addresses both reviewers' combined fix round on 909a0ab, item by item: - Ctrl+C ordering: the network unblock is now deferred onto the shared teardown right after the restore, so a signal mid-scenario unblocks before restoring (item 1). - The scenario's own finally now re-reads and numerically verifies the restore, failing and naming the value on mismatch (item 2). - Write #1 now points away from the 100 upper bound; write #2 targets the original value, not original-1 (item 3). - The stale-container guard also checks the mount source resolves to this repo's checkout, names both paths on mismatch, drops the unused runner param, and prints ISO timestamps (item 4). - SERVICE_CALL_TIMEOUT and the held-wait timeout now derive from POST_WRITE_COOLDOWN_SECONDS plus named margins (item 5). - URLError, TimeoutError and the guard's RuntimeError now end as FAIL lines in main()'s summary; a non-numeric entity state raises a clear ScenarioFailure (item 6). - Early-wake failures now name whether/when "MQTT connection resumed" appeared and how much cooldown remained at unblock (item 7). - The held-line wait is now keyed by write via matches_write_held, so a same-connection write for a different entity can't satisfy it early (item 8). - REST_HOST is now derived from api.py's DATA_URL_TEMPLATE; resolve_host_ips uses `getent ahosts`, deduplicated and IPv4-only (item 9). - The three log-matching regexes are now derived from api.py's new log constants (item 10, api.py change in ce24e5a). - New tests kill mutants M1 (write #1 removed), M2 (write #2 off by one) and M4 (unblock before the held wait), assert the POST value sequence and unblock-before-wake ordering, and cover the previously-untested branches (non-REST first write, NET_ADMIN skip, restore-verify failure, ACCEPT-add failure still tearing down DROP, timestamp parsing without fractional seconds, mount_source failure, no .py files found) (item 11). - Fake clocks now flow into wait_for_early_wake_or_fail from every scenario test path that reaches it, so failures resolve immediately instead of via the global timeout (item 12). - Trimmed docstrings/comments to the one-line-contract standard across both files; rationale that only restated the test name or the git history is gone (item 13). Full suite green (313 tests). comment-intent-guard clean against 909a0ab. One real run against ha-exo-pool-dev correctly refused: its mount points at the main checkout (/Users/Andrew_1/src/projects/homeassistant/exo_pool), not this worktree, so item 4's new check names both paths and stops before touching anything - reported rather than worked around, per instruction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…one (#18) set_pool_value's HA state updates before the cloud write lands (api.py's _apply_desired_update), so reading state straight back after a restore write could report success on a write the device never applied. _verify_restored now forces a homeassistant.update_entity refresh before reading back, and the scenario calls it directly on the normal path (a failed restore now raises instead of the PASS line printing regardless), keeping the deferred teardown only as the Ctrl+C backstop. Also: audited every scenario_forced_held_write() test call site for the early-wake real-clock gap flagged in review - none exist; the flagged test's own held-wait failure short-circuits the early-wake branch, and every other site either fails before that branch, matches the "woken early" log line before its first clock check, or already fakes both clocks. Added coverage for the numeric-mismatch restore branch, the non-NET_ADMIN skip, and a first write that never goes via REST fallback; made WRITE_HELD_BEHIND_COOLDOWN_LOG's round trip through its own regex derivation a test rather than an assumption; pulled SWC_OUTPUT_MAX from number.py instead of mirroring it by hand; stopped _verify_restored's and _resumed_diagnostic's own exceptions from masking a scenario's real error; collapsed main()'s repeated exception tuple into SCENARIO_EXCEPTIONS; renamed StaleContainerError to MountedCodeError now that it also covers a mount-source mismatch; and a couple of test nits (an assertion against a hardcoded 60.0 now reads SERVICE_CALL_TIMEOUT, a stray blank line gone). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…ation (#18) README's outage-reconnect section still described the old finally-block restore and only the staleness half of assert_mounted_code_is_loaded(). Cover the mount-source mismatch check and the need to run the harness from the checkout ha-exo-pool-dev actually mounts, the write direction flipping at the max value, and restore now being a deferred-teardown backstop plus an explicit, device-refreshed verification. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
) scenario_forced_held_write issued real number.swc_output writes against the dev container's actual physical chlorinator to exercise #12's early-wake-on-reconnect behaviour. A run left the real device stuck at 0% output for ~6 hours because the restore check read HA's optimistic state (set synchronously by _apply_desired_update) rather than a device-confirmed value, so it could never detect a real mismatch. Andrew's call: retire the live write entirely rather than harden it - #12's early-wake behaviour is already covered by tests/test_api_write_manager_mqtt_throttle.py's test_reconnect_callback_wakes_a_held_write_before_the_cooldown_ends and doesn't need a real device to prove. Removed scenario_forced_held_write and everything only it used: the write/service-call helpers, the allow-list MQTT-blocking mechanism, the restore/verify helpers, and the log-format matchers. resolve_swc_output_entity_id and its error type also went, since the removed scenario was their only caller. Added a source-scan regression guard so no future scenario can reintroduce a device write: test_harness_source_never_references_a_write_service fails if any write-service string literal (set_value, turn_on, turn_off, select_option, set_temperature) appears anywhere in the script. custom_components/exo_pool/api.py is untouched - its ce24e5a log-format constants and the #12 wake logic stay, since production _execute_write still uses them independently of this harness. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
The harness that used to derive regexes from these constants is gone (this branch's earlier commit). Nothing outside api.py used them even before that removal, so custom_components/ now matches origin/main exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
test_harness_source_never_references_a_write_service was bypassable -
a call built from a runtime string (f"set_{kind}") or a literal path
(POST /api/services/number/set_value with no set_value token) would
sail past a source-text regex.
_ha_request is now the one place the script ever touches the network:
it raises DisallowedHaRequestError before any request goes out unless
the call is a GET, or a POST to exactly a config-entry reload path.
Covered by tests that a write-service POST and PUT/DELETE never reach
urlopen (monkeypatched), and that a reload POST is let through.
Two structural tests back the chokepoint itself: urlopen is called
from nowhere but _ha_request (AST walk over every function), and the
script imports no alternate HTTP client (http.client/requests/aiohttp)
that could route around it. Mutation-tested both: a urlopen call added
to another function, and a `import requests` added, each correctly
fail their respective test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…#18) test_reconnect_callback_wakes_a_held_write_before_the_cooldown_ends only killed the {sleep_task, event_task} -> {sleep_task} mutant (the wake event dropped from the race) via pytest's default 30s ceiling - a slow but not immediately obvious signal, and any test elsewhere that also hits that ceiling would mask it. Tried asyncio.wait_for(..., timeout=1.0) first per the brief, but this suite's fake_clock fixture monkeypatches time.monotonic globally (the same module object api.py's `import time` sees), which backs asyncio's own loop.time(). fake_clock's `+= 3` jump instantly blew past a 1s deadline computed against that same frozen/jumping clock, failing the good code path too. pytest-timeout's thread-based marker times out against the OS-level clock instead, which the monkeypatch never touches - @pytest.mark.timeout(1) on just this test gets a real 1s bound unaffected by fake_clock. Re-ran the mutant with the bound in place: fails in ~1s instead of ~30s. Reverted from the scratchpad snapshot; custom_components/ is byte-identical to origin/main again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
- Removed resolve_host_ips and its 4 tests: only caller was the now-deleted scenario_forced_held_write's REST-host resolution. - Restored wait_for_log_pattern to origin/main's signature - the now/sleep injectable params and the Callable-predicate branch on `pattern` had no caller or test exercising them; every call site passes a compiled regex and none pass now=/sleep=. - Dropped the sys.path.insert(_REPO_ROOT) - nothing in the script imports from custom_components any more (that ended with the device-write removal). Kept _REPO_ROOT itself; EXPECTED_EXO_POOL_SOURCE still needs it. - Inlined the entity_ids temporary in main() - used exactly once. - Removed a stray extra blank line before the teardown section header. - Deleted the SCENARIO_EXCEPTIONS comment and trimmed assert_mounted_code_is_loaded's docstring to one line - the name and the two checks' own error messages already carry it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…ree dev (#18) - Replaced the "never writes to any entity" claim with the actual guarantee: no HA service calls (the _ha_request GET/reload-only allow-list), and named that container exec calls aren't guarded the same way - scoped to iptables/ss/getent/docker-image only. - Dropped the incident-history sentence about an earlier harness version's real-device write; that belongs to git history, not to what a reader needs to use this script safely. - Updated the write-safety paragraph to describe the current guard (the _ha_request chokepoint and its urlopen/import structural tests) instead of the now-removed source-text regex. - make dev didn't pass -p exo_pool, so running it from a worktree (a differently-named directory) stood up a second compose project with its own empty config volume instead of reusing ha-exo-pool-dev - the same class of mismatch assert_mounted_code_is_loaded exists to catch, but one step earlier. Fixed the Makefile target and documented it next to the mount-guard section, which already used -p exo_pool for its own repoint command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
The two structural tests had gaps: the import check was a blocklist (missed http.client via `from http import client`, which adds "http" and "http.client" - neither of which was ever checked against), and the urlopen-caller check only walked FunctionDef bodies, missing module-level code, lambdas, and async def. test_harness_source_imports_exactly_the_expected_modules replaces the blocklist with an exact allow-list of every name the script's own imports produce - `from X import Y` recorded as both X and X.Y - so any addition at all, not just a known-bad one, fails loudly. test_forbidden_http_names_are_used_only_inside_ha_request walks the whole module (not just FunctionDef bodies) for any Name or Attribute named urlopen/build_opener/OpenerDirector/Request/HTTPConnection/ HTTPSConnection, and fails on any of them found outside _ha_request's own subtree (found once via id() membership, not scoped by node type - covers module level, lambdas, and async def alike), plus a direct check for any `from urllib.request import ...`. Mutation-tested all 5 known bypasses against these two tests, each correctly killed: - from urllib.request import urlopen as _u: both tests (extra import name, and the explicit ImportFrom check) - module-level lambda calling urlopen: forbidden-names test - build_opener().open(...): forbidden-names test - from http import client: import-allow-list test - async def calling urlopen: forbidden-names test (proves the FunctionDef-only walk was the actual gap) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
"scoped to iptables/ss/getent/docker-image commands only" overstated it - Container.exec and the netns sidecar's docker run carry no such scoping themselves; they run whatever they're given. Replaced with what's actually true: unguarded, but the only things this script ever asks them to do are read the HA log, manage iptables, and read/rewrite /etc/hosts. Also updated the write-safety paragraph to describe what the hardened guards (prior commit) actually prove: an exact import allow-list (not a blocklist), and the forbidden-name check covering the whole module, not just other functions' bodies - so a module-level call, a lambda, or an async def can't route around it either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…18) _ALLOWED_RELOAD_PATH_RE used .match() with ^...$ anchors and a [^/]+ entry-id class. Three gaps: Python's $ matches just before a trailing newline (not only end-of-string), so a path with a trailing "\n" still matched; [^/]+ allows anything but a slash, so ".../entry/x?a=/reload" and ".../entry/../reload" both matched too. Switched to .fullmatch() (no anchor semantics to get wrong - the whole string must match) with an explicit [0-9A-Za-z]+ entry-id class. Parametrized RED test covering all three cases, confirmed failing against urlopen (monkeypatched) before the fix, passing after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
make dev already passed -p exo_pool (prior commit); make stop and dev-setup.py's start_docker() didn't, so stopping or first-time-starting from a worktree could still target/create a differently-named compose project instead of ha-exo-pool-dev's. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
pytest-timeout's per-test clock includes fixture setup, and a loaded
CI runner could plausibly eat more than 1s of that before the test
body even starts, flaking a passing test. 5s keeps the mutant-catching
margin (real early-wake code still passes in ~0.17s) while giving
slow CI room. Re-ran the {sleep_task, event_task} -> {sleep_task}
mutant with the wider bound: still correctly times out, in ~5.1s wall
time versus the previous ~1.1s. Reverted; custom_components/ confirmed
byte-identical to origin/main again.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…#18) test_harness_source_imports_exactly_the_expected_modules (an exact set equality) failed on any unrelated new import, like itertools, training whoever hit that failure to just add the name to the allow-list without thinking about why the test exists. Replaced with test_harness_source_imports_no_alternate_http_stack: a denylist of http/http.client/socket/ssl/requests/aiohttp/httpx/urllib3, plus the existing from-urllib.request-import check. Mutation-checked: `from http import client` and `import httpx` both fail it, `import itertools` passes. test_forbidden_http_names_are_used_only_inside_ha_request is unchanged. README: corrected "can't route around it" - the guards catch accidental reintroduction of a device write, not deliberate evasion (a string-built getattr, a host-side subprocess shelling out to curl, a container exec are all out of scope for a source-level test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #18 (forced held-write harness scenario proving #12 early wake).
What
The forced held-write scenario was built, used to prove #12 (wake held writes on MQTT reconnect) live twice, then retired: its restore step left the real chlorinator at swc 0 for ~6h on 2026-09-19 (see the incident comment on #18). #12's early wake now rests on unit coverage only, per Andrew's call.
What this PR lands:
_ha_requestallows GET plus POST to the exact config-entry reload path; anything else raises before any network call. Static tests: the forbidden HTTP names (urlopen,build_opener,Request, ...) appear only inside_ha_requestanywhere in the module, and no alternate HTTP stack is imported (a denylist, so unrelated imports don't trip it). These catch accidental reintroduction, not deliberate evasion.Container.execis unguarded, and the README says so.assert_mounted_code_is_loaded()fails if the container mounts a different checkout, or started before the newest mounted file changed. A config-entry reload doesn't re-import modules, so this once ran pre-Wake writes held behind a REST cooldown on MQTT reconnect #12 code and looked like a prod bug.-p exo_poolon every compose call (Makefiledev/stop,scripts/dev-setup.py), so running from a worktree reuses the existing dev project and config volume instead of creating an empty one.test_reconnect_callback_wakes_a_held_write_before_the_cooldown_endsgets@pytest.mark.timeout(5). Without the wake, the mutant used to die only at pytest's 30s global timeout; now it dies in ~5s.asyncio.wait_forcan't be used here, because thefake_clockfixture patchestime.monotonic, which also drives the loop clock. This is the one change outside the harness files.custom_components/is untouched.Verification
urlopenimport, a lambda,build_opener,from http import client, anasync def, and a widened reload regex all fail.{sleep_task, event_task}->{sleep_task}) is killed.🤖 Generated with Claude Code
https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG