Skip to content

fix(mcp): preserve platform selectors across namespace rollout - #4468

Merged
kwakayama merged 2 commits into
mainfrom
fix/mcp-platform-namespace
Sep 10, 2026
Merged

fix(mcp): preserve platform selectors across namespace rollout#4468
kwakayama merged 2 commits into
mainfrom
fix/mcp-platform-namespace

Conversation

@kwakayama

@kwakayama kwakayama commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The Veryfront API can advertise veryfront__ platform names, while saved selections and runtime allow/deny rules still use legacy names. Request legacy discovery explicitly for the Veryfront API MCP source so staggered API deployments keep tool selection, policy checks, invocation, and stream identities stable.

Service and stream-time platform discovery both request legacy names. Optional listMeta is sent on every tools/list page. Generic and Studio MCP sources keep their existing requests, and tools/call payloads are unchanged. Older APIs that ignore naming metadata still work.

Validation: red/green regression tests for discovery, pagination, and old/new API policy behavior; 78 tests with 223 steps passed across agent services, remote MCP, MCP policy, and hosted tool access. Targeted typecheck, formatting, lint, module boundaries, and generated documentation checks passed. The final feedback batch additionally passed 129 focused test steps, full lint, targeted typecheck, and the semantic test classification audit. Protocol compatibility coverage lives in the integration suite; pure configuration assertions remain unit tests. Local review found no actionable issues.

Part of veryfront/veryfront-issue-inbox#1055. The issue remains open until consumer rollout and staging evidence are complete.

Summary by CodeRabbit

  • New Features

    • Improved compatibility between legacy and canonical tool names for Veryfront API integrations.
    • Existing tool selections, access policies, and streamed tool names remain stable while APIs use canonical naming by default.
    • Tool discovery remains consistent across paginated results, while tool actions continue using the selected tool identity.
  • Bug Fixes

    • Corrected tool-name translation so permitted actions continue reaching the expected remote tools.

Copilot AI lite review requested due to automatic review settings September 9, 2026 23:24

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ca8af5f7-1d02-4c57-ad0e-eae8f477501d

📥 Commits

Reviewing files that changed from the base of the PR and between 7002e1c and 1397617.

📒 Files selected for processing (8)
  • docs/architecture/21-agent-tool-registration-current-state.md
  • src/agent/service/mcp-server-config.test.ts
  • src/agent/service/mcp-server-config.ts
  • src/server/handlers/request/agent-stream.handler.test.ts
  • src/server/handlers/request/agent-stream.handler.ts
  • src/tool/remote-mcp.test.ts
  • src/tool/remote-mcp.ts
  • tests/integration/agent/mcp-naming-compatibility.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds list-only metadata support to remote MCP discovery. Veryfront API sources request legacy tool names on every tools/list page, while tools/call requests remain unchanged. Tests cover pagination, stream policies, naming modes, and tool execution.

Changes

MCP naming compatibility

Layer / File(s) Summary
Remote MCP list metadata
src/tool/remote-mcp.ts, src/tool/remote-mcp.test.ts
RemoteMCPToolSourceConfig accepts listMeta. listTools sends it on every paginated tools/list request and excludes it from tools/call requests.
Veryfront API metadata wiring
src/agent/service/mcp-server-config.ts, src/agent/service/mcp-server-config.test.ts, src/server/handlers/request/agent-stream.handler.ts
Veryfront API MCP sources configure veryfront/tool-names as legacy for tool discovery.
Naming compatibility validation
src/server/handlers/request/agent-stream.handler.test.ts, tests/integration/agent/mcp-naming-compatibility.test.ts, docs/architecture/21-agent-tool-registration-current-state.md
Tests cover legacy and canonical discovery names, policy filtering, exact call names, and paginated metadata. The architecture document records the behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 13976

Veryfront API discovery now requests legacy tool names while preserving unchanged tool-call payloads and non-Veryfront sources. The change is covered across pagination and policy behavior, with no actionable merge-blocking risk identified.

Sequence Diagram(s)

sequenceDiagram
  participant AgentStreamHandler
  participant RemoteMCPToolSource
  participant VeryfrontAPIMCPServer
  AgentStreamHandler->>RemoteMCPToolSource: Configure legacy list metadata
  RemoteMCPToolSource->>VeryfrontAPIMCPServer: tools/list with _meta and cursor
  VeryfrontAPIMCPServer-->>RemoteMCPToolSource: Tool names and nextCursor
  RemoteMCPToolSource->>VeryfrontAPIMCPServer: tools/call without _meta
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 7 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preserving platform selectors during the MCP namespace rollout.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 7 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mcp-platform-namespace

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-09-09T23:38:28.414976Z 1397617 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 289 2304 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

@gitar-bot

gitar-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

Gitar is working

Gitar

Copy link
Copy Markdown
Contributor Author

Code Review: 80/100 — Good, small well-scoped fix; one test doesn't cover what it claims to

Strengths

  • Narrow, additive change: listMeta on RemoteMCPToolSourceConfig is opt-in per source, so Generic and Studio MCP sources are untouched, and it's correctly excluded from tools/call (only sent on tools/list), matching the PR description.
  • createRemoteMCPToolSourceWithFetch's params-building logic (src/tool/remote-mcp.ts:1028-1036) is correct for all four combinations of cursor/listMeta presence, including sending _meta on every page (verified against mcp-server-config.test.ts and remote-mcp.test.ts expectations).
  • Architecture doc (docs/architecture/21-...md) is updated in the same PR to describe the new behavior — good hygiene.
  • Correctly scoped to the Veryfront API source only (mcp-server-config.ts), which is the one source whose backing API is rolling out the namespace change.

Concerns

  • The new test "keeps legacy invocation and deny policies across API naming versions" (mcp-server-config.test.ts:157) loops over supportsNamingMode of [true, false] but the mock's canonical flag is supportsNamingMode && meta !== "legacy". Since the real client under test always sends _meta: {"veryfront/tool-names": "legacy"}, meta !== "legacy" is always false, so canonical is false in both iterations — the two loop passes are behaviorally identical. This test doesn't actually exercise an "old API" vs. "new API" distinction as its name/PR description claim; it only proves the metadata is sent, which remote-mcp.test.ts already covers. Worth either removing the loop or adding a case where the mock server ignores _meta and returns canonical names anyway.
  • That missing case matters because there's no client-side fallback: normalizeToolListPage/wrapRemoteToolSourceWithMcpPolicy do exact string matching on tool names with no veryfront__ stripping anywhere in src/. If a deployed API instance ignores the _meta hint mid-rollout and returns canonical names regardless, persisted selectors and the deny/allow policy lists would silently stop matching — the exact failure mode this PR is meant to prevent, but only covered by trusting server compliance rather than defense-in-depth. This is presumably intentional given the linked issue is deliberately left open pending staging evidence, but it's worth being explicit in the PR description that this fix depends entirely on the API honoring the hint, with no local mitigation if it doesn't.
  • Minor: mcp-server-config.test.ts reorders/adds imports without alphabetical or grouping consistency (assertEquals import now sits after unrelated imports) — trivial, but check ci (lint)/ci (format) pass since those jobs were still queued/in-progress at review time.

Actionable suggestions

  1. Rework the supportsNamingMode test loop to actually assert differing behavior (e.g., an "ignores metadata" branch), or drop the loop if only the compliant-server case is meant to be covered.
  2. Consider noting the "no client fallback if the server ignores _meta" tradeoff explicitly in the PR description/linked issue, since that's the crux of the staged-rollout risk.

Otherwise this is a clean, well-documented, backward-compatible fix. Nice to see the architecture doc updated alongside the code.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e5a664b141

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/agent/service/mcp-server-config.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is narrowly scoped to discovery request metadata, includes targeted regression tests (pagination and policy behavior), and does not alter tools/call request construction beyond existing behavior.

Pull request overview

This PR keeps remote MCP tool discovery stable during the rollout from legacy tool names (for example get_file) to canonical platform-prefixed names (for example veryfront__get_file) by allowing the Veryfront API MCP source to request legacy naming explicitly via per-page discovery metadata.

Changes:

  • Extend RemoteMCPToolSourceConfig with optional listMeta, sent as params._meta on every tools/list page (including paginated requests).
  • Configure the Veryfront API MCP source to request legacy tool names during discovery (_meta["veryfront/tool-names"] = "legacy"), without changing tools/call payload behavior.
  • Add regression tests covering pagination + metadata preservation, and policy stability across old/new API naming behavior; document the behavior in architecture docs.
File summaries
File Description
src/tool/remote-mcp.ts Adds listMeta and ensures it is included on every tools/list request page.
src/tool/remote-mcp.test.ts Tests that discovery metadata is sent on every page and is not added to tool calls.
src/agent/service/mcp-server-config.ts Sets listMeta for Veryfront API MCP discovery to request legacy tool names.
src/agent/service/mcp-server-config.test.ts Verifies legacy allow/deny and invocation stability across API naming versions.
docs/architecture/21-agent-tool-registration-current-state.md Documents why and how legacy naming metadata is requested for Veryfront API discovery.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 13976174ee

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@sonarqubecloud

sonarqubecloud Bot commented Sep 9, 2026

Copy link
Copy Markdown

@kwakayama
kwakayama added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit 25ad140 Sep 10, 2026
73 checks passed
@kwakayama
kwakayama deleted the fix/mcp-platform-namespace branch September 10, 2026 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants