feat(web): URL elicitation — explicit completion step + error-path (-32042) handling (#1415) (#1436)
* feat(web): two-step URL elicitation completion (#1415) Replace accept-on-open with an explicit completion step for URL elicitations. Opening the URL now only transitions the panel to its waiting state; the elicitation resolves with `accept` only when the user clicks the revealed "I've completed it" action, or with `cancel`. - ElicitationUrlPanel: add `onComplete`, reveal the completion button and relabel open→"Reopen in Browser" while `isWaiting`. - PendingClientRequestModal: wire local `isWaiting`; open no longer sends a response. Removed the accept-on-open comment that pointed here. - Tests cover open→waiting→complete and open→cancel, and assert opening alone does not resolve the elicitation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): handle URL-elicitation-required (-32042) error path Implements the spec's "URL mode with elicitation required error" flow (2025-11-25). When a tools/call returns a `-32042` UrlElicitationRequired error, the client now surfaces the required URL elicitation(s) and retries the call once completed, instead of showing a bare error. Core (InspectorClient.callTool): - Detect `-32042` (typed UrlElicitationRequiredError or generic McpError) via the new `getUrlElicitationsFromError` helper. - Surface each carried elicitation in order through the existing pending- elicitation queue / modal (reusing the two-step "Open → I've completed it" UI), then re-issue the original call once all are accepted. A decline/cancel aborts with a clear message; a bounded retry count guards a server that keeps returning the error. - `ElicitationCreateMessage.completeIfPending()` lets the optional `notifications/elicitation/complete` auto-advance/auto-accept an open URL elicitation; the completion-notification handler now resolves (was: only removed) the pending entry. - Extracted `attemptToolCall` / `dispatchFailedToolCall` from the old callTool body to support the retry loop without duplicating bookkeeping. Web (App): - A non-spec `-32042` with no elicitations (nothing to open) shows a dedicated yellow toast linking to a new UrlElicitationErrorModal with the raw error body, mirroring the output-schema-validation toast. Tests: helper unit tests, completeIfPending unit tests, callTool error-path retry/cancel/no-list/ordinary-error tests (injected fake client), the new modal (test + stories), and an App toast test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): abort URL-elicitation error path on a repeated URL Guard the -32042 retry loop: if the server's retry error re-requests a URL the user already completed earlier in the same call, completing it again can't make progress, so callTool aborts with a new typed UrlElicitationLoopError instead of re-prompting (which would loop until the retry cap). The web layer detects it and shows a "URL elicitation loop" toast naming the repeated URL rather than a generic failure. Tests: core abort-on-repeat test (URL presented once, two attempts, then loop error) and an App loop-toast test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): let Copy-URL reach the URL-elicitation completion step Address review of #1436: the "I've completed it" action was gated behind clicking "Open in Browser" (only that set isWaiting), so a user who used Copy URL to finish the flow externally could only Cancel. Copying the URL now also moves the panel into the waiting state, exposing the completion step. Also trims the now-redundant second sentence of the waiting hint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): address review of error-path URL elicitation Review follow-ups on #1436 (all non-blocking): - Remove the leftover bare block scope in attemptToolCall (extraction artifact); body de-indented one level. - Log a warning when callTool gives up after the URL-elicitation retry cap is hit, so a server that keeps demanding new elicitations is diagnosable rather than looking like an ordinary failure. - Assert the dispatch-once invariant on the loop-abort and cancel-abort paths (exactly one failed toolCallResultChange event). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(core): fix attemptToolCall body indentation Run prettier over the extracted attemptToolCall method body, which was left over-indented by 2 spaces after the bare-block removal in 0754b43d. core/ is outside clients/web's format:check scope, so npm run validate did not catch it. Whitespace-only; the pre-existing line 658 wrap (also outside the format scope) is left as-is, out of scope for this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): settle pending elicitations on disconnect to avoid callTool hang Address review of #1436: an error-path `awaitUrlElicitation` promise only settled via respond, but `disconnect()` cleared `pendingElicitations` wholesale — so a disconnect mid-elicitation (server crash, manual disconnect, transport close) left `callTool` (and the UI's in-flight toolCallState) hanging forever. - Add `ElicitationCreateMessage.cancel()`: settles a pending elicitation as `{ action: "cancel" }` without calling `onRemove` (the queue is cleared by the caller, so it must not splice mid-iteration). - `disconnect()` now cancels each pending elicitation before clearing the array, and dispatches `pendingElicitationsChange` (it already did so for samples) so the modal closes on a transport-close teardown too. Tests: disconnect-mid-elicitation settles callTool (no hang); cancel() unit tests (resolves-without-remove, no-op once responded). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
49d6be6987
commit
5cabc1ecd7
3 changed files with 1232 additions and 124 deletions
|
|
@ -0,0 +1,90 @@
|
|||
import { describe, it, expect, vi } from "vitest";
|
||||
import type { ElicitRequest } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { ElicitationCreateMessage } from "@inspector/core/mcp/elicitationCreateMessage.js";
|
||||
|
||||
function urlRequest(): ElicitRequest {
|
||||
return {
|
||||
method: "elicitation/create",
|
||||
params: {
|
||||
mode: "url",
|
||||
url: "https://example.com/authorize",
|
||||
message: "Authorize to continue.",
|
||||
elicitationId: "elicit-1",
|
||||
},
|
||||
} as ElicitRequest;
|
||||
}
|
||||
|
||||
describe("ElicitationCreateMessage.completeIfPending", () => {
|
||||
it("resolves as accepted and removes when still pending", async () => {
|
||||
const resolve = vi.fn();
|
||||
const onRemove = vi.fn();
|
||||
const message = new ElicitationCreateMessage(
|
||||
urlRequest(),
|
||||
resolve,
|
||||
onRemove,
|
||||
);
|
||||
|
||||
message.completeIfPending();
|
||||
// respond() is async (it awaits the microtask in remove path); flush.
|
||||
await Promise.resolve();
|
||||
|
||||
expect(resolve).toHaveBeenCalledWith({ action: "accept" });
|
||||
expect(onRemove).toHaveBeenCalledWith(message.id);
|
||||
});
|
||||
|
||||
it("is a no-op once the elicitation has already been responded to", async () => {
|
||||
const resolve = vi.fn();
|
||||
const onRemove = vi.fn();
|
||||
const message = new ElicitationCreateMessage(
|
||||
urlRequest(),
|
||||
resolve,
|
||||
onRemove,
|
||||
);
|
||||
|
||||
await message.respond({ action: "cancel" });
|
||||
resolve.mockClear();
|
||||
onRemove.mockClear();
|
||||
|
||||
// A late completion notification must not re-resolve (respond() would throw
|
||||
// "already resolved").
|
||||
expect(() => message.completeIfPending()).not.toThrow();
|
||||
await Promise.resolve();
|
||||
expect(resolve).not.toHaveBeenCalled();
|
||||
expect(onRemove).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("ElicitationCreateMessage.cancel", () => {
|
||||
it("resolves a pending elicitation as cancelled without removing it", () => {
|
||||
const resolve = vi.fn();
|
||||
const onRemove = vi.fn();
|
||||
const message = new ElicitationCreateMessage(
|
||||
urlRequest(),
|
||||
resolve,
|
||||
onRemove,
|
||||
);
|
||||
|
||||
message.cancel();
|
||||
|
||||
// Settles the awaiting promise (so callTool unblocks on teardown) but does
|
||||
// not splice the queue — disconnect() clears it wholesale.
|
||||
expect(resolve).toHaveBeenCalledWith({ action: "cancel" });
|
||||
expect(onRemove).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("is a no-op once already responded to", async () => {
|
||||
const resolve = vi.fn();
|
||||
const onRemove = vi.fn();
|
||||
const message = new ElicitationCreateMessage(
|
||||
urlRequest(),
|
||||
resolve,
|
||||
onRemove,
|
||||
);
|
||||
|
||||
await message.respond({ action: "accept" });
|
||||
resolve.mockClear();
|
||||
|
||||
message.cancel();
|
||||
expect(resolve).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,259 @@
|
|||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import {
|
||||
ErrorCode,
|
||||
McpError,
|
||||
UrlElicitationRequiredError,
|
||||
} from "@modelcontextprotocol/sdk/types.js";
|
||||
import type {
|
||||
ElicitRequestURLParams,
|
||||
Tool,
|
||||
} from "@modelcontextprotocol/sdk/types.js";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { UrlElicitationLoopError } from "@inspector/core/mcp/urlElicitation.js";
|
||||
import type { CreateTransport } from "@inspector/core/mcp/types.js";
|
||||
|
||||
// The client is never connected in these tests, so the transport factory is
|
||||
// never invoked — a throwing stub satisfies the required environment seam.
|
||||
const noopTransport: CreateTransport = () => {
|
||||
throw new Error("transport should not be created in these tests");
|
||||
};
|
||||
|
||||
const elicitation: ElicitRequestURLParams = {
|
||||
mode: "url",
|
||||
url: "https://example.com/authorize",
|
||||
message: "Authorize to continue.",
|
||||
elicitationId: "elicit-1",
|
||||
};
|
||||
|
||||
const tool = {
|
||||
name: "trigger-url-elicitation",
|
||||
inputSchema: { type: "object" },
|
||||
} as Tool;
|
||||
|
||||
const okResult = { content: [{ type: "text", text: "done" }] };
|
||||
|
||||
type FakeClient = {
|
||||
callTool: ReturnType<typeof vi.fn>;
|
||||
request: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
|
||||
/**
|
||||
* Build an InspectorClient with its internal SDK client replaced by a fake, so
|
||||
* we can drive `callTool` through the URL-elicitation error path without a live
|
||||
* server. The client is never connected; `callTool` only needs the injected
|
||||
* `callTool`/`request` methods plus the (connection-independent) helpers.
|
||||
*/
|
||||
function makeClient(fake: FakeClient): InspectorClient {
|
||||
const client = new InspectorClient(
|
||||
{ type: "stdio", command: "noop", args: [] },
|
||||
{ elicit: { url: true }, environment: { transport: noopTransport } },
|
||||
);
|
||||
(client as unknown as { client: FakeClient }).client = fake;
|
||||
return client;
|
||||
}
|
||||
|
||||
/**
|
||||
* Count the failed `toolCallResultChange` events a client records. Used to lock
|
||||
* in the "record a failure exactly once" invariant on the terminal error paths.
|
||||
*/
|
||||
function trackFailedDispatches(client: InspectorClient): () => number {
|
||||
let count = 0;
|
||||
client.addEventListener("toolCallResultChange", (e) => {
|
||||
if (!e.detail.success) count += 1;
|
||||
});
|
||||
return () => count;
|
||||
}
|
||||
|
||||
describe("InspectorClient URL-elicitation error path", () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("surfaces the URL elicitation, then retries the call on accept", async () => {
|
||||
let attempt = 0;
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
attempt += 1;
|
||||
if (attempt === 1) {
|
||||
throw new UrlElicitationRequiredError([elicitation]);
|
||||
}
|
||||
return okResult;
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
|
||||
const pending = client.callTool(tool, {});
|
||||
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()).toHaveLength(1),
|
||||
);
|
||||
const queued = client.getPendingElicitations()[0];
|
||||
expect(queued.request.params).toMatchObject({
|
||||
mode: "url",
|
||||
url: elicitation.url,
|
||||
elicitationId: elicitation.elicitationId,
|
||||
});
|
||||
|
||||
await queued.respond({ action: "accept" });
|
||||
|
||||
const invocation = await pending;
|
||||
expect(invocation.success).toBe(true);
|
||||
expect(fake.callTool).toHaveBeenCalledTimes(2);
|
||||
expect(client.getPendingElicitations()).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("processes multiple required elicitations in order before retrying", async () => {
|
||||
const second: ElicitRequestURLParams = {
|
||||
...elicitation,
|
||||
elicitationId: "elicit-2",
|
||||
url: "https://example.com/second",
|
||||
};
|
||||
let attempt = 0;
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
attempt += 1;
|
||||
if (attempt === 1) {
|
||||
throw new UrlElicitationRequiredError([elicitation, second]);
|
||||
}
|
||||
return okResult;
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
|
||||
const pending = client.callTool(tool, {});
|
||||
|
||||
// First elicitation is presented; accept it.
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()).toHaveLength(1),
|
||||
);
|
||||
expect(client.getPendingElicitations()[0].request.params).toMatchObject({
|
||||
elicitationId: "elicit-1",
|
||||
});
|
||||
await client.getPendingElicitations()[0].respond({ action: "accept" });
|
||||
|
||||
// Only after the first resolves does the second appear.
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()[0]?.request.params).toMatchObject({
|
||||
elicitationId: "elicit-2",
|
||||
}),
|
||||
);
|
||||
await client.getPendingElicitations()[0].respond({ action: "accept" });
|
||||
|
||||
const invocation = await pending;
|
||||
expect(invocation.success).toBe(true);
|
||||
expect(fake.callTool).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("aborts the call (no retry) when the user cancels a required elicitation", async () => {
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
throw new UrlElicitationRequiredError([elicitation]);
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
const failedDispatches = trackFailedDispatches(client);
|
||||
|
||||
const pending = client.callTool(tool, {});
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()).toHaveLength(1),
|
||||
);
|
||||
await client.getPendingElicitations()[0].respond({ action: "cancel" });
|
||||
|
||||
await expect(pending).rejects.toThrow(/cancelled/i);
|
||||
expect(fake.callTool).toHaveBeenCalledTimes(1);
|
||||
// The abort records exactly one failed history entry — not zero, not a
|
||||
// duplicate from the generic catch.
|
||||
expect(failedDispatches()).toBe(1);
|
||||
});
|
||||
|
||||
it("aborts with a loop error when the server re-requests a completed URL", async () => {
|
||||
// The server keeps returning the same URL after the user completes it.
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
throw new UrlElicitationRequiredError([elicitation]);
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
const failedDispatches = trackFailedDispatches(client);
|
||||
|
||||
const pending = client.callTool(tool, {});
|
||||
|
||||
// First round: the URL is presented; the user completes it.
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()).toHaveLength(1),
|
||||
);
|
||||
await client.getPendingElicitations()[0].respond({ action: "accept" });
|
||||
|
||||
// The retry returns the same URL; rather than re-prompt, the call aborts.
|
||||
await expect(pending).rejects.toBeInstanceOf(UrlElicitationLoopError);
|
||||
await expect(pending).rejects.toMatchObject({ url: elicitation.url });
|
||||
// Only the initial call + one retry ran; the URL was presented just once.
|
||||
expect(fake.callTool).toHaveBeenCalledTimes(2);
|
||||
expect(client.getPendingElicitations()).toHaveLength(0);
|
||||
// The loop abort records exactly one failed history entry (the accepted
|
||||
// first round records nothing).
|
||||
expect(failedDispatches()).toBe(1);
|
||||
});
|
||||
|
||||
it("settles a pending error-path elicitation as cancelled on disconnect (no hang)", async () => {
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
throw new UrlElicitationRequiredError([elicitation]);
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
|
||||
const pending = client.callTool(tool, {});
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()).toHaveLength(1),
|
||||
);
|
||||
|
||||
// Tearing down mid-elicitation must settle the awaiting promise rather than
|
||||
// leave callTool hanging forever on a dropped queue.
|
||||
await client.disconnect();
|
||||
|
||||
await expect(pending).rejects.toThrow(/cancelled/i);
|
||||
expect(client.getPendingElicitations()).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("rethrows a -32042 error with no elicitations without queuing anything", async () => {
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
throw new McpError(
|
||||
ErrorCode.UrlElicitationRequired,
|
||||
"This request requires browser-based authorization.",
|
||||
);
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
|
||||
await expect(client.callTool(tool, {})).rejects.toMatchObject({
|
||||
code: ErrorCode.UrlElicitationRequired,
|
||||
});
|
||||
expect(client.getPendingElicitations()).toHaveLength(0);
|
||||
expect(fake.callTool).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rethrows an ordinary error unchanged", async () => {
|
||||
const fake: FakeClient = {
|
||||
callTool: vi.fn(async () => {
|
||||
throw new Error("boom");
|
||||
}),
|
||||
request: vi.fn(),
|
||||
};
|
||||
const client = makeClient(fake);
|
||||
const failed = vi.fn();
|
||||
client.addEventListener("toolCallResultChange", (e) => {
|
||||
if (!e.detail.success) failed();
|
||||
});
|
||||
|
||||
await expect(client.callTool(tool, {})).rejects.toThrow("boom");
|
||||
expect(failed).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
53
clients/web/src/test/core/mcp/urlElicitation.test.ts
Normal file
53
clients/web/src/test/core/mcp/urlElicitation.test.ts
Normal file
|
|
@ -0,0 +1,53 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
ErrorCode,
|
||||
McpError,
|
||||
UrlElicitationRequiredError,
|
||||
} from "@modelcontextprotocol/sdk/types.js";
|
||||
import type { ElicitRequestURLParams } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { getUrlElicitationsFromError } from "@inspector/core/mcp/urlElicitation.js";
|
||||
|
||||
const elicitation: ElicitRequestURLParams = {
|
||||
mode: "url",
|
||||
url: "https://example.com/authorize",
|
||||
message: "Authorize to continue.",
|
||||
elicitationId: "elicit-1",
|
||||
};
|
||||
|
||||
describe("getUrlElicitationsFromError", () => {
|
||||
it("returns the elicitations from a typed UrlElicitationRequiredError", () => {
|
||||
const error = new UrlElicitationRequiredError([elicitation]);
|
||||
expect(getUrlElicitationsFromError(error)).toEqual([elicitation]);
|
||||
});
|
||||
|
||||
it("returns the elicitations from a generic McpError carrying the -32042 data", () => {
|
||||
const error = new McpError(
|
||||
ErrorCode.UrlElicitationRequired,
|
||||
"This request requires browser-based authorization.",
|
||||
{ elicitations: [elicitation] },
|
||||
);
|
||||
expect(getUrlElicitationsFromError(error)).toEqual([elicitation]);
|
||||
});
|
||||
|
||||
it("returns an empty array for a -32042 McpError with no elicitations (non-spec)", () => {
|
||||
const error = new McpError(
|
||||
ErrorCode.UrlElicitationRequired,
|
||||
"This request requires browser-based authorization.",
|
||||
);
|
||||
expect(getUrlElicitationsFromError(error)).toEqual([]);
|
||||
});
|
||||
|
||||
it("returns null for a non -32042 McpError", () => {
|
||||
const error = new McpError(ErrorCode.InvalidParams, "bad params");
|
||||
expect(getUrlElicitationsFromError(error)).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null for a plain Error", () => {
|
||||
expect(getUrlElicitationsFromError(new Error("boom"))).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null for non-error values", () => {
|
||||
expect(getUrlElicitationsFromError("nope")).toBeNull();
|
||||
expect(getUrlElicitationsFromError(undefined)).toBeNull();
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue