Repository navigation
fix(client): report rate limiting during MCP endpoint discovery - #3443
Conversation
When every MCP discovery probe got HTTP 429, discovery reported "None responded to MCP protocol" with the wrong-path hint. Stop at the first 429 (or 503 with Retry-After) and throw AgentRateLimitedError with the status, URL and Retry-After instead of probing the remaining candidates. Closes #3442 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean fix for adcp-client#3442 (MCP endpoint discovery now fails closed on 429/503 with AgentRateLimitedError).
No blocking or medium findings. Both connect paths route through createNegotiatedClient so Retry-After plumbing is consistent; probeModernMCPConnection re-throws 429/503 while preserving the v1 fallback only for status-less failures; tests cover 429±Retry-After, 503+Retry-After, 404 control, and constructor sanitization (credentials/query stripped, seller header text never reaching the message). No new unbounded I/O; existing timeout/abort preserved. New code: 'RATE_LIMITED' aligns with existing transient retry policy.
high_risk is true (src/lib/protocols/mcp-modern.ts modified matching src/lib/protocols/**), but row 5 does not fire — no medium findings. No deletions. gated_paths is false.
Medium findings
None.
Non-blocking nit:
- .changeset/mcp-discovery-rate-limit.md:2 — Changeset typed
patchfor a new public export (additive, semver-minor); not a wire-break, so low severity only.
Falls through to row 9.
|
On the Ladon nit ( |
Closes #3442
Problem
When every MCP discovery probe got HTTP 429,
discoverMCPEndpointWithConnectionPolicykept probing the other candidate URLs, then threwFailed to discover MCP endpoint … None responded to MCP protocolwith the #1234 hint thatagent_urilacks the MCP path. The path was fine; the seller was rate-limiting the caller.Change
SingleAgentClient.ts): a probe returning 429, or 503 withRetry-After, stops discovery at that URL and throwsAgentRateLimitedError. No further candidates are probed and the wrong-path hint is not added. The 401 path, the generic failure (including plain 503 withoutRetry-After), and success paths are unchanged.AgentRateLimitedError(src/lib/errors, exported from@adcp/sdk):code: 'RATE_LIMITED'(the AdCP transient code), withstatus,agentUrl(credentials, query, and fragment stripped),retryAfterandretryAfterMs.src/lib/errorshad no client-side rate-limit class.RateLimitedErrorinserver/decisioningis the server-side throw helper and does not fit here.retryAfteris kept and echoed only when it is valid delta-seconds or an IMF-fixdate, so seller-controlled header text does not reach the message.mcp-modern.ts): the MCP SDK'sSdkHttpErrorcarries the status but not the response headers.createNegotiatedClientrecords the last 429/503 response'sRetry-Afterin its transport fetch and attaches it as a non-enumerableretryAfteron the connect error it throws. Nothing else in the fetch chain changes.Scope: the A2A agent-card loop (
fetchA2ACanonicalUrl) is a different shape. It fetches cards through the A2A client and surfaces that client's error, not a wrong-path hint, so it is unchanged here.Not addressed: the issue's secondary note about per-client session volume in SDK 17 (two
initializesessions per new client and call) is separate and left for follow-up.Tests
test/lib/discover-mcp-endpoint-rate-limit.test.js(real HTTP stub server):Retry-After: 30:AgentRateLimitedErrorwith status 429, URL,retryAfter30 andretryAfterMs30000. The message names the rate limit and HTTP 429 and has no "None responded" or wrong-path hint. Only the first candidate path was requested.Retry-After: still a rate-limit error, and probing still stops.Retry-After: 5: rate-limit error, and probing stops.Retry-Afteris not echoed, URL credentials and query are stripped, and an HTTP-date is parsed.Ran
npm run build:lib,npm run typecheck,npm run lint,npm run lint:boundaries,prettier --check,npm run ci:docs-check, andTEST_CONCURRENCY=2 npm run test:fileon the new test plus 16 related discovery/auth/MCP-negotiation suites (228 tests, all passing). The pre-push hook passed normally;--no-verifywas not used.🤖 Generated with Claude Code