feat(web): URL elicitation — explicit completion step + error-path (-32042) handling (#1415)

[cliffhall]

Closes #1415. Builds the URL-elicitation experience out in two related parts.

Screen capture

Testing against the Everything server (on pending PR that fixes server behavior: modelcontextprotocol/servers#4285).

elicitation-error-path.mov

1. Two-step completion (the original #1415)

Open in Browser previously fired onRespond({ action: "accept" }) synchronously the moment window.open returned — optimistic, since the inspector can't observe an external flow. Replaced with the two-step flow the panel was already stubbed for:

• Open in Browser opens the URL and moves the panel to its waiting state — no response sent.
• A revealed "I've completed it" sends accept; Cancel sends cancel. While waiting the open button relabels to Reopen in Browser.
• ElicitationUrlPanel gains onComplete; isWaiting is now wired (was hardcoded false).
• Removed the feat(web): render pending sampling/elicitation requests (#1407) #1411 accept-on-open code comment that pointed here.

2. Error-path URL elicitation (-32042)

Handles the spec's "URL mode with elicitation required error" flow (2025-11-25). When a tools/call returns a -32042 UrlElicitationRequired error, the inspector now surfaces the required URL elicitation(s) and retries the call, instead of showing a bare error.

• getUrlElicitationsFromError detects -32042 (typed UrlElicitationRequiredError or generic McpError).
• The carried elicitations are surfaced in order through the existing pending-elicitation modal (reusing the two-step UI above), then the original call is retried once all are accepted. Decline/cancel aborts with a clear message; a bounded retry count backstops a misbehaving server.
• Loop guard: if a retry's error re-requests a URL the user already completed this call, the inspector aborts with a typed UrlElicitationLoopError and a "URL elicitation loop" toast rather than re-prompting forever.
• A non-spec -32042 with no elicitations shows a dedicated toast linking to a new UrlElicitationErrorModal with the raw error body (mirrors the output-schema-validation toast).
• ElicitationCreateMessage.completeIfPending() lets the optional notifications/elicitation/complete auto-accept an open URL elicitation; the completion-notification handler now resolves (was: only removed) the pending entry — also fixes a latent request-path orphan where the server's request went unanswered.
• Extracted attemptToolCall / dispatchFailedToolCall from callTool to support the retry loop without duplicating history bookkeeping.

Acceptance criteria (#1415)

• Opening the URL does not by itself resolve the elicitation.
• The user explicitly confirms completion to send accept.
• ElicitationUrlPanel's isWaiting state is wired.
• Tests cover open → waiting → complete and open → cancel.
• Removed the accept-on-open comment.

Testing

• npm run validate — 2053 unit/integration tests pass; per-file coverage gate green.
• npm run test:storybook — 333 pass.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 7s —— View job

---

Review: two-step URL elicitation completion

• Read changed files & diff
• Analyze correctness, edge cases, tests
• Post review feedback

Solid change — it fixes a real correctness bug (optimistic accept-on-open) cleanly. The two-step flow is wired correctly and the tests genuinely cover the new behavior. A few observations below; nothing blocking.

✅ What's right

• The core fix is correct. onOpenInBrowser no longer resolves the elicitation — it only flips isWaiting. Acceptance now requires the explicit onComplete action. This matches URL elicitation: error path handling + replace accept-on-open with explicit completion  #1415.
• once is applied to exactly the right handlers. onComplete and onCancel are wrapped (they resolve the request and must be idempotent — respond() throws on a second call), while onOpenInBrowser is intentionally not wrapped so the user can reopen. The busy/disabled lockout on the resolving buttons is preserved.
• Per-request state reset is handled. ElicitationUrlModalBody is keyed by request.id (PendingClientRequestModal.tsx:251), so the local isWaiting resets when the active request changes — no stale waiting state leaking across queued requests.
• Tests are meaningful, not just coverage filler: the open→waiting test asserts onElicitationRespond was not called, and the cancel test asserts it was called exactly once. Good.

💡 Suggestions (non-blocking)

1. Copy-URL users can't confirm completion. The "I've completed it" button is gated behind clicking Open in Browser in-app (only that sets isWaiting). A user who instead clicks Copy URL, pastes into a browser, and finishes the flow has no way to send accept — only Cancel is available. Consider also revealing the completion step after a copy, or otherwise letting the user reach the waiting state without the in-app window.open. Worth a follow-up if not in scope here.

2. Waiting hint is now two sentences in one line (ElicitationUrlPanel.tsx:88-91). Reads fine, but since there's already a Loader + "Waiting for completion..." plus the "I've completed it" button right below, the trailing "Confirm once you've finished in the browser." is somewhat redundant. Minor.

3. window.open return value is ignored. Pre-existing, but now that opening is a distinct first step, a popup blocked by the browser (window.open returns null) silently still moves the panel to the waiting state, where "Reopen in Browser" is the only recovery. Acceptable, just noting the failure mode is now slightly more visible to users.

Nit

• ElicitationUrlPanel.test.tsx switched the waiting-indicator assertions from exact string to regex (/Waiting for completion/) to tolerate the expanded copy — good call, keeps the test resilient to the wording.

Overall LGTM. The one I'd actually consider acting on is #1 (Copy-URL path can't confirm), since it's a reachable dead-end for a legitimate user flow.

[cliffhall]

Thanks for the thorough review! Addressed in c3c6ec0c.

1. Copy-URL dead-end — fixed. Good catch; that was a reachable dead-end. onCopyUrl now also flips isWaiting, so copying the URL (paste-into-browser flow) reveals "I've completed it" just like Open in Browser does. Added a modal test (reveals the completion step after Copy URL so a copy-paste flow can accept) covering copy → complete → accept. This also mitigates suggestion #3: a blocked popup is no longer a Cancel-only dead-end — the user can Copy URL and still confirm completion.

2. Redundant waiting hint — fixed. Trimmed back to just Waiting for completion...; the "I've completed it" button right below already conveys the action. (The test's /Waiting for completion/ regex still matches.)

3. window.open return ignored — noted, leaving as-is. Still pre-existing and out of scope to fully solve here, but #1's fix softens the failure mode (Copy URL → complete works even when the popup is blocked). A proper "popup was blocked" affordance is worth a follow-up if we want to detect window.open() === null and surface it explicitly.

Nit (regex assertion) — agreed, keeping it.

Note for context: this branch now also carries the error-path (-32042) URL elicitation handling (the PR title/description were updated) — that builds on this same two-step UI, so the Copy-URL fix benefits it too.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 5s —— View job

---

Review: URL elicitation — completion step + error-path (-32042)

• Read changed files & diff
• Analyze correctness, edge cases, tests
• Post review feedback

This is a strong, well-documented PR. The two-step completion fix is correct and the Copy-URL dead-end from the prior round is properly closed. The new -32042 error-path is the bulk of the change and it's structured well — the refactor preserves the "record a failure exactly once" invariant and the retry/loop guards are sound. LGTM.

✅ What's right

• Dispatch-once invariant holds. Extracting dispatchFailedToolCall out of attemptToolCall means attemptToolCall now only throws; every terminal throw in callTool's loop (loop error, decline/cancel abort, generic/exhausted) calls dispatchFailedToolCall exactly once, while the continue (accepted-retry) path deliberately records nothing. Intermediate -32042s don't pollute history. Verified there's no path that throws without recording and no path that records twice. (inspectorClient.ts:1351-1456)
• Loop guard is correct. presentedUrls is populated per round, and a retry that re-requests an already-presented URL aborts with the typed UrlElicitationLoopError instead of re-prompting. A new URL on retry (legitimate multi-step flow) is allowed through — only repeats trip the guard. The MAX_URL_ELICITATION_RETRIES cap backstops the genuinely-new-URL-forever case. Both are exercised by tests.
• completeIfPending() fixes a real latent bug. Switching the completion-notification handler from pending.remove() to pending.completeIfPending() (inspectorClient.ts:951-958) both unblocks the error-path awaitUrlElicitation and answers the server's still-open request on the request path — the prior remove() orphaned that request. The resolvePromise guard makes it a safe no-op against the manual-click race.
• App-layer branch ordering is right. UrlElicitationLoopError is checked first (it isn't an McpError, so getUrlElicitationsFromError returns null for it anyway), then the no-list length === 0 toast, then generic. Each branch returns so nothing falls through. (App.tsx:1312-1356)
• Tests are meaningful. inspectorClientUrlElicitation.test.ts covers accept→retry, multiple-in-order (asserting the second only appears after the first resolves), cancel-abort (no retry), the loop abort (asserting the URL was presented just once / callTool ran twice), no-list rethrow, and ordinary-error rethrow. Good branch coverage.

💡 Observations (all non-blocking)

1. Redundant block scope in attemptToolCall. The extracted method body is wrapped in a bare { … } block (inspectorClient.ts:1442-1444) — an artifact of keeping the diff's indentation stable. Harmless, but it can be unwrapped for clarity now that it's its own method. Fix this →

2. Retries-exhausted UX is the plain error panel. If a server returns -32042 with a new URL every round, after MAX_URL_ELICITATION_RETRIES the error still carries a non-empty elicitations list, so the App's no-list toast branch doesn't fire and the user lands on the generic error panel showing the raw -32042 message — not the friendlier toast/modal. This is a misbehaving-server edge case, but a one-line log/warn on exhaustion (mirroring the "no silent caps" practice) would make it diagnosable rather than looking like a normal failure.

3. Loop-path history entry isn't asserted. The loop test checks the rejection and callTool call count but not that dispatchFailedToolCall fired exactly once (no duplicate Tools-panel/history entry). Given the dispatch-once invariant is the trickiest property of this refactor, a toolCallResultChange listener assertion on the loop and abort paths (like the one the ordinary-error test already has) would lock it in.

4. completeIfPending swallows via void. void this.respond({ action: "accept" }) (elicitationCreateMessage.ts:87) drops the returned promise. It's guarded so respond won't throw "already resolved", and respond's only async work is resolvePromise(...) + remove(), so nothing meaningful is lost — just noting the rejection would be silently dropped if that ever changed.

Nit

• The two UrlElicitationErrorModal description strings (UrlElicitationErrorModal.tsx:49-51) differ only by the leading "toolName" prefix; could be a single template with a conditional lead-in, but the explicit form reads fine.

Nothing here blocks merge — items 1–3 are the ones worth a moment if you're touching this again.

[cliffhall]

Thanks — great review. Addressed the actionable items in 0754b43d.

1. Redundant block scope in attemptToolCall — fixed. Removed the leftover bare { } and de-indented the body one level. Pure cosmetics, as you noted.

2. Retries-exhausted UX — added a warn. On hitting MAX_URL_ELICITATION_RETRIES with a still-non-empty list, callTool now logs this.logger.warn({ tool, attempts }, "...still required URL elicitations after N attempts; giving up.") before the generic rethrow, so a looping/misbehaving server is diagnosable instead of looking like a normal failure. Left the user-facing surface as the generic error panel (the no-list toast intentionally only fires for an empty list) — the warn covers the "no silent caps" gap without inventing a new UI state for a misbehaving-server edge case.

3. Loop-path history assertion — added. Both the loop-abort and the cancel-abort tests now attach a toolCallResultChange listener and assert exactly one failed dispatch (trackFailedDispatches helper), locking in the dispatch-once invariant on the terminal paths — including that the accepted first round records nothing.

4. completeIfPending void — leaving as-is. Agreed it's safe: it's guarded so respond() can't throw "already resolved", and respond's only async work is resolvePromise(...) + remove(). Noted for the future if respond ever grows awaited work.

Nit (duplicate modal description strings) — leaving as-is for the explicit/readable form, as you suggested.

Re-validated: npm run validate (2054 tests) + npm run test:storybook (333) green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 58s —— View job

---

Review: URL elicitation — completion step + error-path (-32042)

• Read changed files & diff
• Analyze correctness, edge cases, tests
• Post review feedback

The follow-up commits land cleanly. Re-verified the items from the last round: the attemptToolCall block scope was removed (#1), the retries-exhausted logger.warn is in place (#2), and the loop/cancel-abort tests now assert a single failed dispatch via trackFailedDispatches (#3). The two-step completion flow and the Copy-URL fix are correct, and the -32042 retry/loop-guard logic reads soundly. LGTM.

I found one new concrete issue while verifying #1.

🔧 One thing worth fixing

attemptToolCall's body is still over-indented by 2 spaces. Commit 0754b43d removed the leftover bare { } block braces but only de-indented the first few lines — the rest of the method body (core/mcp/inspectorClient.ts:1464-1547, from const stringArgs through return invocation) is still at the old 6-space level while the method opens at 4 (inspectorClient.ts:1459-1463):

1463	    let convertedArgs: Record<string, JsonValue> = args;   // 4 spaces
1464	      const stringArgs: Record<string, string> = {};        // 6 spaces ← stale
...
1547	      return invocation;                                     // 6 spaces ← stale
1548	  }

This is purely cosmetic, but it's not caught by npm run validate: the web client's format:check only targets clients/web/src (prettier --check src), so nothing in core/ is prettier-checked. I confirmed npx prettier --check core/mcp/inspectorClient.ts flags the file and --write re-indents exactly this block. Worth running prettier over the file so the extraction looks like the rest of the codebase. Fix this →

Note: prettier also flags one pre-existing line (inspectorClient.ts:658, a new Error(...) wrap) unrelated to this PR — same root cause (core/ is outside the format scope). Out of scope here, but the core/ formatting gap is a latent maintenance hazard worth a separate look.

💡 Minor (non-blocking, unchanged from prior rounds)

• The two UrlElicitationErrorModal description strings (UrlElicitationErrorModal.tsx:50-51) and the toast-vs-modal lead-in differ only by the "toolName" prefix — fine as-is, you already opted for the explicit form.
• window.open() return value still ignored (a blocked popup silently enters the waiting state) — the Copy-URL → complete path mitigates it, and you've flagged a proper "popup blocked" affordance as a follow-up. Agreed.

Nothing blocks merge. The indentation cleanup is the only thing I'd act on.

[cliffhall]

Thanks — addressed the one actionable item in 9f7fb6b4.

🔧 attemptToolCall over-indentation — fixed. Ran prettier over the file; the method body (the const stringArgs → return invocation block) is now de-indented to match the 4-space method scope. The change is whitespace-only — verified with git diff --ignore-all-space (zero non-whitespace lines), so no behavioral change. Good catch that npm run validate misses this: clients/web's format:check only covers src/, so nothing under core/ is prettier-checked in CI.

Pre-existing line 658 + the core/ format gap — left out of scope. I reverted prettier's collapse of the line 658 new Error(...) wrap so this PR stays limited to the extraction cleanup, as you suggested. That does mean prettier --check core/mcp/inspectorClient.ts still warns on that one pre-existing line — but since core/ isn't in the CI format scope, it doesn't affect validate. The broader "core/ is outside format:check" gap is a real latent hazard and worth a separate PR (either widen the prettier scope to core/ or add a root-level format:check).

Minor items (modal description strings, window.open() return) — unchanged. As agreed in prior rounds: explicit modal strings kept for readability, and the blocked-popup affordance remains a follow-up (mitigated by the Copy-URL → complete path).

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 43s —— View job

---

Review: URL elicitation — completion step + error-path (-32042)

• Read changed files & diff
• Analyze correctness, edge cases, tests
• Post review feedback

This PR is in great shape after the prior three rounds — every actionable item from those rounds landed: the attemptToolCall block scope and over-indentation are gone (inspectorClient.ts:1442+), the retries-exhausted logger.warn is in place, the loop/cancel-abort tests assert single-dispatch via trackFailedDispatches, and the Copy-URL dead-end is closed. Re-verifying from a fresh checkout, the dispatch-once invariant, loop guard, and retry cap all read correctly and are well-tested. LGTM.

I did find one genuinely new edge case not raised before, plus a couple of small notes.

🔎 One new observation (non-blocking, edge case)

Error-path elicitation promise never settles on disconnect → callTool can hang. awaitUrlElicitation (inspectorClient.ts) returns a promise that only resolves via the ElicitationCreateMessage's resolve callback (on respond):

return new Promise<ElicitResult["action"]>((resolve) => {
  const message = new ElicitationCreateMessage(
    request,
    (result) => resolve(result.action),
    (id) => this.removePendingElicitation(id),
  );
  this.addPendingElicitation(message);
});

But disconnect() clears the queue wholesale without resolving or rejecting anything:

// inspectorClient.ts:1028
this.pendingElicitations = [];

If the connection is torn down (server crash, manual disconnect, transport close) while an error-path URL elicitation is awaiting confirmation, the queue is emptied, the modal disappears, but the underlying promise never settles — so await this.runUrlElicitations(...) hangs, and with it the whole callTool promise and the UI's toolCallState (it stays in its in-flight state, never reaching error). The pre-PR accept-on-open path resolved synchronously, so this dangling-await failure mode is new to the error path.

This is a real-but-narrow case (disconnect mid-elicitation). A clean fix would have disconnect() reject/resolve outstanding elicitation promises before clearing — e.g. give ElicitationCreateMessage a cancel()/abort() that resolves with cancel (or rejects), and have the teardown call it for each entry instead of dropping the array. That would also cover the request-path elicitation/sampling promises, which have the same latent dangling-await on teardown. Fix this →

💡 Minor notes (non-blocking)

• Multi-step flows that re-list a completed URL are treated as a loop. If a legitimate multi-step server returns round 2 = [URL-A (done), URL-B (new)], the loop guard finds URL-A in presentedUrls and aborts with UrlElicitationLoopError before reaching the new URL-B. The PR intentionally treats any repeat as a loop (and documents it), so this is a deliberate trade-off — just flagging that a server re-confirming prior URLs alongside a new one would trip it. Fine as a follow-up if it ever shows up against a real server.
• formatErrorDetails assumes serializable data. JSON.stringify({ code, message, data }, null, 2) would throw on a circular data (the App's try/catch is already left behind at that point — this runs inside the toast-building branch). McpError data from the wire is JSON, so circular is effectively impossible; noting only for completeness.

Prior-round minors (explicit modal description strings; window.open() return ignored, mitigated by Copy-URL → complete) remain as agreed.

Nothing here blocks merge — the disconnect-hang is the only item I'd consider, and only as a follow-up given how narrow it is.

[cliffhall]

Thanks — and good find on the disconnect hang. Fixed in fbe540a7.

Disconnect mid-elicitation → callTool hang — fixed. You're right that the error-path awaitUrlElicitation promise only settled via respond, so tearing down the queue left it (and callTool/toolCallState) dangling. Implemented your suggested shape:

• Added ElicitationCreateMessage.cancel() — settles a still-pending elicitation as { action: "cancel" }, without calling onRemove (the queue is cleared wholesale by the caller, so it must not splice mid-iteration). No-op once already resolved.
• disconnect() now cancel()s each pending elicitation before clearing the array, so the error-path await resolves cancel → runUrlElicitations returns cancel → callTool aborts with its "required URL elicitation was cancelled" failure → toolCallState reaches error instead of hanging. This also settles any request-path elicitation in flight (same loop).
• Also added the missing pendingElicitationsChange dispatch on teardown (it already fired pendingSamplesChange) so the modal closes on a transport-close disconnect, not just the client-null UI path.
• Tests: disconnect-mid-elicitation settles callTool (the test would time out if it still hung), plus cancel() unit tests (resolves-without-remove; no-op once responded).

Scoped to elicitations since that's the path that blocks callTool. Sampling has the same latent dangling-await you noted, but nothing awaits it the way callTool awaits an elicitation (it's the SDK request handler's promise, torn down with the transport), so I left it for a separate pass rather than widen this PR.

Multi-step re-list treated as a loop — yep, deliberate and documented; any repeat trips the guard even alongside a new URL. Left as-is; easy to relax to "abort only if every required URL is a repeat" if a real server ever needs it.

formatErrorDetails circular data — agreed it's effectively impossible (McpError data comes off the wire as JSON). Leaving as-is.

Prior-round minors (explicit modal strings, window.open() return) remain as agreed.

Re-validated: npm run validate (2057 tests) + npm run test:storybook (333) green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 36s —— View job

---

Review: URL elicitation — completion step + error-path (-32042)

• Read changed files & diff (focus on latest fbe540a7 disconnect fix)
• Analyze correctness, edge cases, tests
• Post review feedback

The disconnect-hang fix from the last round landed correctly and the PR is now in solid shape across all five review passes. LGTM.

✅ Verified this round

• cancel() settles without splicing. ElicitationCreateMessage.cancel() (elicitationCreateMessage.ts:99-105) resolves a still-pending elicitation as { action: "cancel" }, clears both resolvePromise and rejectCallback, and deliberately does not call onRemove — so disconnect()'s for (… of this.pendingElicitations) { cancel() } loop (inspectorClient.ts:1031-1034) can't mutate the array it's iterating. The wholesale this.pendingElicitations = [] that follows does the clearing. Correct ordering.
• The hang is genuinely closed. Tracing the error path: awaitUrlElicitation's promise resolves "cancel" → runUrlElicitations returns "cancel" → callTool's action !== "accept" branch fires dispatchFailedToolCall + throws the abort error → toolCallState reaches error instead of dangling. dispatchFailedToolCall touches no this.client, so it's safe even though disconnect() has torn the client down. The new test (inspectorClientUrlElicitation.test.ts:202-222) would time out if the promise still leaked — good regression guard.
• Only the in-flight elicitation is ever queued. runUrlElicitations awaits each awaitUrlElicitation serially, so at most one error-path elicitation sits in pendingElicitations at a time — disconnect can't orphan a not-yet-presented one in a multi-URL round. Settles cleanly.
• completeIfPending no-op guard holds. The if (this.resolvePromise) guard plus the modal's own once() keeps the manual-click ↔ completion-notification ↔ disconnect races from triggering respond()'s "already resolved" throw. Unit tests cover both the still-pending and already-responded cases (elicitationCreateMessage.test.ts).
• pendingElicitationsChange now dispatched on teardown (inspectorClient.ts:1049), so a transport-close disconnect closes the modal, matching the existing pendingSamplesChange.

💡 Two small observations (non-blocking, neither needs action)

1. cancel() resolves task-augmented request-path elicitations as "cancel" rather than invoking reject. A declined task-augmented elicitation normally calls reject(error) so the server's tasks/result sees the error; on disconnect, cancel() instead resolves { action: "cancel" } and drops rejectCallback unused. That's the right call here — the transport is gone, so neither the resolve nor a reject reaches the wire, and "cancel" is the more honest action for a teardown. Just noting the request-path semantics differ slightly from a user decline. No change needed.

2. The disconnect test doesn't assert the pendingElicitationsChange dispatch. It checks getPendingElicitations() is empty and the call rejects, but not that the event fired (the bit that actually closes the modal on a transport-close disconnect). A one-line listener assertion would lock in the new line 1049 the same way trackFailedDispatches locks in the dispatch-once invariant. Minor — the behavior is covered indirectly.

Prior-round items remain as agreed: multi-step re-list is deliberately treated as a loop; formatErrorDetails circular data is effectively impossible; the explicit modal description strings stay; window.open() return ignored (mitigated by Copy-URL → complete); and the core/ prettier-scope gap is a separate-PR follow-up.

Nothing blocks merge.