Conversation
There was a problem hiding this comment.
Pull request overview
Updates the generated MCP Docker command to always pull the latest container image, and adjusts mcp.json handling to better preserve existing configuration while avoiding overwriting invalid JSON.
Changes:
- Add
--pull=alwaysto the Dockerruninvocation for both Windows and macOS/Linux. - Read existing
.vscode/mcp.jsonas raw text first, then parse with explicit handling for invalid JSON (stop + show error). - Preserve existing top-level
mcp.jsonkeys by spreading...existingMCPJsoninto the newly generated content.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const fileData = await vscode.workspace.fs.readFile(mcpJsonPath); | ||
| const mcpJsonContent = Buffer.from(fileData).toString("utf8"); | ||
| existingMCPJson = JSON.parse(mcpJsonContent); | ||
| existingRawContent = Buffer.from(fileData).toString("utf8"); | ||
| } catch { | ||
| /* Do nothing - just means there is not an existing mcp.json */ | ||
| /* File doesn't exist - OK, we'll create a new one */ | ||
| } |
There was a problem hiding this comment.
The readFile() catch currently ignores all errors, but the comment assumes the file doesn't exist. This will also silently ignore cases like permission issues or transient FS errors and then proceed to overwrite/create mcp.json. Consider only swallowing the specific "file not found" error and surfacing other failures to the user (e.g., showErrorMessage and return).
| if (existingRawContent !== null) { | ||
| try { | ||
| existingMCPJson = JSON.parse(existingRawContent); | ||
| } catch (error: any) { |
There was a problem hiding this comment.
After JSON.parse, the code assumes the parsed value is an object shaped like MCPConfiguration. If the file contains a valid non-object JSON value (array/string/number/null), later code will behave unexpectedly and may generate an invalid config. Add a runtime guard that the parsed value is a non-null plain object (and that servers, if present, is also an object) before treating it as MCPConfiguration; otherwise show an error and return.
| // File exists but is not valid JSON (e.g., trailing comma, comments). | ||
| // Do NOT overwrite — tell the user to fix it manually. | ||
| vscode.window.showErrorMessage( | ||
| `Existing .vscode/mcp.json could not be parsed as JSON (${error?.message || error}). Please fix or remove the file before running this command.`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This PR is titled "add --pull=always" but it also changes createMCPJson behavior: it now preserves top-level keys via ...existingMCPJson and, more importantly, it stops and shows an error if an existing mcp.json is invalid JSON (previously it would proceed and overwrite). Please update the PR title/description to reflect these additional user-facing behavior changes, or split into separate PRs.
No description provided.