fix(everything): prevent URL elicitation error-path infinite loop

[cliffhall]

Description

Fixes an infinite-loop bug in the everything server's trigger-url-elicitation tool error path (errorPath=true).

Previously the tool threw UrlElicitationRequiredError (-32042) carrying the same URL that failed, and re-threw it on every retry. A client would complete the "prerequisite", retry, hit the identical error, and loop — tripping its own loop detection ("The server requested the same URL again after you completed it").

Two changes break the loop:

1. The prerequisite elicitation now points at a different URL than the failing request (https://modelcontextprotocol.io).
2. The server records that a prerequisite was issued for a request, keyed on the inputs a client resends verbatim on retry (sessionId + url + caller-supplied elicitationId). When the client satisfies the prerequisite and retries the same call, the retry is recognized, ignores errorPath, and proceeds via the normal request path instead of re-throwing.

Server Details

• Server: everything
• Changes to: tools (trigger-url-elicitation), tests, docs

Motivation and Context

Without this, the error-path demonstration of URL elicitation is unusable: any client following the spec's retry-after-prerequisite flow loops forever. This makes the reference behavior correct and demonstrates the intended error-path → satisfy-prerequisite → retry → proceed sequence.

How Has This Been Tested?

• Tested interactively in an MCP client: the error path now resolves after satisfying the prerequisite and the original request proceeds without looping.
• Unit tests (src/everything/__tests__/tools.test.ts): the -32042 case asserts the prerequisite points at a different URL than the failing request; a new test performs a plain retry (identical args, no echoed id) and asserts the retry takes the request path (elicitation/create) instead of throwing.
• npm run build and full vitest suite pass (105 tests).

Screen capture

elicitation-error-path.mov

Breaking Changes

None. Behavior change is confined to the errorPath=true retry flow.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Protocol Documentation
• My changes follows MCP security best practices
• I have updated the server's README accordingly (server docs under src/everything/docs/)
• I have tested this with an LLM client
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have documented all environment variables and configuration options

Additional context

Design note on retry correlation: a real client retries with the original tool-call arguments and does not echo the prerequisite's server-generated elicitationId. The marker is therefore keyed on stable resent inputs rather than the resolved/random elicitationId (which would change per call and never match). The marker is one-shot — cleared on the recognized retry — so a fresh attempt re-arms the error path.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Review: prevent URL elicitation error-path infinite loop

Thanks @cliffhall — the core fix is sound and well-documented. The two-part approach (point the prerequisite at a different URL and record a one-shot marker so the retry proceeds via the request path) correctly breaks the loop, and the new test exercises the originating-call → retry sequence. A few things worth addressing, one of them before merge.

---

🔴 Blocking: literal NUL bytes embedded in the source file

The diff renders as Bin 4935 -> 8217 bytes and file reports the source as data, not text. The reason is line 115:

const errorPathKey = `${sessionId}\x00${url}\x00${requestedElicitationId ?? ""}`;

The two separators between sessionId/url/elicitationId are raw NUL (\x00) bytes, not spaces. cat -v confirms it: ${sessionId}^@${url}^@${requestedElicitationId ?? ""} with NULs at byte offsets 4796 and 4803.

Using NUL as a collision-proof delimiter is a fine idea (it can't appear in a URL or session id), but embedding the raw byte in source has real downsides:

• Git treats the file as binary, so the actual logic change is invisible in the PR diff — no reviewer can see what changed inline.
• The file is misclassified by tooling (file → data), and some editors/formatters/linters choke on or silently strip control bytes.

Use the escape sequence instead of a literal byte — identical runtime behavior (still a collision-proof delimiter), but the file stays plain text and diffs become reviewable:

const errorPathKey = `${sessionId}${url}${requestedElicitationId ?? ""}`;

(Or a visible delimiter like \n / | if you don't strictly need NUL.) Fix this →

---

🟡 Unbounded growth of issuedErrorPathElicitations

issuedErrorPathElicitations (line 64) is a module-level Set. An entry is added on the originating error-path call (line 144) and removed only on a recognized retry (line 140). But a client that triggers the error path and then never retries — declines the prerequisite, gives up, or the session ends — leaves its key in the Set forever. In a long-lived Streamable-HTTP process serving many sessions, this grows without bound.

For a reference/demo server this is low-severity, but consider a Map<string, timestamp> with TTL-based eviction, or at least a comment flagging it as a deliberate demo simplification.

🟡 Module-global mutable state & test isolation

The Set lives at module scope, shared across every McpServer instance in the process. The session-scoped key makes cross-session collisions unlikely, so it's functionally OK here — but note the tests never reset it between cases; they avoid collisions only because each test happens to use a distinct sessionId/elicitationId. That's fragile to future edits and test reordering. A beforeEach that clears the Set (via an exported reset helper) would make the suite robust:

// e.g. export a __resetErrorPathState() for tests, call it in beforeEach

🟢 Nits

• Identical errorPath=true calls silently toggle between the error path and the request path. A caller re-running the error-path demo without having satisfied anything is treated as a "retry" and proceeds via the request path. This is inherent to keying on resent inputs and is covered in the docstring, so it's acceptable — just slightly surprising. Worth a one-line note in docs/features.md that the marker is one-shot per (session, url, id).
• Prerequisite reuses the caller's message (line 148 → "Authorization is required to continue.") while pointing at https://modelcontextprotocol.io. Minor semantic mismatch; a dedicated prerequisite message would read more clearly.

---

Summary

The loop fix is correct and the rationale is well captured in comments/docs. The NUL-byte delimiter is the one thing to change before merge — not for correctness, but because it makes the file binary and hides the diff. The memory-growth and test-isolation points are worth a follow-up but aren't blockers for a demo server.

I reviewed the logic statically; I did not run the suite in this pass (the PR reports 105 passing).
· branch feat/url-elicitation-error-path

[cliffhall]

Thanks for the thorough review — all points addressed in 5e11559.

🔴 NUL bytes (blocking): Fixed. The delimiters were meant to be spaces but ended up as raw NUL bytes during editing, which is why git flagged the file binary and hid the diff. Replaced them with the \\u0000 escape sequence — same collision-proof delimiter at runtime, but the source is plain UTF-8 again and the file now renders inline in the PR diff (76/8, no longer Bin).

🟡 Unbounded growth of issuedErrorPathElicitations: Kept the Set for the reference server but added a comment calling out the tradeoff explicitly and pointing at the production approach (a Map<string, timestamp> with TTL eviction). Entries are still only removed on a recognized retry, so this is a deliberate demo simplification rather than an oversight.

🟡 Module-global state / test isolation: Added an exported __resetIssuedErrorPathElicitations() helper and a beforeEach in the trigger-url-elicitation describe block that clears it, so the suite no longer relies on each case happening to use a distinct session/id.

🟢 Nits:

• One-shot toggle surprise — documented in docs/features.md: the marker is one-shot per (session, url, elicitationId) and cleared on the recognized retry.
• Prerequisite message — now uses a dedicated message ("Open this link to satisfy the prerequisite, then retry the request.") instead of reusing the caller's, fixing the semantic mismatch with the modelcontextprotocol.io URL.

Full vitest suite still green (105 passing) and npm run build clean.

[cliffhall]

@claude review

[claude]

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

---

Re-review: prevent URL elicitation error-path infinite loop

Thanks @cliffhall — I re-reviewed 5e11559 against the prior feedback. Every point from the last pass is resolved, and I confirmed the build and tests locally.

✅ Verified fixes

Prior finding	Status
🔴 Literal NUL-byte delimiters (file rendered as binary)	Fixed. Line 128 now uses `` escapes. file reports the source as `JavaScript source, Unicode text, UTF-8 text` (no longer `data`), and the diff renders inline.
🟡 Unbounded Set growth	Addressed. Lines 64–68 document it as a deliberate demo simplification and point at the production approach (Map<string, timestamp> + TTL eviction).
🟡 Module-global state / test isolation	Fixed. Exported __resetIssuedErrorPathElicitations() (line 76) + beforeEach reset in the describe block (lines 714–718). The suite no longer relies on incidentally-distinct ids.
🟢 One-shot toggle surprise	Documented in docs/features.md — marker is one-shot per (session, url, elicitationId), cleared on the recognized retry.
🟢 Prerequisite reused caller's message	Fixed. Dedicated message at line 161–162 ("Open this link to satisfy the prerequisite, then retry the request."), matching the modelcontextprotocol.io URL.

The new retry test (lines 965–1022) exercises the full sequence well: first call throws -32042 with sendRequest never invoked, then the identical retry takes the request path (elicitation/create) — and it deliberately omits elicitationId to prove correlation rests on stable inputs (session + url), not the per-call random id.

Local verification

• npm run build — clean (tsc + docs copy).
• vitest run __tests__/tools.test.ts — 59 passed.
• Confirmed source is plain UTF-8 text and the changed logic is now reviewable inline in the diff.

Optional follow-up (non-blocking)

The error-path key incorporates requestedElicitationId ?? "", so a caller who does pass an explicit elicitationId and a caller who omits it are tracked under different keys — correct, and matches the documented contract. No change needed; just confirming the keying is intentional and consistent with the test that omits the id.

LGTM. 👍

--- · branch feat/url-elicitation-error-path

[cliffhall]

Thanks for the re-review and the local verification — glad it's all confirmed.

On the optional follow-up: yes, the split is intentional. An explicit-elicitationId caller and an omit-elicitationId caller are deliberately tracked under different keys — the contract is "retry with the same arguments," so an explicit id is part of the request identity that must match on retry, while the omit case correlates on session + url alone (the random fallback is never used in the key, precisely so it stays stable across the retry). No change needed, just confirming it's by design.

[olaservo]

Thanks for finding and resolving this!