fix(witan-code): stop _git_context leaking across test boundaries - #178
Merged
Conversation
test_repo_scope_route_and_fanout failed in the full local suite but passed
alone and in CI. The cause is not residue in a store, as first suspected, but
server._git_context: it memoizes detect()/store_branch() for a 2s TTL that
outlives a single test.
An earlier test (test_bridge.py) sets WITAN_REPO=test/repo-a and memoizes it.
If test_crossrepo starts inside that 2s window, its own
monkeypatch.setenv("WITAN_REPO", "") -- the whole point of which is to make
detect() return None so the query fans out -- is silently ignored.
_resolve_clients then takes the single-repo branch, repo-a's store does exist
in the test's tmp code dir, and the fan-out collapses to one repo. Hence the
reported "Extra items in the left set: repo-b".
Whether the leak lands is a race against wall-clock, decided by how fast the
preceding tests ran, which is why it reproduced locally and not in CI.
Clear it between tests in an autouse fixture, alongside the identical one that
already exists for identity._cache. Thirteen hand-rolled _git_context.clear()
calls across the suite were working around this per-test; test_crossrepo was
one of the tests that did not know it had to. The clears that run mid-test
(after a checkout or actor switch) are still required and are left in place.
No production behaviour changes -- the TTL is unchanged and still amortizes
git subprocesses across an agent turn. The source comment claimed "short
enough that no test needs to know about it", which is what made the trap
invisible; it now says the opposite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pq9itT1Y44g86kvBEEqJZn
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a flaky witan-code test failure caused by the module-level server._git_context TTL cache persisting across test boundaries, allowing earlier tests’ memoized repo/branch detection to affect later tests.
Changes:
- Update
witan_code.server’s cache commentary to explicitly document cross-test leakage risk and the expected test mitigation. - Add an autouse pytest fixture to clear
server._git_contextbefore/after each test (mirroring the existing identity cache reset pattern). - Add a small regression pair in
test_branches.pyto ensure git-context memoization does not persist into subsequent tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mcp/servers/witan-code/witan_code/server.py |
Clarifies the intent and testing implications of the _git_context TTL cache. |
mcp/servers/witan-code/tests/conftest.py |
Adds an autouse fixture to clear _git_context between tests to prevent leakage. |
mcp/servers/witan-code/tests/test_branches.py |
Adds regression tests asserting _git_context is cleared between tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tate Addresses Copilot review feedback on #178. test_git_context_survives_within_a_test inherited whatever the preceding test memoized. test_cached_git_refreshes_after_ttl runs immediately before it and leaves a fresh "b" in _git_context, so within the 2s TTL _cached_detect() returned "b" and the test failed for the wrong reason. That matters precisely in the scenario the pair exists to support: toggling the autouse fixture off to confirm its removal is still detected. Verified before the fix -- with autouse off, BOTH tests failed, and the second one reported residue "b" rather than LEAKED_REPO, so it was demonstrating an unrelated predecessor's leak rather than the one it sets up. After the fix the same run fails exactly once, citing LEAKED_REPO. Clearing at the start does not weaken the pair: the residue the second test checks is written by _cached_detect() later in the first test, so what gets left behind is unchanged -- only its provenance is now guaranteed. Full suite 438 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pq9itT1Y44g86kvBEEqJZn
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.
What & why
tests/test_crossrepo.py::test_repo_scope_route_and_fanoutfailed in the full local suite but passed alone and in CI (tk-witan-code-test-repo-scope-route-and-fanout-fail-348361).The task filed against this suspected residue in a shared store. It is not that. The cause is
server._git_context(witan_code/server.py:107), which memoizesdetect()/store_branch()for a 2 second TTL that outlives a single test.The failure sequence:
test_bridge.pysetsWITAN_REPO=https://github.com/test/repo-aand memoizes it.test_crossrepostarts inside that 2s window, its ownmonkeypatch.setenv("WITAN_REPO", "")— whose entire purpose is to makedetect()returnNoneso the query fans out — is silently ignored._resolve_clients(None)therefore takes the single-repo branch instead of the fan-out branch. repo-a's store does exist in the test's tmp code dir, so it returns one client.Extra items in the left set: 'https://github.com/test/repo-b'.Whether the leak lands is a race against wall-clock, decided by how fast the preceding tests happen to run. That is why it reproduced locally and not in CI — and why a green run does not prove absence.
The fix
Clear
_git_contextbetween tests in an autouse fixture, directly alongside the identical_fresh_identityfixture that already exists foridentity._cache.This was already a known hazard: 13 hand-rolled
srv._git_context.clear()calls are scattered across the suite working around it per-test.test_crossrepowas simply one of the tests that did not know it had to. The clears that run mid-test (after a checkout or an actor switch) are still load-bearing and are left in place.No production behaviour changes. The TTL is unchanged and still amortizes git subprocesses across an agent turn. The source comment claimed the TTL was "short enough that no test needs to know about it" — the assumption that made this trap invisible. It now says the opposite.
Verification
Both directions were mutation-checked by toggling
autouseoff and re-running:a previous test's git context leakedtest_crossrepoleak scenarioExtra items in the left set: 'repo-b'verbatimThe regression test is a deliberate ordered pair: the first leaves residue, the second asserts it is gone. Delete the fixture and the second fails — so the fixture cannot be quietly removed later.
Full suite: 438 passed (436 + the 2 new tests).
prekclean on all changed files.