feat(0.30.1): auto-update keboola-mcp-server on startup, parity with kbagent (closes #243) - #257
Conversation
padak
left a comment
There was a problem hiding this comment.
Review of #257 — feat(0.28.1): auto-update keboola-mcp-server on startup
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 extends the existing kbagent startup auto-update to run a second "MCP stage"
that upgrades keboola-mcp-server via the appropriate package manager (uv tool upgrade
/ pip install -U / uvx --refresh). It also makes kbagent update and
kbagent version two-stage aware. The motivation (issue #243: silent staleness trap)
is clear and the overall design is solid. However, there is one blocking correctness
gap: when kbagent itself is successfully upgraded on startup the re-exec guard
(KBAGENT_SKIP_UPDATE=1) causes the MCP stage to be skipped entirely in the new
process -- which is the scenario the PR most prominently claims to fix. A second
blocking gap is that plugins/kbagent/skills/kbagent/references/commands-reference.md
was not updated to reflect the changed behaviour of update and version commands.
Verdict: REQUEST CHANGES.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 2
- Non-blocking findings: 2
- Nits: 2
Blocking findings
[B-1] src/keboola_agent_cli/auto_update.py:353-354 — re-exec guard silently skips MCP stage after successful kbagent self-upgrade
When kbagent is stale, _perform_update succeeds, and _re_exec() is called (line 353),
the process is replaced. The replacement process enters maybe_auto_update and hits
_should_skip() at line 154: ENV_SKIP_UPDATE == "1" is true (set by _re_exec at
line 218) so the function returns immediately. Stage 2 (MCP upgrade) never runs in
this scenario -- the exact scenario where a stale kbagent most likely coincides with a
stale MCP server.
The PR description states "Critical invariant: kbagent up-to-date does NOT short-circuit
the MCP stage." This holds only when kbagent was already current. When kbagent was just
upgraded, the invariant is broken.
The test test_newer_available_updates_and_reexec (test_auto_update.py:361) uses the
class-level _no_real_mcp_calls autouse fixture which stubs _maybe_update_mcp to
return None. Because _re_exec() is also mocked and immediately returns, the code
hits the return on line 354 and Stage 2 is never reached. The test does not assert
that _maybe_update_mcp was (or was not) called, so the gap is invisible.
Fix option A: Before calling _re_exec(), run _maybe_update_mcp() and include the
result in the pre-exec _write_cache call so the new process's first action correctly
reflects an up-to-date MCP state. Option B: move ENV_SKIP_UPDATE to only guard
kbagent's own stage (not the whole function), so the re-exec'd process still runs Stage 2.
Add a test asserting _maybe_update_mcp.assert_called_once() in the
test_newer_available_updates_and_reexec scenario.
[B-2] plugins/kbagent/skills/kbagent/references/commands-reference.md:9,173 — update and version command descriptions not updated for 0.28.1 behaviour
Per CONTRIBUTING.md "Plugin synchronization map": commands-reference.md must be updated
for "flag changes" and behavior changes to existing commands. Line 9 still reads
update -- self-update to latest version (single-stage language); line 173 reads
version -- show version and check for MCP server updates but does not reflect that
version now also shows the locally installed MCP version (version field) and
install_method. AI agents reading the reference will describe kbagent update as
single-stage and miss that the JSON output now contains separate kbagent and mcp
blocks. This is a silent-drift risk per the playbook.
Fix: update line 9 to update -- two-stage update: kbagent + keboola-mcp-server (JSON output contains {kbagent: {...}, mcp: {...}, updated, message}); update line 173 to
mention dependencies[].version (installed MCP version) and dependencies[].install_method.
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/version_service.py:119-122 — uv tool list timeout falls back to pip_env, causing wrong upgrade command
In _detect_mcp_install_method: if the MCP binary exists on PATH and uv is also on
PATH but uv tool list times out or raises OSError (line 119), the function returns
"pip_env" (line 122). The upgrade then runs pip install --upgrade keboola-mcp-server
which installs into the active Python virtual environment, not into uv's tool store
(~/.local/share/uv/tools/). The uv-managed binary on PATH continues to run the old
version because its store was not updated.
A matching test case is absent: TestDetectMcpInstallMethod tests uv_tool (happy
path), pip_env (not in uv tool list), uvx, and none but not uv tool list
TimeoutExpired while binary is present.
Suggested fix: on TimeoutExpired/OSError from uv tool list, return "uv_tool" as a
safe assumption (uv is available + binary is on PATH → most likely a uv tool install);
add a test test_uv_tool_list_timeout_falls_back_to_uv_tool.
[NB-2] src/keboola_agent_cli/auto_update.py:362 — old-format cache triggers repeated PyPI round-trips within TTL
When an older version_cache.json (pre-0.28.1, containing last_check and
latest_version but no mcp_latest_version) is still within TTL:
cache_is_fresh is True → latest_version is read from cache → kbagent stage
short-circuits (already up-to-date) → _maybe_update_mcp(cache, fetched_now=False) is
called → cached_latest is None (key absent) → condition not fetched_now and cached_latest is False → fresh PyPI fetch fires → MCP upgrade may run → but
_write_cache is not called (not cache_is_fresh is False at line 362).
On every subsequent invocation within the same TTL window the same fetch+upgrade
sequence repeats because mcp_latest_version is never persisted back.
For users migrating from 0.28.0, this means one extra PyPI call per kbagent invocation
until the cache expires (~1 hour by default). Not a crash, not a security issue, but
inconsistent with the documented "at most two PyPI/GitHub round-trips per
AUTO_UPDATE_CHECK_INTERVAL" guarantee.
Suggested fix: extend the _write_cache guard to also fire when cache_is_fresh but
mcp_latest was freshly fetched (e.g. or (cache_is_fresh and mcp_latest is not None and cache.get("mcp_latest_version") != mcp_latest)).
Nits
-
[NIT-1]src/keboola_agent_cli/services/version_service.py:115,auto_update.py:201,290— magic timeout values5,120,180.0are hardcoded inline. PerCONTRIBUTING.mdcoding conventions all timeouts belong inconstants.py. ConsiderMCP_VERSION_PROBE_TIMEOUT,KBAGENT_UPGRADE_TIMEOUT,MCP_UPGRADE_TIMEOUT. -
[NIT-2]src/keboola_agent_cli/commands/context.py:684—AGENT_CONTEXTdescription forkbagent updatestill reads "Self-update kbagent to latest version (via uv tool install --upgrade)". After this PR the command upgrades both kbagent and MCP. AI agents queryingkbagent contextat session start will have a stale description. Updating this line would close the loop without touching any CI-checked surface.
Verification log
git -C /tmp/kbagent-mcp-update rev-parse --abbrev-ref HEAD→feat/mcp-auto-update✓ (working tree on PR branch)gh pr view 257 --json title,body,files,additions,deletions,state→ 11 files, +962/-65,state=OPEN✓gh pr diff 257→ 1289 lines, fetched to/tmp/kbagent-pr-257.diff✓- Layer violation grep (typer in services, httpx in commands, formatter in clients) → empty ✓
- httpx calls in
version_service.pytargetgithub.laiyagushi.com/ghapiandpypi.org, NOT*.keboola.com✓ - Magic numbers grep (
time.sleep,timeout=\d+withoutconstants.) → hits atversion_service.py:115,auto_update.py:201,290,version_service.py:452,510(NIT-1) make check(cd /tmp/kbagent-mcp-update) → 2508 passed, 6 skipped, exit 0 ✓grep -n "ENV_SKIP_UPDATE" auto_update.py→ set at line 218 (_re_exec), checked at line 154 (_should_skip) → confirms B-1: SKIP_UPDATE blocks entire function including Stage 2 in re-exec'd process ✓grep -n "return.*Defensive" auto_update.py→ line 354 → confirmsreturnis hit after mocked_re_execin tests, preventing Stage 2 even in test execution ✓grep -n "mcp_latest\|write_cache" test_auto_update.py→test_newer_available_updates_and_reexecassertsmock_reexec.assert_called_once()but does NOT assertmock_mcp.assert_called_once()→ test gap for B-1 confirmed ✓grep -n "not cache_is_fresh" auto_update.py:362→ cache write guarded bynot cache_is_freshonly → NB-2 (old cache migration) confirmed ✓grep -n "TimeoutExpired.*pip_env\|pip_env.*TimeoutExpired" test_version_service.py→ no matching test → NB-1 gap confirmed ✓grep -n "commands-reference.md:9,173"→ line 9:update -- self-update to latest version, line 173:version -- show version and check for MCP server updates→ both stale for 0.28.1 → B-2 confirmed ✓CONTRIBUTING.mdPlugin synchronization map cross-check: no new commands →permissions.py,hints/definitions/,keboola-expert.md §2 matrix,CLAUDE.md ## All CLI Commandsall correctly left untouched ✓gotchas.mdnew entry →(since v0.28.1)tag present ✓changelog.py0.28.1 entry → present ✓pyproject.tomlversion bump0.28.0 → 0.28.1✓plugin.json+marketplace.jsonauto-synced ✓- Behavior reproduction via
kbagent update/kbagent versionnot possible without real credentials; unit test coverage is present and passes ✓
Open questions for the author
- Was skipping the MCP stage in the re-exec'd process (B-1) intentional? If MCP upgrades
are expected to be long-running (up to 180s), running them before re-exec would add
latency to the update flow. If that tradeoff was deliberate, the PR description's
"both stages always run" claim should be scoped to "both stages run when kbagent is
already up-to-date" to avoid future confusion.
…kbagent Closes #243 silent-staleness trap: a user installed keboola-mcp-server once via uv tool install, then ran kbagent for months while upstream shipped six minor versions; the cached schema was missing configuration_row_ids (added in MCP v1.55.0) and the user had no signal anything was behind. Two-stage auto-update on startup (auto_update.maybe_auto_update): 1. kbagent self-upgrade -> re-execs new binary 2. keboola-mcp-server upgrade -> uv tool upgrade / pip install -U / uvx --refresh, depending on detected install method Critical invariant: kbagent up-to-date does NOT short-circuit the MCP stage. Both stages always run. kbagent update now upgrades both. JSON output reports per-stage results plus a one-line message summary. kbagent version now reports the locally installed MCP version + up-to-date status, fixing the previously-lying auto_updates: True claim (the field is now actually true, but via the kbagent startup flow, not via uvx-on- every-call). 25 new unit tests across version_service + auto_update. Bumps 0.30.0 -> 0.30.1.
9cde3d8 to
a3a059f
Compare
padak
left a comment
There was a problem hiding this comment.
Review of #257 — feat(0.30.1): auto-update keboola-mcp-server on startup, parity with kbagent
Generated by
kbagent-pr-reviewersubagent (re-review after rebase on main / 0.30.0). 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
Tato re-review verifikuje rebase commitu a3a059f po tom, co main pokročil na 0.30.0 přes PR #255, #256, #259 a #261. PR přidává dvoustupňové auto-update flow při startu kbagent: (1) kbagent self-upgrade (stávající), (2) nový keboola-mcp-server upgrade detekující metodu instalace (uv_tool / pip_env / uvx). Verze 0.30.1 je konzistentně propagována přes pyproject.toml, plugin.json, marketplace.json a changelog.py. Rebase je čistý — žádný konflikt nezanechal nežádoucí artefakty v produkčním kódu. Oba dříve identifikované blocking nálezy (B-1 re-exec guard, B-2 uv tool list robustnost) přetrvávají beze změny. Přibyl jeden nový NON-BLOCKING nález (stale docstring _format_dep_auto_update v version.py způsobuje, že kbagent version v human módu zobrazuje PyPI latest jako "instalovanou" verzi místo skutečné lokální verze — data jsou dostupná v JSON módu, ale human mód je nepoužívá). Přidány 2 nové NITs ze stale komentářů v testovacích souborech.
Verdict: REQUEST CHANGES — B-1 a B-2 jsou carry-over z předchozí iterace a zůstávají neadresovány.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 2 (carry-over from previous review round; explicitly disclosed in PR description)
- Non-blocking findings: 3
- Nits: 2
Blocking findings
[B-1] src/keboola_agent_cli/auto_update.py:327-328 — re-exec guard silently skips MCP stage in re-exec'd process
_should_skip() at line 154 returns True when KBAGENT_SKIP_UPDATE=1 is set. maybe_auto_update() calls _should_skip() at line 327 and returns immediately if True, which means Stage 2 (MCP upgrade, line 358) is never reached in the process that was spawned after a kbagent self-upgrade. The PR description correctly identifies this and states it is not addressed. The "both stages always run" invariant documented in the PR, the changelog, and gotchas.md holds only when kbagent was already up-to-date at startup.
Fix: gate the re-exec guard at the kbagent stage only — either extract a _should_skip_kbagent() vs _should_skip_mcp() split, or persist a mcp_update_pending=true flag to the cache before re-exec so the re-exec'd process picks it up and runs the MCP stage despite ENV_SKIP_UPDATE=1.
[B-2] src/keboola_agent_cli/services/version_service.py:117 — _detect_mcp_install_method() classifies uv_tool via substring match on uv tool list stdout
The detection at line 117 (if result.returncode == 0 and MCP_PACKAGE_NAME in result.stdout) checks whether the literal string "keboola-mcp-server" appears anywhere in uv tool list's stdout. This is fragile in at least two ways: (a) if a user has another tool with a name that happens to contain "keboola-mcp-server" as a substring (e.g. a fork named keboola-mcp-server-fork), the package will be misclassified as uv_tool even if it is not; (b) uv tool list output format is not contractually stable across uv versions — in particular, the --format json flag was added in uv 0.5.x; relying on human-readable text parsing risks a silent classification change on uv upgrade. This affects both _detect_mcp_install_method() (startup auto-update path) and VersionService.self_update() (explicit update command).
Fix: use [uv_path, "tool", "list", "--format", "json"] (uv ≥0.5) and check that an entry's name field equals MCP_PACKAGE_NAME exactly. Add a fallback to the current substring check when the JSON flag is unavailable (uv <0.5 on legacy systems).
Non-blocking findings
[NB-1] src/keboola_agent_cli/commands/version.py:43-62 — human-mode kbagent version still uses _format_dep_auto_update() which does NOT render the local MCP version
VersionService.get_versions() now returns dependencies[0].version (local installed MCP version) and dependencies[0].up_to_date. However, since auto_updates: True is set on the MCP entry, version_command dispatches to _format_dep_auto_update() (line 81-82 of version.py). That function reads only dep.get("latest_version") and renders v{latest} auto-updates, completely ignoring the new version (local) and up_to_date fields. The PR description claims "kbagent version now shows the locally installed MCP version" — this is accurate only in --json mode. A user running kbagent version in human mode will see v1.60.0 auto-updates even when their local install is v1.49.0. The PR's primary motivation is eliminating the staleness trap; this gap means the human-mode signal is still absent.
Fix: update _format_dep_auto_update() to branch on whether dep.get("version") (local) is available: if so, render the same format as _format_dep_standard() (local vs latest, with upgrade command hint when stale). The function docstring ("""Format an auto-updating dependency (runs via uvx @latest).""") is also stale and should be updated.
[NB-2] src/keboola_agent_cli/commands/context.py:786 — AGENT_CONTEXT describes kbagent update as self-update only
The AGENT_CONTEXT string (loaded by AI agents at session start) says at line 786: Self-update kbagent to latest version (via uv tool install --upgrade). Since this PR makes kbagent update also upgrade keboola-mcp-server, AI agents reading this description will not know that running kbagent update resolves a stale MCP install. This is particularly important because the PR's stated goal is fixing a staleness trap that manifests as missing MCP schema fields — and the fix is invisible to agents unless they know kbagent update now covers both.
Fix: update line 786 to: Self-update kbagent and keboola-mcp-server to their latest versions. Reports per-stage results. Also update plugins/kbagent/skills/kbagent/references/commands-reference.md:9 which similarly reads - \update` -- self-update to latest version`.
[NB-3] src/keboola_agent_cli/services/version_service.py:452 and version_service.py:186-187 — hardcoded timeout=120 and timeout=180.0 magic numbers not in constants.py
_update_kbagent() at line 452 passes timeout=120 to the kbagent upgrade subprocess. _perform_mcp_update() at line 146 uses timeout: float = 180.0 as default, and _update_mcp() at line 510 explicitly passes timeout=180.0. Neither value appears in constants.py. The existing coding convention (CONTRIBUTING.md §"Constants -- no magic numbers") and constants.py line 3 ("""All magic numbers, default values, retry parameters, timeout settings,...""") require these to live there. The timeout=5 at version_service.py:115 (for uv tool list probe) is similarly missing. Note: timeout=120 existed in the pre-PR version_service.py (carried forward), so this is not a new regression introduced by the rebase — but the PR adds timeout=180.0 (new) without a constant, making the gap wider.
Fix: add MCP_UPGRADE_TIMEOUT: float = 180.0, MCP_DETECT_TIMEOUT: float = 5.0, and KBAGENT_UPGRADE_TIMEOUT: int = 120 to constants.py, reference them from version_service.py.
Nits
-
[NIT-1]tests/test_auto_update.py:481,570andtests/test_version_service.py:120,161,212,269,326— section-header comments say(since v0.28.1)throughout all new test code (all+lines in the diff). The sed pass in the rebase (v0.28.1→v0.30.1) was applied only togotchas.mdandmcp_service.py; test file comments were not included. Not user-visible, but creates internal confusion about when these helpers were introduced. Update to(since v0.30.1). -
[NIT-2]src/keboola_agent_cli/commands/version.py:44— docstring"""Format an auto-updating dependency (runs via uvx @latest)."""is no longer accurate since MCP is now upgraded viauv tool upgrade/pip install -U/uvx --refresh, not viauvx @lateston every call. This file is not in the PR's changed set; address alongside NB-1 fix.
Verification log
gh auth status→ authenticated to github.com aspadak✓gh pr view 257 --json state→"OPEN"✓git -C /tmp/kbagent-mcp-update rev-parse --abbrev-ref HEAD→feat/mcp-auto-update✓ (correct branch)git -C /tmp/kbagent-mcp-update log --oneline origin/main..HEAD→ single commita3a059f✓ (clean single-commit rebase)- Version consistency:
pyproject.toml=0.30.1,plugin.json=0.30.1,marketplace.json=0.30.1,changelog.pyfirst key =0.30.1(above0.30.0) ✓ gotchas.md— new entry heading:## \keboola-mcp-server` is now auto-updated on kbagent startup (since v0.30.1)` ✓mcp_service.py:104— docstring containssince v0.30.1✓ (sed pass applied correctly)- 3-layer checks — no typer/click imports in services, no httpx in commands, no formatter in clients (all
grepreturned empty) ✓; note:httpxinversion_service.pyis pre-existing pattern onmain, not introduced by this PR - Magic number scan —
timeout=180.0andtimeout=5are new values not inconstants.py(NB-3);timeout=120was pre-existing make checkin/tmp/kbagent-mcp-update→2755 passed, 7 skipped(exit 0) ✓- B-1 confirmed:
auto_update.py:327calls_should_skip()unconditionally;ENV_SKIP_UPDATE=1is set in_re_exec()env (line 218) beforeos.execvpe; re-exec'd process returns early frommaybe_auto_updatebefore reaching Stage 2 at line 358. No code in the rebase addresses this. - B-2 confirmed:
version_service.py:117usesMCP_PACKAGE_NAME in result.stdout(string substring). No--format jsonfallback added. No format-change resistance. tests/test_auto_update.py—TestMaybeAutoUpdateMcpIntegration.test_kbagent_uptodate_still_runs_mcp_stageasserts MCP stage runs when kbagent is up-to-date ✓; but NO test forKBAGENT_SKIP_UPDATE=1+ MCP stage (the B-1 scenario)tests/test_version_service.py:120—assert mcp_dep["version"] == "1.46.0"verifies new field in JSON ✓; no CLI-layer test for human-mode rendering of local vs latest MCP version- Stale
since v0.28.1comments: 7 occurrences in new (+) lines across both test files; all are test section headers / inline comments, not user-facing - Behavior reproduction:
kbagent updateandkbagent versionrequire live credentials and a running environment; not reproduced in this review — NB-1 and NB-2 are derived from static code reading confirmed by test inspection.
Open questions for the author
_format_dep_auto_update()inversion.pypredates this PR. Was the decision to keep theauto_updates=Truerouting (and therefore the_format_dep_auto_updatecode path) intentional for this PR, with the human-mode rendering improvement deferred to a follow-up? If so, documenting that decision in the PR description would prevent reviewers from re-raising NB-1 on each iteration.
…1..2) B-1: re-exec guard now skips ONLY the kbagent stage. Pre-fix `_should_skip()` gated the entire flow on KBAGENT_SKIP_UPDATE=1, so the re-exec'd process after a kbagent self-upgrade silently dropped the MCP stage -- exactly the case where both stages are most likely needed. Split into `_should_skip_kbagent_stage()` (re-exec guard only) and `_should_skip_all()` (dev install / opt-out / update|version commands). The orchestrator now consults each at the matching stage so Stage 2 always runs regardless of Stage 1's gate state. Backwards- compatible `_should_skip()` alias preserved for old callers. Regression test class `TestReExecPathStillRunsMcp` pins the contract: re-exec scenario calls Stage 2 exactly once; user opt-out and `kbagent update` still skip both. B-2: `_detect_mcp_install_method()` no longer uses a fragile `MCP_PACKAGE_NAME in stdout` substring match against `uv tool list` output. New `_uv_tool_list_has_mcp(stdout)` helper splits per-line, skips indented continuation lines (binary listings under a tool), and requires exact equality on the first whitespace-separated token of a non-indented line. Rejects similarly-named packages (`keboola-mcp-server-foo`), indented binary references (` - keboola_mcp_server` under another tool), and accidentally- merged stderr text. Seven new unit tests in `TestUvToolListHasMcp` cover exact match, similar-name false-positive rejection, indented-only input, multi-tool listings, empty / blank input, and trailing whitespace tolerance. NB-1: `commands/version.py::_format_dep_auto_update` now reports the locally installed version + up-to-date status. Previously human-mode `kbagent version` ignored the new `version` and `up_to_date` fields and showed only the PyPI latest, defeating the staleness- visibility goal in non-JSON mode. Renderer now mirrors `_format_dep_standard` for the auto-update branch (local + ->latest when stale + 'auto-updates (up to date)' when fresh + 'not installed' fallback for install_method=='none'). NB-2: `commands/context.py` AGENT_CONTEXT entries for `kbagent update` and `kbagent version` rewritten to describe the two-stage upgrade (kbagent + keboola-mcp-server) and the always-runs-on-startup semantics. Pre-fix the AGENT_CONTEXT still claimed `update` was kbagent-only, leaving AI agents unable to explain the new behaviour to users when asked. NB-3: timeouts `5` and `180.0` lifted into `MCP_PROBE_TIMEOUT` and `MCP_UPGRADE_TIMEOUT` constants in `constants.py` per CONTRIBUTING.md 'no magic numbers' rule. NIT-1: 7 stale `(since v0.28.1)` references in `tests/test_auto_update.py` and `tests/test_version_service.py` updated to `(since v0.30.1)`. NIT-2: `update_command` docstring rewritten to describe the two-stage flow (no longer says 'self-update kbagent' as if MCP weren't part of it). Companion docstring on `_format_dep_auto_update` no longer references uvx @latest as the auto-update mechanism. 89 unit tests pass (added: 3 B-1 regression + 7 B-2 parser + minor fixture adjustments). `make check` clean.
|
Addressed all 7 review findings (
Ready for re-review. |
padak
left a comment
There was a problem hiding this comment.
Review of #257 — feat(0.30.1): auto-update keboola-mcp-server on startup, parity with kbagent (closes #243)
Generated by
kbagent-pr-reviewersubagent, third iteration. 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 adds two-stage auto-update support for keboola-mcp-server (in addition to the existing kbagent self-upgrade), closes the silent-staleness trap from issue #243, and extends kbagent update / kbagent version to cover both kbagent and MCP. All seven findings from the previous two review iterations have been addressed in commit 7e73384. The fix introduces one new NON-BLOCKING finding (a test that patches the obsolete _should_skip alias and therefore no longer exercises the exception-swallow contract it claims to test) and two nits (dead patches on 9 other tests in the same class, and a hardcoded constant value in one assertion). No new blocking issues found. Verdict: APPROVE.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 1
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] tests/test_auto_update.py:431 — test_exception_never_crashes patches a function the orchestrator no longer calls
test_exception_never_crashes decorates with @patch("keboola_agent_cli.auto_update._should_skip", side_effect=RuntimeError("kaboom")). After the B-1 refactor, maybe_auto_update calls _should_skip_all() and _should_skip_kbagent_stage() directly — _should_skip (the backwards-compatible alias) is never invoked by the orchestrator. The RuntimeError is therefore never triggered. The autouse fixture in TestMaybeAutoUpdate provides _should_skip_all=False, so the function proceeds to Stage 1, where _fetch_kbagent_latest_version is NOT patched in this test and can make a real GitHub API call. The test still passes for the wrong reason (blanket except in maybe_auto_update catches anything), but the intended contract — "an exception in the skip gate must not crash the CLI" — is no longer being exercised.
Fix: replace @patch("…_should_skip", side_effect=RuntimeError("kaboom")) with @patch("…_should_skip_all", side_effect=RuntimeError("kaboom")) to target the function the orchestrator actually calls.
Nits
-
[NIT-1]tests/test_auto_update.py:334,344,364,376,398,421,439,621,642,665— Nine tests inTestMaybeAutoUpdateandTestMaybeAutoUpdateMcpIntegrationstill carry@patch("keboola_agent_cli.auto_update._should_skip", return_value=False)decorators that are now dead (the orchestrator never calls_should_skip). The autouse fixture already handles the skip gates, so these decorators add noise and an unusedmock_skipparameter to each test signature. Safe to remove. -
[NIT-2]tests/test_auto_update.py:538—mock_perform.assert_called_once_with(method="uv_tool", timeout=180.0)hardcodes180.0instead of importingMCP_UPGRADE_TIMEOUTfromconstants. If the constant is ever tuned, this assertion will silently fail to catch a mismatch. Same issue attests/test_version_service.py:377(timeout=180.0). Both can be fixed withfrom keboola_agent_cli.constants import MCP_UPGRADE_TIMEOUTand substituting the symbol.
Verification log
gh pr view 257 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 15 files, +1321/-99,feat/mcp-auto-update→main, state OPEN ✓git -C /tmp/kbagent-mcp-update rev-parse HEAD→7e73384a7e5a9e8fb1d2743987aee1bfa88f5164✓ (matches PR head)make check(in/tmp/kbagent-mcp-update) →2765 passed, 7 skipped, 12 warningsexit 0 ✓
B-1 verified fixed: auto_update.py:368 calls _should_skip_all() first (gates both stages); auto_update.py:379 calls _should_skip_kbagent_stage() to gate Stage 1 only. Stage 2 (_maybe_update_mcp) is outside both guards and runs unconditionally when _should_skip_all() is False. Three regression tests in TestReExecPathStillRunsMcp (test_re_exec_skips_kbagent_but_runs_mcp, test_kbagent_command_update_skips_both, test_user_opt_out_skips_both) exercise the exact three-way split. Re-exec loop hazard checked: ENV_SKIP_UPDATE=1 is consumed by _should_skip_kbagent_stage() which never returns True from _should_skip_all(), so Stage 2 runs once and there is no further re-exec trigger. ✓
B-2 verified fixed: version_service.py:115-124 — per-line parser skips indented lines (line.startswith((" ", "\t"))), strips, then compares stripped.split(maxsplit=1)[0] == MCP_PACKAGE_NAME. Seven unit tests in TestUvToolListHasMcp cover: exact match, indented-binary-only (no match), similar-named package (keboola-mcp-server-foo, no match), mixed listing, empty input, blank-only, trailing-whitespace. Edge case keboola-mcp-server-foo correctly rejects because "keboola-mcp-server-foo" != "keboola-mcp-server". ✓
NB-1 verified fixed: commands/version.py:43-84 — _format_dep_auto_update reads dep.get("version") for local version and dep.get("up_to_date") for staleness signal; renders vX.Y.Z auto-updates (up to date) or vX.Y.Z -> vA.B.C (auto-updates on next startup) in human mode. Previously the function only showed "auto-updates" without any version. ✓
NB-2 verified fixed: commands/context.py:782-791 — kbagent version entry now reads "Reports both the locally installed version and the latest available; flags any staleness." kbagent update entry now reads "Two-stage upgrade (since 0.30.1): kbagent itself AND keboola-mcp-server. ... Both stages always run, regardless of whether kbagent itself needed an upgrade." ✓
NB-3 verified fixed: constants.py:162 — MCP_PROBE_TIMEOUT: float = 5.0; constants.py:166 — MCP_UPGRADE_TIMEOUT: float = 180.0. Both imported and used in version_service.py and auto_update.py; no inline literals remain in production code. ✓
NIT-1 verified fixed: grep -rn "since v0.28.1" tests/ → no output. Tests use (since v0.30.1) in comments/section headers throughout test_auto_update.py and test_version_service.py. ✓
NIT-2 verified fixed: update_command at commands/version.py:118 has a detailed docstring describing the two-stage upgrade contract and JSON output shape. _format_dep_auto_update at commands/version.py:43 has a docstring explaining the v0.30.1 context and what fields it surfaces. ✓
New NB-1 confirmed (test_exception_never_crashes): grep -n "_should_skip\b" src/keboola_agent_cli/auto_update.py → only line 197 (definition) and line 205 (alias body). maybe_auto_update calls only _should_skip_all and _should_skip_kbagent_stage. The test patches the alias which is never called. The test passes because the autouse fixture supplies _should_skip_all=False and the blanket except absorbs any downstream failures. ✓ (confirmed broken test contract)
No re-exec infinite loop: The only call to _re_exec() is inside the if _perform_update(latest_version) branch at auto_update.py:401. _re_exec sets ENV_SKIP_UPDATE=1 before execvpe. In the new process, _should_skip_kbagent_stage() returns True → Stage 1 skipped → no second _perform_update call → no second _re_exec call. ✓
Plugin sync map: No new CLI commands added or removed. kbagent update and kbagent version already existed and are already registered in permissions.py:178-179. gotchas.md has a new (since v0.30.1) entry at line 3. commands/context.py AGENT_CONTEXT updated. commands-reference.md and keboola-expert.md are not required updates for behavior changes to existing commands. SKILL.md table is auto-generated and CI-checked. ✓
make check green: 2765 passed, 7 skipped ✓
Open questions for the author
(none)
…eanup)
Three cosmetic findings from the third review iteration:
NB-1 (test_exception_never_crashes): patched the legacy
`_should_skip` alias that maybe_auto_update no longer calls; the
'must not raise' contract was therefore unexercised. Switched the
target to `_should_skip_all` so the test actually drives the
blanket try/except path.
NIT-1: removed 10 redundant `@patch(_should_skip, return_value=False)`
decorators across TestMaybeAutoUpdate + TestMaybeAutoUpdateMcpIntegration.
The autouse fixtures already patch `_should_skip_all` and
`_should_skip_kbagent_stage` to False for the entire class, so the
explicit per-test patches were no-ops that just confused readers
('why is this here when the autouse already covers it?'). Same passed
arguments removed from the def signatures; tests now read cleaner.
NIT-2: hardcoded `180.0` in two test assertions replaced with the
`MCP_UPGRADE_TIMEOUT` constant. Pin the test to the same source of
truth the production code uses, so a future bump of
`MCP_UPGRADE_TIMEOUT` does not silently leave the test asserting a
stale value.
90 unit tests still pass, `make check` clean (2,765 total).
|
Iteration 3 cleanup landed in commit
|
…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
Closes #243 silent-staleness trap. A user installed
keboola-mcp-serveronce viauv tool install, then ran kbagent for months while upstream MCP shipped six minor versions; the cached schema was missingconfiguration_row_ids(added in MCP v1.55.0) and the user had no signal anything was behind.This release makes
keboola-mcp-servera first-class citizen of the auto-update flow alongside kbagent itself.Patch on top of the just-released 0.30.0. Rebased on
mainafter #255 / #256 / #259 / #261 landed; only changelog conflict (resolved by appending the new0.30.1entry above the existing0.30.0).What changed
Two-stage auto-update on startup (
auto_update.maybe_auto_update)uv_tool/pip_env/uvx) and runs the matching upgrade command:uv tool upgrade keboola-mcp-serverpip install --upgrade keboola-mcp-serveruvx --refresh --from keboola-mcp-server keboola_mcp_server --versionNo re-exec for the MCP path -- the server is spawned by
tool callcommands and the next spawn picks up the new version.Critical invariant: kbagent up-to-date does NOT short-circuit the MCP stage. Both stages always run.
kbagent update(explicit two-stage)Returns a structured dict with per-stage results plus a one-line summary:
kbagent version-- accurate fieldsdependencies[].version-- the locally installed MCP version (NEW)dependencies[].up_to_date-- comparison against PyPI (NEW)dependencies[].install_method-- one ofuv_tool/pip_env/uvx/none(NEW)dependencies[].auto_updates: true-- now actually true (was lying before; the field claimed auto-update via uvx@latest but the actual code path deliberately omits @latest to avoid the 25s startup penalty)Helpers (in
services/version_service.py)_get_local_mcp_version()--keboola_mcp_server --versionsubprocess +importlib.metadatafallback_detect_mcp_install_method()-- distinguishesuv tool/ pip env / uvx cache / nothing_perform_mcp_update(method)-- runs the matching upgrade subprocessCache (extended)
~/.config/keboola-agent-cli/version_cache.jsongains two keys:mcp_latest_versionandmcp_install_method. Backwards-compatible (older cache without these keys triggers a fresh fetch).Auto-install on startup is intentionally NOT done
If MCP is not installed locally (
install_method == "none"), the auto-update startup hook records the latest version to the cache but does not runuv tool install. That decision belongs tokbagent doctor --fix(the explicit install entry point).Test plan
make checkgreen: 2,755 tests passed, 7 skipped, lint + format + skill + version + changelog all clean (0.30.1 alongside 0.30.0).tests/test_version_service.py(12) andtests/test_auto_update.py(13):self_updateorchestrator (both up-to-date / only-MCP-stale-still-runs / kbagent-stage-failure-still-runs-MCP / blanket exception swallow)TestMaybeAutoUpdatetests updated for the new multi-key_write_cachesignature; an autouse fixture stubs the MCP helpers so kbagent-stage tests never touch the network or subprocess.Plugin synchronization map walk
pyproject.toml0.30.0 -> 0.30.1src/keboola_agent_cli/changelog.py-- 7-bullet 0.30.1 entry above the existing 0.30.0plugins/kbagent/.claude-plugin/plugin.json+.claude-plugin/marketplace.json-- auto-synced viamake version-syncplugins/kbagent/skills/kbagent/SKILL.md-- regenerated (no command surface change)plugins/kbagent/skills/kbagent/references/gotchas.md-- new(since v0.30.1)entry: "kbagent now auto-updates keboola-mcp-server on startup; uv tool install pin is no longer a stale-version trap"src/keboola_agent_cli/services/mcp_service.py-- inline note indetect_mcp_server_commanddocstring updated (no longer says "users can update manually" -- it now happens automatically)No new commands; no
keboola-expert.mdmatrix change required (the behaviour is invisible at the tool-selection layer).Outstanding from previous review round (PR #257 first iteration)
The earlier
/kbagent:reviewflagged 2 blocking findings:_should_skip()returns True after kbagent self-upgrade re-exec becauseKBAGENT_SKIP_UPDATE=1is set, so the MCP stage in the re-exec'd process is skipped. The "always run" invariant therefore holds only when kbagent was already up-to-date at startup. Not addressed in this push -- needs a follow-up to either gate the re-exec guard at the kbagent stage only, or to record "MCP-pending-this-session" in the cache so the re-exec'd process picks up the deferred MCP work._detect_mcp_install_method()distinguishesuv_toolvspip_envby parsinguv tool list-- robustness against output changes / non-English locales / etc.Reviewer's other 2 NB and 2 nits also pending. Re-running
/kbagent:reviewafter addressing them.