fix(auth): address Copilot review round 2 on the RFC 7009 revocation (#2144)

- `revokeStoredKeys`' pre-read of `getTokens` sat outside the best-effort
  catch. `getTokens` parses through `OAuthTokensSchema`, so a persisted token
  that no longer validates rejected and abandoned the local delete `--relogin`
  promises — over a grant that could not have been revoked anyway. Caught and
  retained as a failed outcome; the clear proceeds.
- The multi-key bookkeeping was wrong twice. A token is now marked spent only
  on a `revoked` outcome, so a failed or unsupported attempt no longer skips a
  duplicate entry under the other key that may hold the credentials or metadata
  that would have worked. And a failure now outranks an earlier success for
  reporting, so a stale grant left live cannot be hidden behind the first key's
  200.
- `--no-revoke` is now rejected without `--relogin`, ahead of the short-circuit
  returns, matching the parser's existing stance on accepted-but-inert flags
  (`--strict`). On its own it reads as "this run will not revoke anything",
  true only because nothing was being cleared.
- Extracted the App draft merge as `utils/serverWithDraftSettings`, so the rule
  has a test. The hook tests receive whatever they are handed and cannot tell a
  draft from a persisted entry, and `App.test.tsx`'s settings harness connects
  a stdio server where the OAuth section never renders.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/cli/src/clear-stored-auth-for-relogin.ts

@@ -70,13 +70,19 @@ export async function clearStoredAuthForRelogin(
  * under the other spelling is still a live grant at the authorization server,
  * and deleting it locally without revoking is the leak this whole change
  * closes. Two keys holding the *same* token are one grant, so the duplicate is
- * skipped rather than producing a second request for something already ended.
+ * skipped rather than producing a second request for something already ended —
+ * but only once one has actually been *revoked*, since a failed or unsupported
+ * attempt has ended nothing and the other key may hold the credentials that
+ * would have worked.
  *
- * The reported outcome is the first key's that was not "nothing to do", which
- * with `keys` in `findStoredServerState` precedence means the grant the CLI
- * would actually have connected with. When no key holds a token, the last
- * "nothing to do" answer is returned so the caller can still tell "no tokens"
- * from "this authorization server advertises no revocation endpoint".
+ * Reporting prefers a **failure** over any earlier success — a grant still live
+ * at the authorization server is what the user needs to hear about, and an
+ * earlier success would otherwise silence the warning. Failing that, it is the
+ * first key's outcome that was not "nothing to do", which with `keys` in
+ * `findStoredServerState` precedence means the grant the CLI would actually
+ * have connected with. When no key holds a token, the last "nothing to do"
+ * answer is returned so the caller can still tell "no tokens" from "this
+ * authorization server advertises no revocation endpoint".
  */
 async function revokeStoredKeys(
   storage: NodeOAuthStorage,
@@ -87,7 +93,21 @@ async function revokeStoredKeys(
   let reported: TokenRevocationOutcome | undefined;
   let lastSkip: TokenRevocationOutcome | undefined;
   for (const key of new Set(keys)) {
-    const token = selectRevocableToken(await storage.getTokens(key))?.token;
+    // This read is OUTSIDE `revokeStoredOAuthTokens`'s own best-effort catch,
+    // so it needs its own: `getTokens` parses through `OAuthTokensSchema` and
+    // rejects on a persisted token that no longer validates. Letting that
+    // escape would abandon the *local* delete `--relogin` promises, over a
+    // grant we could not have revoked anyway.
+    let token: string | undefined;
+    try {
+      token = selectRevocableToken(await storage.getTokens(key))?.token;
+    } catch (err) {
+      reported ??= {
+        status: "failed",
+        detail: err instanceof Error ? err.message : String(err),
+      };
+      continue;
+    }
     if (token !== undefined && revokedTokens.has(token)) continue;
     const outcome = await revokeStoredOAuthTokens({
       serverUrl: key,
@@ -98,8 +118,21 @@ async function revokeStoredKeys(
       lastSkip = outcome;
       continue;
     }
-    if (token !== undefined) revokedTokens.add(token);
-    reported ??= outcome;
+    // Only a token the authorization server actually accepted counts as spent.
+    // Marking one revoked on a `failed` or `no_endpoint` outcome would skip a
+    // duplicate entry under the other key that may carry usable metadata or
+    // credentials — the one chance left to end that grant.
+    if (token !== undefined && outcome.status === "revoked") {
+      revokedTokens.add(token);
+    }
+    // A failure outranks an earlier success for reporting: a grant that is
+    // still live is what the user needs to hear about, and reporting the
+    // success would print no warning while a stale grant survives.
+    if (outcome.status === "failed") {
+      if (reported?.status !== "failed") reported = outcome;
+    } else {
+      reported ??= outcome;
+    }
   }
   return reported ?? lastSkip;
 }

clients/cli/src/cli.ts

@@ -780,7 +780,7 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
     )
     .option(
       "--no-revoke",
-      "With --relogin, skip the RFC 7009 revocation request that would otherwise end the grant at the authorization server before the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.",
+      "Requires --relogin. Skips the RFC 7009 revocation request that would otherwise end the grant at the authorization server before the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.",
     )
     .option(
       "--wait-for-auth <sec>",
@@ -867,6 +867,15 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
     }
   }
 
+  // `--no-revoke` only means anything alongside `--relogin` — it suppresses the
+  // RFC 7009 request that clear makes. Accepted on its own it is inert, and
+  // worse than inert: it reads as "this run will not revoke anything", which is
+  // true only because nothing was going to be cleared. Rejected here, ahead of
+  // the short-circuit returns, for the same reason `--strict` is (#2144).
+  if (options.revoke === false && !options.relogin) {
+    throw new Error("--no-revoke requires --relogin (it has no other effect).");
+  }
+
   // `--strict` is checked HERE, ahead of every short-circuit return below
   // (`--list-stored-auth`, `--print-handoff`, `servers/list`, `servers/show`),
   // rather than beside the other method-shaped validations further down. Those