From 8ca00fa2d6b0353b7c69c88c812ea32e819837f6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 10:45:26 -0400 Subject: [PATCH] test: make the not-an-OAuthError test assert its own title (review round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test was named "is not an OAuthError, which is why the retry path must not claim it" and asserted only `name` and `typeof tokenEndpoint`. Neither touches the OAuth hierarchy, so a future SDK reparenting the class would leave it passing with its title now false — and the comment promising the #2280 handling would be revisited if that changed was hollow. A test that has quietly stopped testing its subject is worse than no test. It now asserts against the hierarchy both ways, `OAuthError.isInstance(err)` and `instanceof`. Verified by mutation rather than assumed: flipping the assertion to simulate the reparenting fails exactly this test and nothing else (1 failed, 15 passed). Also fixes a doubled "terminal terminal", collateral from the round-2 SEP label removal. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- clients/web/src/hooks/useOAuthRecovery.ts | 2 +- .../src/test/core/auth/insecureTokenEndpoint.test.ts | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 329e6f0f..b1daf721 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -392,7 +392,7 @@ export function useOAuthRecovery({ ); /** - * Report a terminal terminal token-endpoint refusal (#2280) and clear any re-auth banner. + * Report a terminal token-endpoint refusal (#2280) and clear any re-auth banner. * * Every arm goes through this rather than calling the notice helper directly. * The banner clear is not incidental: a banner left from an *earlier* failure diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts index 78ff9ccf..b89aa680 100644 --- a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect } from "vitest"; -import { InsecureTokenEndpointError } from "@modelcontextprotocol/client"; +import { + InsecureTokenEndpointError, + OAuthError, +} from "@modelcontextprotocol/client"; import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js"; const ENDPOINT = "http://tenant.example.localhost:3300/api/oauth/token"; @@ -21,6 +24,13 @@ describe("SDK brand placement", () => { // do not treat it as a transient authorization failure. If a future SDK // changes that, the #2280 handling should be revisited rather than silently // keeping a now-wrong justification. + // + // Asserted against the hierarchy itself, both ways. Checking only `name` + // and `tokenEndpoint` would leave this passing unchanged if the class were + // reparented — the test would keep its title while having stopped testing + // it, which is worse than not having it. + expect(OAuthError.isInstance(err)).toBe(false); + expect(err instanceof OAuthError).toBe(false); expect(err.name).toBe("InsecureTokenEndpointError"); expect(typeof err.tokenEndpoint).toBe("string"); });