fix(0.30.2): MCP version detection now reads uv tool list (the upstream binary has no --version flag) - #262
Conversation
…am binary has no --version flag)
User reported on a fresh v0.30.1 upgrade:
Dependencies:
keboola-mcp-server (Keboola MCP Server)local version unknown (latest on PyPI: v1.59.1)
and `kbagent update` printed:
Success: kbagent v0.30.1 (already up to date) | keboola-mcp-server vunknown -> v1.59.1
Root cause: `_get_local_mcp_version()` ran `keboola_mcp_server --version`
expecting a version string. The upstream MCP binary does NOT honour
`--version` -- it prints its argparse usage block with returncode 0,
the regex finds nothing parseable, and the function returns None.
Fix: prefer `uv tool list` output (canonical for the kbagent
`doctor --fix` install method). The output line
`keboola-mcp-server v1.59.1` carries the exact version. Added
helper `_uv_tool_list_get_mcp_version(stdout)` that:
- splits per line, skips indented continuation lines (binary listings
under another tool),
- requires exact equality on the package name as the first token,
- validates the second token as a semver-ish `vX.Y.Z`,
- strips the leading `v`.
The existing `_uv_tool_list_has_mcp` predicate is kept for the install-
method detector. Resolution order is now:
1. `uv tool list` -- preferred, works for `uv tool install` binaries.
2. `importlib.metadata.version("keboola-mcp-server")` -- works for
pip-in-current-env installs (rare in production).
3. `keboola_mcp_server --version` -- best-effort fallback for a
future MCP server that adds the flag. The cleaned-output filter
now strips `usage:` lines so a future `python3.12.9` path
component cannot be mistaken for a version.
Tests: 8 new in `TestUvToolListGetMcpVersion` (exact match, multi-tool
real-world output, similar-name rejection, indented-binary rejection,
malformed-version rejection, leading-v stripping, pre-release
tolerance, empty input). 5 rewritten in `TestGetLocalMcpVersion`
including a real-world regression test pinning the verbatim upstream
usage-help output -- it must NOT be mistaken for a version. Plus a
forward-compat test for the day MCP adds a real `--version` flag.
`make check` clean: 2,768 tests pass.
padak
left a comment
There was a problem hiding this comment.
Review of #262 — fix(0.30.2): MCP version detection now reads uv tool list
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR hotfixes the silent local version unknown regression introduced in v0.30.1, where _get_local_mcp_version() called keboola_mcp_server --version but the upstream binary prints its argparse usage block (returncode 0) instead of a version string, causing the regex to find no match. The fix correctly re-orders the resolution to prefer uv tool list (canonical for uv tool install binaries), with importlib.metadata and the binary probe as fallbacks. Implementation is clean, well-tested, and architecturally correct. The PR is APPROVE with two non-blocking findings: a test comment inaccuracy about what the pre-fix code actually returned, and a gotchas.md entry that claims kbagent version shows the local MCP version "since v0.30.1" -- but that was broken in v0.30.1 and only becomes reliable in v0.30.2.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/skills/kbagent/references/gotchas.md:26 — "since v0.30.1" tag on the version-display bullet is inaccurate; it should say v0.30.2
The last bullet in the keboola-mcp-server is now auto-updated section reads:
kbagent versionnow shows the locally installed MCP version next to the latest
and carries the (since v0.30.1) section heading. However, this was broken in v0.30.1 (the entire premise of this PR). An AI agent reading gotchas.md on a v0.30.1 install would conclude that version display works, when it actually shows local version unknown. Per CONTRIBUTING.md the version tag "is non-optional -- without it, AI agents recommend behavior on older kbagent installs that do not exist". Fix: append a note to that bullet saying (reliable since v0.30.2; the v0.30.1 detection was broken -- see #262), or split it into its own gotcha with a (since v0.30.2) heading.
[NB-2] tests/test_version_service.py:278 — test_future_binary_with_version_flag does not cover the most likely real-world --version output format
The forward-compat test uses stdout="keboola_mcp_server 2.0.0\n" (space-separated, no leading v). This matches the \b(\d+\.\d+\.\d+)\b regex. However, the most common real-world convention for CLI binaries is keboola_mcp_server v2.0.0 (with a leading v). Because v is a word character, there is no \b boundary between v and 2, so \bdigit fails and the regex returns None. The forward-compat test therefore does not prove the fallback path works for the typical real-world format. Fix: add a second assertion with stdout="keboola_mcp_server v2.0.0\n" and either (a) update the regex in step 3 to re.search(r"v?(\d+\.\d+\.\d+)\b", ...) with group 1, or (b) document explicitly that the fallback only catches bare-digit output and the uv tool list path is expected to remain the primary path.
Nits
-
[NIT-1]tests/test_version_service.py:299— The docstring comment "Returns the X.Y.Z prefix; suffix is ignored by the regex anchor." is inaccurate. The function returns the fullversion_tokenstring (e.g."1.59.1.dev0"), not just theX.Y.Zprefix; the assertion on the same line proves this. The regex only acts as a gate (does this look like a version?), not as an extractor. Suggest: "Returns the full version token including any pre-release suffix (e.g.1.59.1.dev0)." -
[NIT-2]tests/test_version_service.py:257— The test comment "Pre-v0.30.2 the --version path picked the python3 minor (e.g. 3.12) out of the usage line" describes a hypothetical that does not occur with the actual fixture. The actual upstream usage output ("usage: python -m keboola-mcp-server [-h]\n...") contains noX.Y.Zthree-part version at all, so the old regex also returnedNone(not a wrong value). The comment's second clause "OR returned None when the regex required X.Y.Z" is the accurate description. Minor: clarify the comment to match the fixture reality.
Verification log
gh pr view 262 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 7 files, +256/-37, state=OPEN, headRefName=fix/0.30.2-mcp-version-detection✓git -C /tmp/kbagent-mcp-version-fix rev-parse --abbrev-ref HEAD→fix/0.30.2-mcp-version-detection✓ (working tree matches PR branch)make check(in/tmp/kbagent-mcp-version-fix) → 2775 passed, 7 skipped, lint + format + skill + version + changelog clean, exit 0 ✓- 3-layer check:
grep -E '^\+(from typer|import typer|formatter\.)' diff | grep services/→ empty ✓ (no layer violation;version_service.pyhas no Typer imports) grep -E '^\+(from httpx|import httpx)' diff | grep commands/→ empty ✓- Convention checks (magic numbers, bare
except:,print()in prod, rawerror_codestrings) → all empty ✓ - Security checks (token in output, hardcoded credentials) → clean ✓
- No new CLI commands added in this PR (zero new
@*.command(...)decorators) → Plugin synchronization map checklist items for new commands do not apply ✓ permissions.pyandhints/definitions/unchanged → no permission registry gap ✓- Cache schema compatibility:
auto_update.py_read_cache()accepts missingmcp_install_methodkey gracefully (the field is optional in the schema); v0.30.1 cache files are forward-compatible with v0.30.2 ✓ _uv_tool_list_get_mcp_versionexact-token match (parts[0] != MCP_PACKAGE_NAME):"keboola-mcp-server-foo"rejected ✓; indented binary lines skipped ✓; uv(outdated)annotation tokens ignored (third token, not used) ✓- Fallback regex
\b(\d+\.\d+\.\d+)\bafter strippingusage:lines: correctly rejectspython3.12.9in usage lines (stripped), and\brejectspython3.12.9in non-usage lines (no word boundary before digit aftern) ✓. However:\balso rejectsv2.0.0format (no boundary betweenvand2) -- see NB-2. - Pre-release version path:
_uv_tool_list_get_mcp_version("keboola-mcp-server v1.59.1.dev0\n")returns"1.59.1.dev0";packaging.version.Version("1.59.1.dev0")parses successfully;_is_up_to_datecomparison works (dev releases compare as lower than the release version, which is correct PEP 440 semantics) ✓ gotchas.mddiff: no changes in the PR branch; existing v0.30.1 entry has an inaccurate "reliable since v0.30.1" implication → NB-1keboola-expert.mddiff: unchanged ✓ (no new write/destructive commands; no new version gate; fix restores intended behavior rather than introducing new behavior)- Behavior reproduction: cannot run
kbagent versionagainst a live install in this review context; author's PR description includes a real user report and manual verification note confirming the fix ✓
Open questions for the author
(none)
…e flow (#265) * fix(0.30.3): close issue #263 -- bugs B + C + D in MCP auto-update flow PR #262 (v0.30.2) addressed only Bug A (probe returned None for uv-tool-managed installs). Three architectural bugs from the issue remained: Bug B: `_perform_mcp_update` for uvx-cache installs ran the broken chain `uvx --refresh --from <pkg> <bin> --version`. The trailing --version arg is rejected by the upstream MCP binary (no such flag), so the upgrade subprocess always exited non-zero -- the user-facing banner reported failure even when the cache refresh itself worked. Promotes uvx to `uv tool install --upgrade keboola-mcp-server`, matching what `kbagent doctor --fix` already does. Side-effect: the binary lands on PATH, so subsequent runs use the faster `uv_tool` detection path. Bug C: `_maybe_update_mcp` fell through to the upgrade attempt every TTL window when the probe returned None. `up_to_date == None` (not True) bypassed the short-circuit. Adds a `if local_version is None: return` gate that opts out of the upgrade for this TTL window. Cache TTL still ticks; next fresh-cache pass retries detection. Bug D: `maybe_auto_update` re-ran on every `kbagent repl` prompt iteration. Adds a module-level `_AUTO_UPDATE_RAN: bool = False` sentinel that flips to True BEFORE any work (so a crash mid-flow still gates subsequent re-entries). Re-exec'd processes start with a fresh sentinel because the module is reloaded into a new interpreter, so the kbagent-self-upgrade -> re-exec -> MCP-stage chain from PR #257 is preserved. Tests: 4 new regression tests pinning all three contracts: - TestPerformMcpUpdate.test_uvx_promotes_to_uv_tool_install (asserts the new uvx cmd; explicitly checks --version is GONE) - TestPerformMcpUpdate.test_uvx_promotion_requires_uv - TestProbeNoneSkipsUpgrade.test_local_version_none_skips_upgrade (the AC from #263: probe -> None; _perform_mcp_update NOT called) - TestProcessLevelSentinel.test_second_call_short_circuits (the AC from #263: maybe_auto_update body runs once across N calls) - TestProcessLevelSentinel.test_sentinel_is_set_even_when_body_raises Existing TestMaybeAutoUpdate, TestMaybeAutoUpdateMcpIntegration, and TestReExecPathStillRunsMcp autouse fixtures extended to reset _AUTO_UPDATE_RAN between tests so the sentinel does not gate the second test in each class. `make check` clean: 2,778 tests pass. Closes #263 (Bugs B, C, D; Bug A was already closed by PR #262). * fix(0.30.3): address Bug E -- subprocess exit 0 + version unchanged is NOT success @ottomansky reported on v0.30.2 (issue #263 update) that: $ kbagent repl Updating keboola-mcp-server v1.32.0 -> v1.59.1 (via uv_tool)... Updated keboola-mcp-server to v1.32.0. ^^^^^^^ same version we started from Root cause: `uv tool upgrade keboola-mcp-server` exits 0 even when its dependency resolver backtracks to the previously installed version. Real reproducer: keboola-mcp-server v1.59.1 declares `fastmcp==3.2.0` strict-equality constraint that conflicts with the existing venv's `fastmcp==2.13.0.2`, so uv silently resolves to v1.32.0 and exits clean. Pre-fix kbagent reported success; post-fix it tells the truth. Both upgrade paths now compare pre and post versions: - `auto_update.py:_maybe_update_mcp` (startup auto-update banner) - `version_service.py:VersionService._update_mcp` (kbagent update cmd) The success branch now has three sub-cases: - pre != post: claim updated. - post is None: probe failed; cannot verify; assume latest. - pre == post: subprocess exit 0 but version unchanged; emit diagnostic pointing to `uv tool install --reinstall keboola-mcp-server`. The `updated` boolean in self_update output now reflects the actual version delta, not just exit code. New regression test: - TestSelfUpdateTwoStage.test_subprocess_succeeds_but_version_unchanged_reports_not_updated Simulates the @ottomansky reproducer: pre and post both "1.32.0"; mock_perform returns (True, ...). Asserts result['mcp']['updated'] is False AND message contains "still v1.32.0" + "uv tool install --reinstall". Existing test_only_mcp_stale_kbagent_uptodate_still_runs_mcp updated to use side_effect=[pre, post] for the local-version mock so the upgrade actually moves the version (was: same value pre and post, which under the new contract correctly reports no-update). `make check` clean: 2,780 tests pass. * fix(0.30.3): address review iteration -- B-1 user-facing cmd, B-2 fresh-install guard Two blocking review findings on the previous commit (review iteration on PR #265): B-1: `get_versions()` (kbagent version output) showed users the OLD broken `uvx --refresh --from <pkg> <bin> --version` command as recommendation when install_method == 'uvx'. The internal upgrade logic in `_perform_mcp_update` already promotes to `uv tool install --upgrade` (Bug B fix), but the user-facing recommendation in `mcp_upgrade_cmd_by_method` dictionary had not been updated -- a separate data structure that drifts independently from runtime behaviour. Reviewer caught the cross-surface inconsistency. B-2: Bug E guard had a logical hole for fresh-install case. Original form: `actually_updated = bool(success and post_version and local_version and post_version != local_version)`. The AND short-circuits on `local_version`, so when local_version is None (user has no MCP installed; `kbagent update` does the first install) `actually_updated` was False -- and the message branch fell through to "still vNone" which was both wrong (the install DID happen) and misleading (the diagnostic suggests `uv tool install --reinstall` for a system that just installed for the first time). Post-fix, the four success-branch cases are explicit: 1. pre is None, post is set -> fresh install; updated=True 2. pre is set, post is set, != -> normal upgrade; updated=True 3. pre is set, post is set, == -> Bug E no-op; updated=False 4. pre / post unknown -> probe failure; updated=False The auto-update startup path (`_maybe_update_mcp`) does NOT hit case 1 because Bug C's `if local_version is None: return` gate intentionally skips fresh installs on startup -- the user must run `kbagent update` or `kbagent doctor --fix` explicitly. `_update_mcp` (the explicit- update path) DOES need to handle case 1, hence the guard rewrite. New regression tests: - TestVersionService.test_uvx_user_facing_command_uses_uv_tool_install (B-1): pin that install_method=='uvx' produces a user-facing recommendation containing `uv tool install --upgrade` and NOT `--version`. - TestSelfUpdateTwoStage.test_fresh_install_pre_none_post_set_reports_updated (B-2): pin that pre=None + post=set + success=True yields updated=True with a clean (no "still vNone") message. `make check` clean: 2,782 tests pass.
Summary
A real user reported on a fresh v0.30.1 upgrade:
and:
i.e. the v0.30.1 detection said the local MCP version was unknown despite a perfectly working install.
Root cause:
_get_local_mcp_version()rankeboola_mcp_server --versionexpecting a version string. The upstream MCP binary does NOT honour--version-- it prints its argparse usage block with returncode 0:Our regex
r"(\d+\.\d+\.\d+)"finds nothing parseable, falls through toimportlib.metadata(which can't see acrossuv toolvenvs), returns None.Fix
Prefer
uv tool listoutput -- the canonical source for thekbagent doctor --fixinstall method. The output linekeboola-mcp-server v1.59.1carries the exact version.New helper
_uv_tool_list_get_mcp_version(stdout)parses the output line-by-line, requires exact first-token equality on the package name, validates the second token asvX.Y.Z, and strips the leadingv. Robust against:keboola-mcp-server-foo)New resolution order in
_get_local_mcp_version:uv tool list-- preferred (canonical foruv tool installbinaries)importlib.metadata.version("keboola-mcp-server")-- pip-in-current-env fallbackkeboola_mcp_server --version-- best-effort fallback for a future MCP server that adds the flag. The cleaned-output filter now stripsusage:lines so a futurepython3.12.9path component cannot be mistaken for a version.Test plan
make checkgreen: 2,775 tests passed, 7 skipped, lint + format + skill + version + changelog clean.TestUvToolListGetMcpVersion: exact match, real-world multi-tool output, similar-name rejection, indented-binary rejection, malformed-version rejection, leading-v stripping, pre-release tolerance, empty input.TestGetLocalMcpVersionincluding a real-world regression test pinning the verbatim upstream usage-help output -- it must NOT be mistaken for a version. Plus a forward-compat test for the day MCP adds a real--versionflag.kbagent versionandkbagent updatewill now show the correct local MCP version on installs that diduv tool install keboola-mcp-server.Closes the regression user-reported on the v0.30.1 release thread.