test(hermetic): fully contain fresh-import module purges - #540
Merged
Merged
Conversation
Restore exact sys.modules entries and parent-package attributes, remove fresh-only modules, and scope test_verification_stop_caching's second broad purge.\n\nVerified: targeted polluter/victim sequence 16/16; deterministic single-process tests/agent 4949 passed, 15 skipped.
Collaborator
Author
FleetReviewConfidence: 3/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: $12.17 · duration: 25m 42s · rounds: 2 · files examined: 2 |
Kyzcreig
pushed a commit
that referenced
this pull request
Aug 10, 2026
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.
Kyzcreig
pushed a commit
that referenced
this pull request
Aug 10, 2026
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.
Kyzcreig
pushed a commit
that referenced
this pull request
Aug 10, 2026
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.
Kyzcreig
pushed a commit
that referenced
this pull request
Aug 10, 2026
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.
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
1 participant
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.
Summary
sys.modulesrestoration to also restore parent-package child attributes exactly and remove modules imported only inside the fresh-import scopetest_verification_stop_caching.pyRoot causes
test_empty_tool_name_loop_dampening.pyremovedrun_agent,agent.*,tools.*, andhermes_*fromsys.modules. Restoring only the dictionary entries left package attributes such asagent.auxiliary_clientpointing at replacement modules and left fresh-only modules resident. Later tests monkeypatched pre-collected objects while runtime code executed replacement objects.test_verification_stop_caching.py::_fresh_run_agentperformed the same broad purge with no restoration. Its replacement transport package/registry poisoned the four transport victims left after the first fix.Both scopes now snapshot exact module entries and parent attributes with a
_MISSINGsentinel, purge child-first, remove all fresh-scope modules on exit, restore original entries/attributes, and restore even when setup/import raises. The queued-handler teardown from #538 remains; logging noise was not the assertion-failure cause.Measured verification
Deterministic single-process sweep (
pytest tests/agent/ -p no:randomly -q):Bisection / victims:
test_empty_tool_name_loop_dampening.py: 1 failed, 6 passedFinal gates:
ruff checkon both changed files: passscripts/run_tests.sh -j 4 tests/agent/): 420 files, 4949 tests passed, 0 failed in 165.4s--- Logging error ---output in final targeted, combined, or canonical runsOne unrelated wall-clock cancellation test failed once during an earlier loaded full sweep and once on the canonical runner's first attempt, then passed on retry; it passed 10/10 isolated runs, the final combined sweep, and the final canonical sweep. No changes were made to that out-of-scope test.
Concurrent implementation
This branch is an extension of merged #538 (
7644a3eadb), not a competing replacement. Rebase skipped the duplicate cherry-pick and preserved an identical final tree hash (16ad0eeeedb3927cdde7f4e80d0b7830151f5691).