config is not initialized after JSON unmarshal succeeds #1133
Conversation
- If the JSON unmarshal succeeds, but the file on disk does not contain the MCPServers field (or it is null in the JSON), then after unmarshal, config.MCPServers will be nil. This is a subtle bug: If the config file exists and is missing the MCPServers field, unmarshal will not set it, leaving it as nil. - Then, when I try to assign to config.MCPServers["defang"], causing the panic. - Solution: map instantiation guard before assignment.
KevyVo
changed the title
This will test many of the edge cases that were previously not tested, including: - Incorrect/misspelled client name - If the client exists or not - If the config file exists or not - If the config file is empty or not - Check the config contents - Check for expected errors
There was a problem hiding this comment.
LBYL (Look Before You Leap) and EAFP (Easier to Ask Forgiveness than Permission) are two coding styles for handling errors. LBYL involves checking conditions before performing actions, while EAFP focuses on trying to perform actions and handling exceptions if they occur, with EAFP being the preferred idiomatic approach: don't do Stat before reading the file.
src/pkg/mcp/setup.go
Outdated
| switch runtime.GOOS { | ||
| case "darwin": | ||
| return filepath.Join(homeDir, "Library", "Application Support", "Claude", "claude_desktop_config.json"), nil | ||
| case "windows": | ||
| return filepath.Join(getAppData(), "Claude", "claude_desktop_config.json"), nil | ||
| default: | ||
| return filepath.Join(getConfigHome(), "Claude", "claude_desktop_config.json"), nil | ||
| } |
src/pkg/mcp/setup.go
Outdated
| @@ -258,13 +248,21 @@ func SetupClient(client string) error { | |||
|
|
|||
| term.Infof("Updating %q\n", configPath) | |||
|
|
|||
| // Create the directory if it doesn't exist | |||
| // Create the directory path | |||
There was a problem hiding this comment.
I think this is mislabeled now thinking about it, it should say "create the string to the file's directory" instead.
src/pkg/mcp/setup.go
Outdated
| info, err := os.Stat(configDir) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return fmt.Errorf("The client %s you are trying to setup is not install or not found in your system path, please try again after installing.", client) |
There was a problem hiding this comment.
spelling; no capital and no punctuation
There was a problem hiding this comment.
Sorry I was just when you said "no capital and no punctuation". Do you mean like this needs capitation and punctuation or we do not include capitation and punctuation in our errors?
There was a problem hiding this comment.
The go linter doesn’t like it when errors start with a capital or end with punctuation
src/pkg/mcp/setup.go
Outdated
| return fmt.Errorf("failed to check config directory: %w", err) | ||
| } | ||
| if !info.IsDir() { | ||
| return fmt.Errorf("Config path %s exists but is not a directory, please try again after installing or update the client %s to support mcp.", configDir, client) |
There was a problem hiding this comment.
spelling; no capital and no punctuation
I see. Thank you for giving me feedback on my erroring handling. I will try and and catch if it fails then. |
- Added more testcases to setup - Move error handling to EAFP
- Need to add a test for the getClientConfigPath function in setup.go - The test should check if the function correctly returns the path to the client config file
KevyVo
force-pushed
the
kevin/mcpMapBug
branch
5 times, most recently
from
1c2d6af to
76667dd
Compare
| } | ||
|
|
||
| // For testing purposes, this can be overridden | ||
| var currentOS = runtime.GOOS |
There was a problem hiding this comment.
Instead of checking GOOS in code, let's use Go build tags (filename suffix)
|
Update on this, we are going to be using prefix like colonizer. |
Description
If the JSON unmarshal succeeds, but the file on disk does not contain the MCPServers field (or it is null in the JSON), then after unmarshal, config.MCPServers will be nil. This is a subtle bug: If the config file exists and is missing the MCPServers field, unmarshal will not set it, leaving it as nil.
Then, when I try to assign to config.MCPServers["defang"], causing the panic.
Solution: map instantiation guard before assignment.
Linked Issues
Checklist