fix(cli): validate version strings on --revert before pip handoff

[cmeans-claude-dev]

Summary

Closes #40.

cli/version.py::_do_revert previously fed target_version (or the previous_version loaded from ~/.local/state/mcp-synology/global.yaml) straight into subprocess.run(["uv", "tool", "install", "--force", f"mcp-synology=={prev}"]). shell=False already neutralized command injection, but a value like --revert latest, --revert 1.2, or --revert "1.0.0; whatever" produced an opaque pip "Invalid requirement" error instead of an actionable CLI message — and a hand-corrupted previous_version field in the state file would propagate the same garbage on a no-arg --revert.

What changed

• New _validate_version_string() helper applies a loose PEP 440-ish regex (^\d+\.\d+\.\d+([-.]?[a-zA-Z0-9]+)*$) that accepts 0.5.1, 0.5.1-rc1, 0.5.0a1, 1.2.3.post4, etc. and rejects empty / whitespace-only input, missing patch segments, leading/trailing whitespace, latest, v-prefixes, shell metacharacters, and path-traversal payloads. On rejection it raises click.ClickException so click renders the standard Error: ... line and exits 1 — the same shape as the other CLI error paths — and the message names the expected format.
• Validation runs once at the chokepoint inside _do_revert (after prev is resolved from either source) so both the --revert <VER> and corrupt-state code paths are covered with one call site.

Why not also --auto-upgrade?

The issue title mentions both flags, but --auto-upgrade is a click.Choice(["enable", "disable"]) — click rejects anything else before the value reaches our code. And the _do_auto_upgrade upgrade target is a literal mcp-synology@latest, not user input. So no version validation is needed on that path.

Tests

23 new test cases:

• TestValidateVersionString: 8 valid versions accepted, 13 invalid inputs rejected (including the four examples called out in the acceptance criteria: latest, 1.2, , 1.0.0; whatever, plus arg-injection probes like 1.0.0 --extra and ==1.2.3 and a path-traversal payload).
• TestDoRevert: two regression tests verifying that an invalid explicit --revert <VER> and a corrupt state-file previous_version both raise ClickException and never reach subprocess.run.

577 unit tests pass at 96.14% coverage. ruff/mypy clean.

Test plan

• uv run pytest tests/core/test_cli_version.py -v — all 63 tests pass (was 40)
• uv run pytest — full unit suite green
• uv run ruff check src/ tests/
• uv run ruff format --check src/ tests/
• uv run mypy src/
• Smoke (post-install): mcp-synology --revert latest → Error: Invalid version string: 'latest'. Expected MAJOR.MINOR.PATCH ... + exit 1
• Smoke: mcp-synology --revert 0.5.0 still works as before (mocked subprocess in tests; real-world path unchanged)

🤖 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]

LGTM

[cmeans]

QA Round 1 — Failed (one substantive finding, doc drift only)

Verified

Reviewed both commits (7841891 fix + 1f1234d CHANGELOG backfill), regex shape, validator placement, click wiring, and the --auto-upgrade exclusion claim.

• _validate_version_string() uses the regex from issue #40 verbatim, raises click.ClickException with format hint, runs once at the chokepoint inside _do_revert after prev is resolved from either source — so both --revert <VER> and corrupt-state paths are covered with one call site.
• --auto-upgrade exclusion holds. Click Choice(["enable", "disable"]) filters at parse time; _do_auto_upgrade shells mcp-synology@latest (uv) or pipx upgrade mcp-synology — neither path takes user-supplied version input. state["previous_version"] = current writes the result of importlib.metadata.version(), and the revert path validates that value if/when it's read back.
• Issue #40 acceptance criteria: regex (✔), Error: ... + exit 1 + format hint (✔), no other ingress site needs the regex (✔, verified by grep -rn "revert\|_validate_version_string" src/ — main.py:78 is the only ingress), unit tests cover all four AC examples latest / 1.2 / / 1.0.0; whatever (✔), CHANGELOG ### Fixed entry under ## Unreleased (✔).
• Local stack on 1f1234d: uv run pytest reports 577 passed / 100 deselected (= integration+vdsm, run separately) / 96.14% coverage. ruff check, ruff format --check, mypy all clean. Matches the CHANGELOG entry verbatim.
• CI: 12/12 required checks green incl. vdsm integration tests SUCCESS.
• Smoke against the installed CLI (uv run mcp-synology --revert <X>), all returning Error: Invalid version string: '<X>'. Expected MAJOR.MINOR.PATCH with optional pre/post suffix (e.g. 0.5.1, 0.5.1-rc1, 1.2.3.post4). + exit 1: latest, 1.2, , 1.0.0; whatever, v1.2.3, 1.0.0 --extra. Valid --revert 0.5.1 (current) → Already running 0.5.1 — nothing to do. exit 0. Pre-release --revert 0.5.1-rc1 passes the validator and reaches the existing "Cannot detect package manager" branch (because uv run isn't a uv tool-installed entry).

Findings

F1 (substantive — doc drift). PR body + commit 7841891 body understate the new-test count.

The parametrize lists in tests/core/test_cli_version.py add 8 valid + 13 invalid + 2 regression = 23 new test cases. The PR body and commit message both say:

• "Twenty new tests: 8 valid versions accepted, 12 invalid inputs rejected"
• "all 63 tests pass (was 43)"

git show origin/main:tests/core/test_cli_version.py | pytest --collect-only reports 40 as the baseline, and 40 + 23 = 63 (= the actual 577 - 554 delta in the CHANGELOG). The 13th invalid case is "1.0.0 --extra", which is the most security-relevant input in the list (an arg-injection attempt rejected before reaching subprocess.run) — undercutting it in the body is the wrong place to lose visibility.

Suggested fix (PR body only — single edit, no code change needed):

• s/Twenty new tests/Twenty-three new tests/
• s/12 invalid inputs rejected/13 invalid inputs rejected/ (occurs twice in body — Summary and the ## Tests section)
• s/(was 43)/(was 40)/ in the test plan

The CHANGELOG entry's "577 unit tests pass at 96.14% coverage" has no per-test counts and is correct as-is. Commit message of 7841891 carries the same drift; can be amended on the round-2 push if convenient, but PR body is the canonical user-facing description and the higher-priority fix.

PR-body checkboxes

Held off flipping the seven manual-test boxes since F1 requires editing the PR body itself; flipping them now would force a rebase-merge to preserve them. All seven are pre-validated above and will flip in round 2.

Disposition

QA Failed applied as the final act. Once F1 is addressed, the Ready for QA label will be re-applied automatically by the existing label workflow on the next push (or by Dev manually after a body-only edit).

[cmeans]

Applying QA Failed — single substantive finding (F1 above) is doc drift in PR body / commit message about new-test counts (claims 20 new / 12 invalid / was 43; actual 23 new / 13 invalid / was 40). PR-body edit only, no code change required. Code, regex, validator placement, click wiring, scope vs. issue #40 ACs, local stack (577 pass, 96.14%, ruff/mypy clean), and CI (12/12 required green incl. vdsm) are all good. Smoke confirms all four issue-#40 AC inputs rejected with the expected actionable message + exit 1. PR-body checkboxes held off until F1 fix lands.

[cmeans-claude-dev]

F1 addressed via PR-body-only edit (no code change required, per QA's suggestion):

• 20 new test cases → 23 new test cases
• 12 invalid inputs rejected → 13 invalid inputs rejected (and called out the additional 1.0.0 --extra, ==1.2.3, path-traversal probes that the count was undercounting)
• (was 43) → (was 40) in the test plan checkbox

Commit 7841891's message carries the same drift but is left as-is — amending it would force a rebase-merge and lose the QA approval state, and the PR body is the canonical user-facing description.

Toggling QA Failed → Ready for QA.

[cmeans]

QA Round 2 — PASS

F1 closed via PR body edit (no code change, head still 1f1234d):

• "23 new test cases" (was "Twenty")
• "8 valid versions accepted, 13 invalid inputs rejected" (was 12)
• "all 63 tests pass (was 40)" (was "(was 43)")
• Bonus: the parenthetical now explicitly calls out the arg-injection probes (1.0.0 --extra, ==1.2.3) and the path-traversal payload, so the security-relevant inputs aren't lost in the count.

Re-verified on the unchanged head per feedback_verification_before_signoff_label:

• uv run pytest → 577 passed, 100 deselected (= integration+vdsm, run separately), 96.14% coverage. Matches CHANGELOG entry verbatim.
• uv run ruff check src/ tests/ clean.
• uv run ruff format --check src/ tests/ — 69 files already formatted.
• uv run mypy src/ — no issues, 28 source files.
• CI: 12/12 required green incl. vdsm integration tests SUCCESS (CI from round 1 still applies — no new push, same SHA).

Manual smoke from round 1 still applies (same code): all four issue-#40 AC inputs (latest, 1.2, , 1.0.0; whatever) plus arg-injection (1.0.0 --extra) and v-prefix (v1.2.3) all rejected with the actionable Error: Invalid version string: '<X>'. Expected MAJOR.MINOR.PATCH ... + exit 1. Valid --revert 0.5.1 (current) hits the no-op short-circuit.

PR-body manual-test boxes 1–7 flipped now that the body is final. Ready for QA Signoff applied as the final act. Awaiting maintainer's QA Approved.

[cmeans]

Applying Ready for QA Signoff — F1 (the only round-1 finding) closed via PR body edit. All 12 required CI checks green incl. vdsm. Local re-verified on unchanged head 1f1234d: 577 pass, 96.14% coverage, ruff/format/mypy clean. PR-body checkboxes 1–7 flipped (all pre-validated in round 1; body is now final).