fix: address the four suppressed findings from review round 20 (#2144)

Copilot reported "no new comments" again, with four real findings in its
suppressed block.

- The web in-flight lock was global, so a pending clear for server A silently
  swallowed a click on server B's — even though the callback explicitly
  supports clearing a non-active server. Keyed by server id now.
- `else if (!isActive)` skipped the resume-snapshot cleanup when the target IS
  active but has no client yet, which is reachable while a session is being
  built or torn down. The clear then emptied storage and left stale OAuth
  recovery state pointing at credentials that no longer existed.
- `revocationAuthMethods` returned `[]` for an omitted
  `revocation_endpoint_auth_methods_supported`, which does not enforce RFC
  8414's default: an empty list means "the metadata said nothing" to
  `selectClientAuthMethod`, which then honors the registration's own
  `token_endpoint_auth_method`. The default is named literally now.
- The `v8 ignore` on the CLI budget branch was wrong, and the challenge to it
  was right: an injected `budgetMs` of zero reaches it directly, through a seam
  I had added myself the round before. Removed, and covered by a test that
  asserts no request is made and the unattempted key is reported.

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/__tests__/clear-stored-auth-for-relogin.test.ts

@@ -357,6 +357,28 @@ describe("clearStoredAuthForRelogin", () => {
       }
     });
 
+    // A plan reached with no budget left is reported rather than firing a
+    // request it would immediately abandon — and that report must outrank an
+    // earlier success, since the unattempted key's grant may still be live.
+    it("reports exhaustion for a key it never attempted", async () => {
+      seedBothSpellings("live-r", "stale-r");
+      const fetchSpy = vi
+        .spyOn(globalThis, "fetch")
+        .mockResolvedValue(new Response(null, { status: 200 }));
+      try {
+        const outcome = await clearStoredAuthForRelogin("https://example.com", {
+          budgetMs: 0,
+        });
+        expect(fetchSpy).not.toHaveBeenCalled();
+        expect(outcome).toMatchObject({ status: "failed" });
+        expect(outcome?.status === "failed" ? outcome.detail : "").toContain(
+          "budget was exhausted",
+        );
+      } finally {
+        fetchSpy.mockRestore();
+      }
+    });
+
     it("reports a later failure over an earlier success", async () => {
       seedBothSpellings("live-r", "stale-r");
       const fetchSpy = vi

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

@@ -123,10 +123,6 @@ async function sendPlans(
     // may still be live when the key held no grant at all — a false alarm, and
     // one that outranks the real outcome under the failure-first rule below.
     const needsNetwork = plan.outcome === undefined;
-    /* v8 ignore next 10 -- A bound, not a path: `withDeadline` fails any plan
-       that reaches the deadline, so a preceding plan cannot both succeed and
-       leave zero budget. This exists so a plan reached with nothing left is
-       reported rather than firing a request it would immediately abandon. */
     if (needsNetwork && remainingMs <= 0) {
       // Overrides an earlier success rather than deferring to it: this key's
       // grant may still be live at the authorization server, and that is the

clients/web/src/App.tsx

@@ -1321,9 +1321,12 @@ function App() {
   const settingsModalIsStdio = settingsModalServerType === "stdio";
 
   /**
-   * In-flight lock shared by both clear controls (#2144). See `runClear`.
+   * Servers whose clear is in flight (#2144). Keyed by id, not a single flag:
+   * the callback explicitly supports clearing a server other than the active
+   * one, so a global lock would silently drop B's click while A's revocation
+   * was still out. See `runClear`.
    */
-  const clearOAuthInFlightRef = useRef(false);
+  const clearOAuthInFlightRef = useRef<Set<string>>(new Set());
 
   /**
    * Run a clear from a key/click handler, which cannot await.
@@ -1337,16 +1340,17 @@ function App() {
   const runClear = useCallback(
     (server: Parameters<typeof clearServerOAuthAndDisconnect>[0]) => {
       // Shared by BOTH clear controls (Connection Info and Server Settings),
-      // because they drive the same client and the same store entry. A ref
+      // because for one server they drive the same client and the same store
-      // rather than state: a double click delivers both events before React
+      // entry. A ref rather than state: a double click delivers both events
-      // re-renders, so a state-based guard would let both through — and with
+      // before React re-renders, so a state-based guard would let both through
-      // revocation taking up to five seconds, that means concurrent RFC 7009
+      // — and with revocation taking up to five seconds, that means concurrent
-      // requests, concurrent store writes, and two contradictory toasts.
+      // RFC 7009 requests, concurrent store writes, and two contradictory
-      if (clearOAuthInFlightRef.current) return;
+      // toasts. Keyed by server so a *different* server's clear is unaffected.
-      clearOAuthInFlightRef.current = true;
+      if (clearOAuthInFlightRef.current.has(server.id)) return;
+      clearOAuthInFlightRef.current.add(server.id);
       clearServerOAuthAndDisconnect(server)
         .finally(() => {
-          clearOAuthInFlightRef.current = false;
+          clearOAuthInFlightRef.current.delete(server.id);
         })
         .catch((err: unknown) => {
           notifications.show({

clients/web/src/hooks/useOAuthRecovery.ts

@@ -1304,7 +1304,13 @@ export function useOAuthRecovery({
             finalizeExplicitDisconnect();
           }
         }
-      } else if (!isActive) {
+      } else if (!isActive || stillTargetsActiveSession()) {
+        // No client to disconnect — either this is a stored-only clear, or the
+        // active session has none yet (it is being built or torn down). Either
+        // way the resume snapshot is stale and must go; skipping it would leave
+        // OAuth recovery state pointing at credentials that no longer exist.
+        // Guarded so a clear whose session has moved on still publishes
+        // nothing.
         clearOAuthResumeOnExplicitDisconnect();
       }
 

clients/web/src/test/core/auth/revocation.test.ts

@@ -101,16 +101,21 @@ describe("revocationAuthMethods", () => {
           token_endpoint_auth_methods_supported: ["client_secret_post"],
         }),
       ),
-    ).toEqual([]);
+    ).toEqual(["client_secret_basic"]);
   });
 
-  it("yields nothing when the revocation list is absent", () => {
+  // Named literally, not left empty. An empty list means "the metadata said
-    expect(revocationAuthMethods(metadata())).toEqual([]);
+  // nothing" to `selectClientAuthMethod`, which then honors whatever the
+  // client's own registration declares — so a client registered
+  // `client_secret_post` could put credentials in the body of a request to an
+  // endpoint that promised only Basic. (`OAuthClientInformation` does not carry
+  // that field, so this path cannot construct the case today; naming the
+  // default is what keeps it true if the type widens.)
+  it("names the RFC 8414 default when the revocation list is absent", () => {
+    expect(revocationAuthMethods(metadata())).toEqual(["client_secret_basic"]);
   });
 
-  // The empty list is not "no authentication": it is what makes the SDK apply
+  it("the default resolves to Basic for a confidential client", () => {
-  // RFC 8414's actual default.
-  it("an empty list resolves to the RFC 8414 default", () => {
     const { init } = buildRevocationRequest({
       endpoint: REVOKE_URL,
       token: "r",
@@ -121,7 +126,7 @@ describe("revocationAuthMethods", () => {
     expect(headerOf(init, "Authorization")).toBe(`Basic ${btoa("cid:sec")}`);
   });
 
-  it("an empty list leaves a public client unauthenticated", () => {
+  it("the default leaves a public client unauthenticated", () => {
     const { init } = buildRevocationRequest({
       endpoint: REVOKE_URL,
       token: "r",

core/auth/revocation.ts

@@ -104,14 +104,20 @@ export function selectRevocableToken(
  * there send POST credentials to a revocation endpoint that never advertised
  * that method, which a strict server rejects.
  *
- * An empty result is returned rather than a literal `["client_secret_basic"]`
+ * The default is returned **literally** rather than as an empty list. An empty
- * because that is what makes {@link selectClientAuthMethod} apply exactly the
+ * list does not mean "apply the RFC default" to `selectClientAuthMethod`: it
- * RFC's default — `client_secret_basic` when the client holds a secret, `none`
+ * means "the metadata said nothing", and the SDK then honors whatever
- * when it does not — while still honoring a `token_endpoint_auth_method` the
+ * `token_endpoint_auth_method` the client's own registration declares. A DCR
- * client's own registration declares.
+ * client registered `client_secret_post` would therefore put its credentials
+ * in the body of a request to an endpoint that promised only Basic, and the
+ * revocation would fail. Naming the default is what actually enforces it.
  */
 export function revocationAuthMethods(metadata: CachedMetadata): string[] {
-  return metadata.revocation_endpoint_auth_methods_supported ?? [];
+  return (
+    metadata.revocation_endpoint_auth_methods_supported ?? [
+      "client_secret_basic",
+    ]
+  );
 }
 
 export interface RevocationRequestParams {