Skip to content

feat(plugin): add /kbagent:setup one-command first-run setup - #625

Merged
jordanrburger merged 8 commits into
mainfrom
feat/kbagent-setup-slash-command
Aug 25, 2026
Merged

feat(plugin): add /kbagent:setup one-command first-run setup#625
jordanrburger merged 8 commits into
mainfrom
feat/kbagent-setup-slash-command

Conversation

@claude

@claude claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Requested by David Esner · Slack thread

Head is now a merge of main into this branch, which resolved the #627 marketplace-rename conflict (keboola/cli / kbagent@keboola-agent-clikeboola/ai-kit / kbagent@keboola-claude-kit).

Before / After

Today, a new user follows five separate steps across two different places, in the right order, from the docs at keboola.com/cli: generate a Storage API token in the UI → open Claude Code → run the curl … install.sh | sh one-liner in a terminal → kbagent project add --project X --url Y --token …kbagent doctor → back in Claude Code, /plugin marketplace add keboola/ai-kit and /plugin install kbagent@keboola-claude-kit. Nothing in the repo chains those; kbagent doctor detects the missing plugin but deliberately does not fix it.

After this PR, the documented flow is: install the plugin, then run one command.

/plugin marketplace add keboola/ai-kit
/plugin install kbagent@keboola-claude-kit
/kbagent:setup

/kbagent:setup does the rest — and there is no token to generate or paste on the default path.

What it does

Adds one new plugin slash command, /kbagent:setup, that runs first-time setup end to end — install the CLI if missing, connect a project, verify — with every step conditional on a check, so it is idempotent and safe to re-run on a half-finished setup.

How

plugins/kbagent/commands/setup.md is markdown instructions for the agent, in the same shape as the existing keboola.md / review.md (same frontmatter keys, ## Behavior numbered steps, ## Examples, a closing rationale section). It orchestrates existing, already-tested verbs — no new CLI surface, no new code path:

  1. kbagent --json version → if the CLI is absent, run this repo's own install.sh, handling the installer's documented PATH caveat (kbagent is on PATH for the installer's own process only; source $HOME/.local/bin/env or a new shell). If kbagent.install_channel is present, that is a standalone/packaged build — the command respects it and points at upgrade_command / upgrade_hint instead of installing a second kbagent over the top.
  2. kbagent --json project list → if a project is already connected, name the aliases and skip to verification. Never re-registers or overwrites an existing project or alias.
  3. Otherwise kbagent --json auth login --register-projects — browser PKCE with the automatic device-code fallback, so nothing to paste. It reads session_unsupported_features off the result (canonical list: SESSION_UNSUPPORTED_FEATURES in services/_auth_registration.py) and relays it rather than hand-listing it, and falls back to kbagent --json project add --project '' --url when the user needs one of those surfaces or there is no browser (headless, container, CI). The fallback never passes --token on a command line: either KBC_TOKEN is already in the environment, or the command is handed to the user to run in their own terminal, because project add's hidden token prompt needs a real TTY that a tool-run shell does not have.
  4. kbagent --json doctor, interpreted for the user — including the claude_plugin check (pass / version drift → /plugin update kbagent; warn → print the two /plugin lines; skip → Claude Code not detected).
  5. A short "you're set up, here's what to try" close.

The body encodes the rules explicitly: never print or persist a token, --json for anything parsed, don't re-run a step that already passes, and when a step genuinely needs a human (browser login), say so plainly instead of guessing.

Flag placement was verified against the live CLI — --json is a root-callback option, so it goes before the subcommand (kbagent --json project list, not project list --json). All four invocations in the file were run locally.

Doc sync (CLAUDE.md convention #17 / CONTRIBUTING.md "Plugin synchronization map")

None of these have a CI freshness check, so they are the silent-failure surfaces for this change:

  • plugins/kbagent/.claude-plugin/CLAUDE.md — surfaces list (two slash commands → three) and the "For Claude Code users" block now leads with /kbagent:setup.
  • plugins/kbagent/skills/kbagent/SKILL.md — the prose "First-time setup" section points at /kbagent:setup as the one-command path, keeps the manual sequence below it for a plain shell or another agent, and swaps its stale uv tool install git+… line for the install.sh one-liner plus the PATH caveat. The CI-checked auto-generated decision table was not touched (make skill-check regenerates it clean).
  • README.md — the Claude Code plugin block is now the two /plugin lines plus /kbagent:setup.
  • docs/TUTORIAL.md — §5 install flow and the "What the plugin ships" component table.
  • install.sh — the printed "Next steps" now leads with /kbagent:setup for Claude Code users and keeps every existing explicit command under "Or do it by hand", so nothing regresses for people not in Claude Code.
  • src/keboola_agent_cli/commands/context.py (AGENT_CONTEXT) — the "Claude Code Plugin" section, since this is the primary reference agents load at session start.
  • CLAUDE.md — the plugin structure tree and surfaces sentence were already stale (still claimed one slash command and one subagent); corrected while adding setup.md.
  • CONTRIBUTING.md — the sync-map row for plugins/kbagent/commands/keboola.md was stale for the same reason. Widened to commands/*.md and given the explicit follow-through list for adding a new slash-command file, which the map has never had (/kbagent:review was added without it).

No changelog entry, and no version bump: the diff touches no version file (pyproject.toml is untouched) and adds no changelog entry, and CONTRIBUTING defines a release as a version bump. No new tests: per CONTRIBUTING, the mandatory test checklist is scoped to new CLI commands (service / CLI / E2E layers), and no CI gate or test reads plugins/kbagent/commands/*.md at all — tests/test_skill_frontmatter.py targets skills/kbagent/SKILL.md exclusively. Nothing enumerates plugin command files, so there was no such test to extend. Flagging that gap rather than papering over it.

Checks run

Full local sweep on the pre-merge branch was green: make lint, make format-check, make skill-check, make version-check, make command-sync-check, make check-error-codes, make check-sentinel-guards, make loc-check (pre-existing soft-ceiling warnings only, none in files this PR touches), make typecheck (0 errors), make test, and bash -n install.sh. The live CLI invocations — kbagent --json version / project list / doctor — were all verified working with the documented flag placement.

Two caveats, both unchanged by the merge:

  • make test shows two failures locally, tests/test_config_store.py::TestPermissionDenied::{test_save_to_readonly_directory,test_load_unreadable_config_file}. Both are environmental and pre-existing: the local sandbox runs as uid 0, so chmod 0o000 does not deny root and the expected ConfigError never raises. They pass on CI runners, and this diff touches no config-store code.
  • make test-e2e was not run — it needs E2E_API_TOKEN + E2E_URL against a real project, and no CLI behavior changed. make changelog-check was likewise not run: it requires the gh binary, absent from this environment. It is local/release-time only and is deliberately not part of CI's per-PR check job.

Rely on the CI checks on the current head for the authoritative result.

Not in this PR

  • A kbagent setup Python CLI subcommand for agents that are not Claude Code. That is a larger change with real CI follow-through (permissions.py OPERATION_REGISTRY, commands/context.py, commands-reference.md, CLAUDE.md ## All CLI Commands, generate_skill.py SKIP_COMMANDS, plus service/CLI/E2E tests, all gated by scripts/check_command_sync.py) and deserves its own review. A slash command also has a chicken-and-egg limit worth naming: it only helps someone who already installed the plugin, so it cannot cover the /plugin step for the person who most needs it — it can only detect and explain it.
  • doctor was not changed to auto-fix anything. doctor --fix was deliberately removed in 0.85.0; reopening that is out of scope here.
  • The public page at keboola.com/cli lives outside this repo and still needs updating to the new two-step flow. This PR only fixes the in-repo docs.

Open question for reviewers

The repo's own auth guidance forks, and this command had to pick a side.

  • plugins/kbagent/.claude-plugin/CLAUDE.md:97-105 says a static Storage token is not automatically the answer for an unattended context, and prefers kbagent auth login-password (0.84.0+) when account credentials exist.
  • plugins/kbagent/agents/keboola-expert.md:77-78 says the opposite: refuse session auth and point at project add --token.

/kbagent:setup currently prefers browser auth login --register-projects with a project add (hidden-prompt / KBC_TOKEN) fallback — chosen because the product goal is explicitly "no token to paste", and because login-password needs email + password + TOTP that a first-time user is unlikely to have staged. But that is a third position, not a reconciliation. Two things reviewers should decide:

  1. Should the fallback prefer auth login-password over project add when KBC_LOGIN_EMAIL / KBC_LOGIN_PASSWORD / KBC_LOGIN_TOTP_SECRET are present in the environment?
  2. Whichever way that goes, keboola-expert.md:77-78 and .claude-plugin/CLAUDE.md:97-105 should be brought into agreement — they currently give an agent contradictory instructions, independent of this PR. I did not touch either, since picking a winner is a product call and keboola-expert.md is the highest silent-drift-risk file in the repo.

Collapse the documented five-step onboarding (install CLI -> project add
with a hand-pasted Storage API token -> doctor -> two /plugin commands)
into one slash command that runs after the plugin is installed.

plugins/kbagent/commands/setup.md orchestrates existing verbs only --
`--json version` (respecting `install_channel` on standalone builds),
`--json project list` (skip if already connected, never overwrite an
alias), `--json auth login --register-projects` (browser PKCE, so no
token to paste; relays `session_unsupported_features` from the result
rather than hand-listing it) with a `project add` hidden-prompt /
KBC_TOKEN fallback for headless hosts, then `--json doctor` interpreted
for the user including the claude_plugin check. Every step is
conditional on a check, so the command is idempotent.

No new CLI surface, no change to `doctor` (its --fix removal in 0.85.0
stands), no token ever printed or passed on a command line.

Doc sync for the new surface (none of these are CI-checked): plugin
.claude-plugin/CLAUDE.md surfaces list and "For Claude Code users";
skills/kbagent/SKILL.md prose "First-time setup" (auto-generated table
untouched); README.md and docs/TUTORIAL.md so the documented flow is
install plugin -> /kbagent:setup; install.sh "Next steps"; AGENT_CONTEXT
in commands/context.py. Also corrects the already-stale plugin surface
inventory in CLAUDE.md and widens the CONTRIBUTING.md sync-map row from
commands/keboola.md to commands/*.md with the follow-through list for
adding a new slash-command file.
The next-steps hint hardcoded the Claude Code marketplace and plugin
names (`keboola/cli` / `kbagent@keboola-agent-cli`). #627 moves the
marketplace listing to keboola/ai-kit and does not touch install.sh, so
the hardcoded pair would merge silently and then advertise deprecated
names. Point at `kbagent doctor` instead, which already prints the
current `/plugin` commands and is updated by #627 -- correct under
either merge order.
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Merge-order note vs #627 (which moves the plugin's marketplace listing to keboola/ai-kit / keboola-claude-kit).

This PR overlaps #627 in README.md, docs/TUTORIAL.md, src/keboola_agent_cli/commands/context.py and CLAUDE.md. Whichever merges second should, in each conflicted /plugin fence, keep #627's two /plugin lines plus this PR's /kbagent:setup line — the three belong together. In CLAUDE.md, keep #627's rewritten marketplace section with this PR's corrected surfaces list (three slash commands, two subagents). Three more files are touched by both at adjacent hunks and may need a trivial resolution: CONTRIBUTING.md, plugins/kbagent/.claude-plugin/CLAUDE.md, plugins/kbagent/skills/kbagent/SKILL.md.

As of this push, install.sh no longer hardcodes marketplace names — the next-steps hint defers to kbagent doctor, which prints the current /plugin commands and is itself updated by #627, so it stays correct under either merge order.


Generated by Claude Code

Resolves the sync-map conflict in CONTRIBUTING.md. main moved from 0.86.0
to 0.89.0 while this branch sat, and two adjacent table rows collided:

- `keboola-expert.md` row: keep main's "70 000 B prompt budget". v0.88.0
  raised the ceiling from 62 000 B, and tests/test_agent_prompt.py
  (test_documented_budget_matches_enforced_budget, added in #586) now
  gates CONTRIBUTING.md against PROMPT_BYTE_BUDGET, so this branch's
  stale "60 KB" would fail CI.
- `commands/*.md` row: keep this branch's widened row plus its
  follow-through list for adding a new slash-command file.

No version or changelog change: main's #648 moved version bumps into
dedicated release PRs, and this branch bumps nothing.
@claude
claude Bot marked this pull request as ready for review August 24, 2026 13:42
Resolves the conflicts introduced by #627 (kbagent published through
keboola/ai-kit as kbagent@keboola-claude-kit).

- README.md, docs/TUTORIAL.md, commands/context.py: the /plugin fences now
  carry main's marketplace and plugin names plus this branch's
  /kbagent:setup line.
- CLAUDE.md: keeps main's rewritten "source here, publication there"
  section; its stale surfaces sentence is replaced with this branch's
  corrected list, made exact against the tree (3 skills, 3 slash
  commands, 2 subagents). Its install fence gains /kbagent:setup.
- plugins/kbagent/commands/setup.md: the claude_plugin bullet no longer
  paraphrases doctor's drift hint (main qualifies it with the
  marketplace) and now tells the agent to quote doctor's own /plugin
  lines as the single source of truth for the names.

install.sh already defers to `kbagent doctor` and hardcodes no names, so
main's rename does not reach it. No version file and no CI-gated figure
is touched.
@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Merged origin/main in (true merge commit f07ef12, no rebase, no force-push) to clear the conflict #627 created by moving publication to keboola/ai-kit. Four files conflicted:

  • README.md, docs/TUTORIAL.md, src/keboola_agent_cli/commands/context.py — same shape in all three: kept main's /plugin marketplace add keboola/ai-kit + /plugin install kbagent@keboola-claude-kit and appended this branch's /kbagent:setup. The documented flow is now those three lines, with main authoritative on the names.
  • CLAUDE.md — kept main's rewritten "source here, publication there" section wholesale (shim rationale, sync job, ai-kit framing) and swapped only its surfaces sentence for this branch's corrected one. I made that list exact against the tree rather than trusting either side's prose: 3 skills (kbagent, kbagent-cicd-migration, kbagent-promotion-pipeline), 3 slash commands, 2 subagents — both sides still said "four AI surfaces / a skill / a slash command". Main's install fence in that section also gained /kbagent:setup, so it matches the other three docs.

Two things reviewers should look at, since they go slightly beyond mechanical conflict resolution:

  1. plugins/kbagent/commands/setup.md did not conflict, but main changed the message it was paraphrasing: doctor's drift hint is now /plugin update kbagent@<marketplace> (qualified), and there's a new pass-with-migration case for plugins found only under the legacy keboola-agent-cli cache dir. The claude_plugin bullet no longer restates either; it tells the agent to relay doctor's own note verbatim and treat doctor as the single source of truth for the marketplace and plugin names. The file still hardcodes no marketplace or plugin name.
  2. install.sh needed no change. Its next-steps hint points at kbagent doctor to print the /plugin commands rather than naming them, so the rename doesn't reach it. That answers the note in feat(plugin): publish kbagent through keboola/ai-kit, deprecate this repo's marketplace #627 about this line needing the new names — it never carried them.

Untouched on purpose: no version file, no changelog.py entry, and CONTRIBUTING.md's 70 000 B prompt budget is main's value. CONTRIBUTING.md, SKILL.md and the plugin CLAUDE.md auto-merged cleanly.

Also note the PR description above is now stale — its Before/After block still shows keboola/cli / kbagent@keboola-agent-cli. The diff does not.

Green: lint, format-check, typecheck, skill-check, version-check, version-gate-check, command-sync-check, endpoints-check, check-error-codes, check-sentinel-guards, loc-check, bash -n install.sh. Tests: 6151 passed, 13 skipped, 2 failed — only the known test_config_store.py::TestPermissionDenied pair that cannot pass as uid 0 in this sandbox. (make vnext-check fails on 7 pre-existing vNEXT gates in files this branch does not touch; it is release-PR-only and not part of make check.)


Generated by Claude Code

@soustruh soustruh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Open question for reviewers" section asks two things. Both answered here.

1. Yes — try auth login-password before the static token

In step 3, the no-browser fallback goes straight to project add with a static token. Add one step before it.

When KBC_LOGIN_EMAIL and KBC_LOGIN_PASSWORD are set — plus KBC_LOGIN_TOTP_SECRET for an MFA account — run:

kbagent --json auth login-password --register-projects

The command reads those values from the environment (commands/auth.py:388-427). No secret appears in the conversation, and none appears on a command line. Keep the static token as the last resort, for when those variables are absent.

State two guards in the text:

  • Needs kbagent 0.84.0+. On an older version, go straight to the static token.
  • A passkey-only account fails with AUTH_MFA_INVALID. On that error, fall back to the static token.

This is not a new policy. The repo documents this exact order since 0.84.0 (.claude-plugin/CLAUDE.md, auth-workflow.md): browser login → account login from the environment → static token. The setup command should follow that order, not skip the middle step.

2. The two files do not contradict each other — change neither

The static-token line in keboola-expert.md sits inside the version-gate rule. It applies only when the installed kbagent is older than 0.84.0. On those versions login-password does not exist, so a static token is the only option. .claude-plugin/CLAUDE.md describes current versions. Each rule is correct in its own scope.

History confirms this. Commit 8ee25c9 (PR #565) updated both files together, and its message states the goal: the doc surfaces "get their own version-gated section/bullets instead of contradicting the CLI". The same commit added the tool-matrix row in keboola-expert.md that itself prefers login-password and says "static token if no creds". The two files state the same policy.

No change in this PR. A follow-up may add one clarifying sentence to the version-gate line — it reads as a general rule when seen in isolation, and the 2026-08-13 byte-budget trim made it terser.

Addresses @soustruh's review on #625.

**Review item 1 — try `auth login-password` before a static token.** Step 3
was a two-rung fallback (browser login -> `project add --token`), skipping
the middle step the repo has documented since 0.84.0. It is now an explicit
three-rung ladder, 3a/3b/3c, with the reviewer's two guards stated in the
text: `login-password` does not exist before 0.84.0 (compare `kbagent.version`
from step 1, else skip to the static token), and `AUTH_MFA_INVALID` means a
WebAuthn/passkey-only account, which drops to the static token rather than
retrying or looping back to the browser rung. 3b runs only when
KBC_LOGIN_EMAIL + KBC_LOGIN_PASSWORD are already exported (+
KBC_LOGIN_TOTP_SECRET for TOTP MFA); the command reads all three off the
environment, so nothing lands on a command line.

The "never persist a token" rule is widened to any credential, and made
explicit that the command must never *solicit* a password or TOTP seed --
absent env vars close that route, they are not a prompt.

**Review item 2 — the two doc surfaces.** No change, per the review: the
`keboola-expert.md` static-token line sits inside the version-gate rule and
is correct for pre-0.84.0, and `.claude-plugin/CLAUDE.md` describes current
versions. Each is right in its own scope.

**Separately: step 3 could not have worked as written.** `auth login` without
`--stack` resolves the stack from the default project, and step 2 has just
established there is no project registered -- so on the fresh-install path
this command exists to serve, it failed with CONFIG_ERROR ("login is not
stack discovery"). Verified against an empty config dir. Every login
invocation now passes `--stack <STACK_URL>`, taken from $ARGUMENTS or asked
for up front; the argument-hint says so.

Doc surfaces re-synced (CONTRIBUTING.md "Plugin synchronization map", none
CI-checked): README.md, docs/TUTORIAL.md, plugins/kbagent/.claude-plugin/
CLAUDE.md, skills/kbagent/SKILL.md prose, commands/context.py AGENT_CONTEXT.
No version bump, no changelog entry, no CLI surface change.

Verified: `--stack` and `--register-projects` exist on both login commands;
env-var resolution satisfies the required `--email` and reaches the server
with no secret on the argv; `auth login --stack` gets past stack resolution
on an empty config where the flagless form errors. lint, format-check,
typecheck (0 errors, 66 pre-existing warnings), version-check,
version-gate-check, command-sync-check, endpoints-check, check-error-codes,
check-sentinel-guards, loc-check green. Tests: 6154 passed, 181 skipped, 0
failed. skill-check flags SKILL.md only because the hand edit is
uncommitted -- `make skill-gen` regenerates it byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jordanrburger

Copy link
Copy Markdown
Collaborator

Both items handled in b1353e5. Thanks — item 1 turned up a second problem on the way.

1. auth login-password before the static token — done

Step 3 is now an explicit three-rung ladder rather than a two-rung one, in the order you named:

  • 3a auth login --register-projects — browser, nothing to paste.
  • 3b auth login-password --register-projects — only when KBC_LOGIN_EMAIL and KBC_LOGIN_PASSWORD are already exported (plus KBC_LOGIN_TOTP_SECRET for TOTP MFA). Read off the environment, so nothing lands on an argv.
  • 3c project add static token — last resort, when those are absent.

Both guards are stated in the text, as asked:

  • Version. login-password does not exist before 0.84.0 — compare kbagent.version from step 1, and if it is older skip straight to 3c.
  • AUTH_MFA_INVALID. Passkey-only account. Go to 3c; do not retry login-password, and do not loop back to 3a — both routes into 3b are routes where browser login is unavailable or insufficient, so bouncing back to it would just fail again.

I verified the env-var path rather than trusting the flag help: with KBC_LOGIN_EMAIL/KBC_LOGIN_PASSWORD set and no flags, the required --email resolves from the environment and the command reaches the server (AUTH_FLOW_DENIED on deliberately bad credentials), with no secret on the command line.

Two things I tightened while in the same block, both in the same spirit:

  • The "never print or persist a token" rule now covers any credential — --password and --totp-secret included.
  • Added an explicit prohibition on soliciting a password or TOTP seed. Absent env vars close 3b; they are not a cue to ask the user to export secrets, since a scoped token is the smaller blast radius and is the very next rung.

2. The two doc surfaces — unchanged

Agreed, and the version-gate scoping argument holds. Neither file touched.

Separately: step 3 could not have worked as written

Worth a second look, since it is outside what you reviewed.

auth login without --stack resolves the stack from the default project and, per _resolve_stack_url, deliberately never discovers one. Step 2 has just established there is no project registered — so on the fresh-install path this whole command exists to serve, step 3 failed immediately:

$ kbagent --json --config-dir <empty> auth login --register-projects
"code": "CONFIG_ERROR",
"message": "No stack to log into -- login is not stack discovery. Pass --stack <url-or-alias>, ..."

With --stack against the same empty config dir it gets through and prints the device code as expected. So every login invocation now passes --stack <STACK_URL>, and step 3 resolves that URL first — from $ARGUMENTS when it looks like one, otherwise by asking — before running anything. The argument-hint was advertising the stack URL as optional; it now says what is actually needed.

Doc sync + checks

Five non-CI-checked surfaces re-synced for the new ladder: README.md, docs/TUTORIAL.md, plugins/kbagent/.claude-plugin/CLAUDE.md, skills/kbagent/SKILL.md prose, commands/context.py AGENT_CONTEXT. No version bump, no changelog entry, no CLI surface change.

Green locally: lint, format-check, typecheck (0 errors; the 66 diagnostics are pre-existing warnings), skill-check, version-check, version-gate-check, command-sync-check, endpoints-check, check-error-codes, check-sentinel-guards, loc-check. Tests 6154 passed, 181 skipped, 0 failed — note the two test_config_store.py::TestPermissionDenied failures the PR description mentions did not reproduce here; that sandbox ran as uid 0, this one does not, which is consistent with the original diagnosis. CI on the new head is authoritative.

@keboola-pr-reviewer-bot keboola-pr-reviewer-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: auto_approve (risk 2/5) · profile keboola-mcp-server

Docs-plus-plugin-instructions PR adding a /kbagent:setup slash command; no code path, tool, auth, or dependency change.

Concerns:

  • plugins/kbagent/agents/keboola-expert.md: Unreconciled auth guidance contradicts .claude-plugin/CLAUDE.md; pre-existing, left for follow-up

@soustruh soustruh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 3 now follows the requested order, and the --stack fix is correct. I verified it in auth_service.py:845-872: login never derives a stack by itself, so the previous form failed on a fresh config. One routing error remains.

The session-insufficient case must skip 3b. The "Drop to 3b when…" sentence sends two cases into 3b: the user needs a feature from the session_unsupported_features list, or no browser exists. The first case does not belong there. login-password creates the same session type as browser login, with the same unsupported-feature list (.claude-plugin/CLAUDE.md: same auth mode, same downstream command support). For that user, 3b succeeds, the flow stops, and the session still cannot serve the feature they need. Send that case directly to 3c. Only the no-browser case tries 3b before 3c.

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #625 — feat(plugin): add /kbagent:setup one-command first-run setup

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This is a docs-and-plugin-instructions PR: it adds plugins/kbagent/commands/setup.md (a new /kbagent:setup slash command that orchestrates existing, already-tested verbs — version, project list, auth login, auth login-password, project add, doctor) and updates every hand-maintained doc surface that references the plugin's install flow (CLAUDE.md, CONTRIBUTING.md, README.md, docs/TUTORIAL.md, install.sh, plugins/kbagent/.claude-plugin/CLAUDE.md, SKILL.md prose, commands/context.py AGENT_CONTEXT). No Python business logic, no new CLI command, no permissions.py/server-route surface touched. Verdict: APPROVE. The one open item from the review thread (soustruh's CHANGES_REQUESTED, asking for a login-password rung before the static-token fallback) was already addressed in commit b1353e56 on the current HEAD, and I independently re-verified soustruh's second conclusion — keboola-expert.md §1 Rule 6 and .claude-plugin/CLAUDE.md are not actually contradictory, one is a version-gate ("if login-password doesn't exist on this install, fall back to a token"), the other describes current-version behavior — so no code change was warranted there.

Verdict

  • Verdict: APPROVE
  • Blocking findings: 0
  • Non-blocking findings: 1
  • Nits: 1

Blocking findings

(none)

Non-blocking findings

[NB-1] plugins/kbagent/agents/keboola-expert.md:77-78 — the version-gate line reads as a general policy in isolation

Both the PR author's original "Open question for reviewers" and the auto-approving bot's review independently flagged this same line as looking contradictory with .claude-plugin/CLAUDE.md's auth-preference guidance. I traced the actual semantics and confirm soustruh's read is correct — 77-78 sits entirely inside §1 Rule 6 (VERSION GATE) and only fires when the installed kbagent predates 0.84.0 (where login-password genuinely does not exist), while §2's Tool Selection Matrix (keboola-expert.md:151) already prefers login-password for CI-with-credentials on current versions. The two files are consistent, not contradictory. Since three independent readers (PR author, bot, human reviewer) tripped on the same sentence, it is worth a follow-up one-line clarification (e.g. "on kbagent < 0.84.0, ...") to stop it recurring — correctly scoped out of this PR by the author, tracked here so it isn't lost.

Nits

  • [NIT-1] CONTRIBUTING.md (Plugin synchronization map row for plugins/kbagent/commands/*.md) — the row now says "Slash-command UX change (rare)" immediately followed by a fairly non-rare follow-through list for new command files; consider splitting into two rows (existing-file UX tweak vs. new-file addition) next time the table is touched, purely for scanability.

Verification log

  • gh pr view 625 --json title,body,files,additions,deletions,... → 9 files changed, +233/-9, feat(plugin): prefix matches (new user-facing capability) ✓
  • gh pr diff 625 (382 lines) reviewed in full — no code files (src/keboola_agent_cli/{commands,services,client,manage_client.py} etc.) touched except a pure-string addition to commands/context.py AGENT_CONTEXT (exempt from loc-check per CONTRIBUTING.md's explicit _EXEMPT list) → 3-layer-architecture step is N/A, confirmed no typer/httpx layer-violation greps fire (no matches, nothing to check against)
  • No new CLI command added (no @*_app.command(...)) → permissions.py OPERATION_REGISTRY, server/routers/*.py, commands-reference.md, gotchas.md version-tag requirement all correctly out of scope per CONTRIBUTING's own "new slash-command file" checklist (which this PR itself extends in the CONTRIBUTING.md diff)
  • Walked the extended sync-map row this PR adds for commands/*.md: .claude-plugin/CLAUDE.md surfaces list ✓ updated, SKILL.md prose ✓ updated, README.md ✓ updated, docs/TUTORIAL.md ✓ updated, commands/context.py AGENT_CONTEXT ✓ updated, install.sh "Next steps" ✓ updated — every item on the author's own new checklist is actually present in the diff
  • tests/test_skill_frontmatter.py read → confirms it targets SKILL.md only, no enumeration of commands/*.md files that would need a fixture update; ran it (uv run pytest tests/test_skill_frontmatter.py tests/test_agent_prompt.py -q) → 46 passed ✓
  • Checked out PR HEAD (b1353e56, the commit that already answers soustruh's review) into an isolated detached worktree (never touched the user's checkout) and ran make check → exit 0, ruff check clean, ruff format --check clean, ty check clean (1 pre-existing unrelated unresolved-import info-level diagnostic in scripts/hatch_build.py, not part of this diff), SKILL.md is up-to-date, version is in sync, check_version_gates.py all 530 gates resolve (7 unrelated vNEXT awaiting release), check_command_sync.py OK 264/264 commands registered+documented, endpoints-check up-to-date, changelog-check OK, check_error_codes.py OK, check_sentinel_guards.py OK, check_file_size.py only pre-existing soft-ceiling warnings (none in touched files), 6154 passed, 12 skipped
  • gh pr checks 625 → all 3 required checks pass (Windows build, check, test 3.12, pr-reviewer-bot) on current HEAD ✓
  • Reproduced setup.md's factual claims against the live built CLI from the PR worktree:
    • kbagent --json version and kbagent --json --config-dir DIR project list → confirmed root-callback flags (--json, --config-dir) go before the subcommand, exactly as the PR claims
    • kbagent --json --config-dir DIR auth login --register-projects (no --stack) → CONFIG_ERROR, message "No stack to log into -- login is not stack discovery. Pass --stack <url-or-alias>, ..." — verbatim match to setup.md step 3's claimed error
    • kbagent --json --config-dir DIR doctorclaude_plugin check returns a pass status with a version-drift + deprecated-marketplace migration message naming /plugin marketplace add keboola/ai-kit / /plugin install kbagent@keboola-claude-kit — matches setup.md step 4's description of what to relay verbatim
    • src/keboola_agent_cli/commands/auth.py:389-427 (auth_login_password) → email/password/totp_secret all declare envvar= reading KBC_LOGIN_EMAIL/KBC_LOGIN_PASSWORD/KBC_LOGIN_TOTP_SECRET, and a --stack option exists — matches setup.md step 3b's claims about the environment-var contract and citation of commands/auth.py:388-427
    • errors.py:136 (AUTH_MFA_INVALID) and services/auth_service.py:324 → the error code exists and is raised on passkey-only MFA, matching step 3b's guard
    • services/_auth_registration.py:33 (SESSION_UNSUPPORTED_FEATURES) and commands/auth.py (session_unsupported_features on the result) → matches setup.md step 3a's claim about the canonical list and its --json field name
  • bash -n install.sh → syntax OK
  • grep -rn "two slash\|a slash command (\/keboola`)"` across the repo → no stale references left un-updated elsewhere
  • Compared commit timeline: soustruh's CHANGES_REQUESTED review was submitted 2026-08-25T12:45:05Z; commit b1353e56 ("fix(plugin): setup ladder -- login-password rung, and pass --stack") landed 2026-08-25T13:04:03Z, directly after and directly addressing it; keboola-pr-reviewer-bot approved at 13:17:56Z (after the fix), noting the same keboola-expert.md line as a pre-existing, correctly-deferred concern — consistent with my own independent trace above

Open questions for the author

(none)

@padak padak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second review pass — runtime behaviour of /kbagent:setup, not the diff's static correctness

Complementary to the kbagent-pr-reviewer pass above, which I agree with on everything it covered: the doc sync is complete, every factual claim in setup.md matches the source, and soustruh's CHANGES_REQUESTED is already answered by b1353e56 (submitted 12:45, fix landed 13:04) — so the block is procedural at this point, not substantive.

What follows is the axis that pass did not cover: what actually happens when an agent executes these instructions through a tool-run shell. setup.md is prompt text, so its bugs look like plausible instructions rather than bad code, and no CI gate reads commands/*.md at all.

🟡 1. Step 3a tells the agent to run a command the CLI explicitly says an agent must not run — and the call will be killed mid-flow

auth login's own docstring (src/keboola_agent_cli/commands/auth.py:291-295):

Requires a human at a browser -- an AI agent must not attempt this headlessly.

Root CLAUDE.md repeats it: "REQUIRES A HUMAN AT A BROWSER -- never attempt from an unattended AI agent task." Step 3a nonetheless hands the agent the command to execute. A /kbagent:setup session does have a human present, so this is not a flat violation — but the timing makes it fail in practice:

  • PKCE loopback waits AUTH_CALLBACK_TIMEOUT = 115.0 s (src/keboola_agent_cli/constants.py:889).
  • The device-code fallback polls until authorization.expires_in (src/keboola_agent_cli/auth/device.py:75-79) — server-set, typically several minutes.

Claude Code's Bash tool defaults to a 120 s timeout. Failure scenario: the user approves in the browser at t=130 s. The session is written to auth.json server-side and locally, but the agent saw a killed tool call with no result, cannot tell whether login landed, and re-runs step 3a — producing exactly the orphaned-session case commands/auth.py:177-182 warns about (orphaned_session_id, "kbagent auth logout will retry it").

Suggested fix: state an explicit long timeout for this one call, or hand auth login to the user to run in their own terminal — the way step 3c already does for project add's hidden prompt. Step 3c's reasoning ("that prompt needs a real TTY, which a tool-run shell does not have") applies here for the same class of reason, and it would be consistent to apply it.

🟡 2. The verification URL and device code go to stderr, and 3a's snippet does not capture it

commands/auth.py:304:

target_console = formatter.err_console if formatter.json_mode else formatter.console

So in --json mode the device-login panel — the URL and the user code the agent is supposed to relay — lands on stderr, as the docstring confirms ("always printed (to stderr in --json mode)").

Step 1's snippet has 2>&1. Step 3a's does not:

kbagent --json auth login --stack <STACK_URL> --register-projects

And the "Non-negotiable rules" block immediately above instructs: "Use --json for every check you have to parse. Parse the JSON; do not scrape human-mode Rich output." An agent following that rule literally, reading stdout, never sees the code. Suggested fix: add 2>&1 to the 3a snippet, or say in the prose that the code arrives on stderr and is not part of the JSON.

🟡 3. No Windows path in step 1

Step 1's only install route is curl -LsSf … | sh. README.md:31-48 documents that Windows needs either the winget + uv tool install PowerShell block or an explicit trip through Git Bash (& "C:\Program Files\Git\bin\bash.exe" -lc "…"). setup.md does not branch, and its retry advice (source $HOME/.local/bin/env) is POSIX-only.

On a Windows box without Git Bash, first-run setup — the exact audience this command exists for — dies at step 1 with no recovery path. A single sentence pointing at README's PowerShell block would close it.

🟢 Nits

  • CLAUDE.md — the prose was corrected to "three skills (kbagent, kbagent-cicd-migration, kbagent-promotion-pipeline)", but the structure tree directly below still lists only skills/kbagent/. The PR fixed one half of that drift and left the other.
  • plugins/kbagent/.claude-plugin/CLAUDE.md:3 — still "a skill (kbagent)", singular, now inconsistent with root CLAUDE.md's corrected count. If the count was worth fixing in one file, it is worth fixing in both.
  • SKILL.md — points readers at the repo path plugins/kbagent/commands/setup.md. When the skill is loaded from ~/.claude/plugins/cache/…, that path does not exist for the reader.

Verification

Everything above was read from source at origin/main, not inferred:
commands/auth.py:291-304, commands/auth.py:177-182, constants.py:889, auth/device.py:75-79, commands/project.py:172-183, services/auth_service.py:865-875, services/_auth_registration.py:33,119, services/doctor_service.py:563-640, README.md:31-48.

I also confirmed the claims setup.md makes that I did not list as findings — the --json root-callback flag placement, the CONFIG_ERROR text for a missing --stack, project add's TTY-prompt/KBC_TOKEN contract with no --token on argv, login-password's envvar= wiring, install_channel appearing only on a frozen build, and session_unsupported_features riding on LoginResult — all hold.

None of the three 🟡 items are blocking in the sense of being wrong-on-the-page; they are cases where an agent following the instructions faithfully hits a wall. Items 1 and 2 are worth fixing before this becomes the documented front door.

…ht to 3c

Addresses @soustruh's second review on #625. The routing error is real.

The "drop to 3b when *either* ..." sentence funnelled two different reasons
for leaving 3a into the same rung. Only one belongs there.

`login_password()` returns through the SAME `_finalize_login` as `login()` --
the only difference is `method="password"` -- and `session_unsupported_features`
is a `default_factory=default_unsupported_features` field on the shared
`LoginResult`, i.e. a copy of the one `SESSION_UNSUPPORTED_FEATURES` constant,
independent of method. So for a user who needs a surface on that list, 3b
succeeds, the ladder stops at the first rung that "landed", and they are left
holding a session with the identical restriction they were trying to escape.

Step 3 now routes on *why* 3a was not the answer:

- No browser at all (headless, container, CI, SSH without forwarding)
  -> try 3b, then 3c. This is now the ONLY route into 3b.
- Needs a surface named in `session_unsupported_features` -> straight to 3c,
  skipping 3b, because only a static token serves those surfaces.

Two knock-on corrections that would otherwise have contradicted the fix:

- The `AUTH_MFA_INVALID` guard justified "do not loop back to 3a" with "both
  routes into 3b" -- there is one route now, so it cites that instead.
- `docs/TUTORIAL.md` explicitly sent "a surface a browser session does not
  serve" to `login-password`. Rewritten; it was the only downstream surface
  that asserted the wrong routing (README scopes 3b to headless already, and
  the plugin CLAUDE.md / SKILL.md / context.py summaries name the order
  without claiming which reason routes where).

Verified in `auth_service.py` rather than from the prose: `login_password`
-> `_finalize_login` (line 340), `LoginResult.session_unsupported_features`
default factory (line 130), `default_unsupported_features()` returning
`list(SESSION_UNSUPPORTED_FEATURES)`.

lint, format-check, typecheck, skill-check, version-check,
command-sync-check, endpoints-check, check-error-codes,
check-sentinel-guards, loc-check all green. Tests: 6154 passed, 181 skipped,
0 failed. No Python changed this round -- two doc files only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@keboola-pr-reviewer-bot
keboola-pr-reviewer-bot dismissed their stale review August 25, 2026 13:33

Dismissing prior approval — a new commit was pushed and this review was for an earlier SHA. Run @keboola-pr-reviewer-bot review to get a fresh verdict.

@keboola-pr-reviewer-bot

Copy link
Copy Markdown

New commit on 3a50684 — dismissed 1 stale bot approval. Comment @keboola-pr-reviewer-bot review when you want a fresh review.

@jordanrburger

Copy link
Copy Markdown
Collaborator

Fixed in 3a50684. You're right, and it was a real routing bug — thanks for catching it.

I checked the claim in auth_service.py rather than taking the prose for it: login_password() returns through the same _finalize_login as login() (differing only by method="password"), and session_unsupported_features is a default_factory=default_unsupported_features field on the shared LoginResult — a copy of the one SESSION_UNSUPPORTED_FEATURES constant, independent of method. So 3b would have succeeded, stopped the ladder at "first rung that landed", and handed the user a session carrying the identical restriction they were trying to escape.

Step 3 now routes on why 3a was not the answer:

  • No browser at all (headless, container, CI, SSH without forwarding) → try 3b, then 3c. This is now the only route into 3b, and 3b is labelled as such.
  • Needs a surface named in session_unsupported_features → straight to 3c, skipping 3b, with the reason stated inline so nobody re-merges the two cases later.

Two knock-on corrections that would otherwise have contradicted the fix:

  • The AUTH_MFA_INVALID guard justified "do not loop back to 3a" with "both routes into 3b". There is one route now, so it cites that instead.
  • docs/TUTORIAL.md explicitly sent "a surface a browser session does not serve" to login-password — a leftover from the previous round. Rewritten. It was the only downstream surface asserting the wrong routing: README already scopes 3b to the headless case, and the plugin CLAUDE.md / SKILL.md / context.py summaries name the order without claiming which reason routes where.

Green: lint, format-check, typecheck, skill-check, version-check, command-sync-check, endpoints-check, check-error-codes, check-sentinel-guards, loc-check. Tests 6154 passed, 181 skipped, 0 failed. No Python changed this round — two doc files only.

🤖 Addressed by Claude Code

Addresses @padak's second review pass on #625 (runtime behaviour of the
instructions, an axis no CI gate covers -- nothing reads commands/*.md).

**3a no longer tells the agent to run `auth login` itself.** Two independent
reasons, both verified in source:

- `commands/auth.py` docstring: "Requires a human at a browser -- an AI agent
  must not attempt this headlessly." `.claude-plugin/CLAUDE.md:97-99` is
  blunter still: hand the exact command back to the user and wait. 3a was
  handing the agent the command to execute.
- It cannot finish inside a tool-run shell anyway. PKCE blocks on
  `AUTH_CALLBACK_TIMEOUT = 115.0` (constants.py:860) and the device fallback
  polls until the server's `expires_in` (auth/device.py:75). Against a ~120 s
  Bash timeout the call is killed mid-flow, and a kill is indistinguishable
  from a failure -- so the agent retries and produces the
  `orphaned_session_id` case commands/auth.py:178-182 warns about.

3a now hands over the command and waits, mirroring what 3c already does for
`project add`'s hidden prompt, and the agent confirms afterwards with
`auth status`. A standing rule was added: if a login call you did run gets
interrupted, check `auth status` before re-running anything.

**stderr.** The snippet drops `--json` deliberately -- a human wants the
panel, and `commands/auth.py:304` routes it to `err_console` in json mode, so
the device URL and code never appear in the JSON an agent would parse. Made
a standing rule too, with `2>&1` for anyone who runs a login anyway.

Consequence worth stating: `auth status` does NOT carry
`session_unsupported_features` (only `login` / `register-projects` do --
`_auth_registration.py:119`, and context.py:189 says so explicitly). So 3a
now names `auth register-projects --all` as the way to read that list
programmatically, which is a no-op (`exists`) on anything already
registered.

**Windows.** Step 1's only route was `curl … install.sh | sh`, and its retry
advice `source $HOME/.local/bin/env` is POSIX-only -- so first-run setup, the
exact audience, died at step 1 on a Windows box without Git Bash. Step 1 now
branches and points at README's Install section for the winget + `uv tool
install` PowerShell block, including the "open a new shell after
update-shell" caveat, and quotes rather than paraphrases it.

Nits, all three:
- Root `CLAUDE.md` prose said "three skills" while the structure tree
  directly below listed only `skills/kbagent/`. Tree now lists all three.
- `.claude-plugin/CLAUDE.md:3` still said "a skill (`kbagent`)", singular.
- `SKILL.md` pointed readers at the repo path
  `plugins/kbagent/commands/setup.md`, which does not exist when the skill is
  loaded from `~/.claude/plugins/cache/…`. Replaced with the command itself.

Not changed, per both reviewers agreeing it is out of scope here: the
`keboola-expert.md:77-78` version-gate wording (NB-1) and CONTRIBUTING's
sync-map row split (NIT-1) are follow-ups.

lint, format-check, typecheck, version-check, command-sync-check,
endpoints-check, check-error-codes, check-sentinel-guards, loc-check green.
Tests: 6154 passed, 181 skipped, 0 failed. Docs only -- no Python changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jordanrburger

Copy link
Copy Markdown
Collaborator

All three 🟡 items and all three 🟢 nits fixed in f01ca67. This was the most useful pass on the PR — the runtime axis is exactly the one nothing in CI can see, and item 1 was a genuine defect, not a style note.

🟡 1 — 3a no longer tells the agent to run auth login

Fixed by handing it over, not by raising a timeout. Two reasons, both of which I re-verified rather than taking on faith:

  • commands/auth.py docstring says an AI agent must not attempt it headlessly, and .claude-plugin/CLAUDE.md:97-99 is blunter: hand the exact command back to the user and wait. 3a was handing the agent the command to execute, which contradicted the repo's own policy.
  • It cannot finish in a tool-run shell regardless. AUTH_CALLBACK_TIMEOUT = 115.0 — at constants.py:860, not 889, the only correction I have for you — and the device fallback polls to expires_in (auth/device.py:75). Against a ~120 s Bash timeout the call dies mid-flow, and a kill is indistinguishable from a failure, so the retry produces exactly the orphaned_session_id case commands/auth.py:178-182 warns about.

3a now prints the command, says finishing it is the user's, and waits — mirroring 3c's precedent for project add's hidden prompt, as you suggested. The agent confirms afterwards with auth status. I also added a standing rule: if any login call was run and got interrupted, check auth status before re-running anything, since the blind retry is the thing that orphans the session.

🟡 2 — stderr

Fixed, and it fell out of item 1: the 3a snippet is now deliberately without --json, because a human reading their own terminal wants the panel. Also promoted to a standing rule in the header block, with 2>&1 for anyone who runs a login themselves.

One consequence worth flagging, since it bit the fix: auth status does not carry session_unsupported_features — only login and register-projects do (_auth_registration.py:119; context.py:189 states it outright). So handing 3a to the user costs the agent its programmatic read of that list. 3a now names auth register-projects --all as the way to get it, which is a no-op (exists) on anything already registered.

🟡 3 — Windows

Fixed. Step 1 now branches, and the POSIX-only nature of both the one-liner and the source $HOME/.local/bin/env retry advice is stated. The Windows branch points at README's Install section for the winget + uv tool install block, carries the "open a new shell after update-shell" caveat, and says to quote README rather than paraphrase — that block is versioned and setup.md is not.

🟢 Nits — all three

  • Root CLAUDE.md: the tree now lists all three skill dirs. You were right that the PR fixed one half of that drift and left the other.
  • .claude-plugin/CLAUDE.md:3: no longer singular.
  • SKILL.md: the repo path is gone — it referenced a file that does not exist when the skill loads from ~/.claude/plugins/cache/….

Left alone deliberately, since you and the kbagent-pr-reviewer pass agree they are follow-ups: NB-1 (keboola-expert.md:77-78 version-gate wording) and NIT-1 (splitting the CONTRIBUTING sync-map row).

Green: lint, format-check, typecheck, skill-check, version-check, command-sync-check, endpoints-check, check-error-codes, check-sentinel-guards, loc-check. Tests 6154 passed, 181 skipped, 0 failed. Docs only — no Python changed.

🤖 Addressed by Claude Code

@soustruh soustruh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. All three review rounds are resolved on f01ca67, and I verified the new claims in the source:

  • Step 3a is handed to the user, with the reason stated: the 115 s callback wait against a ~120 s tool timeout, and the command's own docstring. The agent confirms with auth status afterward, and the interrupted-login rule prevents the orphaned-session retry.
  • The stderr behavior of the auth commands is now a standing rule, and the 3a snippet drops --json on purpose — correct, commands/auth.py:304 routes the panel to stderr in json mode.
  • auth status lacks session_unsupported_features (AuthStatusResult, auth_service.py:134), so register-projects --all is the right programmatic read: it carries the field and reports exists on registered projects.
  • Step 1's Windows branch matches README's install block: winget, uv tool install, update-shell, the new-shell requirement, and the Git Bash alternative.
  • All three nits are fixed. The two deferred items — the version-gate wording and the sync-map row split — stay follow-ups, as agreed by both reviewers.

@jordanrburger
jordanrburger merged commit 29bf142 into main Aug 25, 2026
3 checks passed
@jordanrburger
jordanrburger deleted the feat/kbagent-setup-slash-command branch August 25, 2026 14:27
padak added a commit that referenced this pull request Aug 25, 2026
Cover #625 in the 0.91.0 changelog (it merged to main while the release PR was open, landing inside the tag but outside its release notes), and move three resolved 0.91.0 version tags out of markdown headings onto section body lines per CONTRIBUTING.md release step 4.

No version change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants