test(hermetic): gate on unrestored sys.modules purges (+2 leakers) - #542
Conversation
|
⏳ Blocked on #540 — merge that first. CI here is red exactly as designed: the gate is catching That's the gate working, not a defect in it. Once #540 lands I'll rebase and CI should go green. Verified independently on #540's branch: |
Codifies the bug class behind the tests/agent pollution work as a durable gate, so it fails AT the leaking test instead of mysteriously downstream. ## The class A test wants a fresh import (a patched module, a new HERMES_HOME), so it deletes `run_agent` / `agent.*` / `tools.*` / `hermes_*` from sys.modules and re-imports — and never puts them back. Every later importer then receives a BRAND-NEW module object, so any subsequent test that captured a reference to (or monkeypatched) one of those modules silently operates on an orphaned copy. The failures land far from the cause and read as unrelated bugs, which is why each instance cost a full per-victim bisect to find. Three instances found in one day: * test_empty_tool_name_loop_dampening.py (#538) — 120 -> 74 suite failures * test_verification_stop_caching.py (#540) — ~20 more * test_kanban_per_profile_cap.py (here) — 1 module (hermes_cli._subprocess_compat) ## The gate tests/sys_modules_leak_gate.py — an autouse fixture that fails a test which removes a pre-existing WATCHED module without restoring it. The failure names the test, lists the leaked modules, and carries the fix recipe (including the parent-attribute restore) so the next person doesn't re-derive it. Opt out with @pytest.mark.allow_sys_modules_purge. Deliberately narrow: * ADDING modules is normal (imports happen); only REMOVING pre-existing ones is the hazard. * `plugins.` is EXCLUDED. Plugin-discovery tests purge `plugins.model_providers.*` on purpose to force a re-scan — that IS the behaviour under test, and the modules are re-imported by the next discovery call. Gating them produced ~34 false positives on tests working as designed (measured, then excluded — not assumed). * third-party prefixes (botocore, urllib3) are out of scope: lazy-import churn would be pure noise. ## Proven to FIRE 19 unit tests covering both directions: fires on an unrestored purge, names the test, carries the recipe, truncates a large leak; stays silent on a restored purge, on added modules, on an untouched run, on unwatched modules, and under the opt-out marker. Prefix matching is asserted narrow (`agentic_unrelated` and `my_agent` must NOT match). Also replayed the REAL leaker's exact purge: the gate reports 41 leaked modules with the correct message. An earlier version of that probe reported "does not fire" — because the probe itself had loaded no watched modules, so nothing could be removed. Worth stating: the first negative result was the probe's bug, not the gate's, and it was only caught by checking why. ## Also fixed here tests/hermes_cli/test_kanban_per_profile_cap.py — the gate caught it leaking `hermes_cli._subprocess_compat`. Save + restore around the purge. Not touched: test_verification_stop_caching.py. I fixed it independently, then found PR #540 fixing the same file with a MORE thorough approach (a context manager that also detaches and restores parent attributes, ordered deepest-first). Dropped mine rather than ship a competing duplicate.
Found BY the sys.modules leak gate on its first full-suite run — exactly the job it exists to do. tests/agent/test_save_url_image.py's http_server fixture purged hermes_constants + agent.image_gen_provider to force a HERMES_HOME re-read and never restored them. Same class as #538/#540: an unrestored purge hands every later importer a brand-new module object, so a subsequent test that captured a reference to (or monkeypatched) one of these silently operates on an orphaned copy. Also moves the server shutdown into a finally, so a failing test no longer leaks the TCPServer thread.
fdc00b0 to
1e9bb99
Compare
|
Rebased onto The slice 9/12 red was inherited, not caused by this PR: it was Verified locally after the rebase: |
|
✅ Rebased on #540 — green now. Full The gate is silent because the leaker it was catching is fixed by #540. Combined with the two additional leakers fixed here ( Journey: 120 failed → 0, from four fixtures of one bug class. |
1e9bb99 to
1cd2987
Compare
FleetReviewConfidence: 2/5 Findings
FleetReview provenance · models: B=gpt-5.6-sol, C=claude-code-opus-4-8, F=gpt-5.6-sol, G=grok-4.5 · cost: $13.65 · duration: 22m 28s · rounds: 1 · files examined: 5 |
Codifies the bug class behind the
tests/agentpollution work as a durable gate — so it fails at the leaking test instead of mysteriously downstream.Companion to #538 (leaker #1) and #540 (leaker #2). Land #540 first — the gate fires on
test_verification_stop_caching.pyuntil #540's fix is in.The class
A test wants a fresh import (a patched module, a new
HERMES_HOME), so it deletesrun_agent/agent.*/tools.*/hermes_*fromsys.modulesand re-imports — and never puts them back. Every later importer then receives a brand-new module object, so any subsequent test that captured a reference to (or monkeypatched) one of those modules silently operates on an orphaned copy.The failures land far from the cause and read as unrelated bugs, which is why each instance cost a full per-victim bisect to find.
Four instances found in one day:
test_empty_tool_name_loop_dampening.py(#538)test_verification_stop_caching.py(#540)test_kanban_per_profile_cap.py(here)hermes_cli._subprocess_compattest_save_url_image.py(here)That last one is the argument for the gate: it caught a leaker nobody had bisected for.
The gate
tests/sys_modules_leak_gate.py— an autouse fixture that fails a test which removes a pre-existing watched module without restoring it. The failure names the test, lists the leaked modules, and carries the fix recipe (including the parent-attribute restore). Opt out with@pytest.mark.allow_sys_modules_purge.Deliberately narrow:
plugins.is excluded. Plugin-discovery tests purgeplugins.model_providers.*on purpose to force a re-scan — that is the behaviour under test. Gating them produced ~34 false positives on tests working as designed (measured, then excluded — not assumed).Proven to FIRE
19 unit tests, both directions: fires on an unrestored purge, names the test, carries the recipe, truncates a large leak; stays silent on a restored purge, added modules, an untouched run, unwatched modules, and under the opt-out marker. Prefix matching asserted narrow (
agentic_unrelated,my_agentmust NOT match).Then replayed the real leaker's exact purge: 41 modules reported, correct message.
An earlier version of that probe said "does not fire" — because the probe itself had loaded no watched modules, so nothing could be removed. Worth stating plainly: the first negative was the probe's bug, not the gate's, and it was only caught by asking why.
Note
I also fixed
test_verification_stop_caching.pyindependently, then found #540 fixing the same file with a more thorough approach (a context manager that also detaches/restores parent attributes, deepest-first). Dropped mine rather than ship a competing duplicate.