Mirror: Fixes broken /slash-commands after continue or interrupted tool-use (#5752)#28
Mirror: Fixes broken /slash-commands after continue or interrupted tool-use (#5752)#28jeremylongshore merged 3 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 addresses a critical bug preventing slash commands from being correctly interpreted within tool interaction results, ensuring that user commands are consistently processed regardless of their context. It also significantly improves the responsiveness of the client-server interaction by implementing automatic capability refreshes based on Model Context Protocol notifications for changes in tools, resources, and prompts. Furthermore, the handling of unacknowledged notifications has been refined to provide a less intrusive experience for the user. 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 QodoFix slash commands in tool_result blocks and add MCP capability refresh handlers
WalkthroughsDescription• Fix slash commands in tool_result blocks by applying parseKiloSlashCommands processing • Add MCP capability refresh handlers for resource, tool, and prompt list changes • Refactor text processing into reusable helper function for consistency • Add comprehensive test coverage for NotificationService and slash command regression Diagramflowchart LR
A["tool_result blocks"] -->|processTextContent| B["parseMentions"]
B -->|output| C["parseKiloSlashCommands"]
C -->|processedText| D["Updated content"]
E["MCP notifications"] -->|ResourceListChanged| F["Refresh capabilities"]
E -->|ToolListChanged| F
E -->|PromptListChanged| F
F -->|callback| G["fetchAvailableServerCapabilities"]
File Changes1. src/core/mentions/processKiloUserContentMentions.ts
|
![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 successfully fixes a bug where slash commands within tool_result blocks were being ignored. The fix is well-implemented by introducing a processTextContent helper function to consolidate mention and slash command processing, which is then applied to both text and tool_result blocks. The addition of a comprehensive regression test is a great way to ensure this issue doesn't reappear.
Additionally, the pull request enhances the Model Context Protocol (MCP) notification handling by adding support for dynamic capability refreshing when a server's tools, resources, or prompts change. The implementation is solid and includes thorough testing. I've noted a minor opportunity to reduce code duplication in the new notification handlers. The change to silently log unhandled notifications instead of displaying them to the user is also a welcome UX improvement.
| // 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 ResourceListChanged, ToolListChanged, and PromptListChanged are nearly identical. To improve maintainability and reduce code duplication, you could extract this logic into a helper function.
| // 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) | |
| } | |
| }) | |
| const createRefreshHandler = (capabilityType: "resources" | "tools" | "prompts") => { | |
| return async () => { | |
| console.log(`MCP ${name}: ${capabilityType} list changed, refreshing capabilities`) | |
| try { | |
| await onRefreshCapabilities?.(name) | |
| } catch (error) { | |
| console.error(`MCP ${name}: failed to refresh capabilities after ${capabilityType} 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")) |
|
Failed to generate code suggestions for PR |
Code Review by Qodo
1. Unmarked processTextContent changes
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| import { ClineRulesToggles } from "../../shared/cline-rules" | ||
|
|
||
| // This function is a duplicate of processUserContentMentions, but it adds a check for the newrules command | ||
| // and processes Kilo-specific slash commands. It should be merged with processUserContentMentions in the future. |
There was a problem hiding this comment.
1. Unmarked processtextcontent changes 📘 Rule violation ⛯ Reliability
New upstream-shared src/ changes were added without clear kilocode_change markers, increasing the risk of future upstream sync merge conflicts. This makes it harder to identify and reconcile Kilo-specific diffs during periodic mirrors.
Agent Prompt
## Issue description
New changes in an upstream-shared `src/` file were introduced without clear `kilocode_change` markers, which can increase merge conflict risk during upstream sync.
## Issue Context
This repo periodically mirrors/syncs from upstream (Roo Code). The compliance rule requires marking Kilo-specific edits in upstream-shared directories using documented `kilocode_change` marker forms.
## Fix Focus Areas
- src/core/mentions/processKiloUserContentMentions.ts[11-72]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // 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.
2. notificationservice logs raw notifications 📘 Rule violation ⛨ Security
The new MCP notification handlers log unstructured messages and, in the fallback handler, log the full notification object which may include sensitive user/tool data. This violates secure logging requirements and can leak sensitive information into extension logs.
Agent Prompt
## Issue description
`NotificationService` logs unstructured strings and logs raw notification objects (`notification`) and raw `error` objects. This can leak sensitive tool/user payloads into logs and makes auditing harder.
## Issue Context
MCP notifications may contain user/tool data. Compliance requires logs to be structured and contain no sensitive data at any log level.
## 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
| { | ||
| "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. Committed .beads artifacts 🐞 Bug ⛯ Reliability
The PR adds a .beads/ runtime state directory (PID, sync state, sqlite metadata/db files), which is machine-specific and will cause repo bloat, constant churn, and potential leakage of local state/errors. These files should not be versioned and should be removed from the PR and ignored.
Agent Prompt
### Issue description
The PR commits local/runtime `.beads/` artifacts (PID, sync-state, sqlite db-related files). These should not be tracked in git.
### Issue Context
`.beads/` contains non-deterministic state (timestamps/backoff, local PID, git error strings) and references a local sqlite database.
### Fix Focus Areas
- .gitignore[1-90]
- .beads/sync-state.json[1-7]
- .beads/metadata.json[1-4]
- .beads/daemon.pid[1-1]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| KILO_AUTO_APPROVAL_ENABLED: "true" | ||
| KILO_AUTO_APPROVAL_EXECUTE_ENABLED: "true" | ||
| KILO_AUTO_APPROVAL_EXECUTE_ALLOWED: "gh issue edit,gh pr edit" | ||
| KILO_AUTO_APPROVAL_EXECUTE_DENIED: "gh issue close,gh issue delete,gh issue transfer,gh issue lock,gh issue unlock,gh pr close,gh pr merge,gh repo,gh auth,gh secret,gh variable,rm,sudo,curl,wget,bash,sh,python,node,npm,npx" |
There was a problem hiding this comment.
4. Broad gh edit allowlist 🐞 Bug ⛨ Security
The new auto-triage workflow enables auto-approved command execution and allowlists `gh issue edit/gh pr edit` as prefixes, which permits arbitrary additional arguments to be auto-approved. This creates a prompt-injection risk where untrusted issue/PR content could steer the agent into executing unintended GitHub edits.
Agent Prompt
### Issue description
Auto-triage enables `KILO_AUTO_APPROVAL_EXECUTE_ENABLED` and allowlists broad command prefixes (`gh issue edit`, `gh pr edit`). The CLI’s allowlist uses prefix matching, so any additional arguments are auto-approved, creating a prompt-injection avenue.
### Issue Context
The job runs on `issues.opened` and `pull_request.opened`, meaning untrusted user-controlled content is included in the model prompt.
### Fix Focus Areas
- .github/workflows/auto-triage.yml[30-126]
- cli/src/services/approvalDecision.ts[28-60]
- cli/src/services/approvalDecision.ts[171-235]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Mirror of upstream PR #5752
Verdict: APPROVE | Tier 5
Review: kilocode-5752-review.md