Mirror: Support refreshing MCP tool, resources, etc lists and avoid prompts (#5410)#27
Mirror: Support refreshing MCP tool, resources, etc lists and avoid prompts (#5410)#27jeremylongshore merged 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Model Context Protocol (MCP) integration by enabling automatic updates of tool, resource, and prompt lists. This ensures that the development environment remains synchronized with server-side changes without requiring manual refreshes, thereby improving responsiveness and user experience. Additionally, the handling of unaddressed server notifications has been refined to prevent disruptive user prompts, opting instead for silent logging to maintain a smoother workflow. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Review Summary by QodoSupport refreshing MCP tool, resources, and prompts lists with automatic capability refresh
WalkthroughsDescription• Add support for refreshing MCP server capabilities when tools, resources, or prompts change • Handle ResourceListChanged, ToolListChanged, and PromptListChanged notifications • Implement RefreshCapabilitiesCallback to automatically refresh server state • Change fallback notification handler to log silently instead of showing user prompts • Add comprehensive test suite for NotificationService with 346 lines of test coverage • Add GitHub Actions auto-triage workflow for automatic issue and PR labeling Diagramflowchart LR
MCP["MCP Server Notifications"]
NS["NotificationService"]
RC["RefreshCapabilitiesCallback"]
HUB["McpHub"]
MCP -->|ResourceListChanged| NS
MCP -->|ToolListChanged| NS
MCP -->|PromptListChanged| NS
NS -->|Invoke Callback| RC
RC -->|fetchAvailableServerCapabilities| HUB
RC -->|notifyWebviewOfServerChanges| HUB
File Changes1. src/services/mcp/kilocode/NotificationService.ts
|
|
Failed to generate code suggestions for PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for refreshing MCP tool, resource, and prompt lists when the server sends a notification about changes. This is achieved by adding new notification handlers in NotificationService and passing a refresh callback from McpHub. The changes are well-implemented and include comprehensive unit tests for the new functionality in NotificationService. I have one suggestion to reduce code duplication in the new notification handlers.
| // Handle resource list changes - refresh capabilities silently | ||
| client.setNotificationHandler(ResourceListChangedNotificationSchema, async () => { | ||
| console.log(`MCP ${name}: resources list changed, refreshing capabilities`) | ||
| try { | ||
| await onRefreshCapabilities?.(name) | ||
| } catch (error) { | ||
| console.error(`MCP ${name}: failed to refresh capabilities after resource list change:`, error) | ||
| } | ||
| }) | ||
|
|
||
| // Handle tool list changes - refresh capabilities silently | ||
| client.setNotificationHandler(ToolListChangedNotificationSchema, async () => { | ||
| console.log(`MCP ${name}: tools list changed, refreshing capabilities`) | ||
| try { | ||
| await onRefreshCapabilities?.(name) | ||
| } catch (error) { | ||
| console.error(`MCP ${name}: failed to refresh capabilities after tool list change:`, error) | ||
| } | ||
| }) | ||
|
|
||
| // Handle prompt list changes - refresh capabilities silently | ||
| client.setNotificationHandler(PromptListChangedNotificationSchema, async () => { | ||
| console.log(`MCP ${name}: prompts list changed, refreshing capabilities`) | ||
| try { | ||
| await onRefreshCapabilities?.(name) | ||
| } catch (error) { | ||
| console.error(`MCP ${name}: failed to refresh capabilities after prompt list change:`, error) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The notification handlers for resource, tool, and prompt list changes contain duplicated logic. To improve maintainability and reduce redundancy, you can extract this logic into a helper function.
const createRefreshHandler = (listType: string) => async () => {
console.log(`MCP ${name}: ${listType} list changed, refreshing capabilities`);
try {
await onRefreshCapabilities?.(name);
} catch (error) {
console.error(`MCP ${name}: failed to refresh capabilities after ${listType} list change:`, error);
}
};
// Handle resource list changes - refresh capabilities silently
client.setNotificationHandler(ResourceListChangedNotificationSchema, createRefreshHandler("resources"));
// Handle tool list changes - refresh capabilities silently
client.setNotificationHandler(ToolListChangedNotificationSchema, createRefreshHandler("tools"));
// Handle prompt list changes - refresh capabilities silently
client.setNotificationHandler(PromptListChangedNotificationSchema, createRefreshHandler("prompts"));
Code Review by Qodo
1. Logs raw notification object
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Handle resource list changes - refresh capabilities silently | ||
| client.setNotificationHandler(ResourceListChangedNotificationSchema, async () => { | ||
| console.log(`MCP ${name}: resources list changed, refreshing capabilities`) | ||
| try { | ||
| await onRefreshCapabilities?.(name) | ||
| } catch (error) { | ||
| console.error(`MCP ${name}: failed to refresh capabilities after resource list change:`, error) | ||
| } | ||
| }) | ||
|
|
||
| // Handle tool list changes - refresh capabilities silently | ||
| client.setNotificationHandler(ToolListChangedNotificationSchema, async () => { | ||
| console.log(`MCP ${name}: tools list changed, refreshing capabilities`) | ||
| try { | ||
| await onRefreshCapabilities?.(name) | ||
| } catch (error) { | ||
| console.error(`MCP ${name}: failed to refresh capabilities after tool list change:`, error) | ||
| } | ||
| }) | ||
|
|
||
| // Handle prompt list changes - refresh capabilities silently | ||
| client.setNotificationHandler(PromptListChangedNotificationSchema, async () => { | ||
| console.log(`MCP ${name}: prompts list changed, refreshing capabilities`) | ||
| try { | ||
| await onRefreshCapabilities?.(name) | ||
| } catch (error) { | ||
| console.error(`MCP ${name}: failed to refresh capabilities after prompt list change:`, error) | ||
| } | ||
| }) | ||
|
|
||
| // Fallback for any other unhandled notifications - log silently, don't notify user | ||
| // This prevents raw JSON-RPC messages from being displayed as VS Code notifications | ||
| client.fallbackNotificationHandler = async (notification) => { | ||
| vscode.window.showInformationMessage(`MCP ${name}: ${JSON.stringify(notification)}`) | ||
| console.log(`MCP ${name}: unhandled notification`, notification) | ||
| } |
There was a problem hiding this comment.
1. Logs raw notification object 📘 Rule violation ⛨ Security
New handlers log full MCP notifications and error objects via console.log/console.error, which can include sensitive data in params and are not structured for auditing. This increases the risk of leaking secrets/PII into logs and makes log monitoring/parsing harder.
Agent Prompt
## Issue description
`NotificationService` logs raw MCP notification payloads (including full `notification` objects) and raw `error` objects using `console.log`/`console.error`. This can leak sensitive data from notification `params` and produces unstructured logs.
## Issue Context
MCP servers can send arbitrary notification `params` (potentially containing secrets, file contents, or other sensitive values). Logging full payloads and raw errors violates the secure logging requirement.
## Fix Focus Areas
- src/services/mcp/kilocode/NotificationService.ts[55-89]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ITEM_NUMBER: ${{ github.event_name == 'issues' && github.event.issue.number || github.event.pull_request.number }} | ||
| ITEM_TITLE: ${{ github.event_name == 'issues' && github.event.issue.title || github.event.pull_request.title }} | ||
| ITEM_BODY: ${{ github.event_name == 'issues' && github.event.issue.body || github.event.pull_request.body }} | ||
| run: | | ||
| # Sanitize body - remove shell metacharacters | ||
| SAFE_BODY=$(echo "$ITEM_BODY" | head -c 2000 | tr -d '`$(){}[]|;&<>\\' | tr '\n' ' ') | ||
|
|
||
| # Determine gh command based on event type | ||
| if [ "$EVENT_TYPE" = "issues" ]; then | ||
| GH_CMD="gh issue edit" | ||
| ITEM_TYPE="issue" | ||
| else | ||
| GH_CMD="gh pr edit" | ||
| ITEM_TYPE="pull request" | ||
| fi | ||
|
|
||
| kilocode --auto "Triage this GitHub ${ITEM_TYPE}: | ||
|
|
||
| Number: ${ITEM_NUMBER} | ||
| Title: ${ITEM_TITLE} | ||
| Body: ${SAFE_BODY} |
There was a problem hiding this comment.
2. Workflow shell injection 🐞 Bug ⛨ Security
Untrusted issue/PR content is interpolated into a double-quoted bash string passed to `kilocode --auto; ITEM_TITLE` is unsanitized and can contain backticks/$() to achieve command execution. Because this workflow also exports an integration token, successful injection could exfiltrate secrets or modify repo state via GitHub CLI.
Agent Prompt
### Issue description
The auto-triage workflow is vulnerable to shell injection because it embeds `${ITEM_TITLE}` (untrusted) into a double-quoted bash string passed to `kilocode --auto`.
### Issue Context
Any user can open an issue with a malicious title containing backticks or `$()`; bash will evaluate these inside the double-quoted string. The step also exports `KILOCODE_TOKEN`.
### Fix Focus Areas
- .github/workflows/auto-triage.yml[47-67]
- .github/workflows/auto-triage.yml[34-39]
### Suggested direction
- Sanitize `ITEM_TITLE` similarly to `ITEM_BODY`, and also remove `"` from both.
- Prefer generating the prompt without shell interpolation (scripted construction/escaping or file-based input) so untrusted text never gets evaluated by bash.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| { | ||
| "last_failure": "2026-02-14T20:42:41.363720889-06:00", | ||
| "failure_count": 7, | ||
| "backoff_until": "2026-02-14T21:12:41.36372132-06:00", | ||
| "needs_manual_sync": true, | ||
| "failure_reason": "git pull failed: exit status 1\nfatal: couldn't find remote ref review/PR-5667\n" | ||
| } No newline at end of file |
There was a problem hiding this comment.
3. Accidental .beads commit 🐞 Bug ⛯ Reliability
The PR commits .beads/ local runtime/state files (pid, timestamps, sync failures) and a SQLite shm artifact; these will churn, bloat the repo, and cause merge conflicts. They should be removed from the PR and added to the root .gitignore.
Agent Prompt
### Issue description
`.beads/` appears to be local runtime/database state and should not be tracked.
### Issue Context
These files include process IDs and sync timestamps/errors and will change frequently.
### Fix Focus Areas
- .beads/sync-state.json[1-7]
- .beads/daemon.pid[1-1]
- .gitignore[35-79]
### Suggested direction
- Remove `.beads/` directory contents from version control.
- Add `.beads/` to root `.gitignore` (optionally also ignore SQLite shm/wal patterns).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review: kilocode Kilo-Org#5410
Checklist
FindingsYELLOW - Missing changesetChangeset-bot flagged this PR. A YELLOW - Potential race condition in rapid notificationsIf an MCP server sends // Each handler independently calls the callback:
client.setNotificationHandler(ToolListChangedNotificationSchema, async () => {
await onRefreshCapabilities?.(name) // Could overlap with resource/prompt handlers
})Consider debouncing the refresh callback or queuing notifications to batch refresh all capabilities once. GRAY - Fallback handler silently logs instead of notifyingThe previous behavior was GRAY - Removed
|
| Check | Result |
|---|---|
| compile | PASS |
| test-extension (ubuntu) | PASS |
| test-extension (windows) | PASS |
| test-jetbrains | PASS |
| test-webview (ubuntu) | PASS |
| test-webview (windows) | PASS |
| test-cli | PASS |
| build-cli | PASS |
| check-translations | PASS |
| Build Docusaurus Site | PASS |
All CI checks pass.
Code Snippets
McpHub callback wiring:
// src/services/mcp/McpHub.ts
this.kiloNotificationService.connect(name, connection.client, async (serverName) => {
await this.fetchAvailableServerCapabilities(serverName, source)
await this.notifyWebviewOfServerChanges()
})NotificationService type:
// src/services/mcp/kilocode/NotificationService.ts
export type RefreshCapabilitiesCallback = (serverName: string) => Promise<void>Verdict
APPROVE -- This is a clean, well-tested fix for an annoying user experience problem (raw JSON-RPC notifications appearing as VS Code toasts). The implementation correctly follows the MCP specification for tools/list_changed, resources/list_changed, and prompts/list_changed notifications. The test coverage is exemplary at 346 lines for 62 lines of production code.
Minor items to address before merge:
- Add a changeset (
patchforkilo-code) - Consider debouncing rapid successive notifications (nice-to-have, not blocking)
The author acknowledges this was "vibe coded" in the description, but the result is well-structured and the test suite is comprehensive enough to validate the behavior.
Review Journal: kilocode Kilo-Org#5410
SummaryFixes annoying MCP tool change notification toasts by properly handling First ImpressionsThe PR screenshot showing 7+ notification popups makes the problem immediately clear. Author (lambertjosh / Joshua Lambert) explicitly notes "vibe coded, tested but not reviewed at a code level, in draft." Despite this disclaimer, the code is well-organized. What I Looked At
AnalysisNotification RoutingThe original code had a
This follows the MCP specification correctly. The notification schemas are imported from Callback ArchitectureThe Race Condition (Minor)Three notifications arriving in quick succession will trigger three concurrent refreshes. In practice, Verification
Lessons Learned
Review methodology: AI PR Review Case Studies | Reviewed with GWI + Claude Code |
1 participant
Mirror of upstream PR #5410
Verdict: APPROVE | Tier 5
Review: kilocode-5410-review.md