fix(web): surface failed resource subscribe/unsubscribe

[cliffhall]

Closes #2174

#2173 merged while this was being written, so this targets v2/main directly rather than stacking on it.

Raised by Copilot on #2173 and deliberately deferred out of it: that PR is a decomposition step with a "no behavior change" contract, and this is a behavior change.

The bug

Both handlers discarded their promise with a bare void:

const onSubscribeResource = useCallback(
  (uri: string) => {
    if (!inspectorClient) return;
    void inspectorClient.subscribeToResource(uri);
  },
  [inspectorClient],
);

Nothing owned those failures. InspectorClient.subscribeToResource throws when the client is disconnected, when the server declares no subscription support, and rethrows a wrapped error after rolling its subscription state back on a failed request; unsubscribeFromResource is the same shape. So a failed subscribe did nothing visible — the button just didn't work — and surfaced only as an unhandled rejection in the console. AGENTS.md permits a bare void only "when the callee already owns its failures", which is not the case here.

These were also the only two commands in the hook skipping the shared recovery, so a mid-session 401 on a subscribe did not trigger re-auth the way a 401 on a refresh does.

The fix

Both now route through runCommandInBackground, like every neighbouring command.

The source is ambient, not resource — the issue proposed resource, and that turns out to be wrong. A resource step-up failure is routed by setSourceScopedError into readResourceState, which is the read-preview panel; it describes a read of whatever resource is selected, not this subscribe. Marking it errored would contradict a read that actually succeeded, and it would do so by mutating a panel the user is looking at. Subscribing has no panel of its own — the tile only flips its button label — which is exactly the position the refreshes, the load-mores and the pagination toggle are in, and they are ambient for the same reason.

Both pass an errorTitle, per the guidance on runCommandInBackground: nothing else records these failures, so without one the button goes on silently doing nothing.

Tests

Five new cases (80 total in the file):

• a failed subscribe toasts rather than being swallowed
• a failed unsubscribe toasts rather than being swallowed
• a lapsed authorization on subscribe recovers and retries, asserting source: "ambient" and the retryOperation
• the same for unsubscribe
• a subscribe whose recovery is left unsatisfied does not toast — the recovery has taken over (a redirect is pending or a step-up prompt is open), so a toast would talk over the prompt the user is looking at

Coverage on the hook is unchanged: 100% statements / lines / functions, 96.58% branches.

Verification

npm run local:gate green end to end.

One caveat worth stating plainly: the gate initially failed three runs in a row on an unrelated flake in scripts/lib/render-smoke.test.mjs, which gives itself a 150ms budget that has to cover PTY + Node startup. That is filed as #2177 and fixed by #2178. The gate run backing this PR was done with that fix cherry-picked in locally; the borrowed commit was then dropped, so the diff here is only the two files above.

🤖 Generated with Claude Code

https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev

[Copilot]

Pull request overview

Routes resource subscription commands through shared error reporting and OAuth recovery. However, real auth errors are wrapped by InspectorClient, preventing the intended recovery.

Changes:

• Adds failure toasts for subscribe/unsubscribe.
• Adds tests for errors and intended auth recovery.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
clients/web/src/hooks/useServerCommands.tsx	Routes subscription operations through the background command wrapper.
clients/web/src/hooks/useServerCommands.test.tsx	Tests notifications and recovery behavior.

Suppressed comments (1)

clients/web/src/hooks/useServerCommands.tsx:602

• The unsubscribe recovery path has the same contract mismatch: InspectorClient.unsubscribeFromResource wraps the typed auth failure in a generic Error (core/mcp/inspectorClient.ts:6295-6299), but runWithCommandAuthRecovery only recognizes a top-level AuthRecoveryRequiredError. The test's direct typed rejection therefore proves behavior the real method cannot produce. Preserve the auth error or unwrap its cause before recovery, and test the wrapped production shape.

      runCommandInBackground(
        () => inspectorClient.unsubscribeFromResource(uri),
        "ambient",
        "Failed to unsubscribe from resource",

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

You are right, and this was the important catch — the PR claimed a recovery that could not have run. Fixed in 35b0607.

Confirmed the whole chain rather than taking it on faith:

• subscribeToResource's catch (inspectorClient.ts:6234-6238) and unsubscribeFromResource's (6295-6299) wrap everything into a plain Error, original only in cause.
• runWithCommandAuthRecovery dispatches solely on err instanceof AuthRecoveryRequiredError.
• And the error really does arise inside that try on the path that matters. It is not the direct transport — subscribeToResource never routes through invokeMcpClient/withDirectAuthRecovery, so nothing typed is raised there at all. It is the remote transport, which is what the web client uses: remoteClientTransport.postSend calls recoverFromAuthChallenge, which throws AuthRecoveryRequiredError (remoteClientTransport.ts:360,367) from inside client.subscribeResource(...).

So a mid-session 401 on a subscribe produced Failed to subscribe to resource: Interactive auth recovery required as a toast, instead of re-authorizing and retrying. Exactly as you described.

Fix: preserve the typed error at the source

Of the two options you offered I took the first. An interactive auth recovery is a control-flow signal, not a failure to describe, and the wrapping catch exists to add context to a generic failure — so it should not consume it:

} catch (error) {
  if (error instanceof AuthRecoveryRequiredError) throw error;
  throw new Error(`Failed to subscribe to resource: …`, { cause: error });
}

Unwrapping the cause chain in runWithCommandAuthRecovery instead would change behavior for every command rather than these two, and would misfire wherever an intermediate layer deliberately converts an auth error into a terminal failure.

And the tests now exercise the production shape

Your point about the mocks was the reason this passed review-by-testing: they rejected the typed error directly from the client method, which is a shape production cannot produce. Two integration tests in inspectorClient-coverage-backfill.test.ts now pin the client's contract — that it rethrows the typed error rather than wrapping it — sitting directly beside the existing "wraps subscribe/unsubscribe failures" tests that pin the other half.

Verified by mutation, not just by going green: removing the two if (error instanceof AuthRecoveryRequiredError) throw error; lines makes both fail (expected Error: Failed to subscribe… to be AuthRecoveryRequiredError). Restored, both pass.

One note for anyone reading later: the subscribe test needs an HTTP server advertising subscriptions: true, because the supportsResourceSubscriptions() guard sits above the try and would otherwise reject first — the wrapping catch is never reached on the stdio fixture.