fix: skip Slack context parameters in tool call execution#124
Conversation
WalkthroughIntroduces a guard in LLMMCPBridge.executeToolCall to ignore Slack-specific context keys ("channel_id", "thread_ts") when merging extraArgs into toolCall.Args, with a debug log on skipped keys. No changes to function signatures or broader flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant LLMMCPBridge as LLMMCPBridge.executeToolCall
participant Tool as ToolExecutor
Caller->>LLMMCPBridge: executeToolCall(toolCall, extraArgs, ctx)
LLMMCPBridge->>LLMMCPBridge: Merge toolCall.Args with extraArgs
note over LLMMCPBridge: Skip keys: "channel_id", "thread_ts"<br/>Log debug when skipping
LLMMCPBridge->>Tool: Invoke with merged args (filtered)
Tool-->>LLMMCPBridge: Result/Response
LLMMCPBridge-->>Caller: Return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/handlers/llm_mcp_bridge.go (2)
451-456: Avoid overriding LLM-provided args with transport extrasPrevent silent clobbering of fields if both the LLM and extraArgs provide the same key.
- toolCall.Args[k] = v + if _, exists := toolCall.Args[k]; !exists { + toolCall.Args[k] = v + } else { + b.logger.DebugKV("Not overriding existing arg with extraArg", "key", k, "tool", toolCall.Tool) + }
457-471: Don’t log full args payloads; log only keys/count to reduce accidental PII leakageCurrent logs emit the entire args map. Safer to log keys and count.
- b.logger.DebugKV("Executing tool call", - "tool", toolCall.Tool, - "args", fmt.Sprintf("%v", toolCall.Args)) + argKeys := make([]string, 0, len(toolCall.Args)) + for k := range toolCall.Args { + argKeys = append(argKeys, k) + } + b.logger.DebugKV("Executing tool call", + "tool", toolCall.Tool, + "args_count", len(toolCall.Args), + "arg_keys", strings.Join(argKeys, ",")) @@ - b.logger.InfoKV("Calling MCP tool", - "tool", toolCall.Tool, - "server", serverName, - "args", fmt.Sprintf("%v", toolCall.Args)) + b.logger.InfoKV("Calling MCP tool", + "tool", toolCall.Tool, + "server", serverName, + "args_count", len(toolCall.Args), + "arg_keys", strings.Join(argKeys, ","))Note: strings is already imported; no extra imports needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/handlers/llm_mcp_bridge.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
internal/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tool handlers and LLM-MCP bridge should be implemented in
internal/handlers/
Files:
internal/handlers/llm_mcp_bridge.go
🧠 Learnings (1)
📚 Learning: 2025-07-20T02:54:03.119Z
Learnt from: CR
PR: tuannvm/slack-mcp-client#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T02:54:03.119Z
Learning: Applies to internal/handlers/**/*.go : Tool handlers and LLM-MCP bridge should be implemented in `internal/handlers/`
Applied to files:
internal/handlers/llm_mcp_bridge.go
🧬 Code graph analysis (1)
internal/handlers/llm_mcp_bridge.go (1)
internal/slack/client.go (1)
c(611-786)
🔇 Additional comments (1)
internal/handlers/llm_mcp_bridge.go (1)
1-3: Placement aligns with project guidelinesLLM–MCP bridge lives under internal/handlers as required. No action needed.
| // Skip Slack context parameters as they're for internal tracking, not tool parameters | ||
| if k == "channel_id" || k == "thread_ts" { | ||
| b.logger.DebugKV("Skipping Slack context parameter", "key", k, "tool", toolCall.Tool) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good guard, but also scrub these keys from toolCall.Args to fully meet the PR objective
Right now we only skip adding Slack context from extraArgs. If the LLM (or upstream) already put channel_id or thread_ts into toolCall.Args, they’ll still be sent to MCP. Scrub them from toolCall.Args as well and centralize the denylist to avoid drift.
Apply:
func (b *LLMMCPBridge) executeToolCall(ctx context.Context, toolCall *ToolCall, extraArgs map[string]interface{}) (string, error) {
- for k, v := range extraArgs {
- // Skip Slack context parameters as they're for internal tracking, not tool parameters
- if k == "channel_id" || k == "thread_ts" {
- b.logger.DebugKV("Skipping Slack context parameter", "key", k, "tool", toolCall.Tool)
- continue
- }
+ // Denylist of Slack context keys that must never be forwarded to MCP tools
+ disallowedKeys := map[string]struct{}{
+ "channel_id": {},
+ "thread_ts": {},
+ }
+
+ // Also scrub these keys if they were present in the LLM-supplied args
+ if toolCall.Args != nil {
+ for k := range disallowedKeys {
+ if _, ok := toolCall.Args[k]; ok {
+ b.logger.DebugKV("Removing disallowed argument from tool args", "key", k, "tool", toolCall.Tool)
+ delete(toolCall.Args, k)
+ }
+ }
+ }
+
+ for k, v := range extraArgs {
+ // Skip Slack context parameters as they're for internal tracking, not tool parameters
+ if _, disallowed := disallowedKeys[k]; disallowed {
+ b.logger.DebugKV("Skipping Slack context parameter", "key", k, "tool", toolCall.Tool)
+ continue
+ }
// Add any extra arguments to the tool call args
if toolCall.Args == nil {
toolCall.Args = make(map[string]interface{})
}
toolCall.Args[k] = v
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Skip Slack context parameters as they're for internal tracking, not tool parameters | |
| if k == "channel_id" || k == "thread_ts" { | |
| b.logger.DebugKV("Skipping Slack context parameter", "key", k, "tool", toolCall.Tool) | |
| continue | |
| } | |
| func (b *LLMMCPBridge) executeToolCall(ctx context.Context, toolCall *ToolCall, extraArgs map[string]interface{}) (string, error) { | |
| // Denylist of Slack context keys that must never be forwarded to MCP tools | |
| disallowedKeys := map[string]struct{}{ | |
| "channel_id": {}, | |
| "thread_ts": {}, | |
| } | |
| // Also scrub these keys if they were present in the LLM-supplied args | |
| if toolCall.Args != nil { | |
| for k := range disallowedKeys { | |
| if _, ok := toolCall.Args[k]; ok { | |
| b.logger.DebugKV("Removing disallowed argument from tool args", "key", k, "tool", toolCall.Tool) | |
| delete(toolCall.Args, k) | |
| } | |
| } | |
| } | |
| for k, v := range extraArgs { | |
| // Skip Slack context parameters as they're for internal tracking, not tool parameters | |
| if _, disallowed := disallowedKeys[k]; disallowed { | |
| b.logger.DebugKV("Skipping Slack context parameter", "key", k, "tool", toolCall.Tool) | |
| continue | |
| } | |
| // Add any extra arguments to the tool call args | |
| if toolCall.Args == nil { | |
| toolCall.Args = make(map[string]interface{}) | |
| } | |
| toolCall.Args[k] = v | |
| } | |
| // ... rest of executeToolCall ... | |
| } |
🤖 Prompt for AI Agents
In internal/handlers/llm_mcp_bridge.go around lines 445 to 449, the current
guard skips adding Slack context keys from extraArgs but does not remove them if
they already exist in toolCall.Args; update the code to centralize a denylist
(e.g., a package-level slice or map containing "channel_id" and "thread_ts") and
before sending to MCP iterate that denylist to delete those keys from
toolCall.Args (and extraArgs if still needed), keeping the existing DebugKV log
when a key is found and removed; ensure the denylist is reused wherever similar
filtering is required to avoid drift.
|
I'm encountering this error as well due to the slack context arguments being injected into a narrowly scoped MCP tool schema. |
|
This has been added to have better context of threads for summarizing threads and accomplish further. As anyway we are adding thread conversation to context history, but sometimes when using slack mcp server requires slack channel id and thread ts. It would be better if we have a flag and disallowed/allowed parameters or disallowed/alllowed tools/mcp servers to have extra parameters. And handling this from config block |
@tuannvm what do you think of this |
3 participants
I noticed some MCP servers don't quite like having the
channel_idandthread_tsfrom Slack context when executing tool calls, namely the Notion MCP was having problems with that and thus I removed that from the tool call.Summary by CodeRabbit