feat: add deterministic MCP tool registry - #47
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 (5)
📝 WalkthroughWalkthroughMiftah now manages upstream tools through immutable, cached per-profile snapshots. Profile revisions coordinate discovery and routing, stable errors cover unknown or incompatible tools, and profile changes or restarts notify clients to refresh tool listings. ChangesProfile-aware tool registry and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MiftahServer
participant ToolRegistry
participant UpstreamProcessManager
Client->>MiftahServer: listTools or callTool
MiftahServer->>ToolRegistry: get active-profile snapshot
ToolRegistry->>UpstreamProcessManager: discover upstream tools
UpstreamProcessManager-->>ToolRegistry: return tools and schemas
ToolRegistry-->>MiftahServer: return snapshot and route
MiftahServer-->>Client: tools or routed call result
Client->>MiftahServer: change or restart profile
MiftahServer-->>Client: tools/list_changed notification
Possibly related issues
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📝 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: 5
🤖 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/upstream/multi-upstream-process-manager.ts`:
- Around line 35-37: Update restartProfile to ensure
toolRegistry.invalidate(profile) executes regardless of whether any
manager.restart(profile) call fails: wrap the Promise.all restart attempts in a
try/finally block and perform invalidation in finally, preserving rejection of
restart errors.
In `@tests/mcp-wrapper.test.ts`:
- Around line 303-327: Strengthen the test “rejects unknown tool calls without
forwarding them upstream” by adding a fixture-visible call marker or invocation
counter, capturing its initial value before the unknown tool request, and
asserting it remains unchanged afterward. Keep the existing TOOL_NOT_FOUND
assertion, and ensure the marker is read from the upstream fixture through its
established mechanism.
- Around line 115-125: Make the notification-count assertions in the test around
client.callTool and the corresponding second occurrence verify stability, not
merely that the count reaches one: after waiting for the first notification, add
a bounded settling period or use an equivalent polling/helper assertion
confirming notifications remains exactly 1, while preserving the existing
timeout behavior.
- Line 411: Move the /TOOL_COLLISION/ regular expression used by the
client.listTools() rejection assertion to module scope as a reusable constant,
then reference that constant in the expect(...).rejects.toThrow() call to
satisfy prefer-static-regex.
In `@tests/multi-upstream.test.ts`:
- Around line 384-409: Replace the polling assertion in the test around
client.listTools() and the notifications counter with a stabilization wait that
allows notification delivery to settle, then assert notifications is exactly 1.
Preserve the existing notification handler and ensure the final assertion can
detect any later duplicate notification.
🪄 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: e3d3ac91-0601-4bf6-9082-0a5c1393796e
📒 Files selected for processing (13)
README.mddocs/architecture.mddocs/config.mdsrc/mcp/server/miftah-server.tssrc/mcp/server/tool-registry.tssrc/profiles/profile-manager.tssrc/routing/routing-engine.tssrc/upstream/multi-upstream-process-manager.tssrc/utils/errors.tstests/fixtures/fake-upstream.mjstests/mcp-wrapper.test.tstests/multi-upstream.test.tstests/profile-manager.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/server/miftah-server.ts (1)
162-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFingerprint-based cross-profile schema guard looks correct.
Resolving
nameagainst both the source and routed profile snapshots and comparingfingerprintbefore dispatch correctly prevents a cold call from silently using a mismatched schema after routing. This matchestools/mcp-wrapper.test.ts'sTOOL_SCHEMA_MISMATCHtest coverage.One readability nit:
const upstreamName = target.originalName;actually holds the raw tool name, not an upstream identifier — the real upstream selector istarget.upstreamName(assigned totargetUpstream). Correctness is unaffected, but the naming makes this section harder to follow and easy to typo-swap in future edits.♻️ Suggested rename for clarity
- const upstreamName = target.originalName; + const originalToolName = target.originalName; const targetUpstream = target.upstreamName; const profile = this.profiles.get(route.profile); - const decision = this.policy.evaluate(profile.policy, upstreamName); + const decision = this.policy.evaluate(profile.policy, originalToolName);(and update the remaining
upstreamNamereferences in this block accordingly)🤖 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 162 - 189, Rename the local variable `upstreamName` in `handleUpstreamTool` to reflect that it stores `target.originalName`, such as `toolName`, and update every reference within the block; keep `targetUpstream` reserved for `target.upstreamName`.
🤖 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/miftah-server.ts`:
- Around line 301-316: Update the miftah_restart_profile handler to await all
upstream restart operations settling before calling toolRegistry.invalidate or
notifyToolListChanged; avoid relying on Promise.all’s early rejection by using
Promise.allSettled or equivalent coordination in
MultiUpstreamProcessManager.restartProfile, while preserving error propagation
after every restart has completed.
In `@tests/mcp-wrapper.test.ts`:
- Around line 17-24: Extract notificationSettleMs and
expectExactlyOneToolListChanged from tests/mcp-wrapper.test.ts into a shared
tests/helpers/notifications.ts module, preserving their behavior and required
imports. Remove the duplicate definitions from mcp-wrapper.test.ts and
multi-upstream.test.ts, then import the shared symbols in both suites.
---
Outside diff comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 162-189: Rename the local variable `upstreamName` in
`handleUpstreamTool` to reflect that it stores `target.originalName`, such as
`toolName`, and update every reference within the block; keep `targetUpstream`
reserved for `target.upstreamName`.
🪄 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: 6d2c2c1b-d1ca-46ee-88f8-d9a539ac1fcf
📒 Files selected for processing (4)
src/mcp/server/miftah-server.tstests/fixtures/fake-upstream.mjstests/mcp-wrapper.test.tstests/multi-upstream.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Validation
npm testnpm run typechecknpm run lintnpm run buildnpm run check:packImplements #8. This PR targets
development; the issue will be closed manually after reviewed merge.Summary by CodeRabbit
New Features
tools.listChangedand sendtools/list_changedafter profile changes and active-profile restarts.<upstream>__<tool>).Bug Fixes
Documentation
Tests