fix(kai,docs,component): honor the "project use" pin when --project is omitted - #700
fix(kai,docs,component): honor the "project use" pin when --project is omitted#700soustruh wants to merge 3 commits into
Conversation
Add a keboola-expert.md gotcha for the new CONFIG_ERROR exit. Correct the stale docstring in server/routers/docs.py. Assert client close in the two new component tests. Rename first_alias to query_alias in _list_via_ai.
soustruh
left a comment
There was a problem hiding this comment.
Review of #700 — fix(kai,docs,component): honor the "project use" pin when --project is omitted
Generated by
kbagent-pr-reviewersubagent. The 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.
Status: resolved
Commit 89f4fbd fixes NB-1, NB-2, NIT-1, and NIT-2. NB-3 is deliberately skipped. The command layer has no branching logic of its own, and the service-layer tests cover the full resolution cascade. The findings below describe the PR state at review time (HEAD 4d4368a).
Summary
The PR fixes issue #684. Without --project, the kai commands (ping/preflight/ask/chat/chat-detail/history), docs query, component detail, component list --query, config examples, and config new (scaffold mode) resolved to the first registered project and ignored the project use pin. The fix moves resolve_pinned_alias() verbatim from ProjectService to BaseService, so every service shares one cascade (explicit --project > KBAGENT_PROJECT env > pin > sole project). It routes the four affected services through that cascade, including the matching kbagent serve endpoints. Verdict: COMMENT — no blocking findings. A few silent-drift and coverage gaps deserve a check before merge.
Verdict
- Verdict: COMMENT
- Blocking findings: 0
- Non-blocking findings: 3
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] plugins/kbagent/agents/keboola-expert.md §3 — no inline gotcha for the new CONFIG_ERROR failure mode
The PR adds a (since vNEXT) entry to plugins/kbagent/skills/kbagent/references/gotchas.md. But plugins/kbagent/agents/keboola-expert.md — the subagent system prompt, the "highest silent-drift risk" file per the Plugin synchronization map in CONTRIBUTING.md — does not mention the change. A grep for 684|resolve_pinned_alias|first registered in that file finds only the unrelated project edit --new-alias row. The change introduces a new failure mode. Before the fix, a subagent that called kai ask, docs query, or component detail without --project in a multi-project session got an answer, from the wrong project. After the fix, it gets CONFIG_ERROR exit 5. A one-line addition to the §3 Inline Gotchas would stop a future subagent session from treating that exit code as a bug in itself. Per this reviewer's own severity table (keboola-expert.md §3 gotchas → NON-BLOCKING), this does not block merge.
[NB-2] src/keboola_agent_cli/server/routers/docs.py:31 — stale docstring after the fix
The route docstring for POST /documentation/query still reads "omitted means the first configured project". That is the behavior the PR removes: ask_docs in docs_service.py now resolves through the pin cascade. The PR corrected every other prose description of this default (CLAUDE.md, commands-reference.md, gotchas.md) and missed only this docstring. A grep for first configured|first registered|first available across server/, services/, and commands/ finds no other leftover. Fix: reword to "omitted resolves through the default-project cascade (--project > KBAGENT_PROJECT env > project use pin > sole project)".
[NB-3] Missing CLI-layer/E2E regression coverage for the fixed default-resolution paths
All four new regression tests (tests/test_component_service.py:337, :372, tests/test_docs_cli.py:217, tests/test_kai_service.py:531) exercise the service layer only. The CliRunner suites for the same commands (tests/test_kai_cli.py, tests/test_component_cli.py) were not touched. So no test proves end-to-end that a CLI entry point resolves an omitted --project to a non-first pin. The command modules (commands/kai.py, commands/component.py, commands/docs.py) contain no branching logic of their own (confirmed by reading them), so the masked risk is low. A CliRunner test, or an E2E test per the "every CLI command must have E2E coverage" rule in CONTRIBUTING.md, would catch a future regression if alias resolution is ever duplicated at the command layer. Not a blocker: the PR adds no new command.
Nits
[NIT-1]tests/test_component_service.py:337and:372— the two new tests (test_list_via_ai_no_alias_honors_pin,test_get_component_detail_no_alias_honors_pin) mock the AI client but never assertmock_ai.close.assert_called_once(). The pre-existing sibling testtest_list_components_via_ai_queryin the same file asserts it.[NIT-2]src/keboola_agent_cli/services/component_service.py:920— in the newelsebranch of_list_via_ai, the variablefirst_aliasholds the cascade-resolved alias (pin/env/sole), not the first of anything. The name is a holdover from theif aliases:branch above and can mislead a future maintainer.
Verification log
- Read
CONTRIBUTING.mdsections "Checklist: Adding a New CLI Command" (lines 339-411), "Plugin synchronization map" (449-481), "Commit & PR Conventions" / "Self-review" (483-573) ✓ - Read
CLAUDE.mdconvention #17 and "## All CLI Commands". Thekaisection carries the(since vNEXT, #684)note this PR added ✓ - Read
plugins/kbagent/agents/keboola-expert.md§1 (lines 23-88) and §3 (163-362) ✓ — no mention of issue #684 found (basis for NB-1) gh auth status→ logged in, scopesgist, read:org, repo✓gh pr view 700 --json title,body,files,...→ OPEN, 14 files, +248/-133, titlefix(kai,docs,component): .... The conventional prefix matches the change type ✓git rev-parse --abbrev-ref HEAD→fix/kbagent-default-project-resolution, matches the PR branch ✓. Working tree clean.- Local
mainwas stale (b5d4be3) vsorigin/main(29bf142, one unrelated merged PR in between). Diffed againstgit merge-base HEAD origin/main(b5d4be3), which reproduced thegh pr viewfile list exactly (14 files, +248/-133) ✓ gh pr diff 700→ 689 lines, read in full ✓- Layer-violation greps (typer/formatter in services, httpx in commands, formatter/typer in clients) → all empty, no violations ✓
- Convention greps (magic numbers, raw error-code strings, bare
except:,print(), token leaks) → all empty or false positives only (doc prose, test fixture variable names) ✓ grep -rn "resolve_pinned_alias" src/ tests/→ all call sites (commands/_helpers.py:250,server/routers/components.py:44,57,77,services/base.py:224, plus tests) still resolve through the moved method. TheProjectService(BaseService)inheritance is intact ✓grep -rn "_resolve_alias_or_first"→ no leftover references to the renamed private method ✓- Confirmed the
token/stream/snapshotservices' commands all declare--projectastyper.Option(...)(required). The PR's "audit, no change needed" claim for those three services checks out ✓ - Confirmed the three
next(iter(...))sites inconfig_store.py+commands/init.py:205repoint the pin itself after a project removal, unrelated to the read-path bug.grep -rn "next(iter("found no other unaudited "first project" read path besides the intentionally-kept explicit-list case incomponent_service.py:916✓ uv sync --all-extras→ resolved 81 packages, no changes needed ✓make check→ 6320 passed, 12 skipped, exit 0 (lint, format-check, typecheck, skill-check, version-check, version-gate-check, command-sync-check, endpoints-check, changelog-check, check-error-codes, check-sentinel-guards, loc-check all passed as prerequisites) ✓make command-sync-check→ "OK: all 267 CLI commands are registered (OPERATION_REGISTRY) and documented (CLAUDE.md, context.py, commands-reference.md)" ✓ — no missing permission-registry/doc entries (expected: no commands added/removed)make version-gate-check→ "All 529 version gates ... resolve to a release. 66 unresolved vNEXT gate(s) awaiting the next release PR" — expected and normal for a feature PR using thevNEXTplaceholder ✓make skill-check→ "SKILL.md is up-to-date" ✓- Live behavior reproduction: ran
uv run kbagent --json component detail --component-id keboola.ex-aws-s3(no--project) against the reviewer's own locally-registered config: four projects, withdefault_projectpinned to a project that is NOT the first-registered alias. The response'sproject_aliasreported the pinned project. This confirms the fix resolves an omitted--projectto the pin, not the first project, on a real multi-project config. Did not test the new "no pin + multiple projects → CONFIG_ERROR" path live (that would require mutating the reviewer's own realconfig.json). The unit-test suite covers that path instead. - Did not reproduce the pre-fix bug by reverting to the parent commit (that would require
git checkout/stash, disallowed by the read-only working-tree constraint). Verified the old-vs-new logic by direct code inspection of the diff instead. Old code:next(iter(projects))afterresolve_projects(None)fanned out to all projects inconfig.projectsinsertion order. New code: theresolve_pinned_alias()cascade.
Open questions for the author
(none)
…s omitted The kai group, docs query, component detail, component search, config examples, and config new ignored config.default_project when --project was omitted. They used the first registered project instead. They now resolve the project through the shared resolve_pinned_alias cascade. The cascade moved from ProjectService to BaseService, so every service shares one copy. Fixes #684.
Add a keboola-expert.md gotcha for the new CONFIG_ERROR exit. Correct the stale docstring in server/routers/docs.py. Assert that the two new component tests close the AI client. Rename first_alias to query_alias in _list_via_ai.
A "#" comment line inside a markdown code fence renders as content. It has no anchor slug, so the slug-breakage rationale behind the heading check (#702) does not apply to it. CLAUDE.md's command section is one giant fence full of such lines. Convention #17 requires feature PRs to tag new notes there with vNEXT. The check therefore failed every PR that follows the documented process. This PR was the first to fail on it. The residue scan is untouched: a fenced gate is still a live gate the release PR must resolve.
602e76f to
2332c91
Compare
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: needs_human (risk 3/5) · profile keboola-mcp-server
Correct, well-tested bug fix, but it changes default command behavior with a new hard-fail mode — a human owner should sign off.
Concerns:
src/keboola_agent_cli/services/base.py: New exit-5 failure when multiple projects and no pin; breaks implicit-first-project scriptsscripts/check_version_gates.py: CI-gate logic change bundled into a behavior fix; widens PR scope
soustruh
left a comment
There was a problem hiding this comment.
Review of #700 (round 2) — focused follow-up
Generated by the
kbagent-pr-reviewersubagent as a focused follow-up pass. The first review (posted above) covered the core #684 fix, so this pass does not repeat it. This pass covers the three commits added after that review: the fence-awarecheck_version_gates.pychange, the rebase's effect oncommands-reference.md, and the scope of the review-fixes commit.
Summary
The PR gained three commits since the first review. 4b090d9 replays the original fix on a rebased main. No separate merge commit exists: the conflict resolution is part of that commit's diff. 1eb961c addresses the first review's findings exactly as described. 2332c91 fixes a real false positive in the vNEXT-in-heading CI check. This PR itself was the first to fail on it. All three areas are sound. The fence state machine is correct, and its one divergence from CommonMark errs toward over-flagging (the safe direction). commands-reference.md lost and duplicated nothing across the rebase. The review-fixes commit matches its message with no extra scope. Verdict: APPROVE (no blocking or non-blocking findings, two NITs).
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 0
- Nits: 2
Blocking findings
(none)
Non-blocking findings
(none)
Nits
[NIT-1]scripts/check_version_gates.py:208-217—FENCE_REaccepts a closing fence with trailing text after the marker. Strict CommonMark requires only spaces or tabs after a closing fence. A "closing" line that carries trailing text therefore closes the block one line early, and the check then flags the still-fenced# ... (since vNEXT)line below it as a heading. Reproduced live on a synthetic fixture. The direction is safe: the result is a loud CI failure, never a silently missed gate, which matches the module's own stated risk asymmetry for fenced blocks. Optional polish, not a defect. Fix, if desired: a closing delimiter must carry only whitespace after the fence characters. An opening fence keeps allowing an info string.[NIT-2]plugins/kbagent/skills/kbagent/references/commands-reference.md:120vs:134— the two parallel "Since 0.90.0" sentences are worded inconsistently. Thecomponent detailbullet now says "fall back to a default project". Theconfig newbullet keeps "fall back to the first configured project". Both are factually correct, and the appended vNEXT sentence corrects the record in each. Cosmetic only.
Verification log
git rev-parse --abbrev-ref HEAD/git rev-parse HEAD→fix/kbagent-default-project-resolution@2332c911def8..., clean working tree ✓ (matches the PR head)gh pr view 700 --json title,state,baseRefName,headRefName,commits→ OPEN, basemain, 3 commits ✓git log --oneline HEAD ^origin/main→ exactly 3 commits ahead ofmain:4b090d9,1eb961c,2332c91✓git merge-base HEAD origin/main→b09ef79(main's HEAD right after #706 merged). This confirms a clean linear rebase: the conflict resolution lives inside4b090d9's diff, not in a merge commit ✓git show 2332c91→ read the fence-aware diff and its 3 new tests in full ✓uv run pytest tests/test_check_version_gates.py -v→ 68 passed, including the 3 new fence tests ✓uv run python scripts/check_version_gates.py --liston the live tree →in headings: 0 marker(s),vNEXT: 13 marker(s)(residue, expected before a release). The fencedkaigate inCLAUDE.mdis excluded from the heading check and still counted as residue ✓- Regression check: ran
find_heading_placeholdersfrom the pre-fix script (git show 1eb961c:scripts/check_version_gates.py, saved to a scratch file, working tree untouched) against the live tree → 2 false positives atCLAUDE.md:964andCLAUDE.md:967. Those are exactly the fenced lines this PR added.2332c91therefore fixes a real, self-inflicted CI break, not a hypothetical one ✓ - Adversarial probe (NIT-1): a synthetic fixture with a "closing" fence line that carries trailing text → 1 false-positive heading flag. This confirms the CommonMark divergence and its safe direction ✓
git diff b09ef79..HEAD -- plugins/kbagent/skills/kbagent/references/commands-reference.md→ read in full. Every hunk adds**Since vNEXT (#684)**prose on top of #706's already-applied tag removal. Nothing is reverted or duplicated ✓- Conflict-marker grep over
commands-reference.md→ empty ✓ - Duplicate-consecutive-line scan over
commands-reference.md→ empty ✓ grep -c vNEXT commands-reference.md→ 6, matching the 6 hunks in the diff ✓git show 1eb961c(full diff) → touches exactly 4 files:keboola-expert.md(new CONFIG_ERROR gotcha),server/routers/docs.py(docstring correction),services/component_service.py(first_alias→query_alias, 5 occurrences),tests/test_component_service.py(2 new close asserts). This matches the commit message exactly, nothing extra ✓grep -rn first_alias src/ tests/ plugins/→ empty. The rename left no orphaned references ✓uv run pytest tests/test_component_service.py tests/test_docs_cli.py tests/test_kai_service.py -q→ 109 passed (one pre-existingRuntimeWarningintest_kai_service.py, unrelated to this PR) ✓uv run ruff check+uv run ruff format --checkon all files touched by the 3 new commits → all pass ✓- Did NOT run the full
make check/ E2E suite — out of scope for this focused follow-up (the first review covered the core #684 fix). Instead this pass ran every directly affected test file and the fullcheck_version_gates.pysuite.
Open questions for the author
(none)
Summary
Fixes #684.
kbagent project use <alias>writesconfig.default_project. Four read paths did not read that pin. They used the first registered project instead. The command then acted on the wrong project. The user got no warning.The fixed paths:
KaiService.resolve_alias—kai ping/preflight/ask/chat/chat-detail/historyDocsService.ask_docs—docs queryComponentService._resolve_alias_or_first—component detail,config examples,config new(scaffold mode)ComponentService._list_via_ai—component list --queryThese paths now resolve the project through the shared cascade: explicit
--project>KBAGENT_PROJECTenv >project usepin > sole registered project. The cascade (resolve_pinned_alias) moved verbatim fromProjectServicetoBaseService, so every service shares one copy. The fix also coverskbagent serve: the/kai/*routes,POST /documentation/query, andGET /components?query=...called the same defective service methods.Behavior change: with several projects and no pin, these commands now fail with
CONFIG_ERROR(exit 5). The message tells the user to pass--project, setKBAGENT_PROJECT, or runproject use. Before, they silently used the first registered project.Audit (issue scope item 3)
snapshot_service,token_service, andstream_serviceeach have a_resolve_project(alias)with a required alias. Their commands declare--projectas required (typer.Option(...)), so no implicit path exists there. They need no change. Thenext(iter(...))calls inconfig_store.pyandcommands/init.pyrepoint the pin itself. They stay unchanged, per the issue.CI gate fix (added after the review)
The vNEXT heading check from #702 failed this PR: it treated the
#comment lines inside CLAUDE.md's fenced## All CLI Commandsblock as markdown headings. A fenced line renders as content and has no anchor slug, so the slug-breakage rationale does not apply to it. Convention #17 requires feature PRs to tag new notes in that block withvNEXT. The check therefore failed every PR that follows the documented process.find_heading_placeholdersnow skips lines inside code fences (backtick and tilde, CommonMark closing rules). The residue scan is untouched: a fenced gate is still a live gate the release PR must resolve. Three new tests pin the behavior.Changes
services/base.py—resolve_pinned_alias()moved here fromProjectService(ProjectService inherits it)services/kai_service.py—resolve_alias()delegates to the cascadeservices/docs_service.py—ask_docs()resolves via the cascadeservices/component_service.py—_resolve_alias_or_firstrenamed to_resolve_alias_or_default, resolves via the cascade._list_via_aiuses it when no alias is given.commands/component.py,commands/config.py,commands/docs.py— corrected the "uses first available" help textsscripts/check_version_gates.py— the heading check skips fenced lines (see above)(since vNEXT)--query) — each registers two projects, pins the second, calls with no--project, and asserts that the command uses the pinned project; 3 tests for the fence-aware heading checkkbagent-pr-reviewerpass (comment below). Its NB-1/NB-2/NIT-1/NIT-2 findings are fixed in the review-fixes commit. NB-3 (CLI-layer test coverage) is deliberately skipped: the command layer has no branching logic of its own, and the service-layer tests cover the full cascade.Verification
main(post-docs(plugin): retire version gates below the 0.80.0 floor #706 tag retirement); the one conflict incommands-reference.mdresolved as main's untagged lines plus this PR'svNEXT (#684)notesmake checkon the rebased branch: 6366 passed, 12 skippedsrc/)component detailwith no--projecton a real 4-project config resolved to the pinned project, not the first registered oneNo version bump, no changelog entry (feature PR per the release process).