spend-limit: name the organization in command output - #200
Conversation
The org was only ever in the transient spinner line, so the rendered table and the JSON payload were both unattributed. show/set/clear now render an Organization row, the set confirmations name the org, and --output json carries an `organization` key (setdefault, so a server-supplied field wins if one ever appears).
Regaddi
left a comment
There was a problem hiding this comment.
Small, focused, and does what it says. _with_org with setdefault is the right shape, the if org: guards are harmless (requires_login resolves an org before any of these commands run, so it is never None in practice), and naming the org in the two set confirmation prompts is a real improvement on the destructive paths.
Approving. Three things worth picking up, none blocking.
1. The JSON "no data" sentinel changed, and the PR does not say so
spend_limit_get is called with not_found_is_empty=True, so a 404 returns (None, None) with no error. On that path show --output json previously emitted {} and now emits:
{
"organization": "acme"
}Confirmed against a mocked 404.
The reviewer note frames the risk as "anything doing an exact-dict comparison will see the new key". This is a different break: a consumer doing if not payload: no_data() now takes the wrong branch, because the empty result became truthy. The human path handles this case with its own explicit message, so only JSON consumers are affected.
"limitCents" in payload still separates the two states, so it is recoverable, but the CHANGELOG entry and the reviewer note should say it out loud. The alternative is to keep emitting {} when data is None in show, and only merge the org into a real payload.
2. set and clear JSON output has no test
Both call sites moved to _with_org(data, org), but the new tests only cover show. Given that the point of the PR is that all three commands attribute their output, at least one of those two deserves an assertion.
3. "pyright clean" in the test plan is not accurate
uv run pyright reports 90 errors on origin/main and 91 on this branch. The extra one is the new test calling show(...) without awaiting (reportUnusedCoroutine, tests/test_spend_limit.py:265). It matches the eight pre-existing occurrences in that same file and pyright is not in CI, so it is not a problem in itself, but the claim should be corrected rather than left standing.
Verified locally
- 443 tests pass.
ruff format --diffandruff checkclean.- Rendered the table by hand: the
Organizationrow lands first, aboveLimit.
Nit
_render_show(data, org=None) keeps a default that renders no Organization row. Every call site passes an org, so the default exists only to keep the old TestRenderShow cases compiling. Making it required would let the signature enforce the invariant this PR is establishing.
Review follow-up. _with_org decorated a None payload, so the 404 path
(spend_limit_get uses not_found_is_empty) emitted {"organization": ...}
where it used to emit {}. That flipped the no-data sentinel from falsy
to truthy for JSON consumers. Only merge the org into a real payload.
Also cover set/clear JSON output, which moved to _with_org untested, and
make org a required parameter of _render_show now that every call site
passes one.
|
Docs automation failed for this PR — its documentation is missing. The Re-run it once the cause is addressed: If this PR genuinely needs no documentation, remove the |
Summary
pipecat cloud spend-limitnever said which organization the numbers described. The org appeared only in the transient spinner line, which disappears, so both the rendered table and the JSON payload were unattributed. That matters as soon as-o/--organizationis used or more than one org is in play.Changes
_render_showtakes the org and renders an Organization row as the first row, soshow,setandclearall attribute their output. The parameter is required, so the signature enforces the invariant.--output json(and the deprecated--jsonalias) emits anorganizationkey. The API response does not carry one, so the CLI fills it from the resolved org via a new_with_orghelper. It usessetdefault, so a server-suppliedorganizationwins if the field ever appears.setname the org. These are the invocations that block sessions, so the target is worth pinning before you hit enter.Note for reviewers
Adding
organizationto the JSON output is an additive change to a machine-readable shape. Anything doing an exact-dict comparison onspend-limit show --jsonwill see the new key.The empty payload is deliberately left alone.
spend_limit_getpassesnot_found_is_empty=True, so an org with no spend-limit record yields(None, None)andshow --output jsonemits{}._with_orgonly decorates a real payload, so that stays falsy andif not payload:keeps working. Covered by a test.Test plan
organizationis not overwritten; the 404 path still emits{};setandclearattribute their JSON.ruff format --diffandruff checkclean.setandclear.pyrightis not clean on this repo and is not in CI: 90 errors onmain, 92 here. The two additions arereportUnusedCoroutineon the new tests calling the@synchronizer.create_blockingwrapper withoutawait, matching the eight pre-existing occurrences in that same file. (An earlier version of this description claimed "pyright clean"; that came from running it scoped to the one changed source file.)🤖 Generated with Claude Code