Skip to content

OAuth recovery: five void-discarded promises whose failures reach nobody #2165

Description

@cliffhall

Raised by Copilot's second review of #2162 (the #2153 extraction), and deliberately left for its own issue: every site below is code that PR moved, not wrote — the same void sits on the same call in v2/main today — and each fix is a real behaviour change on a failure path rather than a refactor.

@typescript-eslint/no-floating-promises (#1959) passes on all five because the promise is explicitly discarded with void. That satisfies the rule, but AGENTS.md is narrower than the rule: void is acceptable only when the callee already owns its failures, or the caller genuinely cannot await — and it asks for a one-line comment saying which. These five are voided without that argument holding.

The five sites

All in clients/web/src/hooks/useOAuthRecovery.ts after the extraction; all reachable from App.tsx before it.

Site What can reject What the user sees today
prepareOAuthRedirect → void oauthClient?.beginInteractiveAuthorization(url) provider-state load / flow recording before navigation unhandled rejection, and the resume snapshot written moments earlier is later read as an abandoned redirect — so the diagnosis the banner offers is wrong
the authChallengeInteractive handler's void (async () => …)() checkAuthChallengeSatisfied, pushRemoteAuthState unhandled rejection and no UI response at all to the challenge
resumePendingReauth (both callers void it) checkAuthChallengeSatisfied, handleAuthChallenge, pushRemoteAuthState the pending slot is already cleared, so the deferred recovery is lost silently
the /oauth/callback effect's void (async () => …)() setupClientForServer throwing, and the post-resume checkAuthChallengeSatisfied happens after the callback URL and the one-shot snapshot are consumed, so there is nothing left to retry with
handleStepUpAuthorize, EMA branch handleAuthChallenge, pushRemoteAuthState, the stored retry operation the finally resets the latch and dismisses the prompt, but the rejection escapes and the panel that asked is never told

Why each needs a decision, not a blanket .catch()

The remedies differ per site and each one picks a user-visible outcome:

  • The redirect and callback sites should probably flag the server (Open the monitoring sidebar on connection failure, with a Console (stderr) tab for stdio servers #1621) and raise the re-auth banner, the way their sibling arms already do.
  • The step-up site has a natural home already: setSourceScopedError(stepUp.source, …), which routes to the panel that issued the command.
  • The ambient-challenge and resume sites need to decide whether a failed recovery attempt is worth a toast, or whether it should restore the pending slot so the next trigger can retry — restoring it is arguably the better behaviour and is a bigger change than a catch.

Each also wants a test, which is why this is not a one-line sweep.

Done when

  • Each of the five terminates its promise, with the failure surfaced through the banner / toast / source-scoped path chosen per site
  • Any void that survives carries the one-line justification AGENTS.md asks for
  • Tests cover the new failure arms; npm run ci green

Activity

  1. added this to the v2.5.0 milestone on Aug 27, 2026
  2. added
    bugSomething isn't working
    v2Issues and PRs for v2
    on Aug 27, 2026
  3. ump45nose commented on Aug 27, 2026

    @ump45nose

    I prepared and locally validated an implementation against v2/main at 95db736.

    The implementation gives each asynchronous failure path an explicit owner:

    • redirect setup failures clear the one-shot resume snapshot, flag the server, and open the re-authorization banner;
    • visible ambient challenge failures are caught by the EventTarget listener and routed to the active server UI;
    • deferred recovery failures restore pendingReauth, so a later visibility or reconnect signal can retry;
    • callback failures outside the existing classified arms are caught after the callback URL is consumed and surfaced against the initiating server;
    • rejected EMA step-up work is reported through setSourceScopedError to the panel that initiated it.

    I added one regression test for each of those five ownership paths. Verification completed locally:

    • focused useOAuthRecovery tests: 82/82;
    • web validation: 314 files, 5,337 tests;
    • root npm run ci: validation, coverage, build gates, Chromium and Firefox smoke tests, and Storybook all passed;
    • web coverage: 98.62% statements and 96.00% branches.

    Per the repository's issue-first external contribution workflow, I have not pushed the local branch or opened a PR.

    AI assistance disclosure: AI was used to discover this opportunity and draft the change or text. I reviewed the reasoning, diff, and verification results before submission.

  4. self-assigned this
    on Aug 29, 2026
  5. added a commit that references this issue on Sep 2, 2026
    f74aa75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingv2Issues and PRs for v2

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions