fix(memory): import Tuple + isolate tool-definition caches — un-red the fork CI - #22
Conversation
The write-time auto-consolidate code (merged to this fork's main via #4) annotates two nested helpers with Tuple[...] at memory_tool.py:813 and :847, but the module imports only Dict, Any, List, Optional. The def statements execute when the enclosing consolidation path runs, so every memory write that reached consolidation raised: NameError: name 'Tuple' is not defined This is fork-only code — origin/main imports Tuple — which is why the failures appeared on fork CI (PR merge refs test against this fork's main) yet never reproduced on origin/main-based local checkouts, and why they masqueraded as flakes across slices 1/5/7/8: different tests hit the consolidation path in different slices. Also downstream of the same error: skip_memory store tests (load path swallowed the NameError into the agent-init try/except, leaving _memory_store None) and the 413 compression memory-snapshot tests. Verified deterministically: on this base, the fcntl-fallback, learning_mutations, skip_memory, and 413_compression families fail in 1.2 s without this import and pass (33/33) with it. ruff --select F821 confirms Tuple was the only undefined name in the file.
test_background_review_installs_thread_local_whitelist fails intermittently
in CI with:
AssertionError: assert 'memory' in {'skill_manage'}
while passing in isolation, in its own file, and on rerun. It is not a stale
CI cache — it is order-dependent process state.
The whitelist under test comes from
get_tool_definitions(enabled_toolsets=["memory", "skills"]), which depends on
two module-level caches that outlive a single test:
* tools.registry._check_fn_cache — per-check_fn verdicts, 30 s TTL, keyed by
function object. An earlier test that probes a memory/skills check_fn
while the 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: memory,
skill_view and skills_list all drop out of the computed toolset, and the test
fails with exactly the CI assertion. Restoring the entries makes it pass.
Adds a suite-wide autouse fixture that drops the False verdicts (a cached
True cannot produce this failure mode, and keeping those avoids re-probing
every available tool) and clears the definition memo, which must go entirely
because its key cannot see the verdicts.
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 carrying the poisoned verdict. 116 test
files touch tool definitions or the registry, so per-file fixtures could not
cover the exposure.
Verified against tests/run_agent/ + tests/tools/test_registry.py: identical
pass/fail counts before and after (21 pre-existing Windows-only failures
unchanged), and the poisoned-neighbour reproduction now passes.
Measured on tests/run_agent/ + tests/tools/test_registry.py (1271 passing): no fixture 515 s blanket clear 606 s clear False verdicts 602 s The "drop only False verdicts" variant was written on the theory that re-probing available tools' check_fns was the expense. It is not — it saved 4 s, inside noise. The cost is dominated by rebuilding _tool_defs_cache, which cannot be preserved because its key does not cover the verdicts underneath it. Reverted to the simpler blanket clear (same cost, less to explain) and recorded the numbers in the docstring so the next person does not retry the same optimization.
There was a problem hiding this comment.
🟡 Not ready to approve
The new cache-reset fixture clears only part of the registry state (and broadly swallows exceptions), so it may still allow order-dependent behavior and hide failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR addresses two CI failure sources on the fork: a runtime NameError in the memory tool caused by a missing Tuple import, and order-dependent test failures caused by process-global tool-definition caches leaking across tests.
Changes:
- Import
Tupleintools/memory_tool.pysoTuple[...]annotations used by memory read helpers don’t raise at runtime. - Add a suite-wide
autousepytest fixture intests/conftest.pyto clear tool-definition-related caches before/after every test, preventing cache poisoning from affecting later tests.
File summaries
| File | Description |
|---|---|
| tools/memory_tool.py | Adds missing Tuple import for type annotations used by memory store helpers. |
| tests/conftest.py | Adds an autouse fixture to reset tool registry and tool-definition caches between tests to avoid order-dependent failures. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| try: | ||
| from tools import registry | ||
|
|
||
| with registry._check_fn_cache_lock: | ||
| registry._check_fn_cache.clear() | ||
| except Exception: | ||
| pass | ||
| try: | ||
| import model_tools | ||
|
|
||
| model_tools._tool_defs_cache.clear() | ||
| except Exception: | ||
| pass |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e002f7281
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with registry._check_fn_cache_lock: | ||
| registry._check_fn_cache.clear() |
There was a problem hiding this comment.
When one test records a successful dynamic check_fn and the next test changes its environment or config so the same function returns False within 60 seconds, this fixture clears only _check_fn_cache; _check_fn_last_good survives, so _check_fn_cached() suppresses the new failure as transient and continues exposing the unavailable tool. This leaves the suite order-dependent in the opposite direction from the failure being fixed. Call invalidate_check_fn_cache(), which clears both maps under the lock, instead of clearing the private TTL dictionary directly.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
Two fixes that together account for every currently-failing Python test slice on this fork's CI.
1.
NameError: name 'Tuple' is not defined(slices 1, 5, 7, 8)The write-time auto-consolidate code (merged to this fork's
mainvia #4) annotates two nested helpers withTuple[...]attools/memory_tool.py:813/:847, but the module imports onlyDict, Any, List, Optional. Every memory write reaching consolidation raised the NameError.Why it masqueraded as flakes: this is fork-only code —
origin/mainimportsTuple— so PR-merge-ref CI (which tests against this fork'smain) hit it while origin-based local checkouts never could. Different tests hit the consolidation path in different slices: the fcntl-fallbackstore.add(), the learning-mutations edits, and — downstream — the skip_memory store tests (agent-init swallowed the load-path NameError, leaving_memory_storeNone) and the 413-compression memory-snapshot tests.Verified deterministically: on this base the four families fail in 1.2 s without the import and pass 33/33 with it.
ruff --select F821confirmsTuplewas the only undefined name in the file. One-word fix.2. Tool-definition cache isolation (slice 4)
Cherry-pick of the fix submitted upstream as NousResearch#75837, so this fork's CI benefits without waiting on the upstream merge:
test_background_review_installs_thread_local_whitelistfails order-dependently when an earlier test poisonstools.registry._check_fn_cache(30 s TTL, keyed by function object) — invisible tomodel_tools._tool_defs_cache's key. Suite-wide autouse fixture clears both around each test; measured cost is ~+17% on a deliberately tool-heavy subset, diluted well below that suite-wide.After merge
PR #15 and the dependabot PRs need branch updates / check re-runs to pick up the fixed base.
🤖 Generated with Claude Code