test: isolate the tool-definition caches between tests

[bbasketballer75]

The symptom

tests/run_agent/test_background_review_toolset_restriction.py::test_background_review_installs_thread_local_whitelist fails intermittently in CI with:

AssertionError: assert 'memory' in {'skill_manage'}

It passes in isolation, passes running its own file, and passes on rerun — the classic shape of order-dependent process state rather than a bad test or a stale CI cache.

Root cause

The whitelist under test is derived from get_tool_definitions(enabled_toolsets=["memory", "skills"]), which depends on two module-level caches that outlive any single test:

cache	what it holds	why it leaks
tools.registry._check_fn_cache	per-check_fn verdicts, 30 s TTL, keyed by function object	a test that probes a memory/skills check_fn while that feature looks unavailable stamps False in for the next 30 s
model_tools._tool_defs_cache	memoized definition lists	its key covers registry._generation and the config fingerprint, but not the check_fn verdicts resolved underneath — so a poisoned entry is invisible to it

Reproduced deterministically by stamping False into the TTL cache and recomputing: memory, skill_view and skills_list all drop out of the toolset, and the test fails with exactly the CI assertion. Restoring them makes it pass. Wiring a poisoning test in as a neighbour reproduces the CI failure on demand.

The fix

A suite-wide autouse fixture in tests/conftest.py that:

• drops only the False verdicts from _check_fn_cache — a cached True cannot cause a tool to vanish, and keeping those avoids re-probing every available tool's check_fn;
• clears _tool_defs_cache entirely, since its key cannot see the verdicts underneath it.

tests/test_get_tool_definitions_cache_isolation.py already established this pattern per-file for _tool_defs_cache. This lifts it suite-wide and adds the check_fn cache, which is the one actually carrying the poisoned verdict. Per-file fixtures couldn't cover the exposure — 116 test files touch tool definitions or the registry.

tests/conftest.py is already where this class of problem is handled; the fixture immediately above this one fixes a structurally identical order-dependent leak in the computer-use approval callback.

Verification

• tests/run_agent/ + tests/tools/test_registry.py: identical pass/fail counts before and after (1271 passed, 21 pre-existing Windows-only failures unchanged, 3 skipped).
• Poisoned-neighbour reproduction: fails without the fixture, passes with it.
• tests/test_get_tool_definitions_cache_isolation.py still passes.

Found while investigating why #15's CI slice 4/8 was red. It's unrelated to that PR — this PR touches none of run_agent, background_review, model_tools, or the registry — so it's split out here rather than bundled.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR addresses an intermittent, order-dependent CI failure caused by process-global tool-definition caching leaking across tests. It adds a suite-wide autouse fixture to reset tool-definition caches so model_tools.get_tool_definitions() results are consistent regardless of test order.

Changes:

• Add an autouse pytest fixture in tests/conftest.py to clear tool-definition caches before and after each test.
• Clear model_tools._tool_defs_cache and prune tools.registry check-function verdict caching to prevent “poisoned” toolsets.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bbasketballer75]

Measured the runtime cost, since a suite-wide autouse fixture deserves a number rather than an assurance.

On tests/run_agent/ + tests/tools/test_registry.py (1271 passing tests):

variant	time
no fixture	515 s
blanket clear	606 s
clear only False verdicts	602 s

So roughly +17% on a deliberately tool-heavy subset; the full suite dilutes well below that. Pass/fail counts are identical across all three (21 pre-existing Windows-only failures unchanged).

I'd initially written the third variant on the theory that re-probing available tools' check_fns was the expense. It isn't — 4 s, inside noise. The cost is dominated by rebuilding _tool_defs_cache, which can't be preserved: its key doesn't cover the verdicts underneath it, so an entry computed from a poisoned False would survive and keep serving the short list. I've reverted to the simpler blanket clear and recorded the numbers in the docstring so nobody retries that optimization.

If +17% is too steep for the value, the fallback is a per-file fixture on tests/run_agent/test_background_review_toolset_restriction.py alone — that fixes the observed failure, but leaves the other 115 files that touch tool definitions exposed to the same class of flake.

🤖 Measured by Claude Code

[teknium1]

Thanks for the careful cache analysis and for measuring the suite-wide fixture cost.

This is an automated hermes-sweeper review. The reported cross-file test-state failure is already addressed on current main:

• 48be2e0e4dbc4489f418e8d58794790c9c830390 introduced per-file pytest subprocess isolation.
• scripts/run_tests_parallel.py:4-15 runs each test file in a fresh interpreter specifically to prevent cross-file module-cache leakage.
• CI uses that path in .github/workflows/tests.yml:118-130.
• The affected file's first test exits at patched AIAgent.__init__ before it reaches get_tool_definitions (tests/run_agent/test_background_review_toolset_restriction.py:57-75), so it cannot seed the claimed cache state for the second test.

The proposed suite-wide per-test cache clearing therefore duplicates an existing isolation guarantee while adding the measured test-runtime cost.