test(subscription): pin auto-switch chain skips .credentials.enc import (#606) - #800
Merged
Conversation
AndriiPasternak31
marked this pull request as draft
May 12, 2026 00:49
AndriiPasternak31
marked this pull request as ready for review
May 12, 2026 01:13
AndriiPasternak31
force-pushed
the
AndriiPasternak31/issue-606-plan
branch
from
May 12, 2026 02:04
ea3a129 to
191959c
Compare
AndriiPasternak31
changed the base branch from
dev
to
AndriiPasternak31/lint-baseline-orphan-test
May 12, 2026 02:04
vybe
requested changes
May 12, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
This PR requires the following changes before merge:
- Retarget base branch to
dev— the current base (AndriiPasternak31/lint-baseline-orphan-test) was merged to dev as PR #796 earlier today. Change base via the GitHub UI: Edit → Base: dev.
Code quality, tests, and docs are all clean. This is the only blocker.
AndriiPasternak31
changed the base branch from
AndriiPasternak31/lint-baseline-orphan-test
to
dev
May 12, 2026 22:10
Contributor
Author
|
Hi Eugene — base is already |
1 task
…rt (#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>
…uto-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>
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>
…ssage 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>
`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
force-pushed
the
AndriiPasternak31/issue-606-plan
branch
from
May 14, 2026 14:14
191959c to
63aab5b
Compare
vybe
approved these changes
May 14, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Rebase landed clean — shared baseline commit auto-skipped by Git. Five PR-specific commits intact. Chain-level regression test design is solid. LGTM.
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
2 participants
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
Adds a chain-level regression test for #606 that pins the SUB-003 auto-switch path against silently regressing the
lifecycle.py:155subscription_modeshort-circuit.#606 was originally fixed in #612/#663 — this PR adds the missing chain-level coverage so a future refactor of
_perform_auto_switchor_restart_agentcannot silently bypass the guard.Why this test, when a leaf test already exists
tests/unit/test_inject_assigned_credentials.py::test_subscription_mode_skips_import_with_clear_reasoncovers the short-circuit in isolation. It would still pass if a future refactor of_perform_auto_switchquietly stopped reachinginject_assigned_credentialsat all — leaving #606 silently un-fixed.This new test enters at the smallest entry point that still exercises the real lifecycle injection chain (
subscription_auto_switch._restart_agent) and asserts:_restart_agentreturns"success"(the chain ran end-to-end)import_to_agentwas never awaited (the file-import path was not reached)inject_assigned_credentialsreturnedreason == "subscription_mode"specifically — proving the line-155 short-circuit fired, NOT the#421container_already_runningskip (which does not apply on the auto-switch path because_restart_agentexplicitly stops the container beforestart_agent_internal)The third assertion is load-bearing: without it, a refactor that coalesced the short-circuit into the
#421branch would silently pass.Test plan
uv run pytest tests/unit/test_subscription_auto_switch_no_cred_import.py -v→ 1 passeddevRefs
Co-Authored-By: Claude noreply@anthropic.com