Add AMP MCP support #382
Add AMP MCP support #382
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughA new boolean Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/mcp/config.go (1)
83-90: Hook invocation looks good but error propagation could be clearerThe hook is invoked and its (possibly mutated) buffer replaces
content, which is fine.
Consider wrapping the error to provide context about which client failed, e.g.:- content, err = c.client.BeforeSaveHook(content) - if err != nil { - return err - } + if content, err = c.client.BeforeSaveHook(content); err != nil { + return fmt.Errorf("BeforeSaveHook (%s): %w", c.client.Name, err) + }This makes debugging easier when multiple client configs are processed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/mcp/config.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/mcp/config.go (2)
42-44: 👍 New hook field is well-isolated from JSON marshallingThe
BeforeSaveHookis tagged withjson:"-", so it will not leak into the persisted config.
No issues spotted here.
56-57: Potential for reference cycles & unintended capturesAdding a back-pointer (
client *MCPClientConfig) makes sense for invoking the hook, but it also:
- Creates a cyclic reference (
MCPClientConfig → MCPConfig → MCPClientConfig) that prevents the GC from freeing either object until both are out of scope. This is fine for short-lived CLI execution, but be mindful if these structs are ever kept globally.- Relies on the pointer being set correctly (see comment below on the
for _, config := range detectedloop).No action required here, but worth noting.
| config.Config.client = &config | ||
| if config.Config.AddIfNotExists(agentuityToolName, executable, append(agentuityToolArgs, "--"+config.Transport), agentuityToolEnv) { | ||
| if err := config.Config.Save(); err != nil { |
There was a problem hiding this comment.
❗️ Taking the address of the loop variable breaks the hook mechanism
for _, config := range detected { ... } creates a new copy of each element; config is re-used on every iteration.
Doing config.Config.client = &config therefore stores a pointer to the same loop variable, so after the loop all client pointers reference the last element.
Hooks for every other client will never fire (or will fire with the wrong data).
Fix:
-for _, config := range detected {
+for i := range detected {
+ config := &detected[i] // pointer to the slice element
...
- config.Config.client = &config
+ config.Config.client = configor any equivalent pattern that avoids taking the address of the loop variable.
📝 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.
| config.Config.client = &config | |
| if config.Config.AddIfNotExists(agentuityToolName, executable, append(agentuityToolArgs, "--"+config.Transport), agentuityToolEnv) { | |
| if err := config.Config.Save(); err != nil { | |
| // change the loop to index-based so we can take the address of each slice element | |
| -for _, config := range detected { | |
| +for i := range detected { | |
| + config := &detected[i] // pointer to the actual slice element | |
| // … other setup … | |
| - config.Config.client = &config | |
| + config.Config.client = config | |
| if config.Config.AddIfNotExists(agentuityToolName, | |
| executable, | |
| append(agentuityToolArgs, "--"+config.Transport), | |
| agentuityToolEnv, | |
| ) { | |
| if err := config.Config.Save(); err != nil { | |
| // … | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/mcp/config.go around lines 230 to 232, the code takes the address of
the loop variable 'config' inside a range loop, causing all pointers to
reference the same variable and leading to incorrect hook behavior. To fix this,
create a new variable inside the loop that copies the current element, then take
the address of that new variable instead of the loop variable. This ensures each
pointer references a distinct element and hooks work correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/mcp/config.go (1)
279-289:⚠️ Potential issueUninstall requires the entry to be present in both maps, making AMP-only installs impossible to remove
The code skips when the agentuity entry is missing from either map, so an install that exists only in
amp.mcpServersis never uninstalled.- if _, ok := mcpconfig.MCPServers[agentuityToolName]; !ok { + if _, ok := mcpconfig.MCPServers[agentuityToolName]; !ok { logger.Debug("config for %s not found in %s, skipping", config.Name, config.ConfigLocation) - continue + // continue only if it is *also* not in the AMP map + if _, ok := mcpconfig.AMPMCPServers[agentuityToolName]; !ok { + continue + } } - if _, ok := mcpconfig.AMPMCPServers[agentuityToolName]; !ok { - logger.Debug("config for %s not found in %s, skipping", config.Name, config.ConfigLocation) - continue - } delete(mcpconfig.MCPServers, agentuityToolName) delete(mcpconfig.AMPMCPServers, agentuityToolName)This allows removal when the tool is present in either map.
♻️ Duplicate comments (1)
internal/mcp/config.go (1)
217-239:⚠️ Potential issueStill taking the address of the range loop variable – same bug as flagged earlier
The pointer stored in
config.Config.client = &configwill reference the re-used loop variable, not the slice element, so only the last client’s hooks behave correctly.This was already pointed out in a previous review and remains unfixed.
-for _, config := range detected { - ... - config.Config.client = &config +for i := range detected { + cfg := &detected[i] // pointer to the slice element + ... + cfg.Config.client = cfg
🧹 Nitpick comments (1)
internal/mcp/config.go (1)
52-57: Consider backwards compatibility for the newamp.mcpServersJSON keyUsing a dotted key is valid JSON, but existing tooling or grep-based scripts may not expect it. If you ever need to rename it, add a complementary load-hook/alias to keep older files working.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/mcp/config.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
| func (c *MCPConfig) AddIfNotExists(name string, command string, args []any, env map[string]any, isAMP bool) bool { | ||
| if isAMP { | ||
| if _, ok := c.AMPMCPServers[name]; ok { | ||
| return false | ||
| } | ||
| c.AMPMCPServers[name] = MCPServerConfig{ | ||
| Command: command, | ||
| Args: args, | ||
| Env: env, | ||
| } | ||
| } else { | ||
| if _, ok := c.MCPServers[name]; ok { | ||
| return false | ||
| } | ||
| c.MCPServers[name] = MCPServerConfig{ | ||
| Command: command, | ||
| Args: args, | ||
| Env: env, | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Possible nil-map panic in AddIfNotExists
c.AMPMCPServers (and c.MCPServers) may be nil when the config was just unmarshalled and the JSON key was absent.
Writing to a nil map panics, so install on a pre-existing file without AMP entries will crash.
-func (c *MCPConfig) AddIfNotExists(name string, command string, args []any, env map[string]any, isAMP bool) bool {
+func (c *MCPConfig) AddIfNotExists(name string, command string, args []any, env map[string]any, isAMP bool) bool {
+ // lazily initialise to avoid nil-map panics
+ if c.MCPServers == nil {
+ c.MCPServers = make(map[string]MCPServerConfig)
+ }
+ if c.AMPMCPServers == nil {
+ c.AMPMCPServers = make(map[string]MCPServerConfig)
+ }Add the above guard before the isAMP branching.
📝 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.
| func (c *MCPConfig) AddIfNotExists(name string, command string, args []any, env map[string]any, isAMP bool) bool { | |
| if isAMP { | |
| if _, ok := c.AMPMCPServers[name]; ok { | |
| return false | |
| } | |
| c.AMPMCPServers[name] = MCPServerConfig{ | |
| Command: command, | |
| Args: args, | |
| Env: env, | |
| } | |
| } else { | |
| if _, ok := c.MCPServers[name]; ok { | |
| return false | |
| } | |
| c.MCPServers[name] = MCPServerConfig{ | |
| Command: command, | |
| Args: args, | |
| Env: env, | |
| } | |
| } | |
| return true | |
| } | |
| func (c *MCPConfig) AddIfNotExists(name string, command string, args []any, env map[string]any, isAMP bool) bool { | |
| // lazily initialise to avoid nil-map panics | |
| if c.MCPServers == nil { | |
| c.MCPServers = make(map[string]MCPServerConfig) | |
| } | |
| if c.AMPMCPServers == nil { | |
| c.AMPMCPServers = make(map[string]MCPServerConfig) | |
| } | |
| if isAMP { | |
| if _, ok := c.AMPMCPServers[name]; ok { | |
| return false | |
| } | |
| c.AMPMCPServers[name] = MCPServerConfig{ | |
| Command: command, | |
| Args: args, | |
| Env: env, | |
| } | |
| } else { | |
| if _, ok := c.MCPServers[name]; ok { | |
| return false | |
| } | |
| c.MCPServers[name] = MCPServerConfig{ | |
| Command: command, | |
| Args: args, | |
| Env: env, | |
| } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
In internal/mcp/config.go around lines 59 to 80, the maps c.AMPMCPServers and
c.MCPServers may be nil if the config was unmarshalled from JSON without those
keys, causing a panic when writing to them. To fix this, add a nil check before
the isAMP conditional and initialize the maps if they are nil to prevent nil-map
panics when adding entries.
…the settings.json vs a normal mcp config
Summary by CodeRabbit
New Features
Enhancements
Chores