feat(billing): kbagent billing credits — PAYG balance (#594 secondary ask) - #597
Conversation
Wraps `GET /credits` on the `billing.{stack}` host, which accepts a plain
per-project Storage token -- the endpoint was reachable all along and simply
had no command over it. Multi-project parallel fan-out by default, `--project`
narrows, `--json` emits `{"credits": [...], "errors": [...]}`.
Three details the API forces:
- Units. The API speaks credits; the Keboola UI displays minutes = credits x 60.
Rows carry the native unit AND derived minutes, and the conversion only ever
runs in that direction, so a unit bug cannot invert into a wrong credit figure.
- Shape. `stats.workspaceJobs` is an ARRAY in the live payload, not the object
the public docs show, so the pydantic models are tolerant (`extra="allow"`,
every field defaulted) rather than strict-against-the-docs.
- Availability. A project without the `pay-as-you-go` flag in `owner.features`
is gated out BEFORE any billing request and reported as the new
`PAYG_NOT_AVAILABLE` code. This is deliberately not a mapped billing 4xx: on
non-PAYG stacks the service index advertises a `billing.` host that does not
resolve, so an ungated call surfaces a DNS failure instead of the real reason.
Per-project failures degrade individually and never abort the run. Mirrored on
`kbagent serve` as `GET /billing/credits`.
Read-only by design: the billing service's `POST /credits` triggers a real
automatic top-up (real money) and is wrapped by nothing in kbagent -- the client
mixin exposes GET only, and there is a regression test guarding that.
Not included, and tracked by #594's still-open primary ask: credit PURCHASE
history and the Stripe invoice IDs Keboola already stores per project. That data
lives on `connection.{stack}` under `/pay-as-you-go/billing/*`, which ignores
`X-StorageApi-Token` entirely and rejects it as a bearer with 401, so no CLI,
script, or scheduled agent can reach it today.
0.85.0 is reserved for the epic #390 `tool`-group removal; taking that number for an additive feature would have made the removal announcement in gotchas.md / context.py / mcp_parity.py read as already-shipped. Renumbers the changelog key, pyproject, and every `(since vX.Y.Z)` doc tag this PR introduced -- the #390 references to v0.85.0 stay untouched.
padak
left a comment
There was a problem hiding this comment.
Review of #597 — kbagent billing credits (PAYG balance)
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 adds kbagent billing credits, a read-only multi-project PAYG credit-balance
lookup, wired straight through all three layers plus the kbagent serve REST mirror,
with the two safety invariants called out in the review focus (no POST to the
billing host anywhere; the PAYG feature gate runs before every billing-host network
call, on both the CLI and serve paths) genuinely enforced rather than merely
documented. The implementation traces closely to issue #594's proposed scope,
including the exact verbatim live payload used as a test fixture. Verdict: APPROVE.
No blocking findings. A couple of small non-blocking notes below (one of which — a
version-number collision with the epic #390 removal — was already caught and fixed
by a follow-up commit pushed to this branch during review, so it is noted only as
confirmed-resolved, not re-raised as live).
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/client/_core.py:466 — money guardrail is enforced by a source-scan test on client/billing.py only, not by the generic _billing_request dispatcher
_billing_request(self, method: str, path: str, **kwargs) in _core.py accepts an
arbitrary HTTP method — it is not billing-specific plumbing, it is the same shape as
every other sub-client dispatcher (_sync_actions_request, etc.). The regression
guard in tests/test_billing_client.py::TestNoTopUpHelper correctly checks that
_BillingMixin exposes only get_credits and that client/billing.py's own source
never contains the string "POST", which is a real, non-decorative guard against
today's code. But nothing stops a future PR from calling
client._billing_request("POST", "/credits") directly from a service or another
mixin — the guard would not catch that, since it only inspects billing.py. This is
a pre-existing pattern shared by every sibling sub-client (sync-actions, workspaces,
…), so it is not a defect introduced by this PR, just a gap the money-guardrail
framing invites scrutiny on. Consider a follow-up: either a repo-wide grep-based
guard for _billing_request\(.*"POST" across src/, or renaming _billing_request
to _billing_get (drop the method parameter) so the type signature itself makes a
POST impossible to construct.
[NB-2] plugins/kbagent/skills/kbagent/references/billing-workflow.md:176-180 — hardcoded observed price ("$8.40 ex. VAT per credit") risks going stale
The "Money is a separate axis again" paragraph bakes in a specific dollar rate
observed on one contract in issue #594. PAYG pricing is contract/stack-specific and
not returned by the API at all (the doc itself says "the CLI has no price field").
An AI agent reading this reference verbatim could quote that figure to a user on a
different contract as if it were universal. Consider rephrasing to make explicit
that the number is a single historical observation, not a platform constant (the
surrounding units explanation — credits vs. minutes — is the part that's actually
invariant and worth keeping verbatim).
Nits
[NIT-1]src/keboola_agent_cli/commands/billing.py:564-582—_format_credits_table's human-mode table omitscomponent_jobs_consumedandworkspace_jobs; both are present in--json. Not required (CONTRIBUTING only asks for "informative" human output, not field parity), but a--verbose-gated breakdown row would match the richness of the JSON envelope for a human operator debugging a workspace-heavy bill.[NIT-2]src/keboola_agent_cli/errors.py:708—ErrorCode.PAYG_NOT_AVAILABLE: "configuration"is added to_ERROR_CODE_TO_TYPE, but the only place this code is currently emitted (billing_service.py) puts it in the per-projecterrorslist rendered viaformatter.warning(), never throughformatter.error()/map_error_to_exit_code. The mapping is harmless (correct classification if the code is ever raised through the error path) but is currently unreachable dead code from this PR's own call sites — worth a one-line note in case a future reviewer wonders why it's unused.
Verification log
Read CONTRIBUTING.md(Checklist: Adding a New CLI Command; Plugin synchronization map; Releasing a new version) ✓Read CLAUDE.mdconvention #17 +## All CLI Commands(billing entry present, correctly documented) ✓Read plugins/kbagent/agents/keboola-expert.md§1 rules; confirmed file is 61,985 bytes against the 62,000-byte hard cap — corroborates the PR description's claim that no room existed for even a one-line pointer; skip is legitimate, not an oversight ✓gh auth status→ authenticated aspadak,repo/workflowscopes ✓gh pr view 597 --json ...→ OPEN, basemain, +1645/-9, 30 files touched ✓git rev-parse --abbrev-ref HEAD→claude/issue-594-popis-2a155a(matches<branch>) ✓gh pr diff 597fetched twice: once mid-review at HEAD1e6db7a(version 0.85.0), then re-fetched at HEAD0e8b10dafter a renumber commit landed mid-review (version 0.84.2, collision with epic #390's reservedtool-removal version resolved). All findings below are against the final, current HEAD0e8b10d✓- 3-layer compliance greps (typer/formatter in services, httpx in commands, formatter/typer in clients) → all empty, no violations ✓
- Convention greps (magic numbers, raw
error_code="..."outside tests, bareexcept:,print(), token leakage, new bad tuple returns) → clean; the oneerror_code="API_ERROR"hit is intests/test_billing_service.py, whichcheck_error_codes.pyexplicitly exempts ✓ services/billing_service.pyworker returnstuple[str, dict, ...]/tuple[str, dict]— matches the grandfatheredBaseServiceparallel-result convention (base.py::_run_parallel), not a new bad tuple ✓- Read
services/base.py::_run_parallelto confirm the 2-tuple-vs-3+-tuple success/error discriminatorBillingServicerelies on ✓ - Read
client/tokens.py::has_feature/get_project_features— confirmed it callsverify_token()(Storage API, not billing host) and caches per client instance, so the PAYG gate genuinely completes before any billing-host call, in both the CLI (commands/billing.py→BillingService.get_credits) and serve (server/routers/billing.py→ sameBillingServiceinstance, no separate implementation) paths ✓ - Read
tests/test_billing_client.py::TestNoTopUpHelper— confirmed the money-guardrail regression test checks (a)_BillingMixinexposes exactly one public method, (b) the module source contains no"POST"/'POST'literal, (c) an actualhttpx_mock-backed call never issues a POST — a real guard, not a name-only assertion ✓ - Cross-checked
models.pyProjectCredits/CreditStats/WorkspaceJobCreditsagainst the verbatim payload in issue #594's body (fetched viagh issue view 594) —workspaceJobsarray shape, tolerantextra="allow", and the credits→minutes-only derivation direction all match the issue's acceptance criteria exactly ✓ - Confirmed
_build_credit_rowinservices/billing_service.py:938-970derivesconsumed_minutes/remaining_minutesascredits * MINUTES_PER_CREDITonly (never the reverse) ✓ make check(background run) → exit 0,5604 passed, 12 skipped, 150 deselected— matches the PR description's own reported numbers ✓- Live CLI smoke test:
kbagent --json billing credits --help→ renders correctly;kbagent --json billing credits --project pabu --project padakagainst two real registered (non-PAYG-verified, static-token) projects → well-formed{"credits": [], "errors": [...]}envelope, exit 0,INVALID_TOKENsurfaced per-project rather than aborting (tokens are stale/rotated on this box, so the PAYG gate itself wasn't exercised live, but the degrade-individually contract was) ✓ - Verified
billing.credits: "read"inpermissions.pyOPERATION_REGISTRY,check_cli_permission(ctx, "billing")incommands/billing.py, 1:1 REST mirror inserver/routers/billing.py, and all six hand-maintained plugin-sync surfaces (context.py,CLAUDE.md,commands-reference.md,gotchas.md, newbilling-workflow.md,SKILL.mddescription trigger + decision table + workflow-link table) present and consistent ✓ - Verified version consistency across
pyproject.toml,plugin.json,marketplace.json,uv.lock,changelog.py(0.84.2, correctly ordered newest-first above 0.84.1) at the final HEAD, and confirmed epic #390's pre-existing0.85.0references inmcp_parity.py/context.py/gotchas.mdwere left untouched by the renumber ✓
Open questions for the author
(none)
…ype level
NB-1: `_billing_request(method, path)` becomes `_billing_get(path)`. The
source-scan regression test only covered `client/billing.py`, so a future
caller elsewhere could still have constructed `_billing_request("POST",
"/credits")` — a real-money automatic top-up. Hardcoding the verb makes that
unconstructible, which no source-scanning test can match. Deliberately breaks
symmetry with the `_queue_request` / `_sync_actions_request` siblings; the
docstring says why. Pinned by a new test asserting the dispatcher is gone and
the replacement takes no `method` parameter.
NB-2: reframe the observed $8.40/credit rate in billing-workflow.md as a single
historical observation rather than a platform constant, and tell the reader not
to quote it — price is contract-specific, the API never returns it, and an agent
reading the doc verbatim could otherwise state it as a user's own rate.
NIT-2: note why PAYG_NOT_AVAILABLE is classified in _ERROR_CODE_TO_TYPE despite
being unreachable from today's only emitter (per-project errors render through
formatter.warning(), not formatter.error()).
NIT-1 (human table omits the per-workspace breakdown) not taken: adding a
verbose breakdown row is scope beyond #594's ask, and --json already carries
both fields.
|
Review addressed in
|
Devin review: `consumed + remaining` equals the amount purchased only if every credit ever added is either still available or already consumed, so against expired, revoked, or promo credits the field would silently under- or over-report while looking authoritative -- and the human table's "Purchased" column header asserted it hardest. The name was worse than a generic inaccuracy in this specific command: credit PURCHASES are exactly what #594 is about not being able to reach. Someone reconciling Stripe invoices could have taken this derived figure for the purchase total the CLI explicitly cannot produce. Renamed everywhere (row key, table column, docs, tests) with the assumption now stated at the derivation site, in the command help, and in billing-workflow.md. Unreleased, so no consumer contract breaks.
Main already carried an unreleased 0.84.2 (billing credits #597, config state-get/state-set #598, kbc->kbagent CI/CD skill #402), and v0.84.1 is the newest published release. Folding clone into that same unreleased version ships one release instead of two, and leaves 0.85.0 free for the `tool` group removal it is already promised to (epic #390 phase 3) -- the doc references to that removal deliberately still say 0.85.0. Version files, the changelog key (clone notes merged above the existing 0.84.2 entries) and the since-tags in CLAUDE.md, context.py, gotchas.md, commands-reference.md, keboola-expert.md and the E2E docstring all move to 0.84.2.
…docs Opens 0.84.3: v0.84.2 is tagged and published at main's HEAD, so there is no in-progress key to append to, and the repo's convention is that the substantive PR carries the bump (0.84.2 <- #594/#597, 0.84.1 <- #589, 0.84.0 <- auth login-password, ...). Neither `changelog-check` (audits that released versions have entries) nor `version-check` (plugin.json / marketplace.json / uv.lock vs pyproject) would have caught the omission -- the silent drift convention #17 warns about. The behaviour change is user-visible, so it also lands in gotchas.md tagged (since v0.84.3). Tests: the PR claimed poll counts are unchanged for every budget but nothing pinned it. test_timeout_raises_storage_job_timeout now records sleeps and asserts none happened -- verified that moving the deadline check after the sleep makes it fail (assert [1.0] == []) where before it merely ran a second slower, since the break still precedes the fetch. Adds test_budget_below_one_interval_still_polls_once for the other half of the claim (0.5s budget -> exactly one poll, overshoot preserved), and test_polled_success_returns_the_polled_body: the happy path was covered only incidentally, by a fixture that returns a terminal body and never enters the loop. Docstring: the "same shape as the sibling pollers" line read as a parity claim. Narrowed -- the check-then-fetch shape matches, the behaviour does not: this poller knows only success/error (so any other terminal status would exhaust the budget and surface as STORAGE_JOB_TIMEOUT, where the queue poller keys off isFinished), and its sleep is not capped to the remaining budget. Both predate this branch. _mk_client is now one module-level helper instead of two byte-identical methods 62 lines apart (the only two in the suite). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the secondary ask of #594. The primary ask (purchase history + Stripe invoice IDs) stays open — see "What this deliberately does not do" below.
Why
GET /creditson thebilling.{stack}host accepts a plain per-project Storage API token. It was reachable all along and simply had no command over it —rg -i "credits|payg|billing" src/ tests/returned zero matches before this PR. Anyone running more than one PAYG project had no scriptable way to read a balance.What lands
Multi-project parallel fan-out by default,
--project(repeatable) narrows,--jsonemits{"credits": [...], "errors": [...]}. Mirrored onkbagent serveasGET /billing/credits.Three details the live API forces, each of which would be a bug if guessed from the public docs:
Units. The API speaks credits; the Keboola UI displays minutes = credits × 60. Rows carry the native unit and derived minutes, and the conversion only ever runs in that direction, so a unit bug cannot invert into a wrong credit figure.
Shape.
stats.workspaceJobsis an array in the live payload, not the object the public docs show. The pydantic models are therefore tolerant (extra="allow", every field defaulted) rather than strict-against-the-docs — a strict model would raise on every real response.Availability. A project without the
pay-as-you-goflag inowner.featuresis gated out before any billing request and reported as the newPAYG_NOT_AVAILABLEcode. This is deliberately not a mapped billing 4xx: on non-PAYG stacks the service index advertises abilling.host that does not resolve, so an ungated call would surface a DNS failure instead of the actual reason. Per-project failures degrade individually; the run never aborts.Money guardrail
The billing service also exposes
POST /credits, which triggers a real automatic top-up. Nothing in kbagent wraps it: the client mixin exposes GET only, and a regression test asserts the mixin issues no POST and has no other public method. If it is ever exposed it belongs in its own PR with--dry-run/--yesand its own decision.What this deliberately does not do
Credit purchase history and the Stripe invoice IDs Keboola already stores per project. That data lives on
connection.{stack}under/pay-as-you-go/billing/*, which ignoresX-StorageApi-Tokenentirely (a Storage-token request returns the byte-identical 302-to-login as an unauthenticated one) and rejects it as a bearer with 401. No CLI, script, or scheduled agent can reach it, so anyone funding several PAYG projects on one billing identity still cannot answer "which project does this Stripe invoice belong to" without matching on(date, amount). #594 asks maintainers whether such an endpoint already exists undocumented, whether/pay-as-you-go/billing/*would accept akbc_at_*session bearer, or whether a newGET /credits/purchasesis needed — that answer is still pending, and the command that consumes it is a follow-up PR.Layers touched
client/billing.py(_BillingMixin.get_credits),client/_core.pybilling sub-client plumbing via the existing_derive_service_url(stack_url, "billing")— no hardcoded hostnamesmodels.py:ProjectCredits/CreditStats/ComponentJobCredits/WorkspaceJobCreditsservices/billing_service.py(BillingService.get_credits)commands/billing.py,cli.py,permissions.py(billing.credits→read)server/routers/billing.py,server/dependencies.py,server/app.pyCLAUDE.md,commands/context.py,commands-reference.md,gotchas.md, newreferences/billing-workflow.md,SKILL.mdplugins/kbagent/agents/keboola-expert.mdwas not updated: it sits at 61,985 bytes against a 62,000-byte hard cap, leaving 15 bytes — not enough for even a one-line pointer. Flagging rather than silently skipping; the other agent surfaces (kbagent context, SKILL.md, gotchas, the new workflow doc) all carry it.Verification
make checkclean (lint, format, typecheck, skill, version, command-sync, changelog, error-codes, sentinel-guards, loc, tests) — 5604 passed, 12 skipped.tests/test_e2e.py.e2e-snowflake5946,e2e-bigquery6100) via a standalone harness. Neither is PAYG-enabled, so this exercised the degradation contract end to end: well-formed envelope, exit 0, no project silently dropped, both reported asPAYG_NOT_AVAILABLE— specifically not a connection/DNS code, which is the proof the feature gate fires before the network call.--projectnarrowing and the exit-5 unknown-alias path also verified live.purchased == consumed + remaining, minutes derivation,workspace_jobsas a list) against a real PAYG payload — no PAYG project is reachable from any local config. Those are covered by unit tests using the verbatim payload captured in billing: no CLI path to PAYG credit purchases and their Stripe invoice IDs;/pay-as-you-go/billing/*is session-only #594, and the harness asserts them conditionally the moment a PAYG project is registered.Version
Bumped to 0.84.2 with a changelog entry. Deliberately not 0.85.0: that number is reserved for the epic #390
tool-group removal, and taking it for an additive feature would make the removal announcement ingotchas.md/context.py/mcp_parity.pyread as already-shipped.