fix(tests): conftest sys.modules baseline + tests-side lint (closes #762) - #791
Merged
Conversation
) Cross-file sys.modules pollution caused all 50 tests in test_audit_log_unit.py to fail at fixture setup when run as part of the full integration suite (`pytest tests/`). Standalone they pass 53/53. Root cause: test_validation.py (line 35) and test_watchdog_unit.py (line 54) assign `sys.modules["utils.helpers"] = _stub` at module- collection time. The stub is missing `iso_cutoff`. When test_audit_log_unit.py's audit_ops fixture runs `del sys.modules["db.audit"]` followed by `from db.audit import ...`, the import chain hits `db/__init__.py → db/schedules.py → from utils.helpers import iso_cutoff` and raises ImportError. This bug class has appeared 4 times across the test suite (#752, #714, #763); a per-file patch would be the 4th occurrence. This PR retires the pattern instead. Changes: - tests/conftest.py: capture a snapshot of canonical backend modules (`utils.helpers`, `models`, `db.*` subtree, `services.*`, `adapters.*`, …) immediately after the existing `_preload_backend_models()` runs. An autouse function-scoped fixture restores the snapshot both before and after every test. Restores ONLY keys whose baseline was a real module object — keys that didn't exist at preload are left alone, so test files can install their own session-scoped stubs without regression. - tests/test_audit_log_unit.py: convert the 3 bare `sys.modules` ops (lines 34-40, 130-131, 347-348) to monkeypatch.delitem; remove the redundant module-level `utils.helpers` stub (conftest preloads the real one). - tests/lint_sys_modules.py (new): AST-based lint script flagging bare `sys.modules[k]=v`, `del sys.modules[k]`, `sys.modules.setdefault/pop/ update(...)` outside the allowlist (conftest.py files, monkeypatch.* calls, `_STUBBED_MODULE_NAMES`+`_restore_sys_modules` pair). Baseline-ratcheted to grandfather the 223 pre-existing violations across 65 files — new violations fail CI, the baseline count is the ceiling. Regenerate with `--regenerate-baseline`. - tests/lint_sys_modules_baseline.txt (new): committed baseline of per-file violation counts. CI fails if any file exceeds. - tests/test_lint_sys_modules.py (new): 25 unit tests covering each banned pattern (positive) and each exempted form (negative — monkeypatch, conftest allowlist, helper-marker pair), plus baseline-diff semantics and the repo-state invariant that the committed baseline matches the current scan. - .github/workflows/backend-unit-test.yml: new `lint-sys-modules` job runs `python tests/lint_sys_modules.py` (stdlib-only, ~2 min). Verification: - target file in isolation: 53/53 pass (unchanged). - target file under polluter pressure (test_validation.py, test_watchdog_unit.py, test_self_execute.py, test_proactive_audit_unit.py, test_inter_agent_timeout_unit.py): 0 errors on test_audit_log_unit across 3 consecutive runs (AC2). - failing-ID-set comparison across all `tests/test_*_unit.py`: baseline 105 failing IDs → with-fix 52 failing IDs (53 fixed, 0 new). - lint tests: 25/25 pass. - lint script clean exit against committed baseline. The importlib spike (`--import-mode=importlib`, plan Step 2) was tried first and came back RED: the pollution class is deliberate `sys.modules[k]=stub` assignment in test code, which importlib mode does not address. Codex's caveat in the plan was correct. Refs: #762
vybe
approved these changes
May 11, 2026
vybe
left a comment
Contributor
There was a problem hiding this comment.
LGTM — clean test-infrastructure fix. Root cause well-identified, approach retires the bug class rather than patching the 4th occurrence. Baseline-ratcheted lint + CI job is the right call.
AndriiPasternak31
added a commit
that referenced
this pull request
May 12, 2026
`tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, 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 (3043631) 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 <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
May 12, 2026
…ervice (#474) Module-level sys.modules mutations in this file (6 of them) flagged the sys.modules lint added in #791. The lint's documented escape hatch is the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` fixture pair established in tests/unit/test_telegram_webhook_backfill.py — recognised by AST as a self-contained snapshot/restore pattern that does not leak between tests. Adds both constructs at module level. The 41 #474 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
May 12, 2026
`tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, 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 (3043631) 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 <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 12, 2026
…leanup_unreachable_orphan.py (#796) #791 (which added the tests/lint_sys_modules.py lint and its baseline) landed AFTER #783 (which added tests/unit/test_cleanup_unreachable_orphan.py). The baseline was generated without including the orphan-test file's 3 top-level `sys.modules[...] = stub` assignments at lines 64, 76, 79 (docker, services.docker_service, database stubs installed before the test module's services.cleanup_service import). Result: every PR opened against dev fails the lint job on these 3 lines, even when the PR's own changes are clean. Fix: grandfather the file into the baseline at its current count (3). This matches how the same shape is handled elsewhere — e.g. test_watchdog_unit.py (3 violations, the precedent the orphan-test file's own comment cites). The lint script's baseline path is the intended mechanism for accepting pre-existing module-level stub installs that cannot use monkeypatch. Verified locally: python3 tests/lint_sys_modules.py emits zero violations in the real test tree (only .venv site-packages remain, which CI runners don't have). Refs #762, #783, #791 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
May 13, 2026
…ervice (#474) Module-level sys.modules mutations in this file (6 of them) flagged the sys.modules lint added in #791. The lint's documented escape hatch is the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` fixture pair established in tests/unit/test_telegram_webhook_backfill.py — recognised by AST as a self-contained snapshot/restore pattern that does not leak between tests. Adds both constructs at module level. The 41 #474 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
dolho
added a commit
that referenced
this pull request
May 13, 2026
…form_default_model The lint job on #443's PR went red on a file that is NOT in the PR's diff — `tests/test_platform_default_model.py` carries 6 bare `sys.modules` mutations that were merged to dev without an entry in `tests/lint_sys_modules_baseline.txt`. Same #802 rebase-race shape as the original #791 vs #783 incident: baseline-introducing PR (#791) and violation-introducing PR landed disjointly, both green pre-merge, dev went red post-merge, the next PR off dev (this one) inherits it. Ratcheting the baseline here unblocks #443. Same precedent as #796 which baselined `test_cleanup_unreachable_orphan.py` for the same reason. The proper fix is to convert the file's bare sys.modules writes to `monkeypatch.setitem` / `monkeypatch.delitem`; that's scope-creep relative to the onboarding work this PR is doing and should land separately. (That file's count is currently 0 against the lint, so a follow-up that converts it can drop the baseline line altogether.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 13, 2026
…race (#828) Issue #802: baseline-style lint (sys.modules pollution check) only ran on pull_request, so two PRs disjointly touching baseline + new test file could both pass pre-merge CI yet leave dev red post-merge. Concrete case: #791 landed the lint baseline 4h after CI green; meanwhile #783 merged a new test file with 3 baselined-elsewhere violations. No re-run on either side caught the resulting state — the next PR off dev (#606) inherited a red CI. Adds a push trigger on dev/main so lint-sys-modules fires post-merge. Per-PR diff jobs (test, diff) are gated to pull_request only since base/head don't exist on a push. Related to #802 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AndriiPasternak31
added a commit
that referenced
this pull request
May 13, 2026
…ervice (#474) Module-level sys.modules mutations in this file (6 of them) flagged the sys.modules lint added in #791. The lint's documented escape hatch is the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` fixture pair established in tests/unit/test_telegram_webhook_backfill.py — recognised by AST as a self-contained snapshot/restore pattern that does not leak between tests. Adds both constructs at module level. The 41 #474 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 13, 2026
* fix(onboarding): six defects in self-host deploy path (#443) Friction surfaced on a fresh clone → start.sh → login walkthrough. Each fix is small and independent; bundled here per the issue's "all are onboarding hygiene" framing. 1. scripts/deploy/start.sh — auto-generate SECRET_KEY and INTERNAL_API_SECRET if blank (same pattern as the existing CREDENTIAL_ENCRYPTION_KEY block, extracted to a helper). Fail fast with a clear message if ADMIN_PASSWORD is blank rather than booting into a state the operator can't log into. 2. .env.example — document FRONTEND_PORT=80 with an inline comment explaining "remap if your host's port 80 is taken." The var was already read by docker-compose.yml and start.sh but wasn't reachable from the quickstart. 3. scripts/deploy/start.sh — replace "login: admin/password" hint with "login: admin / ADMIN_PASSWORD from .env". New users took "password" as the literal default. 4. src/mcp-server/src/index.ts + README.md — drop hardcoded http://localhost:3000/api-keys hint. Port was wrong (frontend default is 80) and ignored FRONTEND_PORT overrides. Replaced with "your Trinity web UI → Settings → MCP Keys". 5. scripts/deploy/clean.sh — new script. Stops compose + removes leftover agent-* containers + trinity-agent-network. Without this, a fresh install inherits Exited zombie agents from previous tests and the first new agent collides on AGENT_SSH_PORT_START (2222). Preserves data volumes intentionally — operators can wipe those manually. 6. src/mcp-server/Dockerfile — point healthcheck at /health, not /mcp. The /mcp endpoint rejects HEAD requests with 400 (which `wget --spider` sends), so `docker compose ps` reported the container as unhealthy despite serving traffic. /health returns 200 to both HEAD and GET. Verified live: post-rebuild, trinity-mcp-server reports `healthy` within 30s. No schema, API, or runtime-behavior changes. No migration needed. Related to #443 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): baseline 6 pre-existing sys.modules writes in test_platform_default_model The lint job on #443's PR went red on a file that is NOT in the PR's diff — `tests/test_platform_default_model.py` carries 6 bare `sys.modules` mutations that were merged to dev without an entry in `tests/lint_sys_modules_baseline.txt`. Same #802 rebase-race shape as the original #791 vs #783 incident: baseline-introducing PR (#791) and violation-introducing PR landed disjointly, both green pre-merge, dev went red post-merge, the next PR off dev (this one) inherits it. Ratcheting the baseline here unblocks #443. Same precedent as #796 which baselined `test_cleanup_unreachable_orphan.py` for the same reason. The proper fix is to convert the file's bare sys.modules writes to `monkeypatch.setitem` / `monkeypatch.delitem`; that's scope-creep relative to the onboarding work this PR is doing and should land separately. (That file's count is currently 0 against the lint, so a follow-up that converts it can drop the baseline line altogether.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 task
AndriiPasternak31
added a commit
that referenced
this pull request
May 13, 2026
…ervice (#474) Module-level sys.modules mutations in this file (6 of them) flagged the sys.modules lint added in #791. The lint's documented escape hatch is the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` fixture pair established in tests/unit/test_telegram_webhook_backfill.py — recognised by AST as a self-contained snapshot/restore pattern that does not leak between tests. Adds both constructs at module level. The 41 #474 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 14, 2026
) (#798) * fix(circuit-breaker): only count TCP unreachability toward circuit (#474) A dropped MCP sync connection used to flip the per-agent circuit breaker because the EPIPE from a half-written response and subsequent ReadTimeouts from concurrent pollers were all counted as agent-health failures. The agent was healthy; the circuit was poisoned by client-side / mid-request noise. agent_client._request() now classifies exceptions via a shared `is_circuit_failure()` helper (`CIRCUIT_FAILURE_EXCEPTIONS` = ConnectError, ConnectTimeout). Read/Write timeouts, PoolTimeout, WriteError, ReadError, RemoteProtocolError still surface as AgentNotReachableError (typed-error contract preserved) but do NOT record a failure. Raw OSError subclasses (BrokenPipeError, ConnectionResetError) propagate uncaught — they're local resource bugs, not agent unhealth. asyncio.CancelledError is re-raised explicitly. monitoring_service.check_network_health() now applies the same rule so the /health probe and inline /api/* requests agree. Any HTTP response (200..599) records success — symmetric with _request(), so a wedged agent's stale failures clear as soon as it answers. aggregate_health() adds an explicit `status_code >= 500 → UNHEALTHY` branch so a listening-but-broken agent isn't silently rated HEALTHY by the new rule. Tests: - tests/unit/test_circuit_breaker.py: 12 cases pinning the classifier contract + disjoint-tuples invariant. - tests/integration/test_circuit_breaker.py: 13 cases driving real AgentClient via MockTransport against real Redis circuit state — hard vs soft failures, mixed interleave, open-circuit fast-fail, recovery on 200, and the deferred half-open soft-failure probe-lock behaviour. - tests/integration/test_monitoring_service.py: NEW — mirrors the classification rule on the /health probe path and pins the new `status_code >= 500 → UNHEALTHY` aggregator branch. * docs(flows): sync agent-monitoring + AgentClient flows with #474 circuit-breaker fix Refresh feature-flow docs to match the failure-classification narrowing landed in 5b51b4c: - agent-monitoring.md: replace the stale check_network_health() snippet with the post-#474 version (lazy import of CIRCUIT_FAILURE_EXCEPTIONS / TRANSIENT_TRANSPORT_EXCEPTIONS, asyncio.CancelledError re-raise, circuit record_success on any 100..599). Add the /health 5xx -> UNHEALTHY rule to Status Aggregation Priority. Append revision-history entry. - execution-queue.md: append 2026-05-12 revision-history entry pinning the contract — only ConnectError/ConnectTimeout increment the threshold; read/write timeouts, pool exhaustion, mid-write broken-pipe/reset, and garbled framing still raise AgentNotReachableError but no longer poison the circuit; raw OSError subclasses propagate uncaught. - scheduling.md: expand the one-sentence RELIABILITY-001 description to spell out the new classifier (was implicit "3 consecutive failures"). - feature-flows.md: add Recent Updates row for #474 pointing at the three flows above. Refs #474 * chore(tests): register test_circuit_breaker.py in registry (#474) Refs #474 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): register integration circuit-breaker + monitoring tests (#474) Follow-up to eac9211, which only registered the unit test. The two integration suites added in PR #798 — tests/integration/test_circuit_breaker.py and tests/integration/test_monitoring_service.py — were on disk but not in tests/registry.json, so the test-runner subagent's catalog was incomplete (pytest auto-discovery still ran them; this is purely a catalog hygiene fix). Refs #474 * chore(tests): adopt _STUBBED_MODULE_NAMES helper in test_monitoring_service (#474) Module-level sys.modules mutations in this file (6 of them) flagged the sys.modules lint added in #791. The lint's documented escape hatch is the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` fixture pair established in tests/unit/test_telegram_webhook_backfill.py — recognised by AST as a self-contained snapshot/restore pattern that does not leak between tests. Adds both constructs at module level. The 41 #474 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(architecture): note #474 narrows circuit-breaker input to TCP failures Addendum to architecture.md§Backend services so the audit trail aligns with the #474 fix: clarifies that only TCP/connection errors count toward the Redis-backed circuit; HTTP 4xx/5xx and 502/503/504 are treated as application errors and skip the failure counter. State names, transitions, and dormant-state behavior (#631) are unchanged — this is a narrowing of what counts as a "failure", not a redesign. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 14, 2026
* chore(tests): baseline 3 pre-existing sys.modules mutations in test_cleanup_unreachable_orphan.py #791 (which added the tests/lint_sys_modules.py lint and its baseline) landed AFTER #783 (which added tests/unit/test_cleanup_unreachable_orphan.py). The baseline was generated without including the orphan-test file's 3 top-level `sys.modules[...] = stub` assignments at lines 64, 76, 79 (docker, services.docker_service, database stubs installed before the test module's services.cleanup_service import). Result: every PR opened against dev fails the lint job on these 3 lines, even when the PR's own changes are clean. Fix: grandfather the file into the baseline at its current count (3). This matches how the same shape is handled elsewhere — e.g. test_watchdog_unit.py (3 violations, the precedent the orphan-test file's own comment cites). The lint script's baseline path is the intended mechanism for accepting pre-existing module-level stub installs that cannot use monkeypatch. Verified locally: python3 tests/lint_sys_modules.py emits zero violations in the real test tree (only .venv site-packages remain, which CI runners don't have). Refs #762, #783, #791 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): override "test" sentinel in security conftest (closes #804) `tests/conftest.py` setdefaults REDIS_PASSWORD/REDIS_BACKEND_PASSWORD to the literal "test" at global pytest import so backend modules can be imported without real Redis creds. `tests/security/conftest.py` was then using `os.environ.setdefault(...)` to overlay real `.env` values — a no-op because the sentinel was already set. Result: `redis-cli` ran with `-a test` against a healthy stack and the ACL acceptance tests failed for the wrong reason. Pop the sentinel before the `.env` overlay and use direct assignment. Add a regression test that asserts the live env value is not the sentinel and (when `.env` defines the keys) matches the `.env` value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 14, 2026
`tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, 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 (3043631) 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 <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
May 14, 2026
…rt (#606) (#800) * test(subscription): pin auto-switch chain skips .credentials.enc import (#606) 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 <noreply@anthropic.com> * docs(feature-flows): sync session-tab cold-turn lock + subscription auto-switch test 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 <noreply@anthropic.com> * fix(tests): adopt sys.modules snapshot/restore in #606 chain test The #606 chain-level regression test added in 967b874 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 <noreply@anthropic.com> * docs(lint): mention named-helper escape hatch in sys.modules error message 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 <noreply@anthropic.com> * fix(tests): adopt sys.modules snapshot/restore in #783 cleanup test `tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, 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 (3043631) 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 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Summary
Cross-file
sys.modulespollution caused all 50 tests intests/test_audit_log_unit.pyto fail at fixture setup when run as partof the full integration suite (
pytest tests/). Standalone they pass53/53. This PR retires the whole bug class instead of patching the
target file alone — it would have been the 4th occurrence of the same
shape (#752, #714, #763).
Root cause (AC3 — polluter identified)
tests/test_validation.py:35andtests/test_watchdog_unit.py:54assign
sys.modules["utils.helpers"] = _stubat module-collectiontime. The stub is missing
iso_cutoff. Whentest_audit_log_unit.py'saudit_opsfixture runsdel sys.modules["db.audit"]followed byfrom db.audit import ...,the import chain hits
db/__init__.py → db/schedules.py → from utils.helpers import iso_cutoffand raises
ImportError: cannot import name 'iso_cutoff' from 'utils.helpers'.Reproducer (deterministic, no
pytest-randomlyneeded because pollutionfires at collection):
uv run pytest tests/test_audit_log_unit.py tests/test_validation.py -q # → 53 errors on tests/test_audit_log_unit.pyWhat changed
tests/conftest.py— capture a snapshot of canonical backendmodules (
utils.helpers,models,db.*subtree,services.*,adapters.*,dependencies,utils.credential_sanitizer)immediately after the existing
_preload_backend_models()runs,before any test file is collected. An autouse function-scoped fixture
restores the snapshot both before and after every test. The
restore is conservative — only keys whose baseline was a real module
object get restored; keys that didn't exist at preload are left alone
so test files can install their own session-scoped stubs without
regression.
Captured at conftest import time, not at fixture setup, per Codex
finding 3 in the plan: a fixture-setup snapshot would record an
already-polluted value and treat it as truth.
tests/test_audit_log_unit.py— convert the 3 baresys.modulesops (lines 34–40, 130–131, 347–348) to
monkeypatch.delitem; removethe now-redundant module-level
utils.helpersstub.tests/lint_sys_modules.py— AST-based lint flaggingsys.modules[k]=v,del sys.modules[k],sys.modules.setdefault/pop/update(...)outside the allowlist(
conftest.pyfiles;monkeypatch.*calls; the_STUBBED_MODULE_NAMES+_restore_sys_modulesself-containedsnapshot pattern precedented by
tests/unit/test_telegram_webhook_backfill.py).Baseline-ratcheted: the survey of
tests/**/*.pyturned up 223pre-existing violations across 65 files. Fixing all of them is out of
scope for fix(tests): test_audit_log_unit.py — 50 collection ImportErrors when run in full suite #762. The lint reads
tests/lint_sys_modules_baseline.txtand fails CI only when a file exceeds its baseline count. Regenerate
with
python tests/lint_sys_modules.py --regenerate-baseline.tests/lint_sys_modules_baseline.txt— committed baseline ofper-file violation counts.
tests/test_lint_sys_modules.py— 25 unit tests covering eachbanned pattern (positive), each exempted form (negative — monkeypatch,
conftest allowlist, helper-marker pair) per Claude subagent's H3,
plus baseline-diff semantics and a repo-state invariant that the
committed baseline matches the current scan.
.github/workflows/backend-unit-test.yml— newlint-sys-modulesjob runspython tests/lint_sys_modules.py(stdlib-only, ~2 min, single Ubuntu runner).
Importlib spike — RED
The plan's Step 2 spiked
--import-mode=importlibfirst. It came backRED: the bug class is deliberate
sys.modules[k] = stubassignment intest code, which importlib mode does not address (Codex's caveat in the
plan was correct). Symptoms persisted unchanged across both modes.
That's why this PR ships the red path (conftest baseline + lint)
rather than the green path (importlib + lint).
Verification
pytest tests/test_audit_log_unit.py(isolation)pytest tests/test_audit_log_unit.py tests/test_validation.py(polluter pair)pytest tests/test_*_unit.pyfailing-ID settests/test_lint_sys_modules.pypython tests/lint_sys_modules.pypytest-randomlyis not installed locally (the project keeps itin-workflow only). The CI plugin set will re-run this verification
under 3 seeds when the workflow fires.
Acceptance criteria
pytest tests/reports 0 ERROR/FAILED lines fortests/test_audit_log_unit.py(verified via deterministiccollection-order reproducer; pytest-randomly will exercise random
order in CI).
mix across 3 consecutive runs.
tests/test_validation.py:35overwritessys.modules["utils.helpers"]with a stub missing
iso_cutoffat module-collection time. Underthe conftest-baseline fix this remains observability of the historical
polluter, not an active bug — the autouse restore neutralizes it,
and the lint rule prevents new occurrences in future tests.
Test plan
backend-unit-testworkflow runs the newlint-sys-modulesjob and the existing pytest matrix.
python tests/lint_sys_modules.pyexits 0.pytest tests/test_lint_sys_modules.py25/25.pytest tests/test_audit_log_unit.py tests/test_validation.py— 0 errors on target.Refs: #762