[Protocol] Upgrade to MCP TypeScript SDK v2 and negotiate the 2026-07-28 era - #381
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughMiftah migrates to MCP v2 packages, supports modern and legacy protocol serving, adds server factories, updates OAuth issuer handling, and adjusts packaging, documentation, and interoperability tests. ChangesMCP v2 serving and runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MiftahHttpServer
participant McpServerFactory
participant MiftahServer
participant UpstreamSession
MCPClient->>MiftahHttpServer: Send modern or legacy MCP request
MiftahHttpServer->>McpServerFactory: Create serving server
McpServerFactory->>MiftahServer: Prepare request-scoped server
MiftahServer->>UpstreamSession: Route tool, resource, or prompt request
UpstreamSession-->>MiftahServer: Return upstream result or cancellation
MiftahServer-->>MiftahHttpServer: Return MCP response
MiftahHttpServer-->>MCPClient: Send protocol response
Possibly related issues
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
CI install failure root cause and fix: the original lockfile inherited a workstation-level legacy-peer-deps=true setting, so default CI resolvers expected an Express optional-peer tree that was absent. Commit dee2c87 regenerates the lock with legacy peer resolution explicitly disabled and moves @modelcontextprotocol/server-legacy to dev-only because it is used only by interoperability fixtures. Verified from a clean archive with the exact Node 24.18.0/npm 11.16.0 runner pair: npm ci, typecheck, build, and check:pack all pass with zero audit findings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/oauth/remote-oauth-client-provider.ts (1)
263-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet the issuer on credential-backed client information.
tokens()rebuildssavedClientwithoutissuer.clientInformation()then returns this value without the issuer that lines 224-227 include for other registrations. A dynamic registration restored from persisted credentials can therefore lose its issuer binding during a later authorization flow.Add
issuer: this.options.binding.issuerwhen assigningsavedClient. Add a regression test that restores a dynamic credential and verifiesclientInformation()includes the issuer.Proposed fix
if (credential.clientId !== undefined) { this.savedClient = { client_id: credential.clientId, + issuer: this.options.binding.issuer, ...(credential.clientSecret === undefined ? {} : { client_secret: credential.clientSecret }) }; }🤖 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 `@src/oauth/remote-oauth-client-provider.ts` around lines 263 - 280, Update the savedClient assignment in tokens() to include issuer: this.options.binding.issuer alongside the persisted client_id and client_secret. Add a regression test covering restoration of a dynamic credential and assert that clientInformation() returns the configured issuer.
🤖 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.
Inline comments:
In `@scripts/build-test-fixture.mjs`:
- Around line 22-24: Update the fixture normalization around bundledSource to
use parser-aware processing that removes unsafe blank-line whitespace without
modifying template-literal runtime contents. Preserve whitespace inside template
literals, and add a regression test covering an indented blank line within a
template literal.
In `@src/cli/main.ts`:
- Around line 75-80: Update the serveStdio invocation in the shutdown flow to
provide an onerror callback that writes the shutdown error and sets
process.exitCode to 1. Keep the existing shutdown behavior in the shutdown
function and ensure failures from server.close are reported rather than
discarded.
In `@src/runtime/create-miftah-runtime.ts`:
- Around line 100-105: Update createHttpRequestMiftahServerFactory and the
shared configuredMiftahServerFactory options to disable resource-subscription
probing and capability registration for per-request HTTP servers. Preserve
resource subscriptions for STDIO and legacy session runtimes by leaving their
existing configuration enabled.
In `@tests/mcp-v2-migration-contract.test.ts`:
- Around line 14-21: Extend the dependency assertions in the migration contract
test to verify that dependencies does not contain
`@modelcontextprotocol/server-legacy`, alongside the existing absence check for
`@modelcontextprotocol/sdk`. Keep the current production dependency expectations
unchanged.
---
Outside diff comments:
In `@src/oauth/remote-oauth-client-provider.ts`:
- Around line 263-280: Update the savedClient assignment in tokens() to include
issuer: this.options.binding.issuer alongside the persisted client_id and
client_secret. Add a regression test covering restoration of a dynamic
credential and assert that clientInformation() returns the configured issuer.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 357cb1d1-f5ac-434b-89e7-c8374d248b6c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (66)
CHANGELOG.mddocs/library-api.mdpackage.jsonscripts/build-test-fixture.mjsscripts/pack-verifier.mjssrc/cli/doctor.tssrc/cli/init.tssrc/cli/main.tssrc/cli/setup-native-oauth.tssrc/config/diagnostics.tssrc/config/schema.tssrc/config/validate-config.tssrc/console/console-application-service.tssrc/console/console-dashboard-application-service.tssrc/http/miftah-http-server.tssrc/identity/identity-manager.tssrc/index.tssrc/mcp/server/management-tools.tssrc/mcp/server/miftah-server.tssrc/mcp/server/resource-prompt-registry.tssrc/mcp/server/tool-registry.tssrc/oauth/loopback-authorization-handoff.tssrc/oauth/oauth-metadata-fetch-guard.tssrc/oauth/remote-oauth-client-provider.tssrc/oauth/remote-oauth-credential-refresher.tssrc/oauth/remote-oauth-discovery.tssrc/oauth/remote-oauth-runtime.tssrc/runtime/create-miftah-runtime.tssrc/setup/native-oauth-onboarding.tssrc/setup/profile-readiness.tssrc/upstream/contained-stdio-transport.tssrc/upstream/multi-upstream-process-manager.tssrc/upstream/progress-preserving-transport.tssrc/upstream/remote-error.tssrc/upstream/upstream-process-manager.tssrc/upstream/upstream-session.tstests/approval-fallback.test.tstests/audit-outcomes.test.tstests/authenticated-request-context-docs-contract.test.tstests/fixtures/fake-upstream-bundled.mjstests/fixtures/fake-upstream-runtime.mjstests/helpers/fake-remote-upstream.tstests/http-server.test.tstests/mcp-v2-migration-contract.test.tstests/mcp-v2-serving.test.tstests/mcp-wrapper.test.tstests/multi-upstream.test.tstests/oauth-loopback-handoff.test.tstests/operation-pipeline.test.tstests/package-contract.test.tstests/plugin-routing-server.test.tstests/profile-context-handle-docs-contract.test.tstests/profile-lease-pipeline.test.tstests/profile-lock-mcp.test.tstests/profile-transition-audit-barrier.test.tstests/progress-preserving-transport.test.tstests/public-api.test.tstests/release-config.test.tstests/remote-oauth-client-provider.test.tstests/remote-oauth-compatibility.test.tstests/remote-oauth-runtime.test.tstests/remote-oauth-transport.test.tstests/remote-transport.test.tstests/stateless-profile-context-runtime.test.tstests/tool-registry.test.tstsup.config.ts
|
Review follow-up in 89d5564: restored dynamic OAuth client information retains its issuer binding; modern request-scoped HTTP no longer probes or advertises connection-bound resource subscriptions; STDIO serving failures are reported safely; fixture normalization is parser-aware; and the production dependency contract excludes the legacy fixture package. Exact-head local gates: 155 of 155 test files, 1,910 passing tests and 34 skips; 96.00 percent line coverage; typecheck; lint; build; CLI smoke; 55-file package contract; fixture freshness; git diff --check; and zero vulnerabilities in production and full audits. The linked-issue matrix warning was stale because the prior complete Linux, macOS, and Windows by Node 20, 22, and 24 matrix passed; that matrix is rerunning for 89d5564. The repository has no 80 percent docstring gate, so no unrelated mass-docstring rewrite was added. |
coderabbitai
Bot
added
area:mcp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/mcp/server/miftah-server.ts (2)
501-503: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport shutdown failures from the
onclosepath.
closeInternal()preserves the first cleanup failure, but this callback discards it. A transport close does not provide a caller that awaitsclose(). An upstream or subscription cleanup failure can therefore remain silent and leave resources running.Report the sanitized error through the warning or runtime error path.
Proposed fix
this.server.onclose = () => { - void this.close().catch(() => undefined); + void this.close().catch((error: unknown) => { + process.emitWarning(this.toSafeError(error).message, { + code: "MIFTAH_SHUTDOWN_FAILED" + }); + }); };This preserves the failure-reporting guarantee of the idempotent shutdown path.
🤖 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 `@src/mcp/server/miftah-server.ts` around lines 501 - 503, Update the server onclose callback to report sanitized failures from the awaited this.close() call instead of swallowing them with catch(() => undefined). Use the existing warning or runtime error reporting path, while preserving the idempotent shutdown behavior implemented by closeInternal().
1059-1074: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReturn
inputRequired(...)for modern approvals.
requireApproval()callsthis.server.elicitInput(), which fails for 2026-07-28 requests. Form-capable modern clients then receive anisErrorresult. ReturninputRequired(...), processinputResponseson retry, and carry approval state inrequestState. Use the SDK legacy shim to retain legacy behavior. The modern HTTP factory creates a freshMiftahServerper request, so the in-memoryApprovalStorecannot retain pending state across retries.🤖 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 `@src/mcp/server/miftah-server.ts` around lines 1059 - 1074, Update the tools/call handler and approval flow around requireApproval so modern 2026-07-28 requests return inputRequired(...) instead of invoking the legacy elicitInput path. Process inputResponses on retries and persist pending approval state through requestState, since each modern HTTP request creates a fresh MiftahServer and ApprovalStore. Route legacy clients through the SDK legacy shim to preserve existing behavior.tests/mcp-v2-serving.test.ts (1)
126-151: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRun the modern stdio case in a child process.
InMemoryTransport.createLinkedPair()connects 2025-era instances only. It cannot exercise modernserveStdiobehavior. Keep linked-pair coverage for the legacy case and spawnserveStdiofor modern stdio coverage.🤖 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 `@tests/mcp-v2-serving.test.ts` around lines 126 - 151, Update the parameterized stdio test around serveStdio and InMemoryTransport.createLinkedPair so the legacy case retains linked-pair coverage while the modern case runs serveStdio in a child process. Preserve the existing era assertion and whoami tool check for both paths, and keep cleanup of the client and spawned server handle.src/runtime/create-miftah-runtime.ts (1)
71-82: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
configuredMiftahServerFactoryera-aware.
serveStdiopasseserato the factory. Whenera === "legacy", omitmodernProfileContext. Whenera === "modern", setresourceSubscriptionsEnabledtofalse. Otherwise, legacy stdio instances use modern_metaand authentication handling, while modern instances advertise the legacyresources.subscribecapability instead of usingsubscriptions/listen.🤖 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 `@src/runtime/create-miftah-runtime.ts` around lines 71 - 82, Update configuredMiftahServerFactory to accept and apply the era passed by serveStdio when constructing the configured server: omit modernProfileContext for legacy instances, and set resourceSubscriptionsEnabled to false for modern instances. Preserve the existing prepareForServing and cleanup behavior while ensuring each era advertises and handles only its corresponding protocol capabilities.
🤖 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.
Inline comments:
In `@scripts/build-test-fixture.mjs`:
- Around line 31-45: Update the whitespace normalization logic around the
replacement scan to inspect each template token for individual whitespace-only
physical line spans rather than testing the complete token value. Add
replacements only for those spans, preserving template delimiters and all
non-whitespace raw content, while retaining the existing tagged-template safety
check.
---
Outside diff comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 501-503: Update the server onclose callback to report sanitized
failures from the awaited this.close() call instead of swallowing them with
catch(() => undefined). Use the existing warning or runtime error reporting
path, while preserving the idempotent shutdown behavior implemented by
closeInternal().
- Around line 1059-1074: Update the tools/call handler and approval flow around
requireApproval so modern 2026-07-28 requests return inputRequired(...) instead
of invoking the legacy elicitInput path. Process inputResponses on retries and
persist pending approval state through requestState, since each modern HTTP
request creates a fresh MiftahServer and ApprovalStore. Route legacy clients
through the SDK legacy shim to preserve existing behavior.
In `@src/runtime/create-miftah-runtime.ts`:
- Around line 71-82: Update configuredMiftahServerFactory to accept and apply
the era passed by serveStdio when constructing the configured server: omit
modernProfileContext for legacy instances, and set resourceSubscriptionsEnabled
to false for modern instances. Preserve the existing prepareForServing and
cleanup behavior while ensuring each era advertises and handles only its
corresponding protocol capabilities.
In `@tests/mcp-v2-serving.test.ts`:
- Around line 126-151: Update the parameterized stdio test around serveStdio and
InMemoryTransport.createLinkedPair so the legacy case retains linked-pair
coverage while the modern case runs serveStdio in a child process. Preserve the
existing era assertion and whoami tool check for both paths, and keep cleanup of
the client and spawned server handle.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99e86134-4853-43f0-a3f4-2616c5836044
📒 Files selected for processing (10)
scripts/build-test-fixture.mjssrc/cli/main.tssrc/mcp/server/miftah-server.tssrc/oauth/remote-oauth-client-provider.tssrc/runtime/create-miftah-runtime.tstests/fixtures/fake-upstream-bundled.mjstests/mcp-v2-migration-contract.test.tstests/mcp-v2-serving.test.tstests/remote-oauth-client-provider.test.tstests/test-harness-resource-contract.test.ts
|
Second review follow-up is in 4c20344:
The child-process suggestion for the modern stdio test was not applied because the SDK v2 serveStdio API explicitly accepts a custom transport, and the current linked transport is passed through that public seam. The test observes the factory context as modern, asserts modern does not advertise legacy resources.subscribe, and completes a real tool call; the legacy case observes legacy and asserts the inverse. Replacing that test with process plumbing would not exercise a different protocol-era branch. Exact 4c20344 local validation: 156/156 test files; 1,920 passed and 34 skipped; 96.00% line and 92.19% branch coverage; typecheck; lint; build; CLI smoke; fixture freshness; 55-file package contract; git diff --check; and zero vulnerabilities in production and full audits. |
Dismissed as stale: all actionable findings were resolved in later commits through 4c20344, exact-head CI is green, and the latest CodeRabbit status is successful but its new review was rate-limited.
1 participant
Closes #363
Summary
Packaging and security
Local validation
The full Linux, macOS, and Windows packaged-artifact matrix remains required before merge.
Summary by CodeRabbit
New Features
createMiftahServerFactoryfor trusted integrations.Bug Fixes
Documentation