feat(token): add token list, stop retrying non-idempotent writes (#599)

[padak]

Closes #599.

What this fixes

Issue #599 reported a persistent upstream 500 from POST /v2/storage/tokens on the europe-west3.gcp stack, and flagged two client-side gaps it exposed. Both are real. The first turned out to be worse than the reporter could see from the outside.

1. Non-idempotent writes were retried on 5xx

BaseHttpClient._do_request retried on 429/500/502/503/504 regardless of HTTP method. Reading Keboola's Storage_Service_Tokens::createToken shows why that is unsafe for a write:

$newToken = $this->_accessTokensTable->createRow($tokenData);
$newToken->save();                                    // <-- token already persisted
$newToken->refreshToken(...)->fillAdminOwner()->save();

if ($bucketPermissions !== null && !$canManageBuckets) {   // <-- can throw
    $bucketIdsMap = $this->getAllProjectBucketIdsMap(...);
    $container = $bucketPermissions->evaluate(...);
    ...
}

The token row is saved and its secret generated before the block that can throw, and unlike updateToken directly below it, createToken wraps none of this in a transaction. A 500 raised there leaves a live token behind that the caller never sees — so three attempts could leave three of them. The issue framed the duplicate-mint risk as hypothetical ("if the API ever creates the token and then fails to respond"); that branch is reachable.

Changes:

• 5xx is retried only on GET/HEAD/OPTIONS/PUT/DELETE (new RETRY_SAFE_METHODS, RFC 9110 idempotent set).
• 429 is still retried on every method — the server is stating it did not process the request, so repeating it is safe.
• Transport failures split by what they prove. A refused connection or a connect/pool timeout never delivered the request and is still retried on any method. A read/write timeout means the request was sent and the outcome is unknown, so it is not retried on POST/PATCH. This second path is the same hazard as the 500 one, only quieter — fixing just the status-code branch would have left it open.
• A 500 on an unretried write now reports retryable: false, so callers do not treat it as a transient blip.

This covers all 29 POST call sites, not only the token mint — config oauth-url mints a token through the very same endpoint.

2. A 500 gave no next step, and threw away the one useful field

_raise_api_error picked up Keboola's generic error: "Application error." and dropped the exceptionId sitting next to it — the only handle Keboola support can trace an incident by. That is now surfaced, along with one of two hints:

• not retried — the method is not idempotent, so verify the resource state before repeating;
• retried and exhausted — the same 5xx survived every attempt, which is an upstream incident: check status.keboola.com and escalate with the exceptionId.

4xx is untouched: the incident hint would be actively misleading there.

This also answers the issue's open question about the escalation channel — the exceptionId is what support needs.

3. kbagent token list (the issue comment)

The token group could mint, revoke and rotate but not enumerate, so there was no way to obtain the --token-id that delete/refresh require without going through the web UI.

It ships here rather than separately because it is also the check the new retry behaviour tells you to run: after an unretried mint failure, token list is how you find out whether the token was created anyway.

Secrets are stripped from every row, --json included. On a project carrying the force-decrypted-token feature, tokenToApiResponse embeds each token's live value in the listing; reproducing that would break the group's "revealed once, at mint" contract for every token in the project at once. Stripped at the service layer and again in the SDK facade.

Full surface: KeboolaClient.list_tokens(), TokenService.list_tokens(), the CLI command, GET /token/{project}/list on serve, token.list: read in the permission registry, and Client.list_tokens() -> list[TokenListEntryResult] on the SDK.

Not in scope

The upstream 500 itself. Worth recording what I found while triaging it, since it narrows the search for whoever picks it up:

• The endpoint was not down. Datadog (env:com-keboola-gcp-europe-west3) has POST https://connection.europe-west3.gcp.keboola.com/v2/storage/tokens "HTTP/1.1 200 OK" from mcp-server at 2026-08-18 08:58:43 UTC — 14 minutes after the issue was filed.
• That caller sends canManageBuckets: true, which is exactly the flag that skips the getAllProjectBucketIdsMap / BucketPermissions::evaluate block. kbagent never sets it, so it always enters that block.
• In the storage-token path (TokenCreateProcessor::process) a Storage_Service_TokensException from that block is not caught — unlike the manage-token path (ProjectTokensService), which converts it into a validation error. An uncaught exception is exactly a generic 500.

I could not find the reporter's failing request in the logs, so this is a hypothesis from reading the source, not a confirmed cause.

Testing

• 15 new tests in tests/test_http_base.py covering the method gate (POST/PATCH not retried on 5xx; PUT/DELETE still are; 429 retried on POST), the timeout split, and all three message shapes.
• New TestListTokens in test_client_device_enrollment.py, test_token_service.py, test_token_cli.py, test_lib_device_enrollment.py — including that no layer lets a secret through.
• E2E: token list inserted into the existing create -> refresh -> delete flow, asserting the minted token appears and no row carries a secret.
• Four existing tests registered three mock responses for a single POST and now register one — a stricter assertion of the new behaviour, not a workaround.
• make check green.

Note for the release PR

No version bump here — this is one of a stack of PRs going out as a single release. The (since v0.86.0) tags in gotchas.md, commands-reference.md and docs/sdk.md assume 0.86.0; the bump PR must confirm that number and adjust them if it differs.

keboola-expert.md is now 61257 bytes against the 62000 cap — roughly 740 bytes of headroom left.

---

[devin-ai-integration]

Devin Review found 2 potential issues.

View 1 additional finding in Devin Review.

[padak]

Review of #616 — feat(token): add token list, stop retrying non-idempotent writes (#599)

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 PR does three related things for issue #599: (1) stops retrying POST/PATCH on 5xx responses (only GET/HEAD/OPTIONS/PUT/DELETE + 429 stay retryable, per RFC 9110 idempotent-method semantics), splitting timeout retries the same way; (2) surfaces the upstream exceptionId plus an actionable hint on 5xx errors; (3) adds kbagent token list (client/service/CLI/server-router/SDK/permissions), with secrets stripped at every layer. Documentation and silent-drift surfaces (context.py, CLAUDE.md, keboola-expert.md, gotchas.md, commands-reference.md, SKILL.md, docs/sdk.md, permissions.py) are all updated and version-tagged (since v0.86.0). make check passes clean (5597 passed, 0 failed) and scripts/check_command_sync.py reports all 259 commands registered/documented. Verdict: APPROVE — no blocking issues found; a couple of small non-blocking notes below.

Verdict

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

Blocking findings

(none)

Non-blocking findings

[NB-1] CLAUDE.md:590 (release-time risk) — (since v0.86.0) tags depend on an unconfirmed future version number

pyproject.toml is still 0.85.0 on this branch, but every new gotcha/reference tag in this PR (gotchas.md:3713, gotchas.md:3754, commands-reference.md:203, keboola-expert.md:173,179) says since v0.86.0. The PR description explicitly flags this ("the bump PR must confirm that number and adjust them if it differs") — so this is not an oversight, but it is a concrete, trackable risk that the version-bump PR must not skip: if the eventual bump lands as e.g. 0.86.1 or a different stacked PR grabs 0.86.0 first, all five tags need a follow-up edit or they mislead the AI agent about the version floor. Recommend the release PR checklist explicitly greps for v0.86.0 in plugins/ before tagging.

[NB-2] src/keboola_agent_cli/client/merge_requests.py:318 (out of scope for this PR, worth a follow-up issue) — RETRY_SAFE_METHODS assumes every PUT in the codebase is a resource replacement, but merge_requests.py uses PUT .../merge, PUT .../request-review, and PUT .../approve as action-transition endpoints, not idempotent resource PUTs

This file is untouched by the current diff, so the behavior here is unchanged (it was already unconditionally retried on 5xx before this PR, and stays so now since PUT is in RETRY_SAFE_METHODS). Flagging only because this PR's new abstraction (RETRY_SAFE_METHODS = "RFC 9110 idempotent set") now formally documents PUT as safe-to-repeat everywhere, and merge_requests.py's action-style PUTs are the one place in the client layer where that classification is closer to POST-shaped ("merge this MR") than to "replace this resource". Worth a follow-up look (not this PR) at whether those three calls should be excluded from the retry-safe set the same way POST/PATCH now are.

Nits

• [NIT-1] PR title/commit — the single commit bundles a feat (new token list command) with what is really a fix (non-idempotent-write retry regression, arguably the more significant change of the two). The PR description justifies bundling them ("it is also the check the new retry behaviour tells you to run"), which is a reasonable call, but a fix(http): prefix would have been a more accurate headline for the retry-safety change if these ever need to be cherry-picked independently.

Verification log

• gh pr view 616 --json title,body,files,additions,deletions,... → 27 files, +886/-53, title feat(token): add \token list`, stop retrying non-idempotent writes (#599)` ✓
• git rev-parse --abbrev-ref HEAD → claude/issue-599-855b69, matches <branch> input ✓ (working tree already on the PR's branch, no checkout needed)
• Read CONTRIBUTING.md (Checklist: Adding a New CLI Command, Plugin synchronization map, Releasing a new version) ✓
• Read CLAUDE.md convention #17 + ## All CLI Commands (token list present at the documented line) ✓
• Read plugins/kbagent/agents/keboola-expert.md §1, §2 (existing token matrix row covers list — no new row needed since it's an addition to an existing group, not a new group), §3 (two new gotcha triggers added) ✓
• gh auth status → authenticated as padak, scopes include repo ✓
• 3-layer grep (typer/formatter in services, httpx in commands, formatter/typer in clients) → all empty, no violations ✓
• Silent-drift grep of git diff main...HEAD -- cli.py commands/**/*.py for new @*_app.command → only token list; cross-checked against context.py AGENT_CONTEXT (present), CLAUDE.md (present), commands-reference.md (present), gotchas.md (present, version-tagged), permissions.py OPERATION_REGISTRY ("token.list": "read" present), server/routers/token.py (new GET /{project}/list route present, 1:1 with CLI) ✓
• uv run python scripts/check_command_sync.py → OK: all 259 CLI commands are registered (OPERATION_REGISTRY) and documented (CLAUDE.md, context.py, commands-reference.md). ✓
• wc -c plugins/kbagent/agents/keboola-expert.md → 61257 bytes, matches the PR description's claimed byte count exactly (62000 cap, ~740 bytes headroom) ✓
• Convention greps (magic numbers, raw error-code literals in src/, bare except:, print() in production code, token leakage in logs, new bare tuple[...] returns) → all clean; the one raw error_code="ACCESS_DENIED" hit is in tests/test_token_cli.py, which make check-error-codes deliberately excludes (test assertions, not production code) ✓
• make check (lint + format + typecheck + skill freshness + version sync + command-sync + changelog-check + error-codes + sentinel-guards + full test suite) → 5597 passed, 12 skipped, 152 deselected, 2 warnings, exit 0 ✓ (ran in background, ~2m02s; output confirms no lint/typecheck/format failures preceded the test run since make check is sequential-and-exits-on-first-failure)
• Reviewed http_base.py retry-gate logic line by line: RETRY_SAFE_METHODS gate on 5xx, 429 exempted, ConnectTimeout/PoolTimeout treated as "never delivered" (still retried on any method) vs. read/write TimeoutException treated as "outcome unknown" (gated), exceptionId extraction + truncation-ordering (hint appended after body truncation, so it always survives) — all internally consistent with the stated design in the PR description and docstrings ✓
• Spot-checked existing PUT call sites across client/ (storage_tables.py, configs.py, merge_requests.py) for whether "PUT stays retry-safe" holds semantically — mostly true (state replacement, sharing toggle); one soft exception noted as NB-2, out of this PR's diff ✓
• Could not run make test-e2e (no E2E_API_TOKEN/E2E_URL in this environment, and per project convention AI agents never handle live API tokens) — relied on the new test_scoped_token_mint_rotate_revoke E2E test's structure (reviewed, not executed) plus the 15 new test_http_base.py unit tests and the service/CLI/lib mock-based tests, all of which did execute and pass under make check

Open questions for the author

(none)

[padak]

Review findings addressed (1865e13)

Three findings from Devin Review, two of them fixed in code. Recording the third here because it is only visible inside the Devin app, not on this PR — it never made it to a GitHub thread, and it was the most serious of the three.

🔴 Security: server-controlled exceptionId bypassed the length/markup guard

(Insecure Output Handling · CWE-117 · http_base.py:420 — Devin app only)

_raise_api_error read exceptionId off an untrusted response body and appended it after the MAX_API_ERROR_LENGTH truncation this codebase applies explicitly "to prevent Rich markup injection and excessive output". Human-mode errors render through Rich with markup enabled (OutputFormatter.error, output.py:183), so a bracket-laden or unbounded value from a misbehaving endpoint reached the terminal as markup, and embedded newlines could forge extra log lines.

What makes this worth writing down: the code comment I wrote next to it asserted "the id and the hint are kbagent-authored and short". That is true of the hint and false of the id — and the wrong premise is exactly what hid the hole. A comment stating an invariant that does not hold is worse than no comment.

Fixed with _safe_exception_id: drops everything outside [A-Za-z0-9._:-] and caps at MAX_EXCEPTION_ID_LENGTH (128; real Keboola ids run ~70 chars). Dropping rather than escaping keeps a genuine id byte-identical and still leaves support a usable handle if a value is partially mangled. The comment now states the invariant that actually holds: only self-bounded strings may be appended past the truncation.

🟡 Bug: a rate-limited write got the wrong recovery advice

Fixed and answered in thread. The counter was wrong as well as the ordering, so both were fixed — a GET that saw 429, 500, 500 used to report "the same 5xx came back on all 3 attempts".

🔍 Analysis: the gate changes behaviour for read-shaped POSTs

Deliberate, answered in thread, with two corrections to the specifics: the auth endpoints are a case where the new behaviour is wanted (single-use grant exchanges), and flow validate fetches its schema over GET. Real cost is two AI-service reads.

From /kbagent:review (APPROVE, 0 blocking)

• (since v0.86.0) tags depend on the version bump — already flagged in the PR description; the bump PR confirms the number.
• RETRY_SAFE_METHODS treats all PUT as retry-safe, which is imprecise for client/merge_requests.py's action-style PUT .../merge / .../approve. Correctly scoped as out-of-diff and pre-existing (every method was retried before this PR, so nothing regresses here). Filed as a follow-up rather than widened into this PR.

Tests: 5 new (test_rate_limited_then_500_on_a_post_warns_about_partial_effect, test_exhausted_hint_counts_server_errors_not_total_attempts, test_exception_id_is_length_capped, test_exception_id_markup_and_newlines_stripped, test_non_string_exception_id_ignored). 5599 pass, make check green.