fix: surface list-fetch failures instead of swallowing them
A list load that failed — a transport error, or a result the SDK's era codec rejects as invalid — reported nothing. The connect-time refresh is fired with no caller to await it (`void this.refresh()`), so its rejection became an unhandled rejection: the header said "Connected", the panel rendered an empty list indistinguishable from a server that has none, and the failed exchange showed in the Protocol panel as a clean success. Two surfaces now tell the truth: - ManagedListState records the failure as observable state (`getError` + an `errorChange` event) and still re-throws, so App's auth-recovery wrapper keeps working. The four `useManaged*` hooks expose it via a shared `useManagedListError`, and the Tools/Prompts/Resources sidebars render a `ListLoadError` alert with the reason and a Retry. - A response the client refused is attributed back to its Protocol entry (`markResponseRejected` → `responseRejected` → `MessageEntry.clientError`), which now renders an Error status and a "Rejected by the Inspector" alert rather than the green resultType badge alone. The SDK gives no request id with a decode failure, so the id is recovered by correlation; see the comment on `markResponseRejected` for why that is exact. Also stops `listAllTools` from dropping a failing excluded-tools walk on the floor — it stays non-fatal but is logged. Closes #1953 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
This commit is contained in:
parent
097bfafdc1
commit
f201032137
1 changed files with 1080 additions and 48 deletions
|
|
@ -0,0 +1,95 @@
|
|||
import { describe, it, expect, beforeEach, afterEach } from "vitest";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { MessageLogState } from "@inspector/core/mcp/state/index.js";
|
||||
import { createTransportNode } from "@inspector/core/mcp/node/transport.js";
|
||||
import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server";
|
||||
import type { MessageEntry } from "@inspector/core/mcp/types.js";
|
||||
|
||||
/**
|
||||
* `markResponseRejected` correlation (#1953).
|
||||
*
|
||||
* A response the server answered successfully but the client then refused (the
|
||||
* SDK codec rejecting the result for the negotiated era) carries no request id
|
||||
* in the SDK's error, so the client recovers it from the last response received
|
||||
* for that method. These tests drive a REAL connection so the correlation runs
|
||||
* against real JSON-RPC ids assigned by the SDK, not hand-built fixtures.
|
||||
*/
|
||||
describe("InspectorClient.markResponseRejected", () => {
|
||||
let client: InspectorClient | null = null;
|
||||
let log: MessageLogState | null = null;
|
||||
|
||||
beforeEach(async () => {
|
||||
const serverCommand = getTestMcpServerCommand();
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: "stdio",
|
||||
command: serverCommand.command,
|
||||
args: serverCommand.args,
|
||||
},
|
||||
{ environment: { transport: createTransportNode } },
|
||||
);
|
||||
log = new MessageLogState(client);
|
||||
await client.connect();
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
log?.destroy();
|
||||
log = null;
|
||||
try {
|
||||
await client?.disconnect();
|
||||
} catch {
|
||||
// Ignore teardown failures — the assertion already ran.
|
||||
}
|
||||
client = null;
|
||||
});
|
||||
|
||||
function entriesFor(method: string): MessageEntry[] {
|
||||
return (log?.getMessages() ?? []).filter(
|
||||
(entry) => "method" in entry.message && entry.message.method === method,
|
||||
);
|
||||
}
|
||||
|
||||
it("annotates the entry for the method's most recent response", async () => {
|
||||
await client!.listTools();
|
||||
const before = entriesFor("tools/list");
|
||||
expect(before).toHaveLength(1);
|
||||
expect(before[0]?.clientError).toBeUndefined();
|
||||
|
||||
client!.markResponseRejected("tools/list", "ttlMs required");
|
||||
|
||||
expect(entriesFor("tools/list")[0]?.clientError).toBe("ttlMs required");
|
||||
});
|
||||
|
||||
it("annotates only the latest call, leaving earlier ones untouched", async () => {
|
||||
await client!.listTools();
|
||||
await client!.listTools();
|
||||
const entries = entriesFor("tools/list");
|
||||
expect(entries).toHaveLength(2);
|
||||
|
||||
client!.markResponseRejected("tools/list", "second one failed");
|
||||
|
||||
expect(entries[0]?.clientError).toBeUndefined();
|
||||
expect(entries[1]?.clientError).toBe("second one failed");
|
||||
});
|
||||
|
||||
it("does not cross method boundaries", async () => {
|
||||
await client!.listTools();
|
||||
await client!.listPrompts();
|
||||
|
||||
client!.markResponseRejected("tools/list", "tools failed");
|
||||
|
||||
expect(entriesFor("tools/list")[0]?.clientError).toBe("tools failed");
|
||||
expect(entriesFor("prompts/list")[0]?.clientError).toBeUndefined();
|
||||
});
|
||||
|
||||
it("is a no-op for a method nothing has answered", () => {
|
||||
// No throw, and nothing annotated — a list that never reached the wire
|
||||
// (e.g. a capability-gated one) must not mislabel some other entry.
|
||||
expect(() =>
|
||||
client!.markResponseRejected("resources/list", "never happened"),
|
||||
).not.toThrow();
|
||||
expect((log?.getMessages() ?? []).some((entry) => entry.clientError)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue