fix(revoke): exit non-zero when requested sessions are not accepted for revocation - #60
Merged
Merged
Conversation
`grant revoke` printed the raw revocation status without inspecting it and exited 0 on any HTTP 200. A live AWS session returning the undocumented REVOCATION_NOT_APPLICABLE therefore reported success while the session stayed live, and a response missing rows for requested sessions did the same. Outcomes are now reconciled against the *requested* session IDs rather than the returned rows: a requested session with no row is unknown, duplicate rows resolve worst-outcome-wins, and rows for unrequested or empty IDs satisfy nothing. Statuses are classified into an outcome enum (revoked/in_progress/not_applicable/unknown) that fails closed on anything outside the spec's two documented values. Exit 0 only when every requested session was accepted; partial results exit 1 after printing the full per-session breakdown. The request is also chunked to the spec's `sessionIds` cap of 100 per call, so `--all` works on tenants with more than 100 active sessions; a mid-sequence batch error keeps the outcomes already collected. Messaging is provider-neutral: observing REVOCATION_NOT_APPLICABLE proves only that the service declined to act, not why, so no AWS/STS mechanism is claimed. The raw status token is kept in parentheses for grepping. The best-effort "expires in ~Xm" note is a standalone informational clause, never a causal one, and is unavailable in direct mode where grant has no session metadata. JSON gains per-entry outcome/accepted/complete/reason with one entry per requested session, and is emitted before the error so `-o json` stays valid on exit 1. There is deliberately no `revoked` boolean: in-progress means accepted, not finished.
…able Addresses two Codex review findings. An unattributed result row (an ID grant never requested, or an empty ID) was classified from its raw status, so it could report outcome "revoked" while both `accepted` and `complete` were false. Consumers keying on `outcome` would read success where consumers keying on the axes read failure. A row grant never asked about is not a success by any reading, so the outcome is now always `unknown`, agreeing with both axes; the raw status is still preserved for the operator. The zero-selected-sessions no-op was unreachable: it sat after the request was built, past the point where the confirmation prompt had already run. It now sits immediately after selection, so an empty selection is treated as cancellation without prompting and never reaches the API.
Bounded simplification of the revocation reporting surface, applied before the JSON shape becomes a compatibility surface on release. Reconciliation, batching, provider-neutral messaging and the file split are unchanged. - Drop `accepted` and `complete` from `revocationOutput`. Both are derivable from `outcome`, and carrying two representations of one concept is what let the unattributed-row rows disagree with themselves in the first place. `outcome` is now the single classification field; the raw `status` stays in both text and JSON. `summarizeRevocations` switches on `outcome` directly, which leaves `RevocationOutcome.Accepted`/`.Complete` with no callers, so they go too. - Drop the expiry hint, the session-ID -> metadata map that fed it, and the `computeRemainingTimeAt` clock seam added for it. The hint rested on incomplete local evidence: it only existed when grant elevated the session on this machine and the tracker had not aged out, and never in direct mode. That makes it a poor fit for authoritative remediation output. `grant status` reports remaining time. `cmd/status.go` is back to its original form and unaffected. - Drop `revocationRecord.Duplicate`. It was set but never rendered and never read as a decision input; worst-outcome-wins already encodes what matters. Exit policy is settled explicitly rather than described as "fail closed", which it never was: exit 0 covers every requested session the service accepted, `REVOCATION_IN_PROGRESS` included, because that is a documented asynchronous success state and failing on it would make legitimate revocations look broken. Exit 1 covers refusals, unrecognized statuses and sessions with no returned row. Exit 0 therefore does not prove access is gone, only that nothing was refused or unaccounted for. Comments, README, CHANGELOG and CLAUDE.md now say exactly that. `ClassifyRevocationStatus` still fails closed; the command does not fail on in-progress.
This was referenced Aug 14, 2026
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.
Behavioural change — exit codes. Found by live-tenant testing, not review.
Revoking an AWS session returned
[{"sessionId":"...","status":"REVOCATION_NOT_APPLICABLE"}]and grant exited 0 while printing the raw token. A user asking to revoke access got a success exit code and a live session.Exit semantics
The command is deliberately not blanket "fail closed", and this PR no longer claims it is.
REVOCATION_IN_PROGRESSREVOCATION_NOT_APPLICABLE)REVOCATION_IN_PROGRESSexits 0 on purpose: the service accepted the command and will act, it is a documented asynchronous success state, and treating it as a failure would make legitimate async revocations look broken. The consequence is stated plainly in the README — exit 0 does not prove every session is gone, only that nothing was refused, unrecognised or unaccounted for. The per-session breakdown always distinguishesrevokedfromin_progress, andgrant statusshows what is still live.What exit 0 does guarantee is that
grant revoke --all && echo safecannot printsafewhile a session was refused or silently dropped. The full per-session breakdown is always printed before the error returns.Reconciliation against the requested set
Classifying only the returned rows was not enough: requesting
A,Band receiving a single success row forAwould have exited 0 — the same bug in a different shape. The requested set is now the source of truth. One default-failure record is created per requested ID, and only matching non-empty response IDs are associated. Missing, duplicate, empty and unexpected IDs are all handled; duplicate response rows resolve worst-outcome-wins. IDs are deduplicated before sending.A row the service returns that grant never requested is always
unknown— never a success, whatever status it carries.Batching
sessionIdsis capped atmaxItems: 100on the request body and--allcan exceed it. Requests are chunked deterministically in order; outcomes aggregate across batches, and a mid-sequence batch error preserves the outcomes already collected rather than discarding them.Classification fails closed
The spec enum contains only
SUCCESSFULLY_REVOKEDandREVOCATION_IN_PROGRESS.REVOCATION_NOT_APPLICABLEis observed live but appears in neither the spec nor the SDK, and the SDK defines no constants at all — so the status set is open. Anything unrecognised, including an empty string, classifies asunknownand fails the command. Note the scope of the claim: it is classification that fails closed, not the command as a whole.Messaging is provider-neutral: observing a status does not prove its cause, so grant reports what the service said rather than asserting an AWS/STS explanation. Raw status tokens are kept parenthesised so operators can still grep them.
JSON
revocationOutputkeepssessionIdand the rawstatusfor backwards compatibility, and adds a singleoutcomefield (revoked/in_progress/not_applicable/unknown) plus areasonon anything short of a confirmed revocation. There are deliberately no derived booleans beside it — norevoked, noaccepted/completeaxes. Two representations of one concept can drift apart and disagree, which is exactly what produced the unattributed-row inconsistency during review. Consumers switch onoutcome. JSON is emitted before the error returns, so-o jsonis complete and valid on exit 1.Also
An empty interactive selection is now treated as cancellation — no confirmation prompt, no API call. Previously it sent an empty request to the API.
No expiry hint is printed. An earlier revision of this branch carried one; it was removed because it rested on incomplete local evidence — it only existed when grant had elevated the session on this machine and the tracker had not aged out, and never in direct mode — which makes it a poor fit for authoritative remediation output.
grant statusreports remaining time.cmd/status.gois untouched by this PR as a result.Testing
Mutation-verified in both directions:
return nilin place of the fail-closed check produces 14 sub-test failures, includingrequested two, one row returned;&& s.inProgress == 0) produces 4 failures, so exit 0 for in-progress is pinned as a decision, not left as an accident;ConfirmRevocation(0).make test,make test-race,make lint,GOOS=windows go vet ./...andgofmt -s -l .all pass.Integration note
No code conflict with #59, but a three-way merge produces two separate
### Fixedsections in CHANGELOG.md — consolidate at merge time.