fix(tests): evict the whole git_service package family — 13 leaked failures in test_ent615 (0.9.5 M1)

[dolho]

Fixes #2859

0.9.5 work order M1 — the red dev (backend-unit-test failing on 3a47f74).

Root cause

#2487 made services.git_service a package. Three test helpers still did sys.modules.pop("services.git_service") before re-importing it with stubbed deps — test_1704_git_service_plugins.py, test_data_paths_allowlist.py, test_data_paths_gitignore.py. Evicting only the parent leaves services.git_service.<sub> cached, and Python binds a cached submodule onto a new parent only on first load — so the re-imported package had no .token_scrub / .conflicts attribute, and every later git_service.<sub> read in test_ent615_token_free_remotes.py failed with AttributeError: module 'services.git_service' has no attribute 'token_scrub'. 13 failures whenever one of those files runs first (random order under pytest-randomly); the file passes alone.

Fix

Evict the whole services.git_service* family (the shape test_reset_preserve_state_guardrails.py already uses) at the three sites, and make every parent-only _restore_sys_modules fixture family-aware (1704 and both data_paths files — a parent restored without its children is the same half-state). Second commit: the fourth polluter from #2859, test_2075_detect_git_dir.py (swept the family but recorded only the package key for undo, so the submodules its own re-import created outlived teardown) — now the linter-sanctioned _STUBBED_MODULE_NAMES + _restore_sys_modules pair.

Regression guard (tests/unit/test_2859_git_service_eviction.py): the mechanism on a throwaway package (parent-only eviction loses the submodule attribute; family eviction rebinds it) + an AST scan of tests/unit that fails on sys.modules.pop("services.git_service") / del sys.modules["services.git_service"] / monkeypatch.delitem(sys.modules, "services.git_service") outside the family shape — self-tested on the exact lines the polluters carried, proven red by reintroducing the pop on a copy.

Verification

• The issue's repro table (-p no:randomly, each polluter then ent615): 13 failed before on each; after → test_1704 102, test_data_paths_allowlist 101, test_data_paths_gitignore 93, test_2075 101 passed, 0 failed.
• tests/lint_sys_modules.py: no new violations.
• Full unit/ with the CI flags (-n auto --dist loadfile -p randomly, seed 12345), first commit: 16521 passed, 31 skipped, 0 failed (11 min). The 13 IDs are not added to tests/unit_failure_baseline.txt.

🤖 Generated with Claude Code

[dolho]

/review Report

Branch: fix/m1-git-service-stub-leak → dev (merge-base 3a47f74)
Files Changed: 5 (+219/−3), tests only
Scope: CLEAN — intent: #2859 (four polluters restore the whole services.git_service* family; a named regression test; no baseline entries). Delivered: exactly that. No product code touched.
Plan Completion (issue ACs): 3 done / 1 unverifiable

• DONE — four files restore package + submodules: test_1704_git_service_plugins.py:52-66, test_data_paths_allowlist.py:56-70, test_data_paths_gitignore.py:53-67 (family snapshot + family-aware _restore_sys_modules), test_2075_detect_git_dir.py:36-59 (new _STUBBED_MODULE_NAMES + autouse family restore, teardown after monkeypatch's undo).
• DONE — repro table passes: 102 / 101 / 93 / 101 passed, 0 failed (each polluter then ent615, -p no:randomly).
• DONE — regression test named for the issue: test_2859_git_service_eviction.py — mechanism on a throwaway package + AST guard; red when the parent-only pop is reintroduced (proven on a copy).
• UNVERIFIABLE from the diff — push-side pytest (push, absolute failures) green on dev: needs the merge. tests/unit_failure_baseline.txt untouched (0-line diff). Local full suite with CI flags, seed 12345: 16521 passed / 0 failed.

Critical Findings (block merge)

None.

Informational Findings (review required)

[I1] Incomplete-fix completeness (4.14): a fifth parent-only eviction lives outside the guard's reach (Confidence: 8/10)
File: tests/git_sync/test_s7_reserve_instance_id.py:107
Evidence: sys.modules.pop("services.git_service", None) at module level, followed by from services import git_service and from services.git_service import provisioning.
Issue: same pattern as the four fixed files. It is outside tests/unit/, so it is not in the backend-unit-test job and not scanned by the new guard (TESTS_UNIT.glob("test_*.py"), test_2859_git_service_eviction.py:118). Harmless today — that directory runs alone — but it is the exact line the guard exists to catch, and a future pytest tests/ invocation would leak it into whatever imports the package next.
Suggestion: follow-up (not this PR — #2859 scopes to tests/unit): family-evict there too, and consider widening the guard's glob to tests/**/test_*.py.

[I2] Test gap (4.9): the guard proves the static rule; the runtime half is proven only by the throwaway-package mechanism test (Confidence: 6/10)
File: tests/unit/test_2859_git_service_eviction.py:36-70
Issue: the mechanism test exercises CPython import semantics on pkg2859, not on services.git_service itself. If a future services/git_service/__init__.py stopped from .x import …-ing its submodules, the real package could regress in a way the throwaway package wouldn't show. The static guard still catches the cause (parent-only pop), so this is belt-and-braces.
Suggestion: optional — one assertion that, after a family eviction + import services.git_service, hasattr(gs, "token_scrub") and hasattr(gs, "conflicts") (the two attributes ent615 reads). Cheap; ties the guard to the real package.

[I3] Consistency (4.7): three copies of the same family-restore block (Confidence: 5/10 → appendix)
Files: test_1704:52-66, test_data_paths_allowlist:56-70, test_data_paths_gitignore:53-67 (+ a fourth variant in test_2075).
Issue: identical snapshot/restore code in four files. A conftest fixture would be one copy — but lint_sys_modules.py explicitly sanctions the per-file _STUBBED_MODULE_NAMES + _restore_sys_modules pair, so the duplication is the linter's own shape. Not worth churn in a red-dev fix.

Clean Categories

• 4.1 SQL / 4.4 credentials / 4.10 frontend / 4.11 perf / 4.12 enums / 4.15 product bar — no product code in the diff.
• 4.2 concurrency — the CI job runs -n auto --dist loadfile; sys.modules is per-worker-process, and loadfile keeps a file on one worker, so the family restore cannot race another file's import. Verified: workflow line -n auto --dist loadfile.
• 4.3 auth — n/a.
• 4.8 error handling — fixtures use try/finally; the AST guard raises on a parse error (fail-closed) rather than skipping a file.
• 4.13 docs — no architecture/flow surface changed. A learnings.md entry for this class ("evicting a package from sys.modules must take its submodules") is warranted — not added on this branch per instruction; note for the author.
• lint_sys_modules.py: no new violations (the 2075 pair and the guard's own tests use the sanctioned shapes).

Summary

• Critical: 0 — none found
• Informational: 2 (+1 appendix) — I1 is a follow-up outside this PR's stated scope; I2 optional
• Scope: clean

Review complete. Next: /validate-pr 2861 for the docs/process pass; file the tests/git_sync sibling (I1) as a follow-up.

[obasilakis]

Reviewed at fc8480cce. No blocking findings.

• Repro table on this head (-p no:randomly, each file then ent615): 1704 → 102 passed, allowlist → 101, gitignore → 93, 2075 → 101, 0 failed. The same 1704 → ent615 pair on dev fails 13, so the probe does detect the leak.
• Guard bites: putting sys.modules.pop("services.git_service", None) back into test_data_paths_gitignore.py turns test_no_test_evicts_only_the_git_service_package red.
• CI head jobs: 16524 passed, 0 failed on all three seeds.
• Teardown order in test_2075 is right: the autouse fixture is set up before monkeypatch, so it tears down after monkeypatch's undo.

Non-blocking notes:

• _ALLOWLIST exempts all of test_2075_detect_git_dir.py, so a future parent-only eviction added elsewhere in that file would pass the guard.
• The guard only knows services.git_service, but the trap applies to any package. The one other parent-only pop of a package, services.agent_client in test_voice_tools.py:237, only runs when the key was absent before the test and a stub was installed, and test_voice_tools → test_circuit_breaker passes (103). So not a live leak today.

[vybe]

merge-train: validated individually against dev 3a47f74 (2-PR run, no train branch). Repro confirmed: each of the four polluters → test_ent615 goes 13 failed → 0 failed; random-order git_service slice clean on seeds 12345/67890/99999.