feat: aggregate multi-upstream resources and prompts - #48
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds multi-upstream aggregation for MCP resources and prompts, including namespacing, routing, redaction, opaque cursors, invalidation, notifications, compatibility behavior, and expanded integration coverage. ChangesMulti-upstream resource and prompt aggregation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MiftahServer
participant ResourcePromptRegistry
participant UpstreamSession
MCPClient->>MiftahServer: list/read resource or list/get prompt
MiftahServer->>ResourcePromptRegistry: aggregate or resolve route
ResourcePromptRegistry->>UpstreamSession: discover or forward remapped request
UpstreamSession-->>ResourcePromptRegistry: capability or content result
ResourcePromptRegistry-->>MiftahServer: namespaced and redacted result
MiftahServer-->>MCPClient: MCP response
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/mcp/server/resource-prompt-registry.ts`:
- Around line 70-76: Update the fan-out logic in the resource and prompt listing
methods, including the Promise.all calls invoking discoverResources() and
discoverPrompts(), to handle each upstream result independently (for example,
with settled results or per-source try/catch). Preserve successful results,
record or otherwise handle individual upstream errors, and allow
listResources()/listPrompts() to return partial results instead of rejecting
when one upstream fails.
In `@src/secrets/redact.ts`:
- Around line 76-89: redactUri() collapses all URL-parse failures to
"[REDACTED]", causing distinct malformed or relative URIs to collide. Replace
the catch fallback with a stable redaction derived from the original uri,
preserving uniqueness without exposing secrets, while retaining the existing
handling for valid urn: and data: URIs.
In `@tests/multi-upstream.test.ts`:
- Around line 374-375: Hoist the inline /RESOURCE_CURSOR_INVALID/ and
/PROMPT_CURSOR_INVALID/ regexes, plus the corresponding regex literals at the
additional call sites, into module-scope constants alongside
resourceCollisionPattern and promptCollisionPattern; update all affected
expect(...).rejects.toThrow calls to use those constants.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb77be41-53a0-4b39-80ad-285ff06fc0f7
📒 Files selected for processing (14)
README.mddocs/architecture.mddocs/config.mdsrc/mcp/server/miftah-server.tssrc/mcp/server/resource-prompt-registry.tssrc/secrets/redact.tssrc/upstream/upstream-session.tssrc/utils/errors.tstests/fixtures/fake-upstream.mjstests/helpers/notifications.tstests/mcp-wrapper.test.tstests/multi-upstream.test.tstests/resource-prompt-registry.test.tstests/secrets.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/package-contract.test.ts`:
- Around line 53-60: Update the runNpm helper used by package-contract tests to
pass a timeout and kill signal to spawnSync for every npm command, and handle
the returned build.error before checking status. Ensure beforeAll’s build
failure path reports execution errors clearly while preserving existing
stdout/stderr diagnostics.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 206f9c27-3086-4c48-9fde-1b438ed94346
📒 Files selected for processing (4)
src/secrets/redact.tstests/multi-upstream.test.tstests/package-contract.test.tstests/secrets.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/package-contract.test.ts`:
- Around line 42-48: Replace the synchronous process execution in the
package-check helper with asynchronous handling that enforces a hard timeout and
cleans up the entire child process tree, including descendants. Update the
helper around spawnSync to use process-group or equivalent tree termination, and
ensure timeout/termination failures resolve or reject promptly without waiting
for inherited children.
- Around line 49-50: The error handling in the spawnSync result incorrectly
labels every result.error as a timeout. In the test command execution logic,
branch on the timeout-specific error code before using the timeout message, and
preserve the original error message for launch failures such as ENOENT or
non-executable npm.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e126bd7d-b483-455e-9fd4-9af750c0fcfd
📒 Files selected for processing (1)
tests/package-contract.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Validation
npm test(169 passing)npm run typechecknpm run lintnpm run buildnode dist/cli/main.js schemanpm run check:packgit diff --checkCloses #9
Summary by CodeRabbit
redactUrifor safer aggregated discovery and returned content metadata.