From 26d2ac1ba3d0ecc35210eac54032acbef465fa8d Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Tue, 12 May 2026 01:43:06 +0100 Subject: [PATCH 1/5] test(subscription): pin auto-switch chain skips .credentials.enc import (#606) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a chain-level regression test for #606 entered at `subscription_auto_switch._restart_agent` — pins that the SUB-003 auto-switch path reaches the `lifecycle.py:155` `subscription_mode` short-circuit and never re-enters the file-based credential import. The existing leaf-level test (`test_inject_assigned_credentials.py::test_subscription_mode_skips_import_with_clear_reason`) covers the short-circuit in isolation but would not catch a future refactor of `_perform_auto_switch` or `_restart_agent` that quietly bypassed the guard. The new test asserts on `reason == "subscription_mode"` specifically (not just `status == "skipped"`) so a refactor that moved the short-circuit into the `#421` `container_already_running` branch would fail explicitly. Test-only — no production code changes. Refs #606 Co-Authored-By: Claude --- ...subscription_auto_switch_no_cred_import.py | 390 ++++++++++++++++++ 1 file changed, 390 insertions(+) create mode 100644 tests/unit/test_subscription_auto_switch_no_cred_import.py diff --git a/tests/unit/test_subscription_auto_switch_no_cred_import.py b/tests/unit/test_subscription_auto_switch_no_cred_import.py new file mode 100644 index 000000000..39659434e --- /dev/null +++ b/tests/unit/test_subscription_auto_switch_no_cred_import.py @@ -0,0 +1,390 @@ +"""Regression test for #606: the SUB-003 subscription auto-switch path must +not re-enter file-based credential injection for subscription-mode agents. + +The chain under test (entered at ``_restart_agent``, the smallest entry +point that still exercises the real lifecycle injection logic):: + + subscription_auto_switch._restart_agent(agent_name) + → container_stop(container) # explicit stop + → start_agent_internal(agent_name) + → container_reload(container) # marks .status = "stopped" + → was_already_running = False # so the #421 skip does NOT apply + → inject_assigned_credentials(...) + → db.get_agent_subscription_id() → "sub-new-id" + → SHORT-CIRCUIT: returns {"status": "skipped", + "reason": "subscription_mode", ...} + +Load-bearing claim this test pins (must remain unmistakable to future +readers): on the auto-switch path, ``_restart_agent`` explicitly calls +``container_stop`` BEFORE ``start_agent_internal``. By the time +``start_agent_internal`` runs ``container_reload``, the container's +``.status`` is ``"stopped"``, so ``was_already_running`` is ``False`` and +the ``#421`` ``skip_injection`` branch does NOT apply. +``inject_assigned_credentials`` is therefore reached every time, and the +ONLY thing shielding us from the spurious ``.credentials.enc`` file-import +log is the subscription-mode short-circuit at +``services/agent_service/lifecycle.py:155``. + +A leaf-level test on ``inject_assigned_credentials`` alone (already +covered by ``test_inject_assigned_credentials.py:: +test_subscription_mode_skips_import_with_clear_reason``) would miss this +chain-level guarantee — a future refactor of ``_perform_auto_switch`` or +``_restart_agent`` could quietly bypass the guard without that leaf test +failing. This test pins the call chain end-to-end so the contract holds +regardless of how the auto-switch internals evolve. + +Explicit non-claim: this is NOT a concurrency test. There is no per-agent +switch/restart lock around ``assign_subscription_to_agent`` + +``container_stop`` + ``start_agent_internal``, so concurrent 429s for the +same agent could race. That's its own concern, out of scope for #606. + +Modules under test: + src/backend/services/subscription_auto_switch.py::_restart_agent + src/backend/services/agent_service/lifecycle.py::start_agent_internal + src/backend/services/agent_service/lifecycle.py::inject_assigned_credentials +""" + +from __future__ import annotations + +import asyncio +import contextlib +import importlib +import importlib.util +import os +import sys +import types +from unittest.mock import AsyncMock, MagicMock, Mock, patch + +import pytest + + +_BACKEND = os.path.abspath( + os.path.join(os.path.dirname(__file__), "..", "..", "src", "backend") +) + + +# ─── Module loaders ────────────────────────────────────────────────────── +# +# Mirrors the approach in ``test_inject_assigned_credentials.py``: load +# ``lifecycle.py`` in isolation with its heavy deps stubbed via sys.modules, +# so we exercise the REAL ``inject_assigned_credentials`` + +# ``start_agent_internal`` without paying for the full backend boot +# (which trips on the #589 REDIS_URL config check at import time). + + +def _preload_credential_encryption(): + """Return the in-process ``services.credential_encryption`` module, + loading it once if needed. **Must be idempotent** — another unit-test + file (``test_inject_assigned_credentials.py``) does the same preload + dance, and an unconditional ``sys.modules[...] = mod`` overwrite here + would invalidate that file's module-level reference and break its + ``patch.object`` calls when tests share a pytest session. + """ + services_stub = types.ModuleType("services") + services_stub.__path__ = [os.path.join(_BACKEND, "services")] + sys.modules.setdefault("services", services_stub) + + existing = sys.modules.get("services.credential_encryption") + if existing is not None: + return existing + + spec = importlib.util.spec_from_file_location( + "services.credential_encryption", + os.path.join(_BACKEND, "services", "credential_encryption.py"), + ) + mod = importlib.util.module_from_spec(spec) + sys.modules["services.credential_encryption"] = mod + spec.loader.exec_module(mod) + return mod + + +_credential_encryption = _preload_credential_encryption() + + +_mock_db = MagicMock() + + +class _HTTPException(Exception): + def __init__(self, status_code: int = 500, detail: str = "") -> None: + self.status_code = status_code + self.detail = detail + + +_SYS_MOCKS = { + "database": Mock(db=_mock_db), + "docker": Mock(), + "services.docker_service": Mock( + docker_client=Mock(), + get_agent_container=Mock(), + ), + "services.docker_utils": Mock( + container_stop=AsyncMock(), + container_remove=AsyncMock(), + container_start=AsyncMock(), + container_reload=AsyncMock(), + volume_get=AsyncMock(), + volume_create=AsyncMock(), + containers_run=AsyncMock(), + ), + "services.settings_service": Mock( + get_anthropic_api_key=Mock(return_value=""), + get_github_pat=Mock(return_value=""), + get_agent_full_capabilities=Mock(return_value=False), + ), + "services.skill_service": Mock(skill_service=MagicMock()), + "fastapi": Mock(HTTPException=_HTTPException), +} + + +def _load_lifecycle(): + """Load ``lifecycle.py`` under a private package name so this test's + copy is independent of any other test file that loads lifecycle. + + The ``services.agent_service`` slot is intentionally left populated + after this function returns: ``subscription_auto_switch._restart_agent`` + does a lazy ``from services.agent_service import start_agent_internal``, + and we attach our loaded ``start_agent_internal`` to that package below. + """ + pkg_name = "agent_service_pkg_under_test_606" + pkg_spec = importlib.util.spec_from_loader(pkg_name, loader=None, is_package=True) + pkg = importlib.util.module_from_spec(pkg_spec) + pkg.__path__ = [os.path.join(_BACKEND, "services", "agent_service")] + sys.modules[pkg_name] = pkg + + helpers_mod = Mock( + check_shared_folder_mounts_match=AsyncMock(return_value=True), + check_api_key_env_matches=Mock(return_value=True), + check_github_pat_env_matches=Mock(return_value=True), + check_resource_limits_match=Mock(return_value=True), + check_full_capabilities_match=Mock(return_value=True), + check_guardrails_env_matches=Mock(return_value=True), + validate_base_image=Mock(), + ) + read_only_mod = Mock(inject_read_only_hooks=AsyncMock(return_value={"success": True})) + file_sharing_mod = Mock(check_public_folder_mount_matches=Mock(return_value=True)) + + pkg.get_accessible_agents = Mock(return_value=[]) + pkg.get_agent_owner_id = Mock(return_value=1) + pkg.list_agents_data = Mock(return_value=[]) + + sys.modules[f"{pkg_name}.helpers"] = helpers_mod + sys.modules[f"{pkg_name}.read_only"] = read_only_mod + sys.modules[f"{pkg_name}.file_sharing"] = file_sharing_mod + sys.modules["services.agent_service"] = pkg + sys.modules["services.agent_service.helpers"] = helpers_mod + sys.modules["services.agent_service.read_only"] = read_only_mod + sys.modules["services.agent_service.file_sharing"] = file_sharing_mod + + if _BACKEND not in sys.path: + sys.path.insert(0, _BACKEND) + + _snapshot: dict[str, object] = {name: sys.modules.get(name) for name in _SYS_MOCKS} + sys.modules.update(_SYS_MOCKS) + + spec = importlib.util.spec_from_file_location( + f"{pkg_name}.lifecycle", + os.path.join(_BACKEND, "services", "agent_service", "lifecycle.py"), + ) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + + # Restore the real modules (or evict our Mock if the slot was empty) so + # we don't pollute downstream tests. The ``services.agent_service`` slot + # is deliberately left populated — see docstring above. + for name, original in _snapshot.items(): + if original is not None: + sys.modules[name] = original # type: ignore[assignment] + else: + sys.modules.pop(name, None) + return mod + + +_mod = _load_lifecycle() + + +def _load_auto_switch(): + """Import ``services.subscription_auto_switch`` with ``database`` stubbed + so import is side-effect-free. + + ``db_models`` is pure Pydantic with no heavy deps — we import the real + one if absent, rather than stubbing. A bare stub would persist in + ``sys.modules`` and break unrelated tests that later need + ``db_models.UserCreate`` or similar (e.g. + ``test_subscription_auto_switch_pingpong.py``'s ``tmp_db`` fixture + loads ``db.users`` transitively). + """ + db_module = types.ModuleType("database") + db_module.db = _mock_db + sys.modules["database"] = db_module + + if "db_models" not in sys.modules: + if _BACKEND not in sys.path: + sys.path.insert(0, _BACKEND) + import db_models # noqa: F401 — registers in sys.modules + + sys.modules.pop("services.subscription_auto_switch", None) + import services.subscription_auto_switch as auto_switch # noqa: WPS433 + importlib.reload(auto_switch) + return auto_switch + + +def _run(coro): + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + +@pytest.fixture(autouse=True) +def _reset(): + _mock_db.reset_mock() + _mock_db.get_agent_subscription_id.return_value = None + _mock_db.get_read_only_mode.return_value = {"enabled": False} + _mock_db.get_agent_skill_names.return_value = [] + # Re-stamp the database stub each test — other unit files evict the + # ``database`` slot in their teardown, and lifecycle's lazy + # ``from database import db`` would otherwise resolve to a fresh real + # ``database.py`` against the real DB. + sys.modules["database"] = _SYS_MOCKS["database"] + + +def test_auto_switch_restart_chain_does_not_invoke_credential_import(): + """SUB-003 auto-switch path: the full chain entry at ``_restart_agent`` + must short-circuit credential injection cleanly for a subscription-mode + agent. + + Pins the chain end-to-end. The companion leaf test in + ``test_inject_assigned_credentials.py`` covers the short-circuit in + isolation; this test pins that the chain ACTUALLY reaches the + short-circuit on the auto-switch path — without it, a future refactor + of ``_perform_auto_switch`` or ``_restart_agent`` could quietly bypass + the guard. + + The ``reason == "subscription_mode"`` assertion is load-bearing: it + proves the line-155 short-circuit fired, NOT the ``#421`` + ``container_already_running`` skip. A refactor that moved the + short-circuit to a different branch (e.g. coalesced it with ``#421``) + would silently pass without that specific assertion. + """ + # Subscription has just been assigned by `_perform_auto_switch`. + _mock_db.get_agent_subscription_id.return_value = "sub-new-id" + + # Sentinel: import_to_agent must NEVER be awaited on this chain. Wired + # via the encryption service so we'd see it even if the short-circuit + # regresses by one layer (e.g. somebody guards the retry loop but not + # the ``get_credential_encryption_service`` call). + sentinel_should_not_be_called = AsyncMock( + side_effect=AssertionError( + "import_to_agent must not be called on the auto-switch chain " + "for a subscription-mode agent — the lifecycle.py:155 " + "short-circuit failed to fire" + ) + ) + mock_encryption_service = MagicMock() + mock_encryption_service.import_to_agent = sentinel_should_not_be_called + + # Mock container starts as "running" so `_restart_agent` proceeds past + # its early-return guard, then flips to "stopped" when + # `container_reload` runs inside `start_agent_internal` — so + # `was_already_running` is False and the #421 skip path does NOT fire. + mock_container = MagicMock() + mock_container.status = "running" + + async def reload_side_effect(c): + c.status = "stopped" + + # Lazy imports that ``_restart_agent`` performs inside its body. + docker_service_stub = Mock( + get_agent_container=Mock(return_value=mock_container), + get_agent_status_from_container=Mock(return_value=Mock(status="running")), + ) + docker_utils_stub = Mock(container_stop=AsyncMock()) + + # ``_restart_agent`` does ``from services.agent_service import + # start_agent_internal``. Attach our loaded lifecycle's + # ``start_agent_internal`` so the chain actually executes (rather than + # picking up a fresh-imported, unmocked copy). + agent_service_pkg = sys.modules.get("services.agent_service") + if agent_service_pkg is None: + agent_service_pkg = types.ModuleType("services.agent_service") + sys.modules["services.agent_service"] = agent_service_pkg + agent_service_pkg.start_agent_internal = _mod.start_agent_internal + + # Capture inject_assigned_credentials' result so we can assert on + # ``reason == "subscription_mode"`` specifically. The chain entry + # point ``_restart_agent`` only returns the string ``"success"`` — + # it discards ``start_agent_internal``'s richer return dict — so we + # interpose a recording wrapper. + captured: dict = {} + real_inject = _mod.inject_assigned_credentials + + async def recording_inject(agent_name, **kwargs): + result = await real_inject(agent_name, **kwargs) + captured["credentials_result"] = result + return result + + auto_switch = _load_auto_switch() + + with contextlib.ExitStack() as stack: + # Stubs for subscription_auto_switch's lazy imports. + stack.enter_context(patch.dict( + sys.modules, + { + "services.docker_service": docker_service_stub, + "services.docker_utils": docker_utils_stub, + "services.agent_service": agent_service_pkg, + }, + )) + # Stubs the chain through start_agent_internal needs. These are + # patched on the loaded ``_mod`` directly because lifecycle.py + # captures these names into its module globals at import time. + stack.enter_context(patch.object( + _mod, "container_reload", AsyncMock(side_effect=reload_side_effect) + )) + stack.enter_context(patch.object(_mod, "container_start", AsyncMock())) + stack.enter_context(patch.object( + _mod, "wait_for_agent_ready", AsyncMock(return_value=True) + )) + stack.enter_context(patch.object( + _mod, "inject_assigned_credentials", recording_inject + )) + stack.enter_context(patch.object( + _mod, "inject_assigned_skills", + AsyncMock(return_value={"status": "skipped"}), + )) + stack.enter_context(patch.object( + _credential_encryption, + "get_credential_encryption_service", + return_value=mock_encryption_service, + )) + + result = _run(auto_switch._restart_agent("sub-agent")) + + # 1. The restart chain ran cleanly end-to-end. + assert result == "success", ( + f"Expected `_restart_agent` to return 'success', got {result!r}" + ) + + # 2. The sentinel was never awaited — the short-circuit fired before + # the file-import path was reached. + sentinel_should_not_be_called.assert_not_awaited() + + # 3. Load-bearing reason check: the short-circuit that fired is + # specifically the line-155 subscription-mode guard — NOT the #421 + # ``container_already_running`` skip. A future refactor that moved + # the short-circuit to a different branch (or coalesced it with + # #421) would silently pass without this assertion. + creds = captured.get("credentials_result") + assert creds is not None, "inject_assigned_credentials was not invoked" + assert creds["status"] == "skipped", ( + f"Expected credentials_result.status == 'skipped', got {creds!r}" + ) + assert creds["reason"] == "subscription_mode", ( + f"Expected credentials_result.reason == 'subscription_mode' " + f"(the lifecycle.py:155 short-circuit), got reason=" + f"{creds.get('reason')!r}. If this says 'container_already_running' " + f"the #421 skip fired instead — that branch does not apply on the " + f"auto-switch path because `_restart_agent` explicitly stops the " + f"container before `start_agent_internal`." + ) From 0d03e10ba39335716ff59febd6a6e57321b33106 Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Tue, 12 May 2026 01:50:53 +0100 Subject: [PATCH 2/5] docs(feature-flows): sync session-tab cold-turn lock + subscription auto-switch test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session-tab.md: step 6 documents the new `_ResumeLock(..., session.id, ...)` signature and the cold-turn key shape `session_lock:cold:{session_id}` (#779). Step 5 drops the "which the lock skips by design" claim about cold turns — no longer accurate. subscription-auto-switch.md: adds `tests/unit/test_subscription_auto_switch_no_cred_import.py` to the Tests table — chain-level regression for #606 that pins the SUB-003 auto-switch path reaches the `lifecycle.py:155` subscription_mode short-circuit and never re-enters file-based credential import. Refs #779 Refs #606 Co-Authored-By: Claude --- docs/memory/feature-flows/session-tab.md | 4 ++-- docs/memory/feature-flows/subscription-auto-switch.md | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/memory/feature-flows/session-tab.md b/docs/memory/feature-flows/session-tab.md index b5304b86f..977fcd25d 100644 --- a/docs/memory/feature-flows/session-tab.md +++ b/docs/memory/feature-flows/session-tab.md @@ -85,8 +85,8 @@ Voice mic and SSE dynamic status labels are **deferred** (each requires a backen 2. Persist the user message immediately so it appears even on failure. 3. Read `cached_claude_session_id`. 4. Resolve dynamic lock TTL via `_resolve_lock_ttl(agent_name)` = `db.get_execution_timeout(agent) + 30s`, capped at 7230s. The static 300s constant was removed in #759 because turns running longer than 5 min would silently drop the lock and allow concurrent JSONL writes. -5. SET in-flight sentinel `session_inflight:{session_id}` with the same TTL (#759). Bracketed via `_InflightSentinel` async context manager so DEL fires on success **and** exception. Distinct from the resume lock — the sentinel covers cold turns too, which the lock skips by design. Drives the `turn_in_progress` field on the GET endpoint. -6. Acquire `_ResumeLock(agent, cached_uuid, ttl_seconds=lock_ttl)` — Redis SET NX EX with async wait-and-retry (250 ms tick, 30 s ceiling). Cold turns skip the lock. Lock key constructed via `_session_lock_key(agent, uuid)` helper (shared by producer + any future probe — regression guard against split-brain typos). +5. SET in-flight sentinel `session_inflight:{session_id}` with the same TTL (#759). Bracketed via `_InflightSentinel` async context manager so DEL fires on success **and** exception. Distinct from the resume lock — the sentinel covers cold turns too. Drives the `turn_in_progress` field on the GET endpoint. +6. Acquire `_ResumeLock(agent, cached_uuid, session.id, ttl_seconds=lock_ttl)` — Redis SET NX EX with async wait-and-retry (250 ms tick, 30 s ceiling). Two key shapes: warm turns use `_session_lock_key(agent, uuid)`; cold turns key on `session_lock:cold:{session_id}` (#779) — same session serialised, different sessions still concurrent. Pre-#779 the cold path short-circuited to no-op and allowed two concurrent first-turn POSTs to race on `update_cached_claude_session_id` and orphan a JSONL. 7. Call `task_execution_service.execute_task(..., resume_session_id=cached, persist_session=True)`. The persist flag is unconditional — even cold turns must write the JSONL so turn 2's resume succeeds. 8. **Resume-failure fallback** (Phase 2.2): if execute_task returned failed with `"no conversation found"` AND we had a cached UUID, clear the cache, `mark_resume_failure`, retry **once** with `resume_session_id=None`. Log structured warning `event=session_resume_fallback`. Anthropic #39667 (cleanupPeriodDays) and #53417 (CLI upgrade) both produce this signal. 9. On success, trust `result.session_id` directly (Phase 1.3 fixed the agent-server stream parser to recognise `{"type":"system","subtype":"init"}` — no execution_log scan needed). Update `cached_claude_session_id` if changed, `mark_resume_success` to reset the failure counter. diff --git a/docs/memory/feature-flows/subscription-auto-switch.md b/docs/memory/feature-flows/subscription-auto-switch.md index 4f943df55..a1abe8fd1 100644 --- a/docs/memory/feature-flows/subscription-auto-switch.md +++ b/docs/memory/feature-flows/subscription-auto-switch.md @@ -70,6 +70,7 @@ import from `backend.services`. Keep the two in sync when editing either. | Frontend | `src/frontend/src/views/Settings.vue` | Toggle in Subscriptions section | | Tests | `tests/test_subscription_auto_switch.py` | Smoke tests | | Tests | `tests/unit/test_subscription_auto_switch_pingpong.py` | Unit regression for #444 ping-pong prevention; `TestRateLimitAging` (#476) pins 2h-window correctness | +| Tests | `tests/unit/test_subscription_auto_switch_no_cred_import.py` | Chain-level regression for #606 — pins `_restart_agent → start_agent_internal → inject_assigned_credentials` reaches the `lifecycle.py:155` `subscription_mode` short-circuit and never re-enters file-based credential import | | Tests | `tests/unit/test_iso_cutoff.py` | Format parity between `iso_cutoff(N)` and `utc_now_iso()` (#476) | | Util | `src/backend/utils/helpers.py::iso_cutoff` | Canonical cutoff helper for ISO-Z TEXT comparisons (#476) | | Spec | `docs/requirements/SUB-003-subscription-auto-switch.md` | Full requirements | From 2a2216cc34bfd6aa747c679497fc100a4a11c03a Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Tue, 12 May 2026 01:56:16 +0100 Subject: [PATCH 3/5] fix(tests): adopt sys.modules snapshot/restore in #606 chain test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #606 chain-level regression test added in 967b8747 introduced 17 new `tests/lint_sys_modules.py` violations — `sys.modules[...] = ...`, `.update(...)`, `.pop(...)`, `.setdefault(...)` writes at both module- import time (inside `_preload_credential_encryption`, `_load_lifecycle`, `_load_auto_switch`) and inside the test body and `_reset` fixture. `monkeypatch.setitem` is fixture-scoped and structurally cannot rewind the stubs planted during module import. Adopt the named snapshot/restore escape hatch documented in `tests/lint_sys_modules.py` (precedent: `tests/unit/test_telegram_webhook_backfill.py`): - Add top-level `_STUBBED_MODULE_NAMES` listing the 19 module slots this file stubs across its four loader paths. - Add autouse `_restore_sys_modules` fixture that snapshots those slots at test setup and restores them at teardown. This bounds the actual #762 concern (cross-file pollution within the same pytest session) without forcing a structurally-impossible rewrite of the import-time loaders. Verification: - `python tests/lint_sys_modules.py` — target file now reports 0 violations (was 17). The remaining `test_cleanup_unreachable_orphan.py` failure is a pre-existing unrelated case. - `pytest tests/unit/test_subscription_auto_switch_no_cred_import.py` — passes. - `pytest tests/unit/test_inject_assigned_credentials.py` — passes, confirming the snapshot/restore doesn't leak into the sibling leaf test that also loads `lifecycle.py`. Test-only — no production code changes. Refs #606 Refs #762 Co-Authored-By: Claude --- ...subscription_auto_switch_no_cred_import.py | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/unit/test_subscription_auto_switch_no_cred_import.py b/tests/unit/test_subscription_auto_switch_no_cred_import.py index 39659434e..9e9934e6a 100644 --- a/tests/unit/test_subscription_auto_switch_no_cred_import.py +++ b/tests/unit/test_subscription_auto_switch_no_cred_import.py @@ -63,6 +63,64 @@ ) +# ─── sys.modules snapshot/restore (Issue #762) ─────────────────────────── +# +# This file loads ``lifecycle.py`` and ``subscription_auto_switch.py`` in +# isolation with their heavy deps stubbed via ``sys.modules`` at IMPORT +# time — the lint's preferred ``monkeypatch.setitem`` is fixture-scoped +# and cannot rewind a stub planted before any test runs. We adopt the +# named snapshot/restore escape hatch documented in +# ``tests/lint_sys_modules.py`` (precedent: +# ``tests/unit/test_telegram_webhook_backfill.py``): the autouse fixture +# snapshots these slots at test setup and restores them at teardown so +# cross-file pollution is bounded to this module's loaders, not leaked +# into unrelated tests sharing the pytest session. + +_STUBBED_MODULE_NAMES = [ + # _preload_credential_encryption + "services", + "services.credential_encryption", + # _SYS_MOCKS (transient during _load_lifecycle, but defensive) + "database", + "docker", + "services.docker_service", + "services.docker_utils", + "services.settings_service", + "services.skill_service", + "fastapi", + # _load_lifecycle (private package + the public services.agent_service slot) + "agent_service_pkg_under_test_606", + "agent_service_pkg_under_test_606.helpers", + "agent_service_pkg_under_test_606.read_only", + "agent_service_pkg_under_test_606.file_sharing", + "services.agent_service", + "services.agent_service.helpers", + "services.agent_service.read_only", + "services.agent_service.file_sharing", + # _load_auto_switch + "services.subscription_auto_switch", + "db_models", +] + + +@pytest.fixture(autouse=True) +def _restore_sys_modules(): + """Snapshot sys.modules before each test and restore after. + + Bounds the blast radius of this file's module-loader stubs so they + cannot leak into other test files in the same pytest session. + """ + saved = {name: sys.modules.get(name) for name in _STUBBED_MODULE_NAMES} + try: + yield + finally: + for name, value in saved.items(): + if value is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = value + + # ─── Module loaders ────────────────────────────────────────────────────── # # Mirrors the approach in ``test_inject_assigned_credentials.py``: load From 90dcbba89c494f2241376ada144c6376e2d16aba Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Tue, 12 May 2026 02:05:07 +0100 Subject: [PATCH 4/5] docs(lint): mention named-helper escape hatch in sys.modules error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lint at `tests/lint_sys_modules.py` documents three ways out of a bare-`sys.modules` violation: `monkeypatch.setitem` / `delitem`, moving to `conftest.py`, and the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` named-helper pair (precedent: `tests/unit/test_telegram_webhook_backfill.py`). Only the first two are surfaced in the per-line error message. For tests that load real backend modules in isolation with their heavy deps stubbed at import time, neither of the documented options applies: - `monkeypatch.*` is fixture-scoped and structurally cannot rewind a stub planted before any test runs (the stubs are needed during `spec_from_file_location` + `exec_module`, which executes at module-import time). - `conftest.py` would leak the stubs to every test in the directory, the exact #762 failure mode the lint exists to prevent. The named-helper escape hatch is the right answer for this case, and the lint already exempts it — but discoverability is poor. PR #783 and #606 both hit the lint, read the error, and re-discovered the escape hatch independently after digging into the script's docstring. Surface it in the error directly so the next author sees all three options at first encounter, not after archaeology. No behavioral change — pure error-text expansion. The lint's exit codes, detection logic, and baseline mechanics are unchanged. The committed baseline file is unaffected. Refs #762 Co-Authored-By: Claude --- tests/lint_sys_modules.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/lint_sys_modules.py b/tests/lint_sys_modules.py index 2e27e4c09..13f513ed3 100644 --- a/tests/lint_sys_modules.py +++ b/tests/lint_sys_modules.py @@ -51,7 +51,11 @@ FIX_HINT = ( "use monkeypatch.setitem(sys.modules, ...) or " "monkeypatch.delitem(sys.modules, ..., raising=False), " - "or move to conftest.py" + "or move to conftest.py. " + "For import-time stubs that monkeypatch can't reach (e.g. preloads " + "of heavy backend deps before any fixture runs), declare a top-level " + "`_STUBBED_MODULE_NAMES = [...]` list + an autouse `_restore_sys_modules` " + "fixture in this file (precedent: tests/unit/test_telegram_webhook_backfill.py)" ) From 63aab5b465dd6c39fea0ccab24cafcc21c6d27aa Mon Sep 17 00:00:00 2001 From: Andrii Pasternak Date: Tue, 12 May 2026 02:05:41 +0100 Subject: [PATCH 5/5] fix(tests): adopt sys.modules snapshot/restore in #783 cleanup test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a64814, PR #783) plants three import-time stubs in `sys.modules`: - `docker` (line 64) — the unit venv doesn't ship the docker package - `services.docker_service` (line 76) — eager-imported by `services/__init__.py` - `database` (line 79) — hit transitively by cleanup_service The lint+baseline shipped in #791 (3043631e) merged ~4 hours after #783, but `#791`'s baseline file omits this entry — almost certainly generated against a pre-#783 dev state and never refreshed before merge. Result: `lint-sys-modules` job in `backend-unit-test.yml` has been red on dev since both PRs landed, and the failure travels to every PR branched off that state — including #606's PR. Apply the same named-helper escape hatch used by the #606 file and the original `tests/unit/test_telegram_webhook_backfill.py` precedent: a top-level `_STUBBED_MODULE_NAMES` list naming the three slots, plus an autouse `_restore_sys_modules` fixture that snapshots them at test setup and restores at teardown. Bounds the cross-file pollution to this test's own scope — the exact invariant #762's lint protects. Verification: - `python tests/lint_sys_modules.py` — clean: 223 violations in 65 files, baseline allows 223, no new violations. - `pytest tests/test_lint_sys_modules.py` — all 25 tests pass, including `test_committed_baseline_matches_current_repo_state` (was failing). - `pytest tests/unit/test_cleanup_unreachable_orphan.py` — passes. Test-only — no production code changes. Cross-PR fix: clears CI on this branch and on dev. The underlying "stale baseline at PR merge" question (why didn't #791's own CI catch this?) is a separate concern worth a follow-up issue. Refs #762 Refs #783 Co-Authored-By: Claude --- tests/unit/test_cleanup_unreachable_orphan.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/unit/test_cleanup_unreachable_orphan.py b/tests/unit/test_cleanup_unreachable_orphan.py index 35708e54e..7acceaeab 100644 --- a/tests/unit/test_cleanup_unreachable_orphan.py +++ b/tests/unit/test_cleanup_unreachable_orphan.py @@ -84,6 +84,42 @@ async def _exec(*a, **kw): monkeypatch.setitem(sys.modules, "database", MagicMock(name="database_stub")) +# ─── sys.modules snapshot/restore (Issue #762) ─────────────────────────── +# +# The three preload blocks above plant stubs at module-import time — +# monkeypatch is fixture-scoped and structurally cannot rewind them, and +# moving them to conftest would leak the stubs across the whole tests/unit/ +# directory (the exact #762 failure mode). Adopt the named snapshot/restore +# escape hatch documented in `tests/lint_sys_modules.py` (precedent: +# `tests/unit/test_telegram_webhook_backfill.py`): the autouse fixture +# snapshots these slots at test setup and restores them at teardown so +# the stubs cannot leak into other tests in the same pytest session. + +_STUBBED_MODULE_NAMES = [ + "docker", + "services.docker_service", + "database", +] + + +@pytest.fixture(autouse=True) +def _restore_sys_modules(): + """Snapshot sys.modules before each test and restore after. + + Bounds the blast radius of this file's import-time stubs so they + cannot leak into other test files in the same pytest session. + """ + saved = {name: sys.modules.get(name) for name in _STUBBED_MODULE_NAMES} + try: + yield + finally: + for name, value in saved.items(): + if value is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = value + + def _iso_past_minutes(minutes: int) -> str: return ( datetime.now(timezone.utc).replace(microsecond=0) - _td(minutes=minutes)