fix: [#986] follow tools/list pagination so no tools are dropped#987
Draft
sahrizvi wants to merge 4 commits into
Draft
fix: [#986] follow tools/list pagination so no tools are dropped#987sahrizvi wants to merge 4 commits into
sahrizvi wants to merge 4 commits into
Conversation
…abric, etc.) Three coupled fixes that together let altimate-code connect to MCP servers gated by short-lived bearer tokens — most prominently Microsoft Fabric Core MCP — without a local proxy or per-hour config edits. Issues addressed: - #791 (dynamic auth for remote MCP servers): static `headers` only. - #792 (external MCP servers not auto-activated at session start): mostly caused by OAuth dynamic-client-registration probes pre-empting valid bearer headers and ending the connect in a non-`connected` status. - A third defect surfaced during this work: the strict `ListToolsResultSchema` from `@modelcontextprotocol/sdk` rejects the `null` annotation hints that Fabric returns, causing `listTools()` to throw and the connection to be marked failed even after a successful initialize. None of these are altimate-specific — they all reproduce on upstream `opencode`. Changes: 1. `McpRemote.headersCommand: Record<string, string[]>` — header values produced by running an argv command (executed via `execFile`, not a shell, so values aren't subject to shell injection). Resolved on every connect so expiring tokens refresh automatically. Example: "headersCommand": { "Authorization": ["sh", "-c", "printf 'Bearer %s' \"$(az account get-access-token ... -o tsv)\""] } 2. OAuth provider is no longer attached when the user supplies an explicit `Authorization` header (statically or via `headersCommand`) and `oauth` is not specified. Prevents Entra ID's DCR rejection from short-circuiting a bearer-authenticated connection. Behavior is unchanged when `oauth` is explicitly configured. 3. `listTools()` is wrapped to retry with a permissive Zod schema on schema-validation errors. The fast path is unchanged for compliant servers; non-compliant servers (Fabric returns `null` for `annotations.{readOnlyHint,destructiveHint,idempotentHint,openWorldHint}`) no longer fail the connection. 4. `normalizeMcpConfig` was silently dropping `oauth` and (would have dropped) `headersCommand` when reconstructing remote entries. Both are now passed through. This was a pre-existing latent bug that made `oauth: false` configurations no-op at runtime. Tests: - 22 new tests across `test/mcp/mcp-bearer-auth.test.ts` (lenient schema, `headersCommand` resolution + execFile-not-shell semantics, Authorization detection, schema round-trip) and `test/mcp/headers.test.ts` (OAuth auto-disable on static bearer, on `headersCommand`, and explicit-OAuth override). All pass. - Zero regressions vs `origin/main` baseline on existing 185 MCP tests. - Live-verified end-to-end against `https://api.fabric.microsoft.com/v1/mcp/core` using `headersCommand` + `az account get-access-token`: server connected, 29 tools registered, no proxy required. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to f6b2e2d resolving all bot review comments: - `isSchemaError`: narrow to `name === ZodError|$ZodError` AND an `issues` array, so unrelated errors carrying an `issues` property no longer trigger the lenient retry (Copilot). - `resolveHeadersCommand`: wrap `execFile` failures with the header key (`headersCommand[<name>] failed: ...`) and append trimmed `stderr` so `mcp list` points to the exact failing command (Copilot). - Drop the now-redundant `headersCommand failed:` prefix at the call site since the thrown message already names the header. - Use Zod v4 `z.looseObject(...)` instead of deprecated `.loose()` for the lenient `tools/list` schemas (CodeRabbit). - `config.ts`: reword `headersCommand` comment to clarify it is argv via `execFile` (no shell interpolation); document why `normalizeMcpConfig` passes malformed array shapes through (schema rejects them with an actionable `invalid_type` rather than silently dropping) (Copilot). - Tests: remove unnecessary `as any` cast; assert against the production `LenientListToolsResultSchema` via `MCP._testing` instead of a duplicated copy; add an end-to-end `listToolsLenient` retry test using a real SDK `$ZodError`, and a test that array-shaped headers are rejected loudly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`listToolsLenient` fetched only the first `tools/list` page and ignored `nextCursor`, so servers that paginate their tool catalog had every tool past the first page silently dropped. Both the strict `client.listTools()` path and the permissive-schema fallback are affected. - Loop on `nextCursor`, concatenating `tools`, across pages. - Pass the cursor through on both the strict and lenient request paths; once a schema error switches to the lenient path, stay lenient for the remaining pages. - Cap at `MAX_TOOL_LIST_PAGES` (100) to guard against a repeating cursor. - Add tests covering strict multi-page pagination and pagination that continues through the lenient fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
suryaiyer95
force-pushed
the
fix/remote-mcp-fabric-compat
branch
from
5b5b8ad to
174896e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
listToolsLenientfetched only the firsttools/listpage and ignorednextCursor, so any server that paginates its tool catalog had every tool past the first page silently dropped — no error, no warning. Both the strictclient.listTools()path and the permissive-schema fallback were affected.Changes
nextCursor, concatenatingtoolsacross pages, inlistToolsLenient.MAX_TOOL_LIST_PAGES(100) to guard against a repeating/never-terminating cursor, logging a warning if hit.Notes
fix/remote-mcp-fabric-compat— the fix lives inlistToolsLenient, which that branch introduces. Rebase ontomainif it lands first.Fixes #986
Summary by cubic
Fixes dropped tools from paginated tool catalogs by following
nextCursorinlistToolsLenient. Applies to both strict and lenient paths; fixes #986.nextCursorand accumulatetoolsacross pages inlistToolsLenient.cursorto bothclient.listTools()and the lenientclient.request(); stay lenient after a schema error.MAX_TOOL_LIST_PAGES(100) and log a warning if hit.Written for commit 5b75ca4. Summary will update on new commits.