fix(0.30.3): close issue #263 -- bugs B + C + D + E in MCP auto-update flow - #265
Conversation
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).
…s 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.
|
Picked up @ottomansky's update on issue #263 -- Bug E is now addressed in commit
The New regression test Updated changelog entry. |
padak
left a comment
There was a problem hiding this comment.
Review of #265 — fix(0.30.3): close issue #263 -- bugs B + C + D + E in MCP auto-update flow
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
PR closes four chained bugs in the MCP auto-update flow (issue #263, partially addressed in v0.30.2). The fixes are architecturally sound: Bug B promotes the uvx-cache upgrade path to a persistent uv tool install --upgrade (self-healing); Bug C adds an early-out gate when the local-version probe returns None in the auto-update path; Bug D adds a per-process sentinel in maybe_auto_update to prevent re-firing on kbagent repl prompt iterations; Bug E adds pre/post version comparison in both the auto-update path and the explicit kbagent update path to distinguish a real upgrade from a uv resolver backtrack. Two issues found. The first is a BLOCKING UX regression in VersionService.get_versions() (displayed by kbagent version) where the suggested upgrade command for users with a uvx-cache install still shows the old broken uvx --refresh --from <pkg> <bin> --version invocation that was specifically removed by Bug B. The second is a BLOCKING logic bug in _update_mcp(): the actually_updated guard ANDs on local_version, so when a fresh uvx-to-uv-tool promotion succeeds (local_version=None pre-upgrade, post_version="1.59.1" post-upgrade), updated is silently reported as False and the message says "still vNone" instead of reporting a successful install.
Verdict: REQUEST CHANGES — 2 blocking findings.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 2
- Non-blocking findings: 3
- Nits: 1
Blocking findings
[B-1] src/keboola_agent_cli/services/version_service.py:437 — get_versions() still suggests the old broken uvx --refresh command to users
The mcp_upgrade_cmd_by_method dict in get_versions() still maps "uvx" to f"uvx --refresh --from {MCP_PACKAGE_NAME} {MCP_BINARY_NAME} --version" -- the exact command that Bug B was introduced to remove. kbagent version displays this string to the user as the "how to upgrade" hint. A user who sees install_method: "uvx" and copies the displayed command will get the same failure that triggered Bug B. The fix promoted _perform_mcp_update("uvx") to uv tool install --upgrade but left the user-facing display instruction unchanged.
Fix: change line 437 to "uvx": f"uv tool install --upgrade {MCP_PACKAGE_NAME}" (matching what _perform_mcp_update now actually runs). Optionally also update the _detect_mcp_install_method docstring at line 222 which still says "uvx -- upgrade with uvx --refresh ...".
[B-2] src/keboola_agent_cli/services/version_service.py:641-663 — fresh-install via uvx promotion reports updated=False and emits "still vNone"
_update_mcp() computes actually_updated = bool(success and post_version and local_version and post_version != local_version). When a user has only a uvx-cache install (_get_local_mcp_version() returns None pre-upgrade, method is "uvx"), _perform_mcp_update now runs uv tool install --upgrade which succeeds, and the post-upgrade probe returns the real version (e.g. "1.59.1"). The and local_version clause short-circuits to False because local_version is None, so actually_updated=False. The message branch falls to the else arm and emits "upgrade exit 0 but local version still vNone" — both the result (updated=False) and the message are wrong for what is actually a successful fresh install.
Note: the same code path in auto_update._maybe_update_mcp is correctly protected by the Bug C gate (if local_version is None: return mcp_latest at line 325) which prevents reaching the pre/post comparison. _update_mcp intentionally skips that gate (explicit kbagent update should attempt the upgrade even without a detected version), but it therefore inherits the edge case.
Fix: in _update_mcp, change actually_updated to also be True when success and post_version is set and local_version was None (i.e. it's a fresh install, not a backtrack). A minimal fix: actually_updated = bool(success and post_version and (local_version is None or post_version != local_version)). Add a corresponding test simulating local_version=None + post_version="1.59.1" asserting updated=True and a success message (not the diagnostic "still vNone" message).
Non-blocking findings
[NB-1] plugins/kbagent/skills/kbagent/references/gotchas.md:13-14 — stale uvx --refresh description in the v0.30.1 MCP auto-update gotcha
The gotcha added in v0.30.1 lists the uvx upgrade command as uvx --refresh on line 13-14. After Bug B's fix, the actual upgrade command for uvx-method users is now uv tool install --upgrade keboola-mcp-server. An AI agent reading this gotcha would suggest the broken command to users who ask how to manually upgrade the MCP server when they have a uvx-cache install. Per CONTRIBUTING.md §17 (silent-drift surfaces), this file has no CI freshness check.
Fix: update line 13-14 to reflect the promoted command, and add a (since v0.30.3) tag to note the change.
[NB-2] src/keboola_agent_cli/services/version_service.py:480 — docstring still says "uvx --refresh" for the MCP upgrade description in self_update
The self_update docstring at line 480 describes Stage 2 as running uvx --refresh as one of the upgrade options. After Bug B, the actual behavior is uv tool install --upgrade. This is documentation drift within the code — less severe than the user-facing display issue (B-1) but still misleads contributors reading the docstring.
Fix: update line 480 to say uv tool install --upgrade for the uvx case.
[NB-3] PR body "What changed" section has no "### Bug E" description
The PR body documents Bugs B, C, and D with dedicated subsections but has no ### Bug E entry. Bug E (the false-success detection from the ottomansky reproducer) is mentioned only in the test plan checkbox and the changelog entry. The commit 046831a that added Bug E was apparently a late addition. This makes the PR history harder to navigate when bisecting future regressions against the fix.
Fix: add a ### Bug E subsection to "What changed" matching the level of detail present for B, C, and D.
Nits
[NIT-1]tests/test_version_service.py:590-598— the comment "Bug E fix: pre-upgrade returns 1.49.0; post-upgrade returns 1.59.1" is correct but the pre-existing test nametest_only_mcp_stale_kbagent_uptodate_still_runs_mcpno longer captures the Bug E contract being tested. Consider a companion test namedtest_local_version_none_post_set_is_fresh_installto cover the B-2 fix when you add it.
Verification log
gh pr view 265 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 9 files, +323/-33, state OPEN, conventionalfix(0.30.3):prefix ✓git rev-parse --abbrev-ref HEADin/tmp/kbagent-263-fix→fix/0.30.3-issue-263✓ (matches PR branch)wc -l /tmp/kbagent-pr-265.diff→ 529 lines ✓- Layer violation checks (typer in services, httpx in commands, formatter in clients) → empty (no violations) ✓
grep -E '@.*\.command' /tmp/kbagent-pr-265.diff→ empty (no new CLI commands; OPERATION_REGISTRY and --hint checks N/A) ✓make checkin/tmp/kbagent-263-fix→ 2780 passed, 7 skipped, 14 warnings ✓ (green)- Layer 3 (version_service.py:437):
grep 'uvx.*refresh' version_service.py→ confirmed staleuvx --refresh --fromupgrade_command at line 437 while_perform_mcp_update("uvx")now runsuv tool install --upgrade— B-1 confirmed _update_mcplogic trace:actually_updated = bool(success and post_version and local_version and ...)— whenlocal_version=None,and local_versionshort-circuits toFalse; message falls toelsebranch emitting"still vNone"— B-2 confirmedauto_update._maybe_update_mcpsame path: Bug C gate at line 325 returns early whenlocal_version is None, so the pre/post comparison is never reached → B-2 does NOT apply to the auto-update path, only to_update_mcp✓grep 'uvx.*refresh' gotchas.md→ line 13-14 still saysuvx --refresh— NB-1 confirmedgrep 'uvx.*refresh' version_service.py→ line 480 (docstring) still saysuvx --refresh— NB-2 confirmed- PR body reviewed: no
### Bug Esection in "What changed" — NB-3 confirmed - PR body test plan says "2,779 tests";
make checkshows 2,780 — one test added in commit046831aafter the original description was written (minor inconsistency only) - Behavior reproduction: could not run against a real uvx-cache install (no matching environment available). B-1 confirmed by static analysis; B-2 confirmed by code trace and absence of a test covering
local_version=None+post_version=set. - Plugin synchronization map: no new commands added, no command renames — OPERATION_REGISTRY, --hint definitions, AGENT_CONTEXT, CLAUDE.md All CLI Commands, commands-reference.md: all N/A for this PR. Version bump 0.30.2 → 0.30.3: changelog.py ✓, plugin.json ✓, marketplace.json ✓, pyproject.toml ✓, uv.lock ✓. keboola-expert.md VERSION GATE: no new version-gated commands added — no update required.
- Security checks (token masking, bare except, magic numbers, print in production): all clean ✓
Open questions for the author
_detect_mcp_install_method(line 222) still documents"uvx"as usinguvx --refresh. After Bug B, a user withuvx-only install who runskbagent updategets promoted to auv tool install. Is there a scenario where the user hasuvxbut NOTuv? If yes, Bug B's guardif uv_path is None: return False, "uv not found on PATH ..."is correct, but the error message should probably suggest installinguvexplicitly. This is not a new gap but worth confirming the expected UX for uvx-without-uv users.
…sh-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.
|
Addressed both blocking findings from the latest review iteration in commit B-1 ( user-facing cmd drift): B-2 (fresh-install guard hole): Rewrote the guard to handle all four success-branch cases explicitly:
The auto-update startup path ( New regression tests:
|
Adds `--new-alias NEW` to `kbagent project edit` so users can rename a project alias without going through `project remove` + `project add` (which forces token re-entry). Mirrors the `kbagent config rename` precedent for the on-disk part of the cascade. Cascading scope: - config.json `projects` dict key (`pop(old)` + insert under `new`) - config.json `default_project` field (when it matched the old alias) - nested-layout sync directory `<cwd>/<old-alias>/` -> `<cwd>/<new-alias>/` (with -2 collision suffix, git-mv-with-shutil-fallback) - WARNS on `*.lineage.json` -- caches embed alias FQNs and are NOT auto-rewritten (partial rewrites are worse than no rewrite) Combined with `--url` and/or `--token` in one call, those mutations target the new alias post-rename: `kbagent project edit --project foo --new-alias bar --token NEW` is one atomic operation. Service / Command: - `commands/project.py` -- new `--new-alias` Typer option; human formatter branch when result has `old_alias` - `services/project_service.py` -- `edit_project` accepts `new_alias` + `search_root`; new `_rename_project_alias`, `_validate_alias_format`, `_rename_nested_sync_dir`, `_move_directory`, `_detect_lineage_cache_warning` helpers ConfigStore: - `config_store.py` -- new `rename_project(old, new)` method (atomic dict-key swap + `default_project` cascade in one save() call) Security hardening (from review iter 2): - Validator regex `[A-Za-z0-9_][A-Za-z0-9_.-]*` plus explicit `..` rejection; rejects path traversal, NUL bytes, leading dot/dash, whitespace anywhere, and characters outside the slug alphabet. Stricter than `project add`'s no-op check; rationale is the rename's filesystem interaction (alias becomes a directory name). - `search_root` resolved via `Path.resolve()` once before the disk rename to collapse symlinks; closes a malicious-cwd vector. - Disk rename failures (`OSError`) trigger a config rollback so config and disk never end up out of sync; rollback's own failure is suppressed via `contextlib.suppress` so the original error wins. - Lineage cache scan depth-capped at 2 levels (top + `*/` + `*/*/`) to bound cost when search_root is a deep tree. Tests: 32 new (28 service + 4 CLI). Pin alias-key swap, collision rejection, default_project cascade, sync-dir disk rename, no-sync-dir no-op, sync-dir collision -2 suffix, combined edit-and-rename, no-op on same-alias-only, parametrized 9-input path-traversal validator, legal slug shapes accepted, OS failure rolls config back, rollback failure surfaces original error, symlink target collision triggers suffix bump. Sync map updates: - AGENT_CONTEXT (commands/context.py) -- new flag mentioned - CLAUDE.md `## All CLI Commands` -- same wording - commands-reference.md -- expanded with cascade scope - gotchas.md -- new `(since v0.30.4)` entry on lineage cache rebuild - keboola-expert.md -- VERSION GATE clause + tool selection matrix row Live-validated against project 1143 (`99_Playground_Max`, europe-west3.gcp.keboola.com): rename to `playground` + reverse rename to baseline; nested sync dir moved on disk; default_project cascaded; all error paths produce correct ConfigError exit-5 messages. Three review iterations: self -> independent -> convergence; zero material findings on the convergence pass. Rebased onto upstream/main after keboola#265 (closes keboola#263) merged at 8dedfd1 took the 0.30.3 slot; this PR ships as 0.30.4.
Summary
PR #262 (v0.30.2) addressed only Bug A from issue #263 (probe returned
Noneforuv tool-managed installs). The reporter analyzed the code and identified four chained bugs; this PR addresses the remaining three.Closes the rest of #263.
What changed
Bug B -- uvx upgrade subprocess used a flag the binary does not honour
_perform_mcp_updateforuvx-cache installs ran:The trailing
--versionis 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.Fix: promote uvx to
uv tool install --upgrade keboola-mcp-server, matching whatkbagent doctor --fixalready does. The binary lands on PATH, so subsequent runs use the fasteruv_tooldetection path. Self-healing.Bug C -- probe-
Nonefell through to the upgrade attempt_maybe_update_mcphad no gate for the case where_get_local_mcp_version()returnsNone.up_to_date == None(notTrue) bypassed the short-circuit, the function fell through to a broken upgrade subprocess every TTL window, and the user saw anUpdating ... vunknown -> v1.59.1banner once per kbagent invocation.Fix: add
if local_version is None: return mcp_latest-- opts out of the upgrade for this TTL window. Cache TTL still ticks; the next fresh-cache pass retries detection.Bug D --
kbagent replre-ran auto-update on every promptmaybe_auto_update()is called fromcli.py:main()on every CLI entry.kbagent replre-enters Click on every prompt iteration, so the auto-update flow fired (and printed banners) once per command typed at the prompt.Fix: a module-level
_AUTO_UPDATE_RAN: bool = Falsesentinel. First call flips it to True; subsequent in-process calls short-circuit. The flag flips BEFORE any work so a crash mid-flow still gates subsequent re-entries. Re-exec'd processes (kbagent self-upgrade ->execvpeto 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.Test plan
make checkgreen: 2,779 tests passed, 7 skipped, lint + format + skill + version + changelog clean.TestPerformMcpUpdate.test_uvx_promotes_to_uv_tool_install-- asserts the new uvx cmd; explicitly checks--versionis GONETestPerformMcpUpdate.test_uvx_promotion_requires_uv-- missing-uvfailure pathTestProbeNoneSkipsUpgrade.test_local_version_none_skips_upgrade-- the AC from v0.30.1: keboola-mcp-server auto-update banner re-fires on every kbagent invocation #263: probe -> None;_perform_mcp_updateNOT calledTestProcessLevelSentinel.test_second_call_short_circuits-- the AC from v0.30.1: keboola-mcp-server auto-update banner re-fires on every kbagent invocation #263:maybe_auto_updatebody runs once across N calls in the same processTestProcessLevelSentinel.test_sentinel_is_set_even_when_body_raises-- guards against flaky upstream PyPI re-firing the banner per promptTestMaybeAutoUpdate,TestMaybeAutoUpdateMcpIntegration, andTestReExecPathStillRunsMcpautouse fixtures extended to reset_AUTO_UPDATE_RANbetween tests so the sentinel does not gate the second test in each class.Acceptance criteria from issue #263 -- final state
kbagent project listprints zero auto-update noise on a healthy install (already true after Bug A fix in v0.30.2; still true here)kbagent replruns the auto-update check at most once per session (Bug D)Nonedoes not fall through to the upgrade attempt (Bug C)uvx-cache installs upgrade cleanly via promotion touv tool install --upgrade(Bug B)_get_local_mcp_version() -> Noneand asserts_perform_mcp_updateis NOT calledmaybe_auto_update()