fix(revoke): exit non-zero when requested sessions are not accepted for revocation

CHANGELOG.md

@@ -9,6 +9,10 @@ All notable changes to this project will be documented in this file.
 - End-to-end self-update tests that replace a real, running binary (`internal/selfupdate/e2e_test.go`, build tag `selfupdate_e2e`). They compile two fixture binaries from a dependency-free module, execute one, and swap it through grant's own apply path while a process is still running from that image — so the Windows file-locking semantics behind the two-rename swap are actually exercised, not just the bookkeeping. Success and rollback paths are both covered, and the rolled-back binary is asserted to still run. No network access is required
 - CI runs the new self-update end-to-end tests on **both** `ubuntu-latest` and `windows-latest`, closing the gap left by the Windows CI leg added in 0.8.0, which only ran `go build` and `go test` and never exercised a binary replacing itself on Windows
 
+### Fixed
+
+- `grant revoke` now exits 1 when the service refused, returned an unrecognized status for, or returned no result at all for any requested session, instead of reporting success; accepted-but-`in_progress` revocations still exit 0, and `--output json` gains a per-session `outcome` field — check scripts relying on exit 0.
+
 ## [0.8.0] - 2026-08-14
 
 ### Added

CLAUDE.md

@@ -40,6 +40,12 @@ Custom `SCAAccessService` follows SDK conventions:
   - `POST /api/access/elevate` — request JIT elevation (AWS responses include `accessCredentials` JSON string)
   - `GET /api/access/sessions` — list active sessions
   - `POST /api/access/sessions/revoke` — revoke sessions by ID (request: `sessionIds[]`, response: `SessionRevocationInfo[]`)
+    - `sessionIds` is capped at `maxItems: 100` per request, so `cmd/revoke_batch.go` chunks the requested set into sequential ≤100-ID calls and aggregates; a mid-sequence batch error keeps the outcomes already collected
+    - `revocationStatus`: the spec enum is only `SUCCESSFULLY_REVOKED` and `REVOCATION_IN_PROGRESS`. The live API also returns the **undocumented** `REVOCATION_NOT_APPLICABLE` (in neither the spec nor the SDK), so the status set is open and `ClassifyRevocationStatus` (`internal/sca/models/revoke.go`) **fails closed** — anything unrecognized, including `""`, is `OutcomeUnknown` and counts as a failure. Match is exact; case variants are unknown
+    - Outcomes are reconciled against the **requested** session IDs, never the returned rows (`cmd/revoke_reconcile.go`): a requested session with no row is `unknown`, duplicate rows resolve worst-outcome-wins, and rows for unrequested or empty IDs satisfy nothing
+    - **Exit policy (deliberate, not blanket "fail closed"):** exit 0 when every requested session was accepted, which *includes* `in_progress` — a documented async success state, so failing on it would make legitimate revocations look broken. Exit 1 on `not_applicable`, unrecognized statuses and missing rows. So exit 0 does not prove access is gone; only that nothing was refused or unaccounted for. State it that precisely in docs — `ClassifyRevocationStatus` fails closed, the *command* does not fail on in-progress
+    - `outcome` (`revoked`/`in_progress`/`not_applicable`/`unknown`) is the single classification field in the JSON. Do not add derived booleans (`accepted`, `complete`, `revoked`) beside it: two representations of one concept drift apart and disagree. Callers switch on `outcome`
+    - Never attribute a cause for `REVOCATION_NOT_APPLICABLE` (e.g. an AWS/STS story). Observing the status does not prove the reason, and grant supports Azure/AWS/GCP plus group sessions. Render provider-neutral text and keep the raw token
   - `GET /api/access/{CSP}/eligibility/groups` — list eligible Entra ID groups (response: `groupId`/`groupName`/`directoryId`)
   - `POST /api/access/elevate/groups` — request group membership elevation (response wrapped in `response` key, same as cloud elevation)
 - **Headers:** `Authorization: Bearer {jwt}`, `X-API-Version: 2.0`, `Content-Type: application/json`

README.md

@@ -118,11 +118,32 @@ Running `grant` with no subcommand elevates cloud permissions (the core behavior
 | `logout` | Clear cached tokens from keyring |
 | `status` | Show auth state and active sessions |
 | `favorites` | Manage saved role favorites (`add`/`list`/`remove`) |
-| `revoke` | Revoke sessions (interactive, by ID, or `--all`) |
+| `revoke` | Revoke sessions (interactive, by ID, or `--all`) — see exit codes below |
 | `request` | Manage access requests through an approval workflow (see subcommands below) |
 | `update` | Self-update to the latest release from GitHub |
 | `version` | Print version information |
 
+### `grant revoke` exit codes
+
+`revoke` reports its outcome against the sessions you **requested**, not against
+whatever rows the service happened to return.
+
+| Exit | Meaning |
+|------|---------|
+| 0 | The service **accepted** revocation for every requested session. This includes sessions still `in_progress` — accepted is not the same as finished, and `revoke` says which is which per session. |
+| 1 | At least one requested session was refused (`not_applicable`), came back with an unrecognized status, or had no result returned for it at all. The full per-session breakdown is printed before the command exits. |
+
+Precisely: exit 0 does **not** prove every session is gone, only that nothing was
+refused, unrecognized or unaccounted for. An in-progress revocation is a
+documented asynchronous success state, so it does not fail the command; run
+`grant status` to see what is still live. What does fail the command is a
+partial result, so `grant revoke --all && echo safe` cannot print `safe` while a
+session was refused or silently dropped.
+
+With `--output json` each entry carries the raw `status` and a single
+`outcome` field (`revoked`, `in_progress`, `not_applicable`, `unknown`), emitted
+on stdout even on exit 1.
+
 ### `grant request` subcommands
 
 | Subcommand | Description |
@@ -183,6 +204,7 @@ favorites:
 | "No eligible targets found" | Verify SCA policies with your Idira admin; try without `--provider` to see all targets |
 | "Failed to elevate" | Check `grant status` for active sessions; verify target/role names |
 | `grant env` errors for Azure/GCP | `env` is AWS-only — Azure and GCP return no credentials, use `grant` directly |
+| `grant revoke` reports "NOT revoked" | The service returned a status other than a successful or in-progress revocation (the raw value is shown in parentheses, e.g. `REVOCATION_NOT_APPLICABLE`), or returned no result for that session. The session may still be active — check `grant status` and raise the raw status with your Idira admin |
 | Permission denied accessing keyring (Linux) | Install and start `gnome-keyring` or `kwalletmanager` |
 
 ## Development

cmd/output_types.go

@@ -49,9 +49,19 @@ type statusOutput struct {
 }
 
 // revocationOutput is the JSON representation of a revocation result.
+// There is one entry per *requested* session, in requested order, plus any
+// results the service returned that could not be attributed to a request.
+//
+// outcome is the single classification field, and deliberately not a boolean:
+// an in-progress revocation is accepted but not complete, which no boolean can
+// say. Derived flags are not emitted alongside it — two representations of one
+// concept can drift apart and disagree.
 type revocationOutput struct {
-	SessionID string `json:"sessionId"`
-	Status    string `json:"status"`
+	SessionID  string `json:"sessionId"`
+	Status     string `json:"status"`           // raw API value; "" when no row was returned
+	Outcome    string `json:"outcome"`          // revoked | in_progress | not_applicable | unknown
+	Reason     string `json:"reason,omitempty"` // explanation when not confirmed revoked
+	Unexpected bool   `json:"unexpected,omitempty"`
 }
 
 // favoriteOutput is the JSON representation of a saved favorite.

cmd/revoke.go

@@ -12,6 +12,20 @@ import (
 	"github.com/spf13/cobra"
 )
 
+// errRevocationIncomplete is returned when the service did not accept
+// revocation for every requested session.
+//
+// Exit policy, precisely. A non-zero exit means at least one requested session
+// was refused (REVOCATION_NOT_APPLICABLE), carried an unrecognized status, or
+// had no result returned for it at all — unrecognized and missing statuses fail
+// closed. A zero exit means every requested session was *accepted*, which
+// includes REVOCATION_IN_PROGRESS: the service took the command and will act,
+// but has not confirmed the access is gone. That is a documented asynchronous
+// success state, so treating it as a failure would make legitimate revocations
+// look broken; the per-session breakdown still distinguishes it from a
+// confirmed revocation, and `grant status` shows what is still live.
+var errRevocationIncomplete = errors.New("not all requested sessions were revoked")
+
 // uiSessionSelector wraps ui.SelectSessions to implement sessionSelector
 type uiSessionSelector struct{}
 
@@ -123,94 +137,103 @@ func runRevoke(
 		return fmt.Errorf("not authenticated, run 'grant login' first: %w", err)
 	}
 
-	// Determine session IDs to revoke
-	var sessionIDs []string
+	// Determine which sessions to revoke.
+	sessionIDs, done, err := resolveRevokeTargets(cmd, args, lister, elig, selector, confirmer, cspFilter, allFlag, yesFlag)
+	if err != nil || done {
+		return err
+	}
 
-	if len(args) > 0 {
-		// Direct mode: session IDs provided as arguments
-		sessionIDs = args
-	} else {
-		// All or interactive mode: need to list sessions first
-		ctx, cancel := context.WithTimeout(context.Background(), apiTimeout)
-		defer cancel()
+	// The requested set is the source of truth for every count and for the exit
+	// code, so deduplicate it before sending and before reconciling.
+	sessionIDs = dedupeSessionIDs(sessionIDs)
 
-		sessions, err := lister.ListSessions(ctx, cspFilter)
-		if err != nil {
-			return fmt.Errorf("failed to list sessions: %w", err)
-		}
+	// A failing batch still returns the results already collected.
+	results, revokeErr := revokeInBatches(context.Background(), revoker, sessionIDs)
 
-		if len(sessions.Response) == 0 {
-			fmt.Fprintln(cmd.OutOrStdout(), "No active sessions to revoke.")
-			return nil
-		}
+	records, unattached := reconcileRevocations(sessionIDs, results)
 
-		if allFlag {
-			// Collect all session IDs
-			for _, s := range sessions.Response {
-				sessionIDs = append(sessionIDs, s.SessionID)
-			}
-
-			// Confirm unless --yes
-			if !yesFlag {
-				confirmed, err := confirmer.ConfirmRevocation(len(sessionIDs))
-				if err != nil {
-					return fmt.Errorf("confirmation failed: %w", err)
-				}
-				if !confirmed {
-					fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.")
-					return nil
-				}
-			}
-		} else {
-			// Interactive mode
-			nameMap := buildWorkspaceNameMap(ctx, elig, sessions.Response)
-
-			selected, err := selector.SelectSessions(sessions.Response, nameMap)
-			if err != nil {
-				return fmt.Errorf("session selection failed: %w", err)
-			}
-
-			for _, s := range selected {
-				sessionIDs = append(sessionIDs, s.SessionID)
-			}
-
-			// Confirm
-			if !yesFlag {
-				confirmed, err := confirmer.ConfirmRevocation(len(sessionIDs))
-				if err != nil {
-					return fmt.Errorf("confirmation failed: %w", err)
-				}
-				if !confirmed {
-					fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.")
-					return nil
-				}
-			}
+	if isJSONOutput() {
+		if err := writeJSON(cmd.OutOrStdout(), buildRevocationJSON(records, unattached)); err != nil {
+			return err
 		}
+	} else {
+		// Always print the full breakdown before returning an error, so a
+		// non-zero exit is never opaque.
+		renderRevocationResults(cmd.OutOrStdout(), records, unattached)
+	}
+
+	if revokeErr != nil {
+		return revokeErr
+	}
+
+	if summary := summarizeRevocations(records); !summary.allAccepted() {
+		return fmt.Errorf("%w: %s", errRevocationIncomplete, summaryLine(summary))
 	}
 
-	// Call revoke API
+	return nil
+}
+
+// resolveRevokeTargets determines the session IDs to revoke. done reports that
+// the command has already finished (nothing to revoke, or the user declined).
+func resolveRevokeTargets(
+	cmd *cobra.Command,
+	args []string,
+	lister sessionLister,
+	elig eligibilityLister,
+	selector sessionSelector,
+	confirmer confirmPrompter,
+	cspFilter *scamodels.CSP,
+	allFlag, yesFlag bool,
+) (sessionIDs []string, done bool, err error) {
+	if len(args) > 0 {
+		// Direct mode: session IDs provided as arguments.
+		return args, false, nil
+	}
+
+	// All or interactive mode: list sessions first.
 	ctx, cancel := context.WithTimeout(context.Background(), apiTimeout)
 	defer cancel()
 
-	result, err := revoker.RevokeSessions(ctx, &scamodels.RevokeRequest{
-		SessionIDs: sessionIDs,
-	})
+	sessions, err := lister.ListSessions(ctx, cspFilter)
 	if err != nil {
-		return err
+		return nil, true, fmt.Errorf("failed to list sessions: %w", err)
 	}
 
-	// Display results
-	if isJSONOutput() {
-		out := make([]revocationOutput, len(result.Response))
-		for i, r := range result.Response {
-			out[i] = revocationOutput{SessionID: r.SessionID, Status: r.RevocationStatus}
+	if len(sessions.Response) == 0 {
+		fmt.Fprintln(cmd.OutOrStdout(), "No active sessions to revoke.")
+		return nil, true, nil
+	}
+
+	selected := sessions.Response
+	if !allFlag {
+		nameMap := buildWorkspaceNameMap(ctx, elig, sessions.Response)
+		selected, err = selector.SelectSessions(sessions.Response, nameMap)
+		if err != nil {
+			return nil, true, fmt.Errorf("session selection failed: %w", err)
+		}
+		// Selecting nothing is a deliberate no-op, equivalent to declining the
+		// confirmation. Handled here rather than after the request is built, so
+		// an empty selection can never be revoked as if it were a request.
+		if len(selected) == 0 {
+			fmt.Fprintln(cmd.OutOrStdout(), "No sessions selected.")
+			return nil, true, nil
 		}
-		return writeJSON(cmd.OutOrStdout(), out)
 	}
 
-	for _, r := range result.Response {
-		fmt.Fprintf(cmd.OutOrStdout(), "  %s: %s\n", r.SessionID, r.RevocationStatus)
+	for _, s := range selected {
+		sessionIDs = append(sessionIDs, s.SessionID)
 	}
 
-	return nil
+	if !yesFlag {
+		confirmed, cerr := confirmer.ConfirmRevocation(len(sessionIDs))
+		if cerr != nil {
+			return nil, true, fmt.Errorf("confirmation failed: %w", cerr)
+		}
+		if !confirmed {
+			fmt.Fprintln(cmd.OutOrStdout(), "Revocation canceled.")
+			return nil, true, nil
+		}
+	}
+
+	return sessionIDs, false, nil
 }

cmd/revoke_batch.go

@@ -0,0 +1,48 @@
+package cmd
+
+import (
+	"context"
+
+	scamodels "github.com/aaearon/grant-cli/internal/sca/models"
+)
+
+// chunkSessionIDs splits ids into consecutive slices of at most size,
+// preserving order.
+func chunkSessionIDs(ids []string, size int) [][]string {
+	if len(ids) == 0 || size <= 0 {
+		return nil
+	}
+	chunks := make([][]string, 0, (len(ids)+size-1)/size)
+	for start := 0; start < len(ids); start += size {
+		end := start + size
+		if end > len(ids) {
+			end = len(ids)
+		}
+		chunks = append(chunks, ids[start:end])
+	}
+	return chunks
+}
+
+// revokeInBatches revokes ids in sequential batches of at most
+// scamodels.MaxRevokeBatchSize, the API's cap on the request body.
+//
+// On a batch failure it returns the results already collected together with the
+// error, so outcomes from earlier batches are never discarded: aborting
+// silently would under-report revocations that actually happened.
+func revokeInBatches(ctx context.Context, revoker sessionRevoker, ids []string) ([]scamodels.RevocationResult, error) {
+	var results []scamodels.RevocationResult
+
+	for _, chunk := range chunkSessionIDs(ids, scamodels.MaxRevokeBatchSize) {
+		batchCtx, cancel := context.WithTimeout(ctx, apiTimeout)
+		resp, err := revoker.RevokeSessions(batchCtx, &scamodels.RevokeRequest{SessionIDs: chunk})
+		cancel()
+		if err != nil {
+			return results, err
+		}
+		if resp != nil {
+			results = append(results, resp.Response...)
+		}
+	}
+
+	return results, nil
+}