feat: close the remaining keboola-mcp-server parity gaps (0.88.0) - #632
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Review of #632 — feat: close the remaining keboola-mcp-server parity gaps (0.88.0)
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 closes five flag-level parity gaps against keboola-mcp-server (v1.76.2): storage tables --include-usage, job detail --log-tail-lines, job list --offset/--sort-by/--sort-order, search --scope, and sharing link --stage. All five are new options on existing commands (no new command group), so no new keboola-expert.md matrix row is required, and none were. Documentation (CLAUDE.md, commands/context.py, commands-reference.md, gotchas.md with correct (since v0.88.0) tags, docs/mcp-migration.md) and tests (service-layer + CLI-layer, 47 new tests) are thorough and well-targeted; make check is fully green (5742 passed). Verdict: REQUEST CHANGES — the one blocking finding is that none of these five new flags were wired into the kbagent serve REST routers, so the REST-facing consumers this PR's own architecture doc says the server exists for (Web UI, scheduled agents, third-party apps) get none of the parity this PR claims to deliver.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 0
Blocking findings
[B-1] src/keboola_agent_cli/server/routers/jobs.py:41-56,60-64, search.py:15-23, sharing.py:22-25,69-78, storage.py:193-200 — none of the 5 new flags are exposed over kbagent serve
Per CONTRIBUTING.md > "Checklist: Adding a New CLI Command" and the Plugin synchronization map, server/routers/<group>.py mirrors the CLI 1:1 so external consumers (Web UI, scheduled agents, third-party apps per CLAUDE.md's own description of kbagent serve) never need to fork a CLI subprocess. This PR adds five new CLI options but touches zero files under server/routers/:
job.list_jobs()router (jobs.py:41) has nooffset/sort_by/sort_orderparams —registry.job.list_jobs()is called without them (jobs.py:50-56), so REST callers are pinned tooffset=0, sort_by=startTime, sort_order=descforever.job.detail()router (jobs.py:60) has nolog_tail_linesparam —registry.job.get_job_detail()is called without it (jobs.py:64); a REST caller can never read the log tail of an already-finished job, which is the entire point of this flag.search()router (search.py:15) has noscopeparam and never forwardsscopes=toregistry.search.search()(search.py:25-30).sharing.link()'sLinkBucketbody model (sharing.py:22-25) has nostagefield, andlink()(sharing.py:69-78) never passesstage=— a REST caller can never link a shared bucket intoout.storage.list_tables()router (storage.py:193) has noinclude_usageparam and never forwards it (storage.py:200).
The literal checklist wording says "add the corresponding route" when adding a new command — these are flags on existing commands, so this is arguably outside the letter of the rule, but not its spirit: the PR's own framing is "MCP tool parity," and MCP's tool-call semantics map far more naturally onto the REST surface (kbagent serve, used by scheduled AI agents) than onto a human typing CLI flags. Right now the parity this PR claims exists only for interactive/CLI callers. If this is intentional (e.g., REST parity deferred to a follow-up), say so in the PR description per the checklist's "skip must be documented" convention; otherwise wire the five params through.
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/storage_service.py:2726-2748 — _fetch_tables's tuple return type widened from bool to Any, conflating two meanings in one slot
This is a modification of a pre-existing grandfathered tuple[str, list[...], bool] | tuple[str, dict[...]] signature (not a brand-new tuple, so it isn't caught by the "no new tuple returns" rule), but the change itself is a small type-safety regression worth naming: the third element used to be a fixed-meaning success sentinel (True); now it is True when include_usage=False and list[dict[str, Any]] (the component listing) when include_usage=True (storage_service.py:2743-2746). The consumer at storage_service.py:1290 has to re-derive which case it's in via include_usage and len(result) > 2. A small @dataclass FetchTablesResult(alias: str, tables: list[dict], components: list[dict] | None = None) would let the consumer branch on components is not None instead of tuple-length sniffing, and would make ty catch a future misuse instead of silently accepting Any.
[NB-2] No E2E coverage added for any of the 5 new flags
tests/test_e2e.py is unchanged in this PR (verified via git diff main...HEAD --stat -- tests/test_e2e.py, empty). CONTRIBUTING.md > "Tests (mandatory!)" says every CLI command must have E2E coverage; strictly these are flags on already-covered commands, but the same silent-drift risk applies at the flag level (an E2E regression in the Queue API's sortBy/sortOrder params, the Storage API stage field, or the component-listing shape include_usage depends on would only be caught by make check's mocked unit suite, not against a live stack). Given the unit/CLI coverage is otherwise excellent (47 new tests, both JSON and human-mode paths for storage tables --include-usage and sharing link --stage), this is a "next PR" item rather than a merge blocker.
Nits
(none)
Verification log
git rev-parse --abbrev-ref HEAD→claude/mcp-cli-tools-analysis-847e3a, matches<branch>;git statusclean ✓gh auth status→ authenticated aspadak✓gh pr view 632 --json ...→ OPEN, +1594/-127, 32 files, conventionalfeat:prefix matches (new CLI behavior) ✓- Read
CONTRIBUTING.md(Checklist, Plugin sync map, Releasing),CLAUDE.mdconv. #17 + All CLI Commands,plugins/kbagent/agents/keboola-expert.md§1/§2/§3 ✓ gh pr diff 632→ 2514 lines, reviewed in full ✓- Layer-violation greps (typer/click in services, httpx in commands, formatter/typer in clients) → all empty, no violations ✓
git diff main...HEAD --stat -- 'src/keboola_agent_cli/server/routers/*.py'→ empty (0 router files touched) → basis for [B-1] ✓- Manually inspected
jobs.py,search.py,sharing.py,storage.pyrouters → confirmed none of the 5 new params are threaded through ✓ make lint→All checks passed!(exit 0) ✓ruff format . --check→440 files already formatted(exit 0) ✓ty check→ 1 diagnostic,warning[unresolved-import]onscripts/hatch_build.py(pre-existing, downgraded/non-blocking rule per CONTRIBUTING) (exit 0) ✓make command-sync-check→OK: all 261 CLI commands are registered (OPERATION_REGISTRY) and documented(exit 0) ✓make check-sentinel-guards→OK: ... all 10 guards covered(exit 0) ✓make check-error-codes→OK: no raw error_code literals; docs/error-codes.md matches the enum(exit 0) — the one rawerror_code="FORBIDDEN"string found by my own grep is inside a test-file mock (tests/test_table_usage.py), outside the script's scan scope, not a real violation ✓make loc-check→OK: 215 modules within budget; only pre-existing soft-ceiling WARNs unrelated to this PR's files (config_service.py,semantic_layer_service.py,sync.py,deep_lineage_service.py,project.py,agent_runner.py,semantic_layer.py) —commands/storage.py/services/storage_service.pydo NOT appear, consistent with the PR's claim that the extraction shrank both files ✓make skill-check→SKILL.md is up-to-date(exit 0) ✓make version-check→version is in sync (plugin.json, marketplace.json, uv.lock)(exit 0) ✓make test(via fullmake checkrun) →5742 passed, 12 skipped, 161 deselectedin 133s (exit 0) ✓- Grep for new tuple returns → only one:
_fetch_tables's modified (not new) annotation → basis for [NB-1] ✓ - Grep for magic numbers, raw error-code strings in
src/, bareexcept:,print(), token leakage → all clean ✓ - Confirmed
_safe_fetch_log_tailreused (not reimplemented) from existingjob_runcode path — good DRY, not a finding ✓ - Confirmed
client.link_bucket(stage=...)already existed onmainpre-PR (not touched in this diff) — service/command layers were the only gap, consistent with PR scope ✓ - Spot-checked
_filter_locations_by_scopeswhole-segment matching logic (storage.invsstorage.input) against the gotchas.md claim — code confirms the claim is accurate ✓ - Did not attempt a live
kbagentrun against a real project (repo convention: AI agents do not handle Keboola API tokens); relied on the mocked unit/CLI suite instead, which is thorough for all 5 flags (both--jsonand human-mode paths tested forstorage tables --include-usageandsharing link --stage)
Open questions for the author
(none)
[B-1] Wire all five 0.88.0 flags through the `kbagent serve` routers. Each
router docstring claims it "Mirrors" its CLI command, and `search` already
exposed `--regex`, so the 1:1 contract demonstrably covers flags and not just
commands -- leaving these out made those docstrings false and left scheduled
agent tasks (which reach kbagent over REST, not argv) without the parity this
release is about:
- `GET /jobs` gains `offset`, `sort_by`, `sort_order`
- `GET /jobs/{project}/{job_id}` gains `log_tail_lines`
- `GET /search` gains repeatable `scope`
- `GET /storage/tables` gains `include_usage`
- `POST /sharing/{project}/link` body gains `stage`
[NB-1] `_fetch_tables` no longer conflates two meanings in its third tuple
slot. It was `True` without a usage scan and the component listing with one,
forcing the consumer to sniff `len(result) > 2` and widening the annotation to
`Any`. `_run_parallel` tells success from error by tuple LENGTH (base.py:253),
never by that element, so the slot now always carries the component listing
(empty when no scan was asked for) and the consumer destructures directly.
[NB-2] Added E2E coverage for all five flags in `TestE2EMcpParityCommands`:
the live-stack contracts the mocked suite cannot check (Queue API accepting
`sortBy`/`sortOrder`, `logTail` on a real finished job, `used_by` shape from a
real component listing) plus the three exit-2 validation paths. NOT RUN here
-- `make test-e2e` needs E2E_API_TOKEN against a real project.
make check green: 5749 passed.
Review addressed — all three findingsThanks, the blocking one was right and worth catching. [B-1] REST parity — fixedI checked the premise before acting on it, and two things settled it: every The framing argument holds too: this release is about MCP-tool parity, and MCP Wired through:
7 new tests in [NB-1] Tuple meaning conflation — fixedFair hit. The key fact is that [NB-2] E2E coverage — added, but not run8 cases in
|
[B-1] Wire all five new flags through the `kbagent serve` routers. Each
router docstring claims it "Mirrors" its CLI command, and `search` already
exposed `--regex`, so the 1:1 contract demonstrably covers flags and not just
commands -- leaving these out made those docstrings false and left scheduled
agent tasks (which reach kbagent over REST, not argv) without the parity this
work is about:
- `GET /jobs` gains `offset`, `sort_by`, `sort_order`
- `GET /jobs/{project}/{job_id}` gains `log_tail_lines`
- `GET /search` gains repeatable `scope`
- `GET /storage/tables` gains `include_usage`
- `POST /sharing/{project}/link` body gains `stage`
[NB-1] `_fetch_tables` no longer conflates two meanings in its third tuple
slot. It was `True` without a usage scan and the component listing with one,
forcing the consumer to sniff `len(result) > 2` and widening the annotation to
`Any`. `_run_parallel` tells success from error by tuple LENGTH (base.py:253),
never by that element, so the slot now always carries the component listing
(empty when no scan was asked for) and the consumer destructures directly.
[NB-2] Added E2E coverage for all five flags in `TestE2EMcpParityCommands`:
the live-stack contracts the mocked suite cannot check (Queue API accepting
`sortBy`/`sortOrder`, `logTail` on a real finished job, `used_by` shape from a
real component listing) plus the three exit-2 validation paths. NOT RUN here
-- `make test-e2e` needs E2E_API_TOKEN against a real project.
Rebased onto #629. This branch carries no version bump: main is already at an
unreleased 0.88.0, so the changelog bullets join that entry and the version is
bumped once for the whole batch at release time. The byte-budget trims to
`keboola-expert.md` are reverted -- #629 raised that ceiling to 70 000 B, so
the prose they paid for is affordable again.
make check green against the rebased base: 5761 passed.
045d621 to
7db5dfd
Compare
[B-1] Wire all five new flags through the `kbagent serve` routers. Each
router docstring claims it "Mirrors" its CLI command, and `search` already
exposed `--regex`, so the 1:1 contract demonstrably covers flags and not just
commands -- leaving these out made those docstrings false and left scheduled
agent tasks (which reach kbagent over REST, not argv) without the parity this
work is about:
- `GET /jobs` gains `offset`, `sort_by`, `sort_order`
- `GET /jobs/{project}/{job_id}` gains `log_tail_lines`
- `GET /search` gains repeatable `scope`
- `GET /storage/tables` gains `include_usage`
- `POST /sharing/{project}/link` body gains `stage`
[NB-1] `_fetch_tables` no longer conflates two meanings in its third tuple
slot. It was `True` without a usage scan and the component listing with one,
forcing the consumer to sniff `len(result) > 2` and widening the annotation to
`Any`. `_run_parallel` tells success from error by tuple LENGTH (base.py:253),
never by that element, so the slot now always carries the component listing
(empty when no scan was asked for) and the consumer destructures directly.
[NB-2] Added E2E coverage for all five flags in `TestE2EMcpParityCommands`:
the live-stack contracts the mocked suite cannot check (Queue API accepting
`sortBy`/`sortOrder`, `logTail` on a real finished job, `used_by` shape from a
real component listing) plus the three exit-2 validation paths. NOT RUN here
-- `make test-e2e` needs E2E_API_TOKEN against a real project.
Rebased onto #629. This branch carries no version bump: main is already at an
unreleased 0.88.0, so the changelog bullets join that entry and the version is
bumped once for the whole batch at release time. The byte-budget trims to
`keboola-expert.md` are reverted -- #629 raised that ceiling to 70 000 B, so
the prose they paid for is affordable again.
make check green against the rebased base: 5761 passed.
7db5dfd to
c4f1842
Compare
[B-1] Wire all five new flags through the `kbagent serve` routers. Each
router docstring claims it "Mirrors" its CLI command, and `search` already
exposed `--regex`, so the 1:1 contract demonstrably covers flags and not just
commands -- leaving these out made those docstrings false and left scheduled
agent tasks (which reach kbagent over REST, not argv) without the parity this
work is about:
- `GET /jobs` gains `offset`, `sort_by`, `sort_order`
- `GET /jobs/{project}/{job_id}` gains `log_tail_lines`
- `GET /search` gains repeatable `scope`
- `GET /storage/tables` gains `include_usage`
- `POST /sharing/{project}/link` body gains `stage`
[NB-1] `_fetch_tables` no longer conflates two meanings in its third tuple
slot. It was `True` without a usage scan and the component listing with one,
forcing the consumer to sniff `len(result) > 2` and widening the annotation to
`Any`. `_run_parallel` tells success from error by tuple LENGTH (base.py:253),
never by that element, so the slot now always carries the component listing
(empty when no scan was asked for) and the consumer destructures directly.
[NB-2] Added E2E coverage for all five flags in `TestE2EMcpParityCommands`:
the live-stack contracts the mocked suite cannot check (Queue API accepting
`sortBy`/`sortOrder`, `logTail` on a real finished job, `used_by` shape from a
real component listing) plus the three exit-2 validation paths. NOT RUN here
-- `make test-e2e` needs E2E_API_TOKEN against a real project.
Rebased onto #629. This branch carries no version bump: main is already at an
unreleased 0.88.0, so the changelog bullets join that entry and the version is
bumped once for the whole batch at release time. The byte-budget trims to
`keboola-expert.md` are reverted -- #629 raised that ceiling to 70 000 B, so
the prose they paid for is affordable again.
make check green against the rebased base: 5761 passed.
c4f1842 to
fab5c52
Compare
[B-1] Wire all five new flags through the `kbagent serve` routers. Each
router docstring claims it "Mirrors" its CLI command, and `search` already
exposed `--regex`, so the 1:1 contract demonstrably covers flags and not just
commands -- leaving these out made those docstrings false and left scheduled
agent tasks (which reach kbagent over REST, not argv) without the parity this
work is about:
- `GET /jobs` gains `offset`, `sort_by`, `sort_order`
- `GET /jobs/{project}/{job_id}` gains `log_tail_lines`
- `GET /search` gains repeatable `scope`
- `GET /storage/tables` gains `include_usage`
- `POST /sharing/{project}/link` body gains `stage`
[NB-1] `_fetch_tables` no longer conflates two meanings in its third tuple
slot. It was `True` without a usage scan and the component listing with one,
forcing the consumer to sniff `len(result) > 2` and widening the annotation to
`Any`. `_run_parallel` tells success from error by tuple LENGTH (base.py:253),
never by that element, so the slot now always carries the component listing
(empty when no scan was asked for) and the consumer destructures directly.
[NB-2] Added E2E coverage for all five flags in `TestE2EMcpParityCommands`:
the live-stack contracts the mocked suite cannot check (Queue API accepting
`sortBy`/`sortOrder`, `logTail` on a real finished job, `used_by` shape from a
real component listing) plus the three exit-2 validation paths. NOT RUN here
-- `make test-e2e` needs E2E_API_TOKEN against a real project.
Rebased onto #629. This branch carries no version bump: main is already at an
unreleased 0.88.0, so the changelog bullets join that entry and the version is
bumped once for the whole batch at release time. The byte-budget trims to
`keboola-expert.md` are reverted -- #629 raised that ceiling to 70 000 B, so
the prose they paid for is affordable again.
make check green against the rebased base: 5761 passed.
fab5c52 to
11d6b4a
Compare
Five read-side flags, each covering a capability the MCP server had and kbagent did not. Verified against keboola/mcp-server v1.76.2: 41 registered tools, all now mapped in docs/mcp-migration.md. - `storage tables --include-usage` -- MCP `get_tables(include_usage=True)`. Only storage input/output mappings count as a reference; a table id inside a transformation's SQL is text that happens to match, and counting it would make "what breaks if I drop this table?" answer with false positives. One extra component listing per project, not per table. - `job detail --log-tail-lines N` -- MCP `get_jobs(include_logs=True)`. The log tail previously existed only on the `job run --wait` path, so reading the logs of an already-finished job had no CLI route. - `job list --offset/--sort-by/--sort-order` -- MCP `get_jobs` paging. - `search --scope PATH` -- MCP `search(scopes=...)`. Scopes are written relative to the configuration body; the `configuration.` / `rows[N].configuration.` wrapper is normalised away. - `sharing link --stage in|out` -- MCP `link_shared_bucket(target_stage=...)`. The default stays `in` rather than deriving from the source bucket the way MCP does: silently relocating where existing scripts' buckets land is the worse surprise. docs/mcp-migration.md gains the two Data Catalog tools it was missing (`get_shared_buckets`, `link_shared_bucket`) and records the mcp-server version the map was verified against. `commands/storage.py` and `services/storage_service.py` are both over their file-size budget, so the code that would have grown them moved out instead: `commands/_storage_tables_render.py`, `services/_storage_tables.py` and `services/table_usage.py`. Both files end up smaller than before.
[B-1] Wire all five new flags through the `kbagent serve` routers. Each
router docstring claims it "Mirrors" its CLI command, and `search` already
exposed `--regex`, so the 1:1 contract demonstrably covers flags and not just
commands -- leaving these out made those docstrings false and left scheduled
agent tasks (which reach kbagent over REST, not argv) without the parity this
work is about:
- `GET /jobs` gains `offset`, `sort_by`, `sort_order`
- `GET /jobs/{project}/{job_id}` gains `log_tail_lines`
- `GET /search` gains repeatable `scope`
- `GET /storage/tables` gains `include_usage`
- `POST /sharing/{project}/link` body gains `stage`
[NB-1] `_fetch_tables` no longer conflates two meanings in its third tuple
slot. It was `True` without a usage scan and the component listing with one,
forcing the consumer to sniff `len(result) > 2` and widening the annotation to
`Any`. `_run_parallel` tells success from error by tuple LENGTH (base.py:253),
never by that element, so the slot now always carries the component listing
(empty when no scan was asked for) and the consumer destructures directly.
[NB-2] Added E2E coverage for all five flags in `TestE2EMcpParityCommands`:
the live-stack contracts the mocked suite cannot check (Queue API accepting
`sortBy`/`sortOrder`, `logTail` on a real finished job, `used_by` shape from a
real component listing) plus the three exit-2 validation paths. NOT RUN here
-- `make test-e2e` needs E2E_API_TOKEN against a real project.
Rebased onto #629. This branch carries no version bump: main is already at an
unreleased 0.88.0, so the changelog bullets join that entry and the version is
bumped once for the whole batch at release time. The byte-budget trims to
`keboola-expert.md` are reverted -- #629 raised that ceiling to 70 000 B, so
the prose they paid for is affordable again.
make check green against the rebased base: 5761 passed.
11d6b4a to
69b1d5a
Compare
1 participant
Why
We removed the local MCP passthrough in 0.85.0 on the premise that every
keboola-mcp-servertool has a native command. This PR audits that premiseagainst the live server and closes what was actually missing.
Verified against
keboola/mcp-server@e2417a7(v1.76.2, 2026-08-21) byenumerating
add_tool()registrations in the source (41 tools, matching thegenerated
TOOLS.md) and diffing againstdocs/mcp-migration.md.Audit result: 36 of 41 tools fully covered, 3 deliberately unported and
documented (
query_data,validate_semantic_query, legacycreate_flow),2 covered but missing from the map. Beyond name coverage, five capability
gaps showed up at the flag level — this PR closes all five.
What
storage tables --include-usageget_tables(include_usage=True)job detail --log-tail-lines Nget_jobs(include_logs=True)job list --offset/--sort-by/--sort-orderget_jobspaging--limit, oldest-firstsearch --scope PATHsearch(scopes=...)storage.inputetc.sharing link --stage in|outlink_shared_bucket(target_stage=...)outPlus
docs/mcp-migration.mdgains the two Data Catalog tools it was missing(
get_shared_buckets→sharing list,link_shared_bucket→sharing link)and now records the mcp-server version the map was verified against.
Decisions worth reviewing
--include-usagecounts mappings, not SQL text. Onlystorage.input.tables[].source/storage.output.tables[].destinationcount.A transformation whose SQL says
SELECT * FROM in.c-main.ordersbut whoseinput mapping omits the table is not reported. The question the column answers
is "what breaks if this table changes?", and matching free text answers it with
false positives. For a genuine "is this string anywhere?" audit,
search --search-type config-basedis still the tool.sharing link --stagedefaults toin, deliberately diverging from MCP.MCP derives the stage from the source bucket, so an
out.*source lands inout. kbagent has always usedin; changing that default would silentlyrelocate where existing scripts' buckets land. The flag makes
outreachablewithout moving anyone's cheese. Called out in the migration doc and gotchas.
Cost of
--include-usageis one component listing per project, not pertable. That listing carries every configuration body, so it is the expensive
call in a big project — hence off by default, with the cost stated in
--help.A project whose components are unreadable (scoped token, transient 5xx)
degrades to an empty
used_byrather than failing the table listing.File-size budgets
commands/storage.pyandservices/storage_service.pyare both already overtheir grandfathered budgets, and
make loc-checkrefuses growth. Rather thanre-baselining (which the Makefile explicitly warns against), the code moved
out into
commands/_storage_tables_render.py,services/_storage_tables.pyand
services/table_usage.py. Both files end up smaller than before thisPR.
Tests
TDD throughout — every behaviour had a failing test first. 47 new tests across
test_table_usage.py,test_job_options.py,test_sharing_cli.pyandadditions to
test_search_service.py/test_search_cli.py/test_sharing_service.py.Three existing tests needed updating for genuinely changed contracts (the
Queue API request URL now carries
sortBy/sortOrder, andlink_bucketnowtakes
stage) — those are contract changes, not test massaging.make checkgreen: lint, format, typecheck, skill, version, command-sync,changelog, error-codes, sentinel-guards, loc-check, 5742 tests.
Not in scope
The python-js data-app draft workflow (
modify_python_js_data_appwithparent_configuration_id,delete_python_js_data_app_draft,deploy_data_app(mode='dev')) and Streamlit-from-inline-source remainuncovered. Those are a genuine feature scope, not a flag, and were explicitly
deferred.