feat(mcp): support remote dual-era negotiation - #2670
Conversation
91b3d4e to
8be1e7d
Compare
📝 WalkthroughWhat this PR solvesThis PR adds remote MCP dual-era protocol negotiation for legacy and It preserves legacy omitted It also:
Source of truth and solution sizeThe PR extends existing MCP configuration, discovery, Runtime, and Desktop paths. It does not create a parallel callable or discovery authority. The immutable Tool snapshot remains the single callable authority. Discovery uses generation-fenced refresh ownership. Desktop uses shared draft conversion helpers. The solution is coherent with the stated requirements. The added protocol states, fallback branches, subscription lifecycle, and validation logic are necessary for dual-era interoperability and SEP-2243 enforcement. Some test fixtures are large because they cover transport failures, cancellation, acknowledgements, reconnects, and race conditions. No clear deletion or simplification is identified without reducing regression coverage. The shared Validation and concrete risksThe PR reports passing MCP, Storage, Runtime, Desktop, Desktop typecheck, build, typecheck, lint, formatting, and CI checks. These results are not independently verified here. No live public MCP endpoint or Electron click-through smoke test was run. Key risks include:
Material changes in user-visible behavior, public contracts, security boundaries, releases, licensing, or governance require independent human review under repository policy. Complexity deltaThe PR adds:
The PR removes or consolidates:
Total maintenance complexity increases because the PR adds protocol eras, lifecycle states, and validation rules. The increase is justified by the compatibility, safety, and consistency requirements. The current evidence supports this conclusion, subject to review of the final diff and test results. Review-relevant risksThe current diff has apparent effects on user-visible Desktop behavior, MCP configuration migration, public MCP types and functions, remote transport behavior, and SEP-2243 security validation. Material changes in these areas require independent human review under repository policy. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughMCP configuration version 2 adds remote protocol preferences and migration rules. Runtime management adds protocol negotiation, SSE fallback, subscriptions, refresh coordination, and header validation. Desktop editing, localization, fixtures, and tests now use the shared configuration model. ChangesMCP dual-era protocol support
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to This PR adds dual-era remote MCP negotiation, fallback, subscriptions, and related import and desktop behavior. The current head still has bounded correctness and diagnostics issues that can hide invalid configuration, reject a valid imported server, expose unsanitized server text in errors, and misstate verification status, so it should not merge until these are fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Astro-Han This branch is now restacked on the reviewed #2663 discovery authority and current main. MCP, Storage, Runtime, Desktop, and Desktop typecheck all pass sequentially; the dual-era snapshot remains one immutable callable authority. I have moved it out of Draft while keeping the #2663 dependency explicit. Could you review the current head when convenient? |
PR Summary by QodoSupport dual-era remote MCP negotiation and unified discovery
AI Description
Diagram
High-Level Assessment
Files changed (25)
|
Code Review by Qodo
1.
|
Astro-Han
left a comment
There was a problem hiding this comment.
The dual-era negotiation and single-snapshot publication design are strong: protocol evidence, fallback boundaries, generation fencing, and config migration all stay at their existing owners. I found two concrete P2 regressions in the shared notification-refresh path. First, legacy unsolicited tools/list_changed compatibility is now accidentally gated by the server capability declaration. Second, the three-pass storm budget is retained for the entire connection lifetime, so four ordinary tool-list changes permanently freeze the snapshot. Both can be fixed at @maka/mcp without adding another registry or refresh authority.
I did not carry forward the separate cross-process config TOCTOU concern as a finding: the production Desktop constructs one store instance and the repository does not currently define a multi-writer contract, so a file-locking layer would be disproportionate without that requirement.
AI-assisted review: Codex coordinated two independent reviewer passes and an OpenCode Go DeepSeek V4 Flash high-effort adversarial pass. I verified the exact head 8be1e7dca1cfe2635603229835bb93427d13a931, the pinned SDK handler registration, production callers, the refresh state machine, and current CI. No local tests were run in this review.
中文审查
双时代协商和单一工具快照发布的总体设计很好:协议证据、fallback 边界、generation fence 和配置迁移都留在已有责任层。本轮确认了两个 P2:legacy unsolicited tools/list_changed 被 SDK 的 capability 声明意外门控;三次刷新预算被保留到整个连接生命周期,第四次正常工具变化会永久冻结快照。两项都应在 @maka/mcp 修复,不需要新增 registry 或刷新权威。
另一个跨进程配置 TOCTOU 候选没有保留为 finding:生产 Desktop 只创建一个 store,仓库目前也没有定义多写入者契约;在缺乏真实需求时引入文件锁不符合奥卡姆剃刀。
本次为 AI 辅助审查:Codex 协调两轮独立 reviewer 审查及 OpenCode Go DeepSeek V4 Flash high 对抗性复核,并核验了精确 head、pin 的 SDK handler 注册、生产调用方、刷新状态机和当前 CI。本轮未运行本地测试。
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d88d6b0c-b725-4cdd-807c-9255b7ba3818
📒 Files selected for processing (25)
apps/desktop/src/main/__tests__/mcp-import.test.tsapps/desktop/src/main/__tests__/mcp-ipc-main.test.tsapps/desktop/src/main/__tests__/mcp-page-model.test.tsapps/desktop/src/main/__tests__/mcp-runtime-e2e.test.tsapps/desktop/src/main/e2e-fixture/scenarios-modules.tsapps/desktop/src/renderer/locales/mcp-copy.tsapps/desktop/src/renderer/mcp-catalog.tsapps/desktop/src/renderer/mcp-import.tsapps/desktop/src/renderer/mcp-page-model.tsapps/desktop/src/renderer/mcp-page.tsxapps/desktop/stories/module-hubs.stories.tsxdocs/architecture/mcp-runtime-architecture-draft.zh-CN.mdpackages/core/src/mcp.tspackages/mcp/src/__tests__/manager-fallback.test.tspackages/mcp/src/__tests__/manager-subscription.test.tspackages/mcp/src/__tests__/manager.test.tspackages/mcp/src/__tests__/modern-manager.test.tspackages/mcp/src/__tests__/sep-2243.test.tspackages/mcp/src/__tests__/tool-output-validation.test.tspackages/mcp/src/index.tspackages/mcp/src/sep-2243.tspackages/mcp/src/tool-output-validation.tspackages/runtime/src/__tests__/mcp-tools.test.tspackages/storage/src/__tests__/mcp-config-store.test.tspackages/storage/src/mcp-config-store.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up review on exact head 2de993e83. The latest microtask gate correctly closes the synchronous initial-refresh race, and the dual-era negotiation still keeps protocol evidence, fallback, generation fencing, and snapshot publication at the existing MCP manager authority.
The two previously posted P2 findings remain reproducible on this head, so I am not duplicating their inline threads: capability-less legacy tools/list_changed is still dropped by the SDK-gated handler, and refreshPasses still accumulates for the entire connection until the fourth ordinary change permanently suppresses refresh. The first-principles fix remains one manager-owned notification path per negotiated era and a burst-scoped—not connection-scoped—refresh budget.
The new inline P3 is independent and non-blocking.
AI-assisted review disclosure: Codex performed the final review using three independent reviewer passes and OpenCode Go DeepSeek V4 Flash (high effort) as an adversarial advisory pass. I verified the two state-machine failures and the UI validation path against exact head 2de993e83; no local tests were run, and current CI is green.
中文评论
对精确 head 2de993e83 的跟进审查。最新的 microtask gate 已正确修复同步 initial refresh race;dual-era negotiation 也仍将协议证据、fallback、generation fence 和 snapshot 发布留在既有 MCP manager 权威中。
此前两个 P2 在此 head 上仍可复现,因此不重复行内线程:未声明 capability 的 legacy tools/list_changed 仍被 SDK 门控丢弃;refreshPasses 仍按整个连接累计,第四次正常变化会永久 suppress refresh。更符合第一性原理的修复仍是:每个协商时代只有一个 manager-owned notification path,并把刷新预算限制在一次 burst,而不是整个 connection。
新增行内 P3 独立且不阻塞。
AI 辅助审查说明:Codex 使用三轮独立 reviewer 审查,并以 OpenCode Go DeepSeek V4 Flash(high effort)进行对抗性辅助审查;我已在精确 head 2de993e83 上核对两个状态机故障与 UI validation 路径。本轮未运行本地测试,当前 CI 为绿色。
Astro-Han
left a comment
There was a problem hiding this comment.
The dual-era negotiation now has a clean authority model: exact modern pins do not downgrade, fallback is limited to protocol-absence evidence, modern subscriptions validate honored filters, and refresh/snapshot/tool bindings are fenced by connection generation. The two earlier P2s—legacy list_changed gating and permanently accumulated refresh budget—are fixed with focused coverage.
No remaining P0-P2 findings on this exact head. The failing Windows recovery check points to an unrelated Runtime Host IPC fixture rather than the MCP diff; it should be rerun before merge.
AI-assisted review disclosure: two Codex reviewers independently reviewed exact head f3fa1d7, protocol/security/error paths, current CI, and all existing review threads.
中文说明
双时代协商现在保持清晰的权威边界:精确 modern pin 不会降级,fallback 只发生在有协议缺失证据时,modern subscription 会验证服务端实际接受的 filter,刷新、snapshot 和 tool binding 都受 connection generation 保护。上一轮两个 P2 已修复。当前没有剩余 P0-P2;无关的 Windows recovery 失败需在合并前重跑。
|
Could you please add screenshots of the Desktop UI/UX changes before this PR is merged? At minimum, please show:
One annotated composite image is fine. Thanks! Posted by Codex on behalf of Astro-Han. |
|
Added both requested Desktop screenshots to the PR body: the remote editor with SSE-forced Legacy protocol preference, and installed-server details showing the negotiated protocol. These are captures of the real Storybook component surfaces. I also added the repository-template AI use section and final squash trailer. |
f3fa1d7 to
a4d96cc
Compare
|
Rebased onto current main at d2d0121; the four discovery-ownership commits already merged through #2663 were dropped, while the dual-era transport/configuration delta and its review fixes were preserved. Local verification on the new head a4d96cc: full repository build and all workspace typechecks; MCP 115/115; Storage 815 passed / 16 skipped; Runtime 2940 passed / 6 skipped; affected Desktop MCP tests 15/15; full Biome format check and diff check. The simplification audit still finds one versioned config field, one negotiation owner, and no duplicate transport authority. The PR body already includes the requested Storybook screenshot and AI-use disclosure. |
|
The three red checks reduce to two known/non-MCP baseline failures, not the dual-era delta. Runtime Host failed only |
Astro-Han
left a comment
There was a problem hiding this comment.
The code review passes on exact head a4d96cc17: I found no remaining P0–P2 correctness issue. The rebase preserves one MCP-manager authority for negotiation, generation-fenced refresh, and callable snapshot publication; legacy and explicit modern protocol behavior remain correctly separated. All eight review threads are resolved, and both requested Storybook screenshots show the real Legacy and negotiated-protocol surfaces.
I am leaving this as COMMENT rather than APPROVE because the required checks are currently red. The Runtime Host failure matches the unrelated #3190 timing flake, the aggregate test follows it, and the Windows installed-CLI job fails before Host registration when the runner cannot load PowerShell Security/Get-Acl; nevertheless, this exact head must rerun green before merge.
The PR body declares Maka assistance, but its section contains literal \n escapes and none of the 18 substantive branch commits has the required Generated-by trailer. Please fix the Markdown and add Generated-by: Maka to the AI-authored commits, or explicitly identify any human-only exceptions. After provenance and CI are green, this head is otherwise ready for approval.
Reviewed with Codex as an AI-assisted code review. I verified the full rebased delta, negotiation/migration authority, fallback and refresh state machines, review threads, screenshots, CI logs, and commit provenance; no external model output was used.
中文说明
代码层面未发现剩余 P0–P2,8 个 thread 已解决,两张 Storybook 截图也确实覆盖 Legacy 与协商协议状态。但 required checks 仍有红灯,即使看起来是无关基线 flake,也需要在当前 head 重跑为绿。PR body 的 AI 段落含字面量 \n,18 个实质性 commits 都没有 Generated-by trailer;请补齐或明确纯人工例外。CI 与 provenance 修好后,代码本身可进入 APPROVE。
Astro-Han
left a comment
There was a problem hiding this comment.
One additional independent pass found a concrete import ambiguity, so my earlier statement that the code had no remaining P0–P2 needs this correction. The P2 is inline below; the rest of the prior review, including the CI, screenshots, and provenance gates, remains unchanged.
中文说明
独立交叉复核发现一个具体 import 歧义,因此更正上一条“无剩余 P0–P2”的结论:下面还有一个 P2。此前关于 CI、截图和 provenance 的其余结论不变。
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Route connect-time tool discovery through the refresh loop so buffered list-changed notifications join the same snapshot fence as refresh requests, keep the connect abort signal attached to discovery, and publish the latest valid snapshot when manual refreshes coalesce while skipping the stale intermediate result of a notification burst. Generated-by: Maka
The editor draft still carried the pre-unification command/args pair while the rest of the page and the validator had moved to the single commandLine field, so serialization round-trips mixed the two shapes. Route the draft through formatCommandLine/parseCommandLine like the rest of the editor. Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Codex
a4d96cc to
083536d
Compare
|
Review remediation is now on |
me2seeks
left a comment
There was a problem hiding this comment.
Resolved the earlier review threads on the current head; submitting these pending replies so the review record is complete.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 083536d9ee3b6c2e710b4852f045a0db12de2190.
The earlier direct-map ambiguity is fixed: a server literally named mcpServers is now recognized when its value is itself a command- or URL-based server definition, with a focused regression. The negotiation and migration authority remains coherent, all review threads are resolved, all checks are green, both requested UI screenshots are present, and all 19 commits carry the declared Maka/Codex provenance trailers. I found no remaining P0–P2 issues.
Non-blocking: the PR body still says the PR is a draft and waits for #2663, although both statements are now stale.
AI-assisted review disclosure: Codex reviewed the exact-head diff, remediation, tests, current CI and threads, screenshots, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
原先 mcpServers 作为真实 server ID 时的解析歧义已修复,并补了针对性回归。当前线程全部解决、CI 全绿、两张 UI 截图有效、19 个提交的 Maka/Codex trailer 完整,没有剩余 P0–P2。PR 正文中“仍为 draft、等待 #2663”已过时,但不阻塞批准。
Summary
auto.2026-07-28Streamable HTTP sessions, keep SSE fallback limited to typed pre-identity 404/405 failures, and expose the negotiated protocol in Desktop status.Refs #1650
Depends on #2663. This is a cross-fork follow-up, so it remains a draft against
main; until #2663 lands, the PR 2-only diff is:me2seeks/maka-agent@fix/1650-mcp-discovery-transaction...feat/1650-mcp-remote-dual-era-v2
Verification
mainthrough the updated fix(mcp): own discovery by connection generation #2663 stacknpm --workspace @maka/mcp test— 104/104npm run buildnpm run typechecknpm run lintnpm run format:checkI did not run a live public MCP endpoint or an Electron click-through smoke test.
Review focus
replaceToolSnapshot()remains the only callable publication transaction. Protocol status, subscription diagnostics, and renderer state are projections; they do not own Tool bindings or a separate revision. Every refresh signal advances the same connection-generation epoch, and publication is fenced before and after synchronous listeners.Modern stdio and config v3 remain the next RFC slice rather than being enabled here.
Checklist
Does this PR entail a change in behavior?
Visual evidence
The real Desktop Storybook surfaces cover both requested states. SSE fixes the protocol preference to Legacy; the installed-server inspector projects the protocol actually negotiated by the MCP manager.
AI use
Tool(s) and scope: Maka authored the existing dual-era MCP implementation, tests, verification, documentation, and visual evidence. OpenAI Codex authored the latest direct-map import ambiguity review remediation and its regression test.
Final squash trailers:
Generated-by: MakaGenerated-by: Codex