Repository navigation
fix: bug sweep — confirmation prompts, env errors, JSON output, revocation endpoint, device polling - #26
Merged
Conversation
…ation endpoint, device polling Five small, self-contained correctness fixes: 1. Confirmation prompts were unreachable dead code on destructive commands (auth logout, client delete, client revoke, vault delete): confirm defaulted to True and --confirm/-y could only set it True, so typer.confirm() never fired. The flag now defaults to False, matching its 'Skip confirmation prompt' help text. logout only prompts when credentials exist, keeping the not-logged-in path a promptless no-op. 2. An invalid ENV/CAMPUS_ENV value raised ValueError out of config.auth_url wherever a command first touched it (auth status, auth login, logout revocation), surfacing as a raw traceback. All auth-URL resolution now goes through common.resolve_auth_url(), which prints an actionable error and exits 1. 3. 'auth refresh --json' and 'auth status --json' printed JSON via console.print(), whose word wrap splits long values mid-string and corrupts machine-readable output. Both now write via typer.echo(). 4. logout revoked tokens against the currently targeted endpoint instead of the endpoint that minted them, so after a target switch revocation silently failed and the old tokens stayed live. Revocation now goes to the stored issuing endpoint (revoke_token gained an auth_url parameter, defaulting to the current target). 5. Device-flow polling slept longer only on the attempt right after a slow_down (RFC 8628 §3.5 wants the raised interval to persist), and a zero/absent server interval zero-divided on the max_attempts computation. The raised interval now persists and a zero interval is clamped, with 5s as the absent-interval default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sweep of five small, self-contained bugs found by reading the CLI end to end. No API/server-side changes; each fix is local to campus-cli with tests.
Bugs fixed
1. Confirmation prompts were dead code
auth logout,client delete,client revokeandvault deleteall declareconfirm: bool = typer.Option(True, "--confirm", "-y"). Typer only generates the set-True--confirmflag, soif not confirm: typer.confirm(...)could never fire — destructive commands never prompted, contradicting the flag's own help text ("Skip confirmation prompt").Change: default flips to
False(prompt by default,-yskips).logoutnow only prompts when credentials exist, so the not-logged-in path stays a promptless no-op.-y. (Dry runs are unaffected; they short-circuit before the prompt.)2. Invalid
ENV/CAMPUS_ENV→ raw tracebackSince #22,
config.auth_urlraisesValueErrorfor an unrecognized environment — and that exception escaped as a Python traceback from whichever command touched it first (auth statushas no try/except;auth loginreads it before its try block;logout's revocation path only catchesCredentialError).Change: new
common.resolve_auth_url()wraps the resolution, printing the actionable message and exiting 1; all auth-URL reads (get_auth_urls,endpoint_mismatch,get_token_status,login_cmd) go through it. Verified live:ENV=bogus campus auth statusnow prints✗ Invalid deployment environment...and exits 1.3.
--jsonoutput could be corrupted by Richauth refresh --jsonandauth status --jsonusedconsole.print(json.dumps(...)); Rich word-wraps at console width and splits long values mid-string, so a 200-char access token comes out as unparseable JSON (reproduced withCOLUMNS=80).Change: both paths write via
typer.echo()— plain stdout, no wrapping or markup processing.4. logout revoked tokens against the wrong endpoint
Revocation posted to the currently targeted endpoint. After a
CAMPUS_AUTH_URL/config switch, the stored tokens are unknown to that endpoint: revocation silently failed and the old tokens stayed live on the server that issued them.Change:
revoke_token()takes an optionalauth_url;logoutpasses the stored issuing endpoint (token_auth_url), falling back to the current target for pre-binding credentials (unchanged legacy behavior).5. Device-flow polling nits
slow_downsleptinterval + 5once, then resumed the original interval; RFC 8628 §3.5 wants the raised interval to persist for the flow. It now does.intervalzero-divided inlogin_cmd'sexpires_in / interval. Absent → 5s (RFC default), zero → clamped to 1s inpoll_for_token.Tests
134 passed (was 126; +8: prompt paths, raw-JSON regression with a long token, revocation endpoint targeting, slow_down persistence, interval clamp, resolve_auth_url happy/invalid paths). Existing logout tests updated to pass
-ynow that confirmation actually fires.ruffclean.