refactor: make MCP support optional per agent#21
refactor: make MCP support optional per agent#21joshuadavidthomas wants to merge 1 commit intogetsentry:mainfrom
Conversation
Make the `mcp` field optional on `AgentDefinition`, matching the existing pattern for `hooks`. Update mcp-writer to skip agents without MCP support (with warnings) and add `mcpWarnings` to `InstallResult`. This enables adding agents that support skills but not MCP.
|
@joshuadavidthomas is attempting to deploy a commit to the Sentry Team on Vercel. A member of the Team first needs to authorize it. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } | ||
| } | ||
|
|
||
| return warnings; |
There was a problem hiding this comment.
Sync command silently discards MCP warnings from write
Medium Severity
writeMcpConfigs now returns McpWriteWarning[] instead of void, but the caller in sync.ts (line 197) still uses await writeMcpConfigs(...) without capturing the return value. When the sync command repairs MCP configs, warnings about agents that don't support MCP are silently discarded, unlike in install.ts where they're properly captured and displayed to the user.
There was a problem hiding this comment.
I was following the existing pattern in writeHookConfigs on line 216 which has the same silent discard on main. Can fix if needed though.
|
pi tried to change the mcp situation and I stopped it to keep thing simpler |
Yeah, I wasn't sure the best way to handle it. Pi's a bit of a unique beast, I'm not sure I know of any other agentic tool that doesn't have MCP support out of the box. (Part of the reason I like using it! Lean and mean.) |
|
hey guys, i am gonna check it in a few hours and plan what we wanna do there. thank you!! |
|
@gricha No hard feelings if you want to close this. I basically unilaterally chose this big change without consulting you or any other maintainers via an issue or discussion and it may not be the direction y'all want to go. |
|
@joshuadavidthomas thank you for contributing here, and sorry for the back and forth but after some consideration, the path we decided to take is to let pi configure it self with a simple instruction (does actually feel like the "pi way" since it's the simplest). More context here: |
|
@bruno-garcia No problem! I was excited about the tool and enthusiastically jumped in to contribute without consulting, totally understand. Thanks for the follow up. |
3 participants
Makes the
mcpfield optional onAgentDefinition, matching how hooks already works. Agents without MCP support get a warning during install instead of crashing or silently writing config files nobody reads.This came out of adding pi agent support — I got scooped by #20 while working on it, but took a different approach. Their PR writes MCP config files that pi doesn't actually read. This PR makes the field optional instead, so agents can honestly declare they don't support MCP. Split it out to keep the review focused since it's a general infrastructure change that any future agent without MCP would need.