fix(plugin,doctor,auth): let an attended agent complete browser login in-chat (#704) - #722
Conversation
… in-chat (#704) /kbagent:setup could not finish in any chat client: step 3a handed `auth login` to the user's terminal, so the documented one-command setup was really "two steps in chat, then go find a terminal". The rule guarded against a real hazard -- a foreground tool shell's ~120s timeout killing the PKCE/device flow mid-flight -- but banned more than the hazard: driving the command was forbidden even though only the browser approval is inherently human. Policy change (docs): in an attended session with a background shell, the agent drives `auth login --device-code --stack URL --register-projects` itself, relays the verification URL + user code into the chat (printed before polling starts), and confirms completion by polling `auth status` (exit 0 = signed in, 3 = not yet). Foreground shells and unattended tasks stay banned; terminal handoff remains the fallback where no background shell exists. Updated consistently in setup.md, plugin CLAUDE.md, keboola-expert.md, root CLAUDE.md, `kbagent context`, SKILL.md and the auth/commands/gotchas references. Claude Desktop has no slash-command surface, so the kbagent skill now carries first-time setup and logout natively: new triggers ("set up keboola", "logout", ...) and a client-agnostic setup ladder instead of deferring to the Claude-Code-only /kbagent:setup. The plugin is now framed as an upgrade, not a prerequisite -- `kbagent project add` + `kbagent context` is a complete setup in any client with a shell -- and per-client marketplace notes are documented (Cursor requires the full https://github.com/keboola/ai-kit URL; the short form fails with [invalid_argument]). Code changes: - doctor `_check_claude_plugin` skip/warn messages now say the check only detects Claude Code's cache layout (a Cursor-side install is invisible to it) and that `kbagent context` is the client-agnostic substitute. - the device-login panel additionally prints `verification_uri_complete` (one-click, code pre-filled) when the server provides it, so a relayed login is a single click. Closes #704
padak
left a comment
There was a problem hiding this comment.
Review of #722 — fix(plugin,doctor,auth): let an attended agent complete browser login in-chat
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 relaxes the "never run auth login yourself" rule: in an attended session with a background shell, the agent may now drive kbagent auth login --device-code itself, relay the printed URL/code, and confirm via auth status polling. The docs-surface rewrite (setup.md, plugin CLAUDE.md, keboola-expert.md, root CLAUDE.md, context.py, SKILL.md, auth-workflow.md, commands-reference.md, gotchas.md) is internally consistent and every behavioral claim I checked against the live code (device-flow prompt fires before the poll loop, --json panel goes to stderr, auth status exit 0/3 contract, atomic auth.json write) is accurate. However, the rewrite missed two authoritative surfaces that still assert the old, now-false absolute rule verbatim: the CLI's own --help text (commands/auth.py) and the canonical docs/auth.md that README links as "Details". Those directly contradict the policy this PR ships everywhere else, so REQUEST CHANGES.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 2
- Non-blocking findings: 2
- Nits: 1
Blocking findings
[B-1] src/keboola_agent_cli/commands/auth.py:8-11,293-295 — --help text and module docstring still state the old absolute prohibition
The module docstring (line 11: "requires a human at a browser -- an AI agent must not attempt it on its own initiative") and the auth_login command's Typer docstring (lines 293-295: "Requires a human at a browser -- an AI agent must not attempt this headlessly") were not updated by this PR. I reproduced this live:
$ uv run kbagent auth login --help
Requires a human at a browser -- an AI agent must not attempt this
headlessly. ...
This flatly contradicts the new policy shipped in setup.md, SKILL.md, keboola-expert.md, CLAUDE.md, context.py's AGENT_CONTEXT, gotchas.md, auth-workflow.md and commands-reference.md, all of which now say an attended agent with a background shell should drive auth login itself. CONTRIBUTING.md's own checklist calls out --help text as "the ultimate fallback when documentation drifts" — this is exactly that drift, on the PR whose entire purpose is closing it. Fix: reword both docstrings to state the background-shell/attended nuance (mirroring the context.py AGENT_CONTEXT wording that was already updated in this same PR).
[B-2] docs/auth.md:8-13 — canonical browser-login doc still says "must never run it on its own initiative"
> AI agent must never run it on its own initiative — if asked to "set up
> kbagent auth", hand the command back to the person and wait for them to
> finish.
docs/auth.md is untouched by this PR, but it is the doc README.md points to for "Details, capability matrix and error codes" (README.md:236), and auth-workflow.md's own header cross-references its section titles. An agent (or human) that follows the README link instead of loading the plugin skill gets the exact opposite instruction from the one this PR ships everywhere else — the class of contradiction the PR description says it fixed. Fix: update the callout box to state the attended/background-shell exception, consistent with auth-workflow.md.
Non-blocking findings
[NB-1] src/keboola_agent_cli/commands/auth.py:314-321 (new verification_uri_complete one-click link) — undocumented in any silent-drift surface
The new device-login panel line ("Or open this link (code pre-filled)") is a genuinely new, user-visible CLI output change, but it is not mentioned anywhere in gotchas.md, commands-reference.md, or AGENT_CONTEXT (context.py). It's covered by CLI-layer tests (tests/test_cli_auth.py), so it isn't undiscoverable, but per the Plugin synchronization map a new observable CLI behavior belongs in gotchas.md tagged (since vNEXT) at minimum, since an agent relaying the panel output verbatim to the user should know the link may or may not be present.
[NB-2] README.md:242-243 — stale relative to the new attended-session policy
"Needs a human at a browser. There is no headless path, so never run auth login from an unattended AI-agent task or a CI step" is not technically false (the unattended prohibition still holds), but it no longer mentions that an attended agent with a background shell should now drive the login itself. A reader who only sees the README (not docs/auth.md, not the plugin skill) is left believing the old absolute rule still applies. Not part of the CONTRIBUTING.md Plugin synchronization map, so lower priority than B-1/B-2, but same root cause.
Nits
[NIT-1]plugins/kbagent/skills/kbagent/references/auth-workflow.md:3-4— the "Audience" line ("a human user of kbagent (or an agent relaying instructions to one)") predates this PR's new attended-agent-drives-it-itself case and reads slightly stale next to the heavily-rewritten body just below it.
Verification log
gh pr view 722 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN, 13 files, +498/-128, conventionalfix(plugin,doctor,auth):prefix (reasonable given issue #704 framing; PR also adds a small feature — acceptable either way) ✓gh pr diff 722→ 919-line diff, reviewed in full ✓git rev-parse --abbrev-ref HEAD→claude/github-issue-704-fix-99659e, matches<branch>✓ (no checkout needed)- Layer-violation greps (typer/click/formatter in services, httpx in commands, formatter/typer in clients) → all empty, no violations (PR is docs + thin doctor/auth message changes) ✓
- Magic-number / raw error-code / bare-except /
print()/ new-tuple-return greps on the diff → all empty ✓ - Token-leakage grep on the diff → only doc prose mentioning "token" in policy text, no real secrets ✓
src/keboola_agent_cli/auth/device.py:71-72→on_prompt(authorization)called immediately afterstart_device_authorization(), strictly before thewhile Truepoll loop — confirms "URL+code printed before polling starts" ✓src/keboola_agent_cli/commands/auth.py:304→target_console = formatter.err_console if formatter.json_mode else formatter.console— confirms "--jsonsends the panel to stderr, human mode to stdout" ✓src/keboola_agent_cli/commands/auth.py:66,505-506→_STATUS_EXIT_3 = frozenset({"expired", "missing"}),raise typer.Exit(code=3)when status is in that set, exit 0 otherwise — confirms the documented exit 0/3 contract exactly (live/refreshed/degraded→ 0;expired/missing→ 3) ✓src/keboola_agent_cli/auth/state_store.py:173-191→os.replace(str(tmp_path), str(self._state_path)), atomic tmp+rename write ✓;services/auth_service.py:391→put_sessioncalled only after tokens are already obtained (post-exchange) — confirms "written atomically on success only" ✓uv run kbagent auth login --help→ live-reproduced B-1: still prints "an AI agent must not attempt this headlessly" ✓grep -rn "never run it yourself\|must never run it on its own initiative\|HUMAN-ONLY" **/*.md src/**/*.py→ founddocs/auth.md:12(B-2), plus benign heading-only matches ingotchas.md/auth-workflow.mdthat are reconciled by their own updated body text ✓make check→6362 passed, 12 skipped, 12 warnings in 71.77s✓uv run pytest tests/test_cli_auth.py tests/test_doctor_service.py -q→112 passed✓make command-sync-check→ "OK: all 267 CLI commands are registered (OPERATION_REGISTRY) and documented" — no new commands added, N/A but confirms no drift introduced ✓make skill-check→ "SKILL.md is up-to-date" ✓wc -c plugins/kbagent/agents/keboola-expert.md→ 54398 bytes, matches PR's claimed 54.4 kB of the 70 kB budget ✓;uv run pytest tests/test_agent_prompt.py -q→42 passed✓make version-gate-check→ "All 334 version gates across 64 versions resolve to a release" ✓; no unresolvedvNEXTin touched files — consistent with the PR's "no CLI-version-gated behavior, no vNEXT needed" claim, modulo NB-1's new panel line ✓
Open questions for the author
(none)
The policy rewrite missed two authoritative surfaces that still carried the old absolute prohibition, contradicting the policy this PR ships everywhere else: - `commands/auth.py` module + `auth_login` docstrings -- the latter is what `kbagent auth login --help` prints, which CONTRIBUTING.md calls the ultimate fallback when documentation drifts. - `docs/auth.md`, the canonical browser-login doc README links as "Details". Both now state the attended/background-shell nuance: a human approves in a browser, an attended agent may drive `--device-code` from a background shell and confirm via `auth status`, never a foreground shell and never unattended. Also: - gotchas.md documents the conditional one-click `verification_uri_complete` panel line, tagged `(since vNEXT)` -- a new observable CLI behavior belongs on a silent-drift surface. - README states the unattended ban without implying the old absolute rule. - The "browser login is human-only" headings in gotchas.md now say "needs a human to approve", with the auth-workflow.md cross-reference updated to match. - Drive-by: docs/auth.md dated `auth login-password` to v0.81.0; the changelog puts it in 0.84.0.
|
All five findings addressed in 27903f5. The two blocking ones were correct and were a scoping miss on my side: the rewrite covered the plugin surfaces and B-1 — B-2 — NB-1 — new panel line undocumented. Agreed it belongs on a silent-drift surface: NB-2 — NIT-1 — Two things beyond the report:
|
…04-fix-99659e # Conflicts: # plugins/kbagent/skills/kbagent/references/gotchas.md
Batches the seven PRs merged since v0.91.0 into one version bump, one changelog entry and one set of resolved version gates: - #719 (#714) `flow triggers` -- table triggers, not just cron - #717 (#711) a 401 is no longer automatically blamed on the token - #722 (#704) setup completes in chat; skill covers setup + logout - #718 (#716) `--conversation-id` global flag - #706 223 stale version gates retired at the 0.80.0 floor - #702 release process enforced rather than remembered - #721 `get_flow_detail` docstring fix Includes a curated What's-new entry (#717's error rework is UI-visible on the Semantic Layer page) and the step 8-11 silent-drift review.
v0.92.0 changed the login flow: the agent now runs the device-code sign-in in a background shell and relays the URL/code into chat for approval, instead of handing off to the user's terminal. Updates the visible text and the provenance comment soustruh flagged as stale. The version bump on _data/cli/command-reference.md (v0.91.0 -> v0.92.0) is intentionally left to connection-docs#1044, which is already doing that sync -- avoids two PRs racing on the same generated file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes #704.
Problem
/kbagent:setupcould not complete in any chat client: step 3a handedauth loginback to the user's terminal, so the documented "one command and you are set up" flow was really "two steps in chat, then go find a terminal anyway" (verified across Claude Code, Cursor and Claude Desktop in the issue). On top of that: Claude Desktop has no slash-command surface at all, Cursor's marketplace add needs an undocumented full URL, anddoctor's plugin check told Cursor users the plugin was missing right after they installed it.Root cause
The "never run
auth loginyourself" rule is documentation-only -- no runtime guard exists -- and it banned more than the hazard it guards against. The real hazards are (a) a foreground tool shell's ~120 s timeout killing the PKCE/device flow mid-flight, and (b) an unattended task with nobody at a browser to approve. Driving the command from a background shell in an attended session was never dangerous: the device flow prints the verification URL + user code before polling starts (flushed),auth statusgives a clean pollable completion contract (exit 0 = signed in, 3 = not yet), andauth.jsonis written atomically only on success, so a killed attempt corrupts nothing.Changes
Login policy (docs, the core fix) -- in an attended session with a background shell, the agent now drives login itself: run
kbagent auth login --device-code --stack <URL> --register-projectsbackgrounded, relay the URL + code into the chat, confirm viaauth statuspolling. Foreground shells and unattended tasks stay banned; terminal handoff remains the fallback where no background shell exists; the orphaned-session guard (checkauth statusbefore any re-run) is unchanged. Updated consistently acrosssetup.md, pluginCLAUDE.md,keboola-expert.md, rootCLAUDE.md,kbagent context,SKILL.mdand the auth/commands/gotchas references.Skill surface for clients without slash commands -- the kbagent skill now carries first-time setup and logout natively: new triggers (
set up keboola,setup,connect project,logout,sign out), a client-agnostic setup ladder in "First-time setup" (instead of deferring to the Claude-Code-only/kbagent:setup), and a "Logging out" section. The description stays under Claude Desktop's 1024-char limit (#447).Plugin framed as an upgrade, not a prerequisite --
kbagent project add+kbagent contextis a complete setup in any client with a shell; aclaude_pluginwarn/skip never blocks declaring setup successful. Stated insetup.md,SKILL.mdand the doctor messages.Per-client marketplace docs -- Cursor requires the full
https://github.com/keboola/ai-kitURL (the short form fails with an opaque[invalid_argument]); Claude Desktop installs via UI and is served by the skill; login is shared across clients via the same local config.Code (small):
doctor_check_claude_plugin: skip/warn messages now say the check only detects Claude Code's cache layout (a Cursor-side install is invisible to it) and point atkbagent contextas the client-agnostic substitute. Statuses, check id and the verbatim/pluginlines are unchanged.verification_uri_complete(one-click, code pre-filled) when the server provides it -- a relayed login becomes a single click.Testing
make checkgreen: 6362 passed, 12 skipped (lint, format, skill-check, version-check, sentinel guards included).test_doctor_service.py(47 pass); new red-first tests for the device panel intest_cli_auth.py(65 pass; full auth cluster 242 pass).keboola-expert.mdat 54.4 kB of the 70 kB budget;scripts/check_command_sync.pyOK (267 commands).No version bump, no changelog entry (per the release-PR-only rule); the policy change is plugin-docs-only and not CLI-version-gated, so no
vNEXTtags were needed.