feat: model dual-era ChatGPT protocol selection - #3828
Conversation
dosubot
Bot
added
the
size:L
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughMCP host profiles now support explicit 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/host-config/canonicalize.ts (1)
888-937: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate stateless protocol pins against the advertised list.
The check excludes stateless pins. Therefore, this profile is accepted:
mcpProtocolVersion: "2026-07-28"withsupportedProtocolVersions: ["2025-11-25"].
MCPClientManagerthen sends the 2026 pin while it advertises only 2025 support. Validate every concrete pin when an advertised list exists. Keep the stateful-only derivation behavior after that validation.Add a canonicalization test for this conflicting 2026 pin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/host-config/canonicalize.ts` around lines 888 - 937, Update the cross-field validation in canonicalization to validate every concrete mcpProtocolVersion, including stateless versions, whenever supportedProtocolVersions or initialize.supportedProtocolVersions is present; throw the existing conflict error when the pin is absent from the advertised list. Preserve the current stateful-only derivation behavior for missing advertised lists, and add a canonicalization test covering a 2026 stateless pin conflicting with a 2025 advertised version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sdk/src/host-config/canonicalize.ts`:
- Around line 888-937: Update the cross-field validation in canonicalization to
validate every concrete mcpProtocolVersion, including stateless versions,
whenever supportedProtocolVersions or initialize.supportedProtocolVersions is
present; throw the existing conflict error when the pin is absent from the
advertised list. Preserve the current stateful-only derivation behavior for
missing advertised lists, and add a canonicalization test covering a 2026
stateless pin conflicting with a 2025 advertised version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62f7b958-05c9-425e-820c-2816c2a27af1
⛔ Files ignored due to path filters (2)
sdk/src/host-compat/catalog.generated.tsis excluded by!**/*.generated.*sdk/tests/__snapshots__/host-config-seed-host-template.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/__tests__/ProtocolTab.versionDropdown.test.tsxmcpjam-inspector/client/src/lib/__tests__/client-config-v2-mcp-profile.test.tsmcpjam-inspector/client/src/lib/client-config-v2.tsmcpjam-inspector/client/src/lib/host-config-field-schema.tssdk/src/host-config/canonicalize.tssdk/src/host-config/defaults.tssdk/src/host-config/host-connection.tssdk/src/host-config/host.tssdk/src/host-config/public-types.tssdk/src/host-config/sdk-evals-normalizer.tssdk/src/host-config/templates/seed-host-template.tssdk/src/host-config/types.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/tests/MCPClientManager.auto-protocol-reconnect.test.tssdk/tests/host-compat-market-hosts.test.tssdk/tests/host-config-canonicalize.test.tssdk/tests/host-config-sdk-evals-normalizer.test.tssdk/tests/host-connection.test.tssdk/tests/host.test.tssdk/tests/support/dual-era-fixture.test.ts
Internal previewPreview URL: https://mcp-inspector-pr-3828.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/tests/support/dual-era-fixture.test.ts">
<violation number="1" location="sdk/tests/support/dual-era-fixture.test.ts:58">
P3: The new `connectAutomatic()` helper duplicates `connectModern()` almost entirely — same handler/capture setup, same client identity, same `supportedProtocolVersions` list, and the same transport; only the `versionNegotiation` mode differs (and `connectLegacy()` shares the same skeleton too). Consider factoring the shared setup into a single parameterized helper that takes the negotiation options, so the fixture's client name/support-list/transport live in one place instead of three near-identical copies that can drift when the fixture evolves.</violation>
</file>
<file name="mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx">
<violation number="1" location="mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx:629">
P1: Editing a legacy host whose protocol data lives in the deprecated nested `initialize` envelope now drops that data from the frontend's connection path. `applyJsonToDraft` migrates by writing the sibling `clientInfo` / `supportedProtocolVersions` and forcing `initialize: undefined`, but the frontend resolver still reads only the nested path: `use-server-state.ts` (buildConnectDefaults) reads `mcpProfile?.initialize?.clientInfo` and `mcpProfile?.initialize?.supportedProtocolVersions`, and `resolveClientInfo` / `resolveSupportedProtocolVersions` in `client-config-v2.ts` do the same with no sibling fallback. So a user who opens the JSON editor and makes any edit to a legacy profile — with no change to the version/client fields — will have those fields cleared out of `mcpProfile` and, on the next connect, the accept-list and client identity silently stop being forwarded to the SDK. The reader side of this migration (`protocolToJson`) correctly prefers sibling-then-initialize, but the consumers that feed the actual connection were not updated to match, so the migrated sibling fields are never picked up. Consider updating the frontend resolvers/connect path to read `?? initialize?.*` (mirroring `protocolToJson`'s precedence) before committing the `initialize: undefined` migration, so migrated rows keep working.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| supportedProtocolVersions, | ||
| // Editing this section migrates the deprecated envelope to the sibling | ||
| // fields above; retaining both would recreate two sources of truth. | ||
| initialize: undefined, |
There was a problem hiding this comment.
P1: Editing a legacy host whose protocol data lives in the deprecated nested initialize envelope now drops that data from the frontend's connection path. applyJsonToDraft migrates by writing the sibling clientInfo / supportedProtocolVersions and forcing initialize: undefined, but the frontend resolver still reads only the nested path: use-server-state.ts (buildConnectDefaults) reads mcpProfile?.initialize?.clientInfo and mcpProfile?.initialize?.supportedProtocolVersions, and resolveClientInfo / resolveSupportedProtocolVersions in client-config-v2.ts do the same with no sibling fallback. So a user who opens the JSON editor and makes any edit to a legacy profile — with no change to the version/client fields — will have those fields cleared out of mcpProfile and, on the next connect, the accept-list and client identity silently stop being forwarded to the SDK. The reader side of this migration (protocolToJson) correctly prefers sibling-then-initialize, but the consumers that feed the actual connection were not updated to match, so the migrated sibling fields are never picked up. Consider updating the frontend resolvers/connect path to read ?? initialize?.* (mirroring protocolToJson's precedence) before committing the initialize: undefined migration, so migrated rows keep working.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx, line 629:
<comment>Editing a legacy host whose protocol data lives in the deprecated nested `initialize` envelope now drops that data from the frontend's connection path. `applyJsonToDraft` migrates by writing the sibling `clientInfo` / `supportedProtocolVersions` and forcing `initialize: undefined`, but the frontend resolver still reads only the nested path: `use-server-state.ts` (buildConnectDefaults) reads `mcpProfile?.initialize?.clientInfo` and `mcpProfile?.initialize?.supportedProtocolVersions`, and `resolveClientInfo` / `resolveSupportedProtocolVersions` in `client-config-v2.ts` do the same with no sibling fallback. So a user who opens the JSON editor and makes any edit to a legacy profile — with no change to the version/client fields — will have those fields cleared out of `mcpProfile` and, on the next connect, the accept-list and client identity silently stop being forwarded to the SDK. The reader side of this migration (`protocolToJson`) correctly prefers sibling-then-initialize, but the consumers that feed the actual connection were not updated to match, so the migrated sibling fields are never picked up. Consider updating the frontend resolvers/connect path to read `?? initialize?.*` (mirroring `protocolToJson`'s precedence) before committing the `initialize: undefined` migration, so migrated rows keep working.</comment>
<file context>
@@ -618,18 +620,13 @@ export function applyJsonToDraft(
+ supportedProtocolVersions,
+ // Editing this section migrates the deprecated envelope to the sibling
+ // fields above; retaining both would recreate two sources of truth.
+ initialize: undefined,
mcpProtocolVersion,
toolParamHeaderMirroring,
</file context>
| { name: "dual-era-fixture-test", version: "1.0.0" }, | ||
| { | ||
| supportedProtocolVersions: ["2025-11-25", "2026-07-28"], | ||
| versionNegotiation: { mode: "auto" }, |
There was a problem hiding this comment.
P3: The new connectAutomatic() helper duplicates connectModern() almost entirely — same handler/capture setup, same client identity, same supportedProtocolVersions list, and the same transport; only the versionNegotiation mode differs (and connectLegacy() shares the same skeleton too). Consider factoring the shared setup into a single parameterized helper that takes the negotiation options, so the fixture's client name/support-list/transport live in one place instead of three near-identical copies that can drift when the fixture evolves.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk/tests/support/dual-era-fixture.test.ts, line 58:
<comment>The new `connectAutomatic()` helper duplicates `connectModern()` almost entirely — same handler/capture setup, same client identity, same `supportedProtocolVersions` list, and the same transport; only the `versionNegotiation` mode differs (and `connectLegacy()` shares the same skeleton too). Consider factoring the shared setup into a single parameterized helper that takes the negotiation options, so the fixture's client name/support-list/transport live in one place instead of three near-identical copies that can drift when the fixture evolves.</comment>
<file context>
@@ -33,7 +33,30 @@ async function connectModern() {
+ { name: "dual-era-fixture-test", version: "1.0.0" },
+ {
+ supportedProtocolVersions: ["2025-11-25", "2026-07-28"],
+ versionNegotiation: { mode: "auto" },
+ },
);
</file context>
2 participants
Purpose
Model ChatGPT as one dual-era host profile instead of mixing protocol support, protocol selection, and legacy
initializedata.Behavior
2025-11-25and2026-07-28once throughsupportedProtocolVersions.mcpProtocolVersion: "auto"means negotiate at connection time.initializeflow.server/discoverand never sendsinitialize.initializeremain readable and keep their historical canonical hashes.Data model
supportedProtocolVersionsandclientInfoare now sibling profile fields. The old nestedinitializeshape remains deprecated read compatibility, not the ChatGPT source of truth.The generated local fallback catalog includes the backend-owned ChatGPT profile from MCPJam/mcpjam-backend#872.
Validation
Companion backend PR: https://github.com/MCPJam/mcpjam-backend/pull/872
Summary by cubic
ChatGPT is now modeled as a dual‑era MCP host with explicit negotiation. We add
mcpProtocolVersion: "auto"and movesupportedProtocolVersions/clientInfoout of legacyinitializeto enable 2026 discovery with 2025 fallback.New Features
supportedProtocolVersions: ["2025-11-25", "2026-07-28"]; defaultmcpProtocolVersion: "auto".server/discoverand falls back to legacyinitialize; per‑server pins override host Automatic."auto"to no wire pin; negotiation is independent from the support list, which Automatic still honors.Host.mcp.protocolVersionaccepts"auto";supportedProtocolVersionsandclientInfoare era‑neutral siblings."auto"and reads/writes the new fields.Migration
initializeremains readable and keeps canonical hashes.initializeare rejected during canonicalization.supportedProtocolVersions.Written for commit 2b185fa. Summary will update on new commits.