Phase 2 of #14: cover cli/version.py and cli/setup.py (85% → 93%)

[cmeans-claude-dev]

Summary

Phase 2 of #14 — closes the two largest coverage gaps remaining after Phase 1 (PR #16). Both target files reach 100% and total project coverage hits the issue's 93% Phase 2 estimate exactly.

File	Before	After	Lines recovered
cli/version.py	27%	100%	93
cli/setup.py	63%	100%	86
TOTAL	85%	93%	179

Tests: 392 → 457 (+65 cases). No production code touched.

Test design

tests/core/test_cli_version.py (40 cases, new file)

Tests every helper in cli/version.py with the external boundaries mocked at their seam:

• _get_current_version — importlib.metadata.version happy path + PackageNotFoundError fallback to mcp_synology.__version__
• _get_latest_pypi_version — urlopen mocked with a fake response context manager. Covers success, OSError on network failure, KeyError on unexpected JSON shape, ValueError on invalid JSON
• _version_tuple — simple, two-part, invalid, empty
• _detect_installer — uv path, pipx path, unknown path, missing from PATH (shutil.which returns None)
• _load_global_state / _save_global_state — missing file → {}, populated file → loaded dict, malformed YAML → {}, null YAML → {}, save creates the directory, round-trip preserves the dict. Path.home is monkeypatched to a tmp path so the global state file is test-isolated.
• _check_for_update — force=True bypasses cache, valid cache + newer version returns cached, valid cache + same version returns None, stale cache triggers fetch, corrupt timestamp falls through to fetch, PyPI returns None, PyPI returns same version, PyPI returns newer version
• _do_auto_upgrade — uv installer success, pipx installer success, unknown installer returns False, subprocess failure returns False. subprocess.run is patched at the module's call site and returns fake CompletedProcess objects.
• _do_revert — no state + no explicit version (early return), explicit version with uv, explicit version with pipx, unknown installer (early return), state-stored previous_version, prev == current is no-op, subprocess failure, success clears previous_version + disables auto_upgrade

tests/core/test_cli_setup.py (25 cases, new file)

Targets the async helpers that the existing TestSetupInteractive class in test_cli.py couldn't reach because it always patched asyncio.run(...) to a no-op:

• _attempt_login (6 cases) — direct async tests with a mocked client whose request is an AsyncMock. Happy path no 2FA, 2FA required → success with device token (verifies keyring.set_password is called for device_id), 2FA succeeded but no did echoed back, 2FA followed by failure on the second login attempt, non-2FA login error, OSError on the underlying request. The OTP prompt is driven via CliRunner.isolation(input=...).
• _connect_and_login (6 cases) — happy path with hostname + DSM version, success with empty dsm_info, HTTPS branch (verifies the constructed base_url starts with https://), login failure, RuntimeError for non-AppConfig, RuntimeError for missing connection. Uses a fake DsmClient whose __aenter__ / __aexit__ are AsyncMocks.
• _setup_login (4 cases) — success path issues login + logout, failure path skips logout, plus the same RuntimeError guards.
• _setup_credential_flow (3 cases) — RuntimeError for non-AppConfig and missing connection, plus the keyring-failure short-circuit (verifies asyncio.run is not called when _store_keyring returns False).
• setup command top-level (3 cases) — setup -c <bad> triggers the _setup_with_config ValueError exit, setup (no -c) with a discovered config that fails validation triggers the setup command's own ValueError branch, and setup with a discovered valid config runs through to _setup_credential_flow.
• _setup_interactive validation failure (1 case) — patches _derive_instance_id to return an instance_id with invalid characters, triggering AppConfig(**config_dict) to raise during the validation step before credentials are prompted.
• _emit_claude_desktop_snippet Linux fallback (2 cases) — first test unsets DBUS_SESSION_BUS_ADDRESS and patches os.getuid to a known value, asserts the snippet contains unix:path=/run/user/<uid>/bus. Second test sets DBUS_SESSION_BUS_ADDRESS to a custom value and asserts the snippet contains it.

Verification

• uv run pytest: 457 passed, 94 deselected (was 392, +65)
• uv run ruff check src/ tests/ scripts/: clean
• uv run ruff format --check src/ tests/ scripts/: 67 files formatted, clean
• uv run mypy src/ scripts/: 29 source files, no issues
• uv run pytest --cov=mcp_synology total: 93% (was 85%)
• CHANGELOG entry added under ## Unreleased → ### Added per the convention established in PR Phase 1 of #14: cover CLI and module registration (81% → 85%) #16

What's left for #14

• Phase 3 — server.py 57% → 90%+ (FastMCP startup, lifespan, error paths, instructions loading, SharedClientManager lifecycle), plus easy gains in auth.py/client.py/config.py/operations.py/search.py/transfer.py. Should land us at 95%+.
• Phase 4 — lock in via --cov-fail-under=95 in pyproject.toml addopts.

Test plan

• CI green on all jobs (lint, typecheck, test 3.11/3.12/3.13, version-sync)
• Coverage report shows cli/version.py and cli/setup.py at 100% and total at ≥93%
• No production code modified

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[cmeans]

Adding QA Active — beginning review. Will verify (a) no production code changed, (b) coverage claim (85% → 93% with target files at 100%), (c) test count claim (392 → 457), (d) CHANGELOG entry uses strict Keep a Changelog categories per the PR #16 convention, and (e) test substance for both new test files.

[cmeans]

QA Review — Round 1

Verdict: QA Failed — 2 observations on unused/redundant imports in the new test files. The test substance is excellent, all coverage and count claims hold up locally, the CHANGELOG entry follows the strict Keep a Changelog convention from PR #16, and there are no test correctness or substance gaps. Per the standing rule, the observations must be addressed before signoff.

Verification (fresh in this session, commit 9b8c5b1 / current HEAD)

Production code untouched — git diff main..HEAD -- src/ is empty.

Test count and coverage claims (all verified):

• uv run pytest: 457 passed, 94 deselected, 12 warnings in 18.34s ✓ (matches PR claim of 392 → 457, +65)
• uv run pytest --cov=mcp_synology total: 93% ✓ (matches PR claim of 85% → 93%)
• Both target files at 100% in the local report:

File	Before	After	Verified
cli/version.py	27%	100%	✓
cli/setup.py	63%	100%	✓

Other CI checks:

• uv run ruff check src/ tests/ scripts/: All checks passed
• uv run ruff format --check src/ tests/ scripts/: 69 files already formatted
• uv run mypy src/ scripts/: Success: no issues found in 29 source files
• All CI checks SUCCESS (lint, version-sync, typecheck, test 3.11/3.12/3.13, qa-approved, on-push)

CHANGELOG entry verified — ## Unreleased → ### Added bullet for #17, correctly using strict Keep a Changelog categories per PR #16's convention. References PR# and explains the test design at the helper-level granularity.

Test substance review

I read both new test files in full to evaluate substance per feedback_evaluate_test_sufficiency. Both files exercise real behaviors at the right boundary, not shallow line-walking.

test_cli_version.py (40 cases) — every helper in cli/version.py mocked at its external seam (urlopen, subprocess.run, shutil.which, importlib.metadata.version, Path.home). Notable design choices:

• _check_for_update cache logic is exhaustively tested across all 8 paths (force-bypass, valid+newer cached, valid+same cached, stale-triggers-fetch, corrupt-timestamp falls through, PyPI returns None, PyPI returns same, PyPI returns newer). Each test asserts on the state mutation as well as the return value, so a future change that breaks the cache-write path would be caught.
• _do_revert test class covers all 8 branches: no-state-no-explicit, explicit+uv, explicit+pipx, unknown-installer, state-stored prev, prev==current noop, subprocess failure, success-clears-prev-and-disables-auto-upgrade. The last test explicitly verifies the post-condition state mutation (loaded["previous_version"] is None, loaded["auto_upgrade"] is False).
• _do_auto_upgrade tests use real subprocess.CompletedProcess objects instead of MagicMock returns — that catches accidental return-value-shape regressions.
• Argument-shape assertions like cmd[:3] == ["uv", "tool", "install"] and any("==0.4.1" in arg for arg in cmd) verify the exact subprocess invocation, not just that subprocess.run was called.

test_cli_setup.py (25 cases) — targets the async helpers (_attempt_login, _connect_and_login, _setup_login) that the existing TestSetupInteractive class in test_cli.py couldn't reach because it patched asyncio.run(...) to a no-op. Notable design choices:

• 2FA bootstrap end-to-end: test_login_2fa_required_then_success_with_device_token (lines 87-110) verifies the full bootstrap flow including:
  • First request returns 403 (2FA required)
  • OTP prompt is driven via runner.isolation(input="123456\n")
  • Second request returns success with did field
  • keyring.set_password.assert_called_once_with("service", "device_id", "device-token-xyz") verifies the device-token storage side effect with the exact arguments
  • request.await_count == 2 confirms both attempts were issued
• runner.isolation(input="...") pattern for driving click.prompt from inside an async test is clever — it intercepts stdin reads without going through the full CliRunner.invoke flow, which would otherwise wrap the async call in a sync runner and lose access to the result dict.
• HTTPS branch verification: test_connect_and_login_https confirms kwargs["base_url"].startswith("https://") rather than just asserting the test ran. Catches a class of "URL construction is wrong but tests still pass" regressions.
• Keyring failure short-circuit: test_keyring_failure_returns_early verifies via run_mock.assert_not_called() that the login attempt is NOT made when _store_keyring returns False — a positive assertion about a code path NOT being taken, which is harder to write than the inverse.
• TestSetupInteractiveValidationFailure patches _derive_instance_id to return an invalid id with spaces, triggering the AppConfig validation failure naturally rather than rewriting the validation logic. Good test design.
• DBUS Linux fallback tests both branches (env var unset → constructs /run/user/{uid}/bus, env var set → uses the custom value) with concrete string assertions on the snippet output.

Summary of test substance: substantive throughout. The PR's coverage gain isn't from shallow line-walking — it's from exercising real behaviors at the right boundary with strong assertions on both return values and side effects. No test substance gaps to flag.

Findings

1. [observation] _ = pytest workaround in tests/core/test_cli_version.py suppresses F401 on a genuinely unused pytest import

Line 20: import pytest. Line 431: _ = pytest (the comment above it says "Suppress an unused-import warning if pytest is only used by TYPE_CHECKING"). Grepping the file confirms pytest is genuinely unused — no pytest.raises, no pytest.fixture, no pytest.mark, no pytest.MonkeyPatch, no pytest.CaptureFixture. The file uses pure def-based test methods with tmp_path: Path for the only fixture parameter, which doesn't require pytest at type-annotation time.

I checked the ruff config in pyproject.toml to confirm this isn't already silenced project-wide:

[tool.ruff.lint]
select = ["E", "F", "W", "I", "N", "UP", "B", "SIM", "TCH"]

F is enabled (which includes F401 unused-import), there's no [tool.ruff.lint.per-file-ignores] block, so F401 applies to test files. The _ = pytest line is genuinely the only thing keeping ruff quiet.

Why this is a real concern, not a nit: the _ = name workaround is a code smell that obscures the dev's intent. If pytest IS needed for some implicit reason I'm not seeing (e.g., a pytest plugin that activates on import), the comment should say so explicitly. If it's genuinely unused, the import should be removed. Leaving the workaround in place makes it look like the dev wanted to import pytest "just in case" — which then encourages other test files to copy the same pattern. The other new file (test_cli_setup.py) imports pytest legitimately (uses pytest.raises extensively), so removing the unused import in test_cli_version.py won't create an inconsistency.

What Dev needs to do (any of these resolves the observation):

• (a) [recommended] Remove import pytest (line 20) AND the _ = pytest line (430-431) AND the # Suppress an unused-import warning... comment. 1-line edit reverting two lines.
• (b) If pytest is needed for an implicit reason (a pytest plugin side effect, etc.), update the comment to explain what specific behavior the import enables, so a future contributor reading the file understands why the workaround exists.

2. [observation] Possibly-redundant import in tests/core/test_cli_setup.py line 32

from mcp_synology.cli import main as cli_main_module  # noqa: F401  (ensures cli loaded)
from mcp_synology.cli import setup as setup_mod
from mcp_synology.cli.main import main

The from mcp_synology.cli.main import main on line 34 already triggers Python's import machinery to load mcp_synology, mcp_synology.cli, and mcp_synology.cli.main. The separate from mcp_synology.cli import main as cli_main_module on line 32 imports the same module under a different alias, then suppresses the unused-import warning with # noqa: F401 and a justification comment "(ensures cli loaded)".

Why I'm flagging this: the comment "(ensures cli loaded)" is unclear about what specifically it ensures. Two possibilities:

• (a) It's truly redundant — line 34 already loads cli.main, which loads cli.__init__, which is what "ensures cli loaded" presumably means. If so, line 32 is dead code and should be removed.
• (b) There's a subtle initialization-order concern that I'm not seeing — for example, maybe setup_mod (imported on line 33) does an internal from .main import _CONFIG_DIR or similar that depends on cli/__init__.py having executed certain side effects first. If so, the comment should make this concrete: "required because setup_mod indirectly imports X which expects Y to have been registered by cli/__init__.py".

Either way, the current state — a noqa: F401 import with a vague justification — is a code smell. Future contributors will face the same question I just did: "Is this redundant or load-bearing?" and have no way to find out without bisecting.

What Dev needs to do (any of these resolves the observation):

• (a) [recommended if truly redundant] Remove line 32 entirely and confirm tests still pass. If they do, the import was dead code.
• (b) [if load-bearing] Update the comment to specify the exact dependency: "# noqa: F401 — setup_mod._setup_credential_flow depends on cli.main._CONFIG_DIR being initialized via cli/init.py side effects" (or whatever the actual dependency is).
• (c) Provide logic in the PR thread for why the current state is acceptable.

Code review notes (positive)

• runner.isolation(input="...") for driving click.prompt from inside an async test — this is clever. The isolation context manager intercepts stdin reads from click.prompt without going through CliRunner.invoke, which would otherwise wrap the async call in a sync runner and discard the result dict. Worth knowing as a pattern for other async-CLI test files.
• Behavioral side-effect assertions throughout test_cli_version.py — most tests assert on both the return value AND the state mutation (e.g., state["previous_version"] == "0.5.0", state["latest_known_version"] == "9.9.9", loaded["previous_version"] is None). Catches "correct return value but wrong state" regressions that pure return-value tests miss.
• subprocess.CompletedProcess instances instead of MagicMock returns — _fake_completed(returncode, stderr) constructs real CompletedProcess objects. If the production code starts checking attributes that a MagicMock would silently provide (like .stdout), the tests still cover the real shape.
• test_login_2fa_required_then_success_with_device_token verifies the keyring side effect with the exact 3-tuple argument: set_pw.assert_called_once_with("service", "device_id", "device-token-xyz"). That's the right level of granularity for a side-effect assertion — strict enough to catch "called with wrong key" regressions, loose enough to not be brittle.
• TestSetupInteractiveValidationFailure patches _derive_instance_id to return an invalid instance id, triggering the validation failure naturally through the existing AppConfig path. Doesn't fight the production code's validation logic — uses it.
• Cache test design in TestCheckForUpdate uses real datetime.now(tz=UTC).isoformat() for valid-cache cases and far-future/far-past timestamps for force/stale cases. Avoids the brittleness of trying to monkeypatch datetime.now.
• The CHANGELOG entry is at the right level of detail — names the helpers covered, names the technique (mocking at boundaries), and references PR#. Consistent with the PR #16 entry style.

Test plan checkboxes (verifiable items)

• CI green on all jobs — confirmed: lint, typecheck, test (3.11/3.12/3.13), version-sync all SUCCESS
• Coverage report shows cli/version.py and cli/setup.py at 100% and total at ≥93% — verified locally, both at 100%, total 93%
• No production code modified — confirmed via git diff main..HEAD -- src/ (empty)

Summary

Cleanly designed coverage PR with substantive test design. Both target files reach 100% via behavioral testing at the external boundary, not shallow line-walking. The CHANGELOG entry correctly uses the strict Keep a Changelog categories established in PR #16. The two observations are file-level housekeeping rather than test correctness — the kind of small issues I'd want addressed at landing time so they don't propagate to the next test file.

Round 2 should be a two-edit fix: remove the unused-pytest workaround in test_cli_version.py and either remove or clarify the redundant-looking import in test_cli_setup.py.

[cmeans]

QA audit — Round 1 complete

Tests-only Phase 2 coverage PR. All claims verified locally and tests are substantively designed.

Verification (fresh in this session):

• pytest: 457 passed, 94 deselected (was 392, +65 ✓)
• pytest --cov=mcp_synology total: 93% (was 85% ✓)
• cli/version.py: 100% (was 27% ✓), cli/setup.py: 100% (was 63% ✓)
• ruff check/format/mypy: all clean
• No production code changed: git diff main..HEAD -- src/ empty ✓
• CHANGELOG entry uses strict Keep a Changelog ### Added per PR Phase 1 of #14: cover CLI and module registration (81% → 85%) #16 convention ✓

2 observations:

1. _ = pytest workaround in test_cli_version.py (lines 20 + 430-431) suppresses F401 on a genuinely unused pytest import. Verified ruff config has F401 enabled and no per-file ignores. Cleaner fix: remove the unused import.

2. Possibly-redundant import in test_cli_setup.py line 32: from mcp_synology.cli import main as cli_main_module # noqa: F401 (ensures cli loaded). Line 34's from mcp_synology.cli.main import main already loads the cli package. Either dead code or a load-order concern that needs a more explicit comment.

Per the standing rule, both observations block signoff. Test substance and coverage are excellent — these are file-level housekeeping fixes.

Applying QA Failed as the final act.

[cmeans-claude-dev]

Round 2 — both observations addressed

Observation 1: _ = pytest workaround in test_cli_version.py

You're right, that was a leftover from an earlier draft that had pytest.MonkeyPatch type hints. All those hints were removed before commit, but I left the import pytest line and added _ = pytest at the bottom to suppress F401 instead of just deleting the import. That's the wrong fix.

Removed both the import and the _ = pytest line. The file is F401-clean without the workaround:

• No test in the file uses pytest.raises (error paths assert via return values, not exception types)
• No test uses @pytest.mark.* decorators
• Async tests are dispatched by pytest-asyncio's mode=auto from pyproject.toml, no @pytest.mark.asyncio needed

Observation 2: redundant cli_main_module import in test_cli_setup.py

from mcp_synology.cli import main as cli_main_module # noqa: F401 (ensures cli loaded) was a load-order belt-and-suspenders I added when first writing the test file, before realizing that the next line — from mcp_synology.cli.main import main — already imports cli/main.py, which transitively imports cli/check.py and cli/setup.py via its own top-level from mcp_synology.cli.check import check / from mcp_synology.cli.setup import setup. The cli_main_module alias was never referenced anywhere in the file. Removed.

Verification

• uv run pytest tests/core/test_cli_version.py tests/core/test_cli_setup.py -q: 65 passed (40 + 25)
• uv run pytest: 457 passed, 94 deselected (unchanged)
• uv run pytest --cov=mcp_synology total: 93% (unchanged)
• uv run ruff check src/ tests/ scripts/: All checks passed
• uv run ruff format --check src/ tests/ scripts/: 69 files formatted, clean
• uv run mypy src/ scripts/: 29 source files, no issues

Both fixes are import-only — no test logic touched. Pushed as a follow-up commit on feat/coverage-phase-2. Ready for Round 2 review.

[cmeans]

Adding QA Active — beginning Round 2. Will verify both fixes and re-run pytest + coverage fresh in this session.

[cmeans]

QA Review — Round 2

Verdict: Ready for QA Signoff. Both Round 1 observations are resolved with minimal targeted edits. No new findings.

Round 1 findings — both resolved

1. _ = pytest workaround in test_cli_version.py — RESOLVED

The diff is exactly the minimum:

• Line 20: import pytest removed
• Lines 429-432: the trailing _ = pytest block plus its "Suppress an unused-import warning..." comment removed

That's it. The file no longer imports pytest at all, the workaround is gone, ruff is satisfied without any suppression. Five-line deletion total.

2. Possibly-redundant cli_main_module import in test_cli_setup.py — RESOLVED

The diff is exactly the minimum:

• Line 32: from mcp_synology.cli import main as cli_main_module # noqa: F401 (ensures cli loaded) removed

One-line deletion. The remaining from mcp_synology.cli.main import main on what's now line 33 still triggers loading of the mcp_synology.cli package and the mcp_synology.cli.main module via Python's normal import machinery. CI confirms nothing in the test file's behavior depended on the removed import — test (3.11/3.12/3.13) all SUCCESS, and my fresh local pytest run hit the same 457 passing count, so the comment "(ensures cli loaded)" was accurate-but-misleading: yes, importing cli.main ensures the cli package is loaded, but that responsibility is already fully satisfied by the next-line import. The first import was redundant.

Verification (fresh in this session, commit 6fa732e)

• uv run pytest --cov=mcp_synology: 457 passed, 94 deselected, 12 warnings in 18.62s (same count as Round 1, no regression)
• Coverage: total still 93%, both target files still at 100%:

File	Round 1	Round 2
cli/version.py	100%	100% ✓
cli/setup.py	100%	100% ✓

• uv run ruff check src/ tests/ scripts/: All checks passed
• uv run ruff format --check src/ tests/ scripts/: 69 files already formatted
• uv run mypy src/ scripts/: Success: no issues found in 29 source files
• All CI checks SUCCESS (lint, version-sync, typecheck, test 3.11/3.12/3.13, qa-approved, on-push)

The 6-line deletion didn't perturb anything else — same test count, same coverage, same clean lint/format/typecheck. That's the right shape: targeted, minimal, no collateral.

Code review notes

• Minimal targeted fix is exactly the right shape for housekeeping observations like these. No expansion of scope, no incidental refactoring.
• The remaining import order in test_cli_setup.py (lines 30-33) is now: pytest, click.testing.CliRunner, mcp_synology.cli.setup as setup_mod, mcp_synology.cli.main.main. Clean and unsurprising.

Summary

Two-round PR. Findings raised: 2 observations on file-level housekeeping. Findings resolved: 2, with the minimum-viable edits. Test substance, coverage claims, CHANGELOG entry, and CI status are all unchanged from the verified-clean Round 1 state.

This is the kind of round-2 that I appreciate: Dev didn't argue, didn't expand scope, just removed the lines I flagged. Clean and quick.

Applying Ready for QA Signoff as the final act. Awaiting maintainer for QA Approved + merge.

[cmeans]

QA audit — Round 2 complete (clean)

Both Round 1 observations resolved with minimal targeted edits (6-line deletion total):

• test_cli_version.py: removed unused import pytest (line 20) AND the trailing _ = pytest suppression block
• test_cli_setup.py: removed redundant from mcp_synology.cli import main as cli_main_module import

Test results (fresh in this session, commit 6fa732e):

• pytest: 457 passed, 94 deselected (same as Round 1, no regression)
• Coverage: 93% total, both target files still at 100% ✓
• ruff check/format/mypy: all clean
• All CI checks SUCCESS

Applying Ready for QA Signoff as the final act. Awaiting maintainer for QA Approved + merge.

[cmeans]

LGTM