Docs/multer middleware priority - #3415
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe upload documentation adds guidance on ChangesUpload middleware authentication documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
6582cb1 to
92663d6
Compare
![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)
packages/platform/platform-mcp/src/services/PlatformMcpModule.ts (1)
48-52: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid sharing one
McpServeracross concurrent request transports.
This creates a new transport per request but connects it to the injected singleton server. If the SDK keeps one active transport on the server, overlapping requests can swap or detach each other’s transport; use a fresh server per request or serialize the connects. Add a concurrent-request integration test before merging.🤖 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 `@packages/platform/platform-mcp/src/services/PlatformMcpModule.ts` around lines 48 - 52, Update the request handling around NodeStreamableHTTPServerTransport so each concurrent request connects to its own fresh McpServer instance instead of the injected singleton, preventing transports from replacing or detaching one another. Preserve the existing server configuration and request behavior, and add a concurrent-request integration test verifying both requests complete independently.
🤖 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 `@packages/platform/platform-mcp/src/fn/definePrompt.ts`:
- Around line 19-21: Add TSDoc immediately above the exported PromptHandler type
documenting both the no-argument and argument-taking callback signatures,
including their ServerContext parameter and GetPromptResult return type.
Explicitly note that ServerContext replaces the v1 extra/request context so the
migration-sensitive contract is clear.
In `@packages/platform/platform-mcp/src/fn/defineTool.spec.ts`:
- Around line 92-99: Update the inputSchema JSON Schema assertion in
defineTool.spec.ts to invoke validation with a valid aliasProp plus an extra
property, then assert the established additionalProperties rejection or
stripping behavior. Preserve the existing aliasing and required-field
expectations so the test continues covering the complete input-validation
contract.
In `@packages/platform/platform-mcp/src/fn/defineTool.ts`:
- Around line 164-166: Add test coverage around the async handler wrapper in
defineTool, using a user handler that inspects its second ctx argument. Assert
that the invocation’s ServerContext is forwarded unchanged, while preserving the
existing input deserialization and result behavior.
In `@packages/platform/platform-mcp/test/mcp.integration.shared.ts`:
- Around line 410-435: Extend the invalid-input test around functional-tool to
track handler invocation with a spy or counter before sendMcpRequest is called.
After the request, assert that the handler was never invoked while preserving
the existing error-payload assertions.
---
Outside diff comments:
In `@packages/platform/platform-mcp/src/services/PlatformMcpModule.ts`:
- Around line 48-52: Update the request handling around
NodeStreamableHTTPServerTransport so each concurrent request connects to its own
fresh McpServer instance instead of the injected singleton, preventing
transports from replacing or detaching one another. Preserve the existing server
configuration and request behavior, and add a concurrent-request integration
test verifying both requests complete independently.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33ffb99f-cfb2-41bd-aa9d-f88368bf2a98
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
docs/docs/upload-files.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/.openspec.yamlopenspec/changes/migrate-platform-mcp-to-sdk-v2/design.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/proposal.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/specs/mcp-endpoint/spec.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/tasks.mdpackages/platform/platform-mcp/package.jsonpackages/platform/platform-mcp/readme.mdpackages/platform/platform-mcp/src/decorators/resource.tspackages/platform/platform-mcp/src/fn/definePrompt.tspackages/platform/platform-mcp/src/fn/defineResource.tspackages/platform/platform-mcp/src/fn/defineTool.spec.tspackages/platform/platform-mcp/src/fn/defineTool.tspackages/platform/platform-mcp/src/index.tspackages/platform/platform-mcp/src/interfaces/PlatformMcpSettings.tspackages/platform/platform-mcp/src/services/McpServerFactory.tspackages/platform/platform-mcp/src/services/PlatformMcpModule.spec.tspackages/platform/platform-mcp/src/services/PlatformMcpModule.tspackages/platform/platform-mcp/src/utils/asResourceResponse.tspackages/platform/platform-mcp/src/utils/asToolResponse.tspackages/platform/platform-mcp/src/utils/fromJsonSchema.spec.tspackages/platform/platform-mcp/src/utils/fromJsonSchema.tspackages/platform/platform-mcp/src/utils/toZod.spec.tspackages/platform/platform-mcp/src/utils/toZod.tspackages/platform/platform-mcp/test/app/resources/TestResource.tspackages/platform/platform-mcp/test/app/tools/TestTool.tspackages/platform/platform-mcp/test/mcp.integration.shared.tspackages/platform/platform-mcp/vitest.config.mts
💤 Files with no reviewable changes (3)
- packages/platform/platform-mcp/src/utils/toZod.spec.ts
- packages/platform/platform-mcp/src/utils/toZod.ts
- packages/platform/platform-mcp/vitest.config.mts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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)
packages/platform/platform-mcp/src/services/PlatformMcpModule.ts (1)
48-52: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid sharing one
McpServeracross concurrent request transports.
This creates a new transport per request but connects it to the injected singleton server. If the SDK keeps one active transport on the server, overlapping requests can swap or detach each other’s transport; use a fresh server per request or serialize the connects. Add a concurrent-request integration test before merging.🤖 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 `@packages/platform/platform-mcp/src/services/PlatformMcpModule.ts` around lines 48 - 52, Update the request handling around NodeStreamableHTTPServerTransport so each concurrent request connects to its own fresh McpServer instance instead of the injected singleton, preventing transports from replacing or detaching one another. Preserve the existing server configuration and request behavior, and add a concurrent-request integration test verifying both requests complete independently.
🤖 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 `@packages/platform/platform-mcp/src/fn/definePrompt.ts`:
- Around line 19-21: Add TSDoc immediately above the exported PromptHandler type
documenting both the no-argument and argument-taking callback signatures,
including their ServerContext parameter and GetPromptResult return type.
Explicitly note that ServerContext replaces the v1 extra/request context so the
migration-sensitive contract is clear.
In `@packages/platform/platform-mcp/src/fn/defineTool.spec.ts`:
- Around line 92-99: Update the inputSchema JSON Schema assertion in
defineTool.spec.ts to invoke validation with a valid aliasProp plus an extra
property, then assert the established additionalProperties rejection or
stripping behavior. Preserve the existing aliasing and required-field
expectations so the test continues covering the complete input-validation
contract.
In `@packages/platform/platform-mcp/src/fn/defineTool.ts`:
- Around line 164-166: Add test coverage around the async handler wrapper in
defineTool, using a user handler that inspects its second ctx argument. Assert
that the invocation’s ServerContext is forwarded unchanged, while preserving the
existing input deserialization and result behavior.
In `@packages/platform/platform-mcp/test/mcp.integration.shared.ts`:
- Around line 410-435: Extend the invalid-input test around functional-tool to
track handler invocation with a spy or counter before sendMcpRequest is called.
After the request, assert that the handler was never invoked while preserving
the existing error-payload assertions.
---
Outside diff comments:
In `@packages/platform/platform-mcp/src/services/PlatformMcpModule.ts`:
- Around line 48-52: Update the request handling around
NodeStreamableHTTPServerTransport so each concurrent request connects to its own
fresh McpServer instance instead of the injected singleton, preventing
transports from replacing or detaching one another. Preserve the existing server
configuration and request behavior, and add a concurrent-request integration
test verifying both requests complete independently.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33ffb99f-cfb2-41bd-aa9d-f88368bf2a98
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
docs/docs/upload-files.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/.openspec.yamlopenspec/changes/migrate-platform-mcp-to-sdk-v2/design.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/proposal.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/specs/mcp-endpoint/spec.mdopenspec/changes/migrate-platform-mcp-to-sdk-v2/tasks.mdpackages/platform/platform-mcp/package.jsonpackages/platform/platform-mcp/readme.mdpackages/platform/platform-mcp/src/decorators/resource.tspackages/platform/platform-mcp/src/fn/definePrompt.tspackages/platform/platform-mcp/src/fn/defineResource.tspackages/platform/platform-mcp/src/fn/defineTool.spec.tspackages/platform/platform-mcp/src/fn/defineTool.tspackages/platform/platform-mcp/src/index.tspackages/platform/platform-mcp/src/interfaces/PlatformMcpSettings.tspackages/platform/platform-mcp/src/services/McpServerFactory.tspackages/platform/platform-mcp/src/services/PlatformMcpModule.spec.tspackages/platform/platform-mcp/src/services/PlatformMcpModule.tspackages/platform/platform-mcp/src/utils/asResourceResponse.tspackages/platform/platform-mcp/src/utils/asToolResponse.tspackages/platform/platform-mcp/src/utils/fromJsonSchema.spec.tspackages/platform/platform-mcp/src/utils/fromJsonSchema.tspackages/platform/platform-mcp/src/utils/toZod.spec.tspackages/platform/platform-mcp/src/utils/toZod.tspackages/platform/platform-mcp/test/app/resources/TestResource.tspackages/platform/platform-mcp/test/app/tools/TestTool.tspackages/platform/platform-mcp/test/mcp.integration.shared.tspackages/platform/platform-mcp/vitest.config.mts
💤 Files with no reviewable changes (3)
- packages/platform/platform-mcp/src/utils/toZod.spec.ts
- packages/platform/platform-mcp/src/utils/toZod.ts
- packages/platform/platform-mcp/vitest.config.mts
🛑 Comments failed to post (4)
packages/platform/platform-mcp/src/fn/definePrompt.ts (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the exported handler contract.
PromptHandleris public and now changes from the v1 extra/request context toServerContext; add TSDoc covering both callback forms and the migration-sensitive context parameter.As per coding guidelines, use TSDoc for public API documentation.
🤖 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 `@packages/platform/platform-mcp/src/fn/definePrompt.ts` around lines 19 - 21, Add TSDoc immediately above the exported PromptHandler type documenting both the no-argument and argument-taking callback signatures, including their ServerContext parameter and GetPromptResult return type. Explicitly note that ServerContext replaces the v1 extra/request context so the migration-sensitive contract is clear.Source: Coding guidelines
packages/platform/platform-mcp/src/fn/defineTool.spec.ts (1)
92-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Keep the input-validation contract covered.
This assertion now checks aliasing and required fields but omits the previous
additionalPropertiesexpectation. Add an invocation containing an extra property and assert the intended rejection or stripping behavior so the JSON Schema migration cannot silently widen accepted tool inputs.🤖 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 `@packages/platform/platform-mcp/src/fn/defineTool.spec.ts` around lines 92 - 99, Update the inputSchema JSON Schema assertion in defineTool.spec.ts to invoke validation with a valid aliasProp plus an extra property, then assert the established additionalProperties rejection or stripping behavior. Preserve the existing aliasing and required-field expectations so the test continues covering the complete input-validation contract.packages/platform/platform-mcp/src/fn/defineTool.ts (1)
164-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for the new
ServerContextcallback contract.These lines now forward
ctxto user handlers, but the supplied tests ignore the second parameter. Add a test handler that asserts it receives the invocation context, preventing future wrappers from silently dropping it.🤖 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 `@packages/platform/platform-mcp/src/fn/defineTool.ts` around lines 164 - 166, Add test coverage around the async handler wrapper in defineTool, using a user handler that inspects its second ctx argument. Assert that the invocation’s ServerContext is forwarded unchanged, while preserving the existing input deserialization and result behavior.packages/platform/platform-mcp/test/mcp.integration.shared.ts (1)
410-435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that invalid input never reaches the handler.
This assertion verifies only the returned error payload; a handler could still execute and produce the same response. Add a spy or invocation counter for
functional-tooland assert it remains untouched after the invalid call.🤖 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 `@packages/platform/platform-mcp/test/mcp.integration.shared.ts` around lines 410 - 435, Extend the invalid-input test around functional-tool to track handler invocation with a spy or counter before sendMcpRequest is called. After the request, assert that the handler was never invoked while preserving the existing error-payload assertions.
|
🎉 This PR is included in version 8.37.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
1 participant
Summary by CodeRabbit