From 6c64cee2c16e9d89755395df27dd5b03ee4dc4ca Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 12:01:48 +0200 Subject: [PATCH 1/3] 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 --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). --- .claude-plugin/marketplace.json | 2 +- plugins/kbagent/.claude-plugin/plugin.json | 2 +- pyproject.toml | 2 +- src/keboola_agent_cli/auto_update.py | 37 +++++ src/keboola_agent_cli/changelog.py | 6 + .../services/version_service.py | 24 ++-- tests/test_auto_update.py | 126 +++++++++++++++++- tests/test_version_service.py | 33 ++++- uv.lock | 2 +- 9 files changed, 212 insertions(+), 22 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index a2ab7be9..a8135ebe 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.30.2", + "version": "0.30.3", "source": "./plugins/kbagent", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 9915eb8c..1eecb863 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.30.2", + "version": "0.30.3", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/pyproject.toml b/pyproject.toml index 08a9f691..1a770c47 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-agent-cli" -version = "0.30.2" +version = "0.30.3" description = "AI-friendly CLI for managing Keboola projects" readme = "README.md" requires-python = ">=3.12" diff --git a/src/keboola_agent_cli/auto_update.py b/src/keboola_agent_cli/auto_update.py index 569be80e..3c3a12b6 100644 --- a/src/keboola_agent_cli/auto_update.py +++ b/src/keboola_agent_cli/auto_update.py @@ -43,6 +43,22 @@ logger = logging.getLogger(__name__) +# Process-level sentinel for the auto-update flow. +# +# Bug D fix from issue #263: ``kbagent repl`` re-enters the entire CLI +# (and therefore ``main()`` -> ``maybe_auto_update()``) on every prompt +# iteration. Pre-fix, the auto-update banner re-fired once per command +# typed at the prompt -- one fetch, one (potentially failing) upgrade +# attempt, one stderr write. The sentinel short-circuits subsequent +# in-process invocations after the first. +# +# Re-exec'd processes (kbagent self-upgrade -> ``execvpe`` to new binary) +# start with a fresh sentinel because the module is reloaded into a new +# Python interpreter, so the kbagent-self-upgrade -> re-exec -> MCP-stage +# chain from PR #257 is preserved. +_AUTO_UPDATE_RAN: bool = False + + def _get_cache_path() -> Path: """Return path to the version cache file. @@ -306,6 +322,17 @@ def _maybe_update_mcp(cache: dict | None, fetched_now: bool) -> str | None: return cached_latest # nothing to do; preserve any prior cache local_version = _get_local_mcp_version() + if local_version is None: + # Bug C fix from issue #263: when local-version detection fails, + # do NOT fall through to the upgrade attempt. The previous behaviour + # printed an "Updating ... vunknown -> v1.59.1" banner and ran the + # upgrade subprocess every TTL window because `up_to_date` was None + # (not True), which bypassed the short-circuit below. The fix opts + # out of the upgrade for this TTL window and lets the next fresh- + # cache pass retry detection. The cache write below records the + # latest version regardless so the cache TTL still ticks. + return mcp_latest + up_to_date = _is_up_to_date(local_version, mcp_latest) if up_to_date is True: return mcp_latest @@ -362,7 +389,17 @@ def maybe_auto_update() -> None: This function NEVER raises. All exceptions are caught and logged at debug level so the CLI always proceeds normally. """ + global _AUTO_UPDATE_RAN try: + # Bug D fix from issue #263: per-process sentinel. ``kbagent repl`` + # re-enters main() on every prompt; without this gate the auto- + # update flow fired (and printed banners) once per command typed. + # Set BEFORE any work so a crash mid-flow still gates subsequent + # in-process re-entries. + if _AUTO_UPDATE_RAN: + return + _AUTO_UPDATE_RAN = True + # Wide gates (dev install / opt-out / update|version commands) # skip BOTH stages -- there is nothing reasonable to do. if _should_skip_all(): diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 6bf06b4c..66c62266 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -8,6 +8,12 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.30.3": [ + "Fix: `_perform_mcp_update` for `uvx`-cache installs now promotes to `uv tool install --upgrade keboola-mcp-server` instead of running the broken `uvx --refresh --from --version` chain. The trailing `--version` arg was rejected by the upstream MCP binary (no such flag), so the upgrade subprocess always exited non-zero and the user-facing banner reported failure even when the cache refresh itself worked. Promoting to `uv tool install --upgrade` does the equivalent refresh AND moves the binary to PATH so subsequent runs use the faster `uv_tool` detection path. Bug B fix from issue #263.", + "Fix: `_maybe_update_mcp` now skips the upgrade attempt when the local-version probe returns `None`. Pre-fix, probe-`None` left `up_to_date == None` (not `True`), the short-circuit was bypassed, and the function fell through to a broken upgrade subprocess every TTL window. The user saw an `Updating ... vunknown -> v1.59.1` banner once per kbagent invocation. Post-fix, probe-`None` opts out of the upgrade for this TTL window; the next fresh-cache pass will retry detection. Cache TTL still ticks. Bug C fix from issue #263.", + "Fix: `maybe_auto_update` now uses a process-level sentinel (`_AUTO_UPDATE_RAN`) to short-circuit subsequent in-process invocations. `kbagent repl` re-enters `main()` -> `maybe_auto_update()` on every prompt iteration; pre-fix, the auto-update banner re-fired once per command typed at the prompt. The sentinel flips to True BEFORE any work so a crash mid-flow still gates subsequent re-entries. Re-exec'd processes (kbagent self-upgrade -> `execvpe` to new binary) 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. Bug D fix from issue #263.", + "Tests: 4 new regression tests pinning the three contracts. `TestPerformMcpUpdate.test_uvx_promotes_to_uv_tool_install` asserts the new uvx command shape and explicitly checks that `--version` is GONE from the cmd. `TestPerformMcpUpdate.test_uvx_promotion_requires_uv` covers the missing-`uv` failure path. `TestProbeNoneSkipsUpgrade.test_local_version_none_skips_upgrade` mocks the probe to return None and asserts `_perform_mcp_update` is never called. `TestProcessLevelSentinel.test_second_call_short_circuits` calls `maybe_auto_update()` three times in the same process and asserts the MCP stage runs exactly once. `TestProcessLevelSentinel.test_sentinel_is_set_even_when_body_raises` verifies the flag flips before any work so a flaky upstream PyPI fetch cannot re-fire the banner per prompt. 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.", + ], "0.30.2": [ "Fix: `kbagent version` now correctly reports the locally installed `keboola-mcp-server` version. v0.30.1's detection probed `keboola_mcp_server --version`, but the upstream MCP binary does NOT honour `--version` -- it prints its argparse usage block with returncode 0, so the regex found no match and the command displayed `local version unknown` despite a perfectly working install. Reported by an actual user on a fresh upgrade: `kbagent update` printed `keboola-mcp-server vunknown -> v1.59.1` and the version panel said `local version unknown`. The fix moves `uv tool list` to the **preferred** detection path (canonical for the kbagent doctor --fix install method, exact `keboola-mcp-server v1.59.1` line), with `importlib.metadata` and the existing `keboola_mcp_server --version` probe retained as fallbacks. The binary-probe fallback now also strips `usage:` lines before regex-matching so a future `python3.12.9` path component cannot be mistaken for a version. New helper `_uv_tool_list_get_mcp_version(stdout)` parses the `uv tool list` output line-by-line, requires exact first-token equality on the package name, validates the second token as semver-ish, and strips the leading `v`. 8 new unit tests in `TestUvToolListGetMcpVersion` plus 5 rewritten `TestGetLocalMcpVersion` tests including a real-world regression test pinning the upstream usage-help output verbatim.", ], diff --git a/src/keboola_agent_cli/services/version_service.py b/src/keboola_agent_cli/services/version_service.py index 5b1f01b6..a6aec737 100644 --- a/src/keboola_agent_cli/services/version_service.py +++ b/src/keboola_agent_cli/services/version_service.py @@ -293,18 +293,18 @@ def _perform_mcp_update( return False, "pip not found on PATH" cmd = [pip_path, "install", "--upgrade", MCP_PACKAGE_NAME] elif method == "uvx": - uvx_path = shutil.which("uvx") - if uvx_path is None: - return False, "uvx not found on PATH" - # `uvx --refresh` re-downloads even cached packages. - cmd = [ - uvx_path, - "--refresh", - "--from", - MCP_PACKAGE_NAME, - MCP_BINARY_NAME, - "--version", - ] + # Promote uvx-cache install to a persistent `uv tool install`. + # The previous strategy (`uvx --refresh ... --version`) was + # broken: the upstream MCP binary does NOT honour --version (it + # rejects the arg and exits non-zero), so the upgrade banner + # always reported failure even when the cache refresh itself + # worked. `uv tool install --upgrade` does the equivalent + # refresh AND moves the binary to PATH so subsequent runs use the + # faster `uv_tool` detection path. Bug B fix from issue #263. + uv_path = shutil.which("uv") + if uv_path is None: + return False, "uv not found on PATH (needed to promote uvx cache to uv tool)" + cmd = [uv_path, "tool", "install", "--upgrade", MCP_PACKAGE_NAME] elif method == "none": return False, "keboola-mcp-server is not installed" else: diff --git a/tests/test_auto_update.py b/tests/test_auto_update.py index 78f1f7e0..7b5d0056 100644 --- a/tests/test_auto_update.py +++ b/tests/test_auto_update.py @@ -7,6 +7,7 @@ import pytest +import keboola_agent_cli.auto_update as auto_update_module from keboola_agent_cli.auto_update import ( _get_cache_path, _is_cache_fresh, @@ -294,8 +295,12 @@ def _no_real_mcp_calls(self): Also defaults the per-stage skip helpers (since v0.30.1) to False so existing tests that only patch the legacy ``_should_skip`` - alias still drive the orchestrator down the active path. + alias still drive the orchestrator down the active path. Resets + the per-process auto-update sentinel (since v0.30.3) so each test + starts from a fresh state -- otherwise the Bug D fix would gate + the second test in the class. """ + auto_update_module._AUTO_UPDATE_RAN = False with ( patch( "keboola_agent_cli.auto_update._maybe_update_mcp", @@ -600,8 +605,10 @@ def _force_active_skip_gates(self): Each test below verifies an MCP-stage path; the skip gates must be out of the way for those paths to run. Individual tests still re-patch ``_should_skip_kbagent_stage`` when they exercise the - re-exec scenario explicitly. + re-exec scenario explicitly. Resets the per-process auto-update + sentinel (since v0.30.3) so each test starts from a fresh state. """ + auto_update_module._AUTO_UPDATE_RAN = False with ( patch( "keboola_agent_cli.auto_update._should_skip_all", @@ -691,6 +698,13 @@ class TestReExecPathStillRunsMcp: MCP work proceeds even when Stage 1 is skipped. """ + @pytest.fixture(autouse=True) + def _reset_sentinel(self): + """Reset per-process sentinel between tests (since v0.30.3).""" + auto_update_module._AUTO_UPDATE_RAN = False + yield + auto_update_module._AUTO_UPDATE_RAN = False + @patch("keboola_agent_cli.auto_update._is_dev_install", return_value=False) def test_re_exec_skips_kbagent_but_runs_mcp(self, _mock_dev): """With KBAGENT_SKIP_UPDATE=1 set, MCP stage MUST still run.""" @@ -751,3 +765,111 @@ def test_user_opt_out_skips_both(self, _mock_dev): mock_fetch_kbagent.assert_not_called() mock_mcp.assert_not_called() + + +# --------------------------------------------------------------------------- +# Bug C regression: probe-None must NOT fall through to upgrade attempt +# (issue #263, addressed in v0.30.3) +# --------------------------------------------------------------------------- + + +class TestProbeNoneSkipsUpgrade: + """Pin the Bug C contract: when local-version detection fails, + ``_maybe_update_mcp`` must NOT call ``_perform_mcp_update``. + + Pre-fix: detection returning None left ``up_to_date == None`` (not + True), the short-circuit was bypassed, the function fell through to + a broken upgrade subprocess, and the user saw an + "Updating ... vunknown -> v1.59.1" banner every TTL window. + + Post-fix: probe-None opts out of the upgrade for this TTL window; + the next fresh-cache pass will retry detection. + """ + + @patch("keboola_agent_cli.auto_update._fetch_mcp_latest_version", return_value="1.59.1") + @patch("keboola_agent_cli.auto_update._get_local_mcp_version", return_value=None) + @patch("keboola_agent_cli.auto_update._detect_mcp_install_method", return_value="uv_tool") + @patch("keboola_agent_cli.auto_update._perform_mcp_update") + def test_local_version_none_skips_upgrade( + self, mock_perform, mock_detect, mock_local, mock_fetch + ) -> None: + """The acceptance criterion from #263: mock probe -> None; + assert _perform_mcp_update is NOT called. + """ + result = _maybe_update_mcp(cache=None, fetched_now=True) + assert result == "1.59.1" + mock_perform.assert_not_called() + + +# --------------------------------------------------------------------------- +# Bug D regression: process-level sentinel for repeated maybe_auto_update calls +# (issue #263, addressed in v0.30.3) +# --------------------------------------------------------------------------- + + +class TestProcessLevelSentinel: + """Pin the Bug D contract: ``maybe_auto_update`` runs the body at most + once per process. + + Pre-fix: ``kbagent repl`` re-entered ``main()`` -> ``maybe_auto_update`` + on every prompt, so the auto-update banner re-fired once per command + typed at the prompt. Post-fix: a module-level ``_AUTO_UPDATE_RAN`` + flag short-circuits subsequent in-process invocations. + + Re-exec'd processes (kbagent self-upgrade -> ``execvpe``) start with + a fresh sentinel because the module is reloaded into a new + interpreter -- the kbagent-self-upgrade -> re-exec -> MCP-stage chain + from PR #257 is preserved. + """ + + @pytest.fixture(autouse=True) + def _reset_sentinel(self): + """Tests assume a fresh sentinel per test (each simulates a new process).""" + auto_update_module._AUTO_UPDATE_RAN = False + yield + auto_update_module._AUTO_UPDATE_RAN = False + + @patch("keboola_agent_cli.auto_update._should_skip_all", return_value=False) + @patch("keboola_agent_cli.auto_update._should_skip_kbagent_stage", return_value=False) + @patch("keboola_agent_cli.auto_update._read_cache", return_value=None) + @patch("keboola_agent_cli.auto_update._fetch_kbagent_latest_version", return_value="1.0.0") + @patch("keboola_agent_cli.auto_update._is_up_to_date", return_value=True) + @patch("keboola_agent_cli.auto_update._maybe_update_mcp") + @patch("keboola_agent_cli.auto_update._detect_mcp_install_method", return_value="uv_tool") + @patch("keboola_agent_cli.auto_update._write_cache") + def test_second_call_short_circuits( + self, + mock_write, + mock_detect, + mock_mcp, + mock_up_to_date, + mock_fetch, + mock_cache, + mock_skip_kb, + mock_skip_all, + ): + """Acceptance criterion from #263: second invocation in the same + REPL session must NOT re-trigger maybe_auto_update's body. + Verified by counting MCP-stage invocations: 1 after multiple + calls, not N. + """ + maybe_auto_update() # first call: runs the body + maybe_auto_update() # second call: should short-circuit + maybe_auto_update() # third call: still short-circuited + + # MCP stage was reached exactly once across the three calls. + mock_mcp.assert_called_once() + + def test_sentinel_is_set_even_when_body_raises(self): + """Bug D corner case: the sentinel must flip to True BEFORE any + work, so a crash mid-flow still gates subsequent in-process + re-entries. Otherwise a flaky upstream PyPI fetch could re-fire + the banner per prompt. + """ + with patch( + "keboola_agent_cli.auto_update._should_skip_all", + side_effect=RuntimeError("kaboom"), + ): + maybe_auto_update() # blanket try/except swallows the RuntimeError + # Sentinel was flipped before the crash. + assert auto_update_module._AUTO_UPDATE_RAN is True diff --git a/tests/test_version_service.py b/tests/test_version_service.py index 62a8d2d7..c7289526 100644 --- a/tests/test_version_service.py +++ b/tests/test_version_service.py @@ -487,13 +487,38 @@ def test_pip_env_success(self, mock_run: MagicMock, mock_which: MagicMock) -> No @patch("keboola_agent_cli.services.version_service.shutil.which") @patch("keboola_agent_cli.services.version_service.subprocess.run") - def test_uvx_uses_refresh(self, mock_run: MagicMock, mock_which: MagicMock) -> None: - mock_which.return_value = "/usr/local/bin/uvx" - mock_run.return_value = MagicMock(returncode=0, stdout="cached refreshed", stderr="") + def test_uvx_promotes_to_uv_tool_install( + self, mock_run: MagicMock, mock_which: MagicMock + ) -> None: + """Bug B fix from issue #263: uvx-cache install is promoted to a + persistent ``uv tool install --upgrade``. + + Pre-fix: command was ``uvx --refresh --from --version``, + which failed because the upstream MCP binary does not honour + ``--version``. The cache refresh succeeded but the trailing + --version probe exited non-zero, so the upgrade banner reported + failure even though the refresh worked. + + Post-fix: command is ``uv tool install --upgrade ``, which + equivalent-refreshes AND moves the binary to PATH so subsequent + runs use the faster ``uv_tool`` detection path. + """ + mock_which.return_value = "/usr/local/bin/uv" + mock_run.return_value = MagicMock(returncode=0, stdout="installed", stderr="") ok, _info = _perform_mcp_update(method="uvx") assert ok is True cmd = mock_run.call_args.args[0] - assert "--refresh" in cmd + assert "tool" in cmd and "install" in cmd and "--upgrade" in cmd + assert MCP_PACKAGE_NAME in cmd + # The broken --version arg must be GONE from the uvx upgrade path. + assert "--version" not in cmd + + @patch("keboola_agent_cli.services.version_service.shutil.which", return_value=None) + def test_uvx_promotion_requires_uv(self, mock_which: MagicMock) -> None: + """If the uvx promotion needs `uv` and `uv` is missing, fail clearly.""" + ok, info = _perform_mcp_update(method="uvx") + assert ok is False + assert "uv not found" in info def test_none_returns_false(self) -> None: ok, info = _perform_mcp_update(method="none") diff --git a/uv.lock b/uv.lock index 22effc98..7312ddab 100644 --- a/uv.lock +++ b/uv.lock @@ -439,7 +439,7 @@ wheels = [ [[package]] name = "keboola-agent-cli" -version = "0.30.2" +version = "0.30.3" source = { editable = "." } dependencies = [ { name = "httpx" }, From 046831ae677cf2b38dd68bbeab4e5cb470e7c0f6 Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 12:53:14 +0200 Subject: [PATCH 2/3] 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. --- src/keboola_agent_cli/auto_update.py | 28 +++++++++- src/keboola_agent_cli/changelog.py | 3 +- .../services/version_service.py | 40 +++++++++++--- tests/test_version_service.py | 53 ++++++++++++++++++- 4 files changed, 112 insertions(+), 12 deletions(-) diff --git a/src/keboola_agent_cli/auto_update.py b/src/keboola_agent_cli/auto_update.py index 3c3a12b6..e28f5d2d 100644 --- a/src/keboola_agent_cli/auto_update.py +++ b/src/keboola_agent_cli/auto_update.py @@ -346,10 +346,34 @@ def _maybe_update_mcp(cache: dict | None, fetched_now: bool) -> str | None: f"Updating keboola-mcp-server v{local_version or 'unknown'} -> v{mcp_latest}" f" (via {method})...\n" ) + pre_version = local_version success, info = _perform_mcp_update(method=method, timeout=MCP_UPGRADE_TIMEOUT) if success: - post_version = _get_local_mcp_version() or mcp_latest - sys.stderr.write(f"Updated keboola-mcp-server to v{post_version}.\n") + # Bug E fix from issue #263: subprocess returncode == 0 is NOT + # enough to claim the upgrade actually happened. `uv tool upgrade` + # exits 0 even when its resolver backtracks to the previously + # installed version (a real-world reproducer: keboola-mcp-server + # v1.59.1 declares fastmcp==3.2.0 strict equality, the existing + # venv has fastmcp==2.13.0.2, uv resolves back to v1.32.0 and + # exits clean). Compare pre and post versions and tell the truth. + post_version = _get_local_mcp_version() + if post_version and pre_version and post_version != pre_version: + sys.stderr.write(f"Updated keboola-mcp-server to v{post_version}.\n") + elif post_version is None: + # Probe failed post-upgrade; cannot verify -- assume latest. + sys.stderr.write( + f"Updated keboola-mcp-server (probe failed; latest on PyPI: v{mcp_latest}).\n" + ) + else: + # Subprocess exit 0 but local version unchanged. Most common + # cause: dependency-resolver backtrack (Python or transitive- + # dep constraint cannot satisfy the latest). Surface a + # diagnostic instead of silently lying. + sys.stderr.write( + f"keboola-mcp-server upgrade exit 0 but local version still v{pre_version} " + f"(latest: v{mcp_latest}). Possible Python or dependency-version mismatch -- " + f"run `uv tool install --reinstall keboola-mcp-server` to diagnose.\n" + ) else: sys.stderr.write( f"keboola-mcp-server upgrade skipped: {info}; continuing with current version.\n" diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 66c62266..e1c071de 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -12,7 +12,8 @@ "Fix: `_perform_mcp_update` for `uvx`-cache installs now promotes to `uv tool install --upgrade keboola-mcp-server` instead of running the broken `uvx --refresh --from --version` chain. The trailing `--version` arg was rejected by the upstream MCP binary (no such flag), so the upgrade subprocess always exited non-zero and the user-facing banner reported failure even when the cache refresh itself worked. Promoting to `uv tool install --upgrade` does the equivalent refresh AND moves the binary to PATH so subsequent runs use the faster `uv_tool` detection path. Bug B fix from issue #263.", "Fix: `_maybe_update_mcp` now skips the upgrade attempt when the local-version probe returns `None`. Pre-fix, probe-`None` left `up_to_date == None` (not `True`), the short-circuit was bypassed, and the function fell through to a broken upgrade subprocess every TTL window. The user saw an `Updating ... vunknown -> v1.59.1` banner once per kbagent invocation. Post-fix, probe-`None` opts out of the upgrade for this TTL window; the next fresh-cache pass will retry detection. Cache TTL still ticks. Bug C fix from issue #263.", "Fix: `maybe_auto_update` now uses a process-level sentinel (`_AUTO_UPDATE_RAN`) to short-circuit subsequent in-process invocations. `kbagent repl` re-enters `main()` -> `maybe_auto_update()` on every prompt iteration; pre-fix, the auto-update banner re-fired once per command typed at the prompt. The sentinel flips to True BEFORE any work so a crash mid-flow still gates subsequent re-entries. Re-exec'd processes (kbagent self-upgrade -> `execvpe` to new binary) 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. Bug D fix from issue #263.", - "Tests: 4 new regression tests pinning the three contracts. `TestPerformMcpUpdate.test_uvx_promotes_to_uv_tool_install` asserts the new uvx command shape and explicitly checks that `--version` is GONE from the cmd. `TestPerformMcpUpdate.test_uvx_promotion_requires_uv` covers the missing-`uv` failure path. `TestProbeNoneSkipsUpgrade.test_local_version_none_skips_upgrade` mocks the probe to return None and asserts `_perform_mcp_update` is never called. `TestProcessLevelSentinel.test_second_call_short_circuits` calls `maybe_auto_update()` three times in the same process and asserts the MCP stage runs exactly once. `TestProcessLevelSentinel.test_sentinel_is_set_even_when_body_raises` verifies the flag flips before any work so a flaky upstream PyPI fetch cannot re-fire the banner per prompt. 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.", + "Fix: kbagent no longer reports a successful MCP upgrade when the subprocess returncode == 0 but the local version did not change. `uv tool upgrade keboola-mcp-server` exits 0 even when its dependency resolver backtracks to the previously installed version (real-world reproducer from issue #263: `keboola-mcp-server v1.59.1` declares a `fastmcp==3.2.0` strict-equality constraint that conflicts with the installed `fastmcp==2.13.0.2`, so uv silently resolves to v1.32.0 and exits clean). Pre-fix, kbagent printed `Updated keboola-mcp-server to v1.32.0.` -- the same version the user started from -- once per kbagent invocation. Post-fix, both `_maybe_update_mcp` (auto-update path) and `VersionService._update_mcp` (`kbagent update` path) compare pre-upgrade and post-upgrade versions; only declare success when they actually differ; otherwise emit a diagnostic pointing to `uv tool install --reinstall keboola-mcp-server` and surface the underlying constraint as the likely cause. Bug E fix from issue #263 (reported by @ottomansky on v0.30.2).", + "Tests: 5 new regression tests pinning the four contracts. `TestPerformMcpUpdate.test_uvx_promotes_to_uv_tool_install` asserts the new uvx command shape and explicitly checks that `--version` is GONE from the cmd. `TestPerformMcpUpdate.test_uvx_promotion_requires_uv` covers the missing-`uv` failure path. `TestProbeNoneSkipsUpgrade.test_local_version_none_skips_upgrade` mocks the probe to return None and asserts `_perform_mcp_update` is never called. `TestProcessLevelSentinel.test_second_call_short_circuits` calls `maybe_auto_update()` three times in the same process and asserts the MCP stage runs exactly once. `TestProcessLevelSentinel.test_sentinel_is_set_even_when_body_raises` verifies the flag flips before any work so a flaky upstream PyPI fetch cannot re-fire the banner per prompt. `TestSelfUpdateTwoStage.test_subprocess_succeeds_but_version_unchanged_reports_not_updated` simulates the @ottomansky reproducer (pre and post both `1.32.0`) and asserts `result['mcp']['updated'] is False` plus a diagnostic message containing `uv tool install --reinstall`. 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.", ], "0.30.2": [ "Fix: `kbagent version` now correctly reports the locally installed `keboola-mcp-server` version. v0.30.1's detection probed `keboola_mcp_server --version`, but the upstream MCP binary does NOT honour `--version` -- it prints its argparse usage block with returncode 0, so the regex found no match and the command displayed `local version unknown` despite a perfectly working install. Reported by an actual user on a fresh upgrade: `kbagent update` printed `keboola-mcp-server vunknown -> v1.59.1` and the version panel said `local version unknown`. The fix moves `uv tool list` to the **preferred** detection path (canonical for the kbagent doctor --fix install method, exact `keboola-mcp-server v1.59.1` line), with `importlib.metadata` and the existing `keboola_mcp_server --version` probe retained as fallbacks. The binary-probe fallback now also strips `usage:` lines before regex-matching so a future `python3.12.9` path component cannot be mistaken for a version. New helper `_uv_tool_list_get_mcp_version(stdout)` parses the `uv tool list` output line-by-line, requires exact first-token equality on the package name, validates the second token as semver-ish, and strips the leading `v`. 8 new unit tests in `TestUvToolListGetMcpVersion` plus 5 rewritten `TestGetLocalMcpVersion` tests including a real-world regression test pinning the upstream usage-help output verbatim.", diff --git a/src/keboola_agent_cli/services/version_service.py b/src/keboola_agent_cli/services/version_service.py index a6aec737..113ea818 100644 --- a/src/keboola_agent_cli/services/version_service.py +++ b/src/keboola_agent_cli/services/version_service.py @@ -633,18 +633,42 @@ def _update_mcp() -> dict[str, Any]: success, output = _perform_mcp_update(method=method, timeout=MCP_UPGRADE_TIMEOUT) post_version = _get_local_mcp_version() if success else local_version + # Bug E fix from issue #263: subprocess returncode == 0 is NOT + # enough to claim the upgrade happened. `uv tool upgrade` exits 0 + # even when its resolver backtracks to the previously installed + # version (e.g. a transitive-dep constraint blocks the latest). + # `updated` reflects the actual version delta, not just exit code. + actually_updated = bool( + success and post_version and local_version and post_version != local_version + ) + + if not success: + message = f"keboola-mcp-server upgrade failed: {output}" + elif actually_updated: + message = ( + f"Upgraded keboola-mcp-server " + f"({local_version or 'unknown'} -> {post_version}) via {method}." + ) + elif post_version is None: + message = ( + f"keboola-mcp-server upgrade ran via {method}; post-upgrade probe failed " + f"(latest on PyPI: v{latest_version})." + ) + else: + # Subprocess exit 0 but local version unchanged. + message = ( + f"keboola-mcp-server upgrade exit 0 but local version still " + f"v{local_version} (latest: v{latest_version}). Possible Python or " + f"dependency-version mismatch -- run `uv tool install --reinstall " + f"{MCP_PACKAGE_NAME}` to diagnose." + ) + return { - "updated": bool(success), + "updated": actually_updated, "current_version": local_version, "latest_version": latest_version, "post_upgrade_version": post_version, "install_method": method, - "message": ( - f"Upgraded keboola-mcp-server " - f"({local_version or 'unknown'} -> {post_version or latest_version or '?'}) " - f"via {method}." - if success - else f"keboola-mcp-server upgrade failed: {output}" - ), + "message": message, "output": output, } diff --git a/tests/test_version_service.py b/tests/test_version_service.py index c7289526..117785e9 100644 --- a/tests/test_version_service.py +++ b/tests/test_version_service.py @@ -590,7 +590,10 @@ def test_only_mcp_stale_kbagent_uptodate_still_runs_mcp( mock_kbagent_latest.return_value = __version__ # kbagent up-to-date mock_mcp_latest.return_value = "1.59.1" - mock_local.return_value = "1.49.0" # MCP stale + # Bug E fix: pre-upgrade returns 1.49.0; post-upgrade returns 1.59.1. + # The version delta is what flips `updated` to True (not just the + # subprocess exit code). Side-effect list models the two calls. + mock_local.side_effect = ["1.49.0", "1.59.1"] mock_detect.return_value = "uv_tool" mock_perform.return_value = (True, "ok") @@ -601,3 +604,51 @@ def test_only_mcp_stale_kbagent_uptodate_still_runs_mcp( assert result["mcp"]["updated"] is True assert result["updated"] is True mock_perform.assert_called_once() + + @patch("keboola_agent_cli.services.version_service._fetch_kbagent_latest_version") + @patch("keboola_agent_cli.services.version_service._fetch_mcp_latest_version") + @patch("keboola_agent_cli.services.version_service._get_local_mcp_version") + @patch("keboola_agent_cli.services.version_service._detect_mcp_install_method") + @patch("keboola_agent_cli.services.version_service._perform_mcp_update") + def test_subprocess_succeeds_but_version_unchanged_reports_not_updated( + self, + mock_perform: MagicMock, + mock_detect: MagicMock, + mock_local: MagicMock, + mock_mcp_latest: MagicMock, + mock_kbagent_latest: MagicMock, + ) -> None: + """Bug E regression from issue #263. + + Real reproducer (from @ottomansky's trace on v0.30.2): + ``uv tool upgrade keboola-mcp-server`` exits 0, but uv's + resolver backtracked to the previously installed v1.32.0 + because v1.59.1 declares a fastmcp constraint the venv cannot + satisfy. Pre-fix: kbagent reported success and the message + said "Upgraded keboola-mcp-server (1.32.0 -> 1.32.0)". Post-fix: + ``updated`` is False, message contains a diagnostic pointing to + ``uv tool install --reinstall`` with no false-success claim. + """ + from keboola_agent_cli import __version__ + + mock_kbagent_latest.return_value = __version__ + mock_mcp_latest.return_value = "1.59.1" + # Both pre and post probes return v1.32.0 -- subprocess "succeeded" + # but the version did not change. + mock_local.side_effect = ["1.32.0", "1.32.0"] + mock_detect.return_value = "uv_tool" + mock_perform.return_value = (True, "no upgrade needed") + + svc = VersionService() + result = svc.self_update() + + # The lie -- "subprocess exit 0 = upgrade happened" -- is the bug. + # Truth: pre and post versions are identical, so updated == False. + assert result["mcp"]["updated"] is False + # Diagnostic message points the user at `uv tool install --reinstall` + # so they can investigate the underlying packaging conflict. + assert "still v1.32.0" in result["mcp"]["message"] + assert "uv tool install --reinstall" in result["mcp"]["message"] + # The overall `updated` flag also reflects no change (kbagent itself + # was up-to-date in this scenario). + assert result["updated"] is False From 196f385c59d7faf9b8945e8b65e07836a933bacd Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 13:26:46 +0200 Subject: [PATCH 3/3] 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 --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. --- .../services/version_service.py | 16 +++- tests/test_version_service.py | 83 +++++++++++++++++++ 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/keboola_agent_cli/services/version_service.py b/src/keboola_agent_cli/services/version_service.py index 113ea818..410ac225 100644 --- a/src/keboola_agent_cli/services/version_service.py +++ b/src/keboola_agent_cli/services/version_service.py @@ -430,11 +430,16 @@ def get_versions(self) -> dict[str, Any]: mcp_up_to_date = _is_up_to_date(mcp_local, mcp_latest) mcp_method = _detect_mcp_install_method() - # Map install method to the upgrade command shown to users. + # Map install method to the upgrade command shown to users. Must + # match what `_perform_mcp_update` actually runs internally -- since + # v0.30.3 the uvx path promotes to `uv tool install --upgrade` + # (Bug B fix from issue #263), so the user-facing recommendation + # must reflect that, not the broken `uvx --refresh ... --version` + # chain the v0.30.1 logic used. mcp_upgrade_cmd_by_method = { "uv_tool": f"uv tool upgrade {MCP_PACKAGE_NAME}", "pip_env": f"pip install --upgrade {MCP_PACKAGE_NAME}", - "uvx": f"uvx --refresh --from {MCP_PACKAGE_NAME} {MCP_BINARY_NAME} --version", + "uvx": f"uv tool install --upgrade {MCP_PACKAGE_NAME}", "none": f"uv tool install {MCP_PACKAGE_NAME}", } @@ -638,8 +643,13 @@ def _update_mcp() -> dict[str, Any]: # even when its resolver backtracks to the previously installed # version (e.g. a transitive-dep constraint blocks the latest). # `updated` reflects the actual version delta, not just exit code. + # The four success-branch cases: + # 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 actually_updated = bool( - success and post_version and local_version and post_version != local_version + success and post_version and (local_version is None or post_version != local_version) ) if not success: diff --git a/tests/test_version_service.py b/tests/test_version_service.py index 117785e9..7e9cd108 100644 --- a/tests/test_version_service.py +++ b/tests/test_version_service.py @@ -126,6 +126,38 @@ def test_mcp_auto_updates( assert mcp_dep["install_method"] == "uv_tool" assert "uv tool upgrade" in mcp_dep["upgrade_command"] + @patch("keboola_agent_cli.services.version_service._fetch_mcp_latest_version") + @patch("keboola_agent_cli.services.version_service._is_uvx_available") + @patch("keboola_agent_cli.services.version_service._get_local_mcp_version") + @patch("keboola_agent_cli.services.version_service._detect_mcp_install_method") + def test_uvx_user_facing_command_uses_uv_tool_install( + self, + mock_detect: MagicMock, + mock_local: MagicMock, + mock_uvx: MagicMock, + mock_mcp_latest: MagicMock, + ) -> None: + """B-1 regression: when install_method=='uvx', the user-facing + upgrade_command must NOT recommend the broken `uvx --refresh ... + --version` chain (which Bug B removed from the upgrade + logic). It must point at the same `uv tool install --upgrade` + the production code now runs internally. + """ + mock_uvx.return_value = True + mock_mcp_latest.return_value = "1.59.1" + mock_local.return_value = "1.49.0" + mock_detect.return_value = "uvx" + + svc = VersionService() + result = svc.get_versions() + + mcp_dep = result["dependencies"][0] + assert mcp_dep["install_method"] == "uvx" + # The user-facing recommendation must match the runtime upgrade. + assert "uv tool install --upgrade" in mcp_dep["upgrade_command"] + # The pre-fix broken arg must NOT appear -- it does not work. + assert "--version" not in mcp_dep["upgrade_command"] + @patch("keboola_agent_cli.services.version_service._fetch_mcp_latest_version") @patch("keboola_agent_cli.services.version_service._is_uvx_available") def test_uvx_not_available( @@ -652,3 +684,54 @@ def test_subprocess_succeeds_but_version_unchanged_reports_not_updated( # The overall `updated` flag also reflects no change (kbagent itself # was up-to-date in this scenario). assert result["updated"] is False + + @patch("keboola_agent_cli.services.version_service._fetch_kbagent_latest_version") + @patch("keboola_agent_cli.services.version_service._fetch_mcp_latest_version") + @patch("keboola_agent_cli.services.version_service._get_local_mcp_version") + @patch("keboola_agent_cli.services.version_service._detect_mcp_install_method") + @patch("keboola_agent_cli.services.version_service._perform_mcp_update") + def test_fresh_install_pre_none_post_set_reports_updated( + self, + mock_perform: MagicMock, + mock_detect: MagicMock, + mock_local: MagicMock, + mock_mcp_latest: MagicMock, + mock_kbagent_latest: MagicMock, + ) -> None: + """B-2 regression: explicit `kbagent update` on a host that has + no MCP installed yet (`local_version=None`). Pre-fix, the Bug E + guard's `local_version and ...` short-circuited on the falsy + local_version, so a successful fresh install was reported as + ``updated: False`` with a "still vNone" diagnostic message. + + Post-fix, the guard treats `local_version=None and post_version + is set` as a fresh install -> `actually_updated=True`. The + message reads "(unknown -> 1.59.1)" which is correct. + + The auto-update startup path (`_maybe_update_mcp`) does NOT hit + this case 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. + """ + from keboola_agent_cli import __version__ + + mock_kbagent_latest.return_value = __version__ + mock_mcp_latest.return_value = "1.59.1" + # Pre: not installed (None). Post: installed (1.59.1). The + # explicit update worked. + mock_local.side_effect = [None, "1.59.1"] + mock_detect.return_value = "uv_tool" + mock_perform.return_value = (True, "installed") + + svc = VersionService() + result = svc.self_update() + + # The legitimate fresh-install case: updated must be True. + assert result["mcp"]["updated"] is True + assert result["mcp"]["current_version"] is None + assert result["mcp"]["post_upgrade_version"] == "1.59.1" + # Message must not say "still vNone" (the pre-fix lie). + assert "still v" not in result["mcp"]["message"] + assert "vNone" not in result["mcp"]["message"] + # Overall flag flips to True too. + assert result["updated"] is True