Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… existing files - Modified ensureMCPConfig to detect existing .vscode/mcp.json and render console output with required changes - Modified ensureCopilotSetupSteps to detect existing .github/workflows/copilot-setup-steps.yml and render console output - Added renderMCPConfigUpdateInstructions helper to display JSON configuration for MCP - Added renderCopilotSetupUpdateInstructions helper to display YAML steps for copilot setup - Updated tests to verify files are NOT modified when they already exist - Updated tests to expect console output instructions instead of file editing - Maintains backward compatibility with upgrade command which still modifies files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Verified all tests pass with new behavior - Manually tested init command with existing files - Confirmed files are NOT modified when they already exist - Console output provides clear instructions for manual updates - Maintains backward compatibility with upgrade command Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the init command behavior to avoid modifying existing user files for MCP/Copilot setup, and instead prints console instructions for manual updates.
Changes:
- Update MCP config initialization to create
.vscode/mcp.jsononly when missing; otherwise render a JSON snippet with instructions. - Update Copilot setup workflow initialization to create
.github/workflows/copilot-setup-steps.ymlonly when missing; otherwise render YAML step instructions (while keepingupgrade’s version-update behavior). - Adjust unit tests to assert that existing files remain unchanged when present.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_config_file.go | Stops editing existing .vscode/mcp.json; prints update instructions instead. |
| pkg/cli/copilot_setup.go | Stops editing existing copilot-setup-steps.yml during init; prints update instructions instead; preserves upgrade path. |
| pkg/cli/mcp_config_file_test.go | Updates expectations for existing MCP config files to remain unchanged. |
| pkg/cli/init_mcp_test.go | Updates MCP init tests to verify existing files aren’t modified. |
| pkg/cli/copilot_setup_test.go | Updates workflow setup tests to verify existing files aren’t modified. |
Comments suppressed due to low confidence (1)
pkg/cli/mcp_config_file.go:114
renderMCPConfigUpdateInstructionsignores thejson.MarshalIndenterror. If marshaling fails for any reason, the function will print an empty/partial snippet without indicating failure. Handle the error and print a fallback message (or return an error so callers can surface it).
// Generate the JSON to add
serverJSON, _ := json.MarshalIndent(map[string]VSCodeMCPServer{
serverName: serverConfig,
}, "", " ")
fmt.Fprintln(os.Stderr, string(serverJSON))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ensureCopilotSetupStepsWithUpgrade creates .github/workflows/copilot-setup-steps.yml | ||
| // If the file already exists, it renders console instructions instead of editing | ||
| // When upgradeVersion is true and called from upgrade command, this is a special case | ||
| func ensureCopilotSetupStepsWithUpgrade(verbose bool, actionMode workflow.ActionMode, version string, upgradeVersion bool) error { | ||
| copilotSetupLog.Printf("Creating copilot-setup-steps.yml with action mode: %s, version: %s, upgradeVersion: %v", actionMode, version, upgradeVersion) | ||
|
|
There was a problem hiding this comment.
In ensureCopilotSetupStepsWithUpgrade, the os.Stat(setupStepsPath) existence check later in this function treats any non-nil error as "file doesn't exist" and will fall through to os.WriteFile(...). If Stat fails for reasons other than ErrNotExist (permissions/IO), this can overwrite an existing workflow—contradicting the new "don’t edit existing files" init behavior. Recommend handling ErrNotExist explicitly and returning an error on other Stat failures.
| // Check if file already exists | ||
| if data, err := os.ReadFile(mcpConfigPath); err == nil { | ||
| mcpConfigLog.Printf("File already exists: %s", mcpConfigPath) | ||
|
|
||
| // Parse existing config | ||
| var config MCPConfig | ||
| if err := json.Unmarshal(data, &config); err != nil { | ||
| return fmt.Errorf("failed to parse existing mcp.json: %w", err) | ||
| } | ||
|
|
||
| // Check if the server is already configured correctly | ||
| if existingConfig, exists := config.Servers[ghAwServerName]; exists { | ||
| existingJSON, _ := json.Marshal(existingConfig) | ||
| newJSON, _ := json.Marshal(ghAwConfig) | ||
| if string(existingJSON) == string(newJSON) { | ||
| mcpConfigLog.Print("Configuration is identical, skipping") | ||
| if verbose { | ||
| fmt.Fprintf(os.Stderr, "MCP server '%s' already configured in %s\n", ghAwServerName, mcpConfigPath) | ||
| } | ||
| return nil | ||
| } | ||
| return nil | ||
| } | ||
| mcpConfigLog.Print("Configuration differs, updating") | ||
|
|
||
| // File exists but needs update - render instructions instead of editing | ||
| mcpConfigLog.Print("File exists, rendering update instructions instead of editing") | ||
| renderMCPConfigUpdateInstructions(mcpConfigPath, ghAwServerName, ghAwConfig) | ||
| return nil | ||
| } | ||
|
|
||
| // File doesn't exist - create it | ||
| mcpConfigLog.Print("No existing config found, creating new one") | ||
| config := MCPConfig{ |
There was a problem hiding this comment.
ensureMCPConfig treats any os.ReadFile error as "file doesn't exist" and proceeds to create/write .vscode/mcp.json. If the file exists but is unreadable (permissions/IO error), this can overwrite a user’s existing config—contradicting the new "do not edit existing files" behavior. Handle os.ErrNotExist explicitly and return an error for other read failures (and avoid writing in that case).
| if existingConfig, exists := config.Servers[ghAwServerName]; exists { | ||
| existingJSON, _ := json.Marshal(existingConfig) | ||
| newJSON, _ := json.Marshal(ghAwConfig) | ||
| if string(existingJSON) == string(newJSON) { |
There was a problem hiding this comment.
Errors from json.Marshal are ignored when comparing existingConfig vs ghAwConfig. Even if errors are unlikely here, ignoring them can mask unexpected marshal failures and lead to incorrect "identical" detection. Prefer handling the error (or compare the structs directly, e.g., field-by-field / reflect.DeepEqual for the slice).
| copilotSetupLog.Print("Extension install step already exists, file is up to date") | ||
| if verbose { | ||
| fmt.Fprintf(os.Stderr, "Skipping %s (already has gh-aw extension install step)\n", setupStepsPath) |
There was a problem hiding this comment.
The message "Extension install step already exists, file is up to date" is stronger than what the code verifies (it only checks for presence of an install step substring, not correctness/version/mode). Consider rewording to something like "install step detected; leaving file unchanged" to avoid misleading logs/output.
| copilotSetupLog.Print("Extension install step already exists, file is up to date") | |
| if verbose { | |
| fmt.Fprintf(os.Stderr, "Skipping %s (already has gh-aw extension install step)\n", setupStepsPath) | |
| copilotSetupLog.Print("Extension install step detected; leaving file unchanged") | |
| if verbose { | |
| fmt.Fprintf(os.Stderr, "Skipping %s (detected existing gh-aw extension install step)\n", setupStepsPath) |
| @@ -55,14 +55,16 @@ func TestEnsureMCPConfig(t *testing.T) { | |||
| verbose: true, | |||
| wantErr: false, | |||
There was a problem hiding this comment.
These cases are now named "renders instructions…" but the test only asserts the file is unchanged; it doesn’t verify that the expected instructions/snippet were actually printed. Consider capturing os.Stderr (as done in other tests in this repo) and asserting it contains e.g. "Existing file detected" and the github-agentic-workflows JSON block.
| @@ -93,7 +93,7 @@ func TestEnsureCopilotSetupSteps(t *testing.T) { | |||
| verbose: false, | |||
| wantErr: false, | |||
| validateContent: func(t *testing.T, content []byte) { | |||
There was a problem hiding this comment.
This test asserts the workflow file remains unchanged, but it doesn’t assert that update instructions were rendered. Since the new behavior is "render to console instead of editing", please capture os.Stderr and verify it includes a recognizable instruction header (e.g. "Existing file detected") and the expected step snippet for the chosen action mode.
The
initcommand was modifying existing.vscode/mcp.jsonand.github/workflows/copilot-setup-steps.ymlfiles, disrupting user formatting and customizations.Changes
File detection and console rendering
ensureMCPConfig: Detects existing.vscode/mcp.json, renders JSON snippet to console instead of editingensureCopilotSetupSteps: Detects existing.github/workflows/copilot-setup-steps.yml, renders YAML steps to console instead of editingrenderMCPConfigUpdateInstructionsandrenderCopilotSetupUpdateInstructionshelpers for formatted outputBehavior
Example output
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.