feat: add MCP server registration and OpenAI API key configuration to init command#14
feat: add MCP server registration and OpenAI API key configuration to init command#14
Conversation
Add interactive MCP server registration during project initialization with support for multiple platforms and configuration types. Features: - Interactive prompt with arrow key navigation (promptui) - Support for 4 platforms: * Claude Desktop (global config) * Claude Code (project .mcp.json) * Cursor (project .cursor/mcp.json) * VS Code/Cline (project .vscode/mcp.json) - Platform-specific JSON formats (VS Code uses different structure) - New flags: --register-mcp (registration only), --skip-mcp (skip prompt) - Automatic backup creation before modification - Project-specific configs enable team collaboration via version control Changes: - Add promptui dependency for interactive selection - Create internal/cmd/mcp_register.go with registration logic - Update internal/cmd/init.go with MCP registration flow - Support both global and project-specific MCP configurations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add interactive API key configuration during project initialization with support for both environment variables and .sym/.env file. Features: - Interactive prompt only when API key not found - Priority: system env var > .sym/.env file - Masked input for API key entry - Basic validation (sk- prefix, length check) - Automatic .gitignore update for .sym/.env - File permissions set to 0600 for security New flags: - --setup-api-key: Setup API key only (skip roles/policy init) - --skip-api-key: Skip API key configuration prompt Changes: - Create internal/cmd/api_key.go with key management logic - Add promptAPIKeyIfNeeded() to init.go workflow - Update convert, validate, mcp commands to use getAPIKey() - Support loading API key from .sym/.env file Benefits: - No need to set environment variables manually - Project-specific API keys (team collaboration) - Secure file storage with restrictive permissions - Backward compatible with existing env var setup - Can be configured later with 'sym init --setup-api-key'
Remove '\n' from fmt.Println calls to fix go vet errors. fmt.Println automatically adds a newline, so explicit '\n' is redundant.
…ance - Add error checking for file.Close() calls in api_key.go - Add error checking for os.WriteFile() calls in mcp_register.go - Display warning messages when backup file creation fails - Fixes all errcheck linter violations in CI
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the developer onboarding experience by adding automated MCP (Model Context Protocol) server registration and OpenAI API key configuration to the sym init command. The changes introduce interactive prompts for setting up these optional features while maintaining backward compatibility through skip flags.
Key Changes:
- Added interactive MCP server registration for Claude Desktop, Claude Code, Cursor, and VS Code
- Implemented secure OpenAI API key configuration with
.sym/.envstorage and automatic.gitignoreupdates - Refactored existing commands to use centralized
getAPIKey()function for consistent API key retrieval
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/mcp_register.go | New file implementing MCP server registration logic with backup creation and multi-platform support |
| internal/cmd/api_key.go | New file handling API key prompts, validation, secure storage, and gitignore management |
| internal/cmd/init.go | Enhanced with new flags (--register-mcp, --skip-mcp, --setup-api-key, --skip-api-key) and integrated prompts |
| internal/cmd/validate.go | Updated to use centralized getAPIKey() function with improved error messaging |
| internal/cmd/mcp.go | Updated to use centralized getAPIKey() function with improved error messaging |
| internal/cmd/convert.go | Updated to use centralized getAPIKey() function with graceful fallback for missing API keys |
| go.mod | Added manifoldco/promptui and chzyer/readline dependencies (as indirect) |
| go.sum | Updated checksums for new dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip existing OPENAI_API_KEY lines | ||
| if !strings.HasPrefix(strings.TrimSpace(line), key+"=") { | ||
| lines = append(lines, line) | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The function filters out lines starting with the key prefix but doesn't preserve blank lines or comments in the original .env file. This means any existing structure (like section comments or blank line separators) will be lost when updating the API key.
Consider preserving the original file structure:
// Skip existing OPENAI_API_KEY lines, but preserve comments and blank lines
trimmedLine := strings.TrimSpace(line)
if !strings.HasPrefix(trimmedLine, key+"=") {
lines = append(lines, line)
}| github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e // indirect | ||
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/inconshreveable/mousetrap v1.1.0 // indirect | ||
| github.com/manifoldco/promptui v0.9.0 // indirect |
There was a problem hiding this comment.
The manifoldco/promptui dependency should be moved from indirect dependencies to direct dependencies since it's imported directly in the new files api_key.go and mcp_register.go. Indirect dependencies are typically for transitive dependencies only.
The line should be:
require (
github.com/bmatcuk/doublestar/v4 v4.9.1
github.com/manifoldco/promptui v0.9.0
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8
github.com/spf13/cobra v1.10.1
)| if err := json.Unmarshal(existingData, &vscodeConfig); err != nil { | ||
| // Invalid JSON, create backup | ||
| backupPath := configPath + ".bak" | ||
| if err := os.WriteFile(backupPath, existingData, 0644); err != nil { |
There was a problem hiding this comment.
[nitpick] Backup files are created with 0644 permissions, which may expose sensitive configuration data to other users on the system. Since the original config files may contain sensitive information or control system behavior, backup files should inherit more restrictive permissions.
Consider using 0600 for backup files:
if err := os.WriteFile(backupPath, existingData, 0600); err != nil {| // Add to .gitignore | ||
| lines = append(lines, "", "# Symphony API key configuration", path) | ||
| content := strings.Join(lines, "\n") + "\n" | ||
|
|
||
| if err := os.WriteFile(gitignorePath, []byte(content), 0644); err != nil { | ||
| return fmt.Errorf("failed to update .gitignore: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The ensureGitignore function has a potential race condition. Between checking if the path exists in .gitignore (lines 212-214) and writing to the file (line 224), another process could modify .gitignore, potentially causing the path to be added multiple times or losing other concurrent modifications.
Consider using file locking or an atomic read-modify-write operation if concurrent modifications are a concern in your use case.
| // Add to .gitignore | |
| lines = append(lines, "", "# Symphony API key configuration", path) | |
| content := strings.Join(lines, "\n") + "\n" | |
| if err := os.WriteFile(gitignorePath, []byte(content), 0644); err != nil { | |
| return fmt.Errorf("failed to update .gitignore: %w", err) | |
| } | |
| // Before writing, re-read the file to avoid race conditions | |
| // (in case another process added the path in the meantime) | |
| latestLines := []string{} | |
| latestFile, err := os.Open(gitignorePath) | |
| if err == nil { | |
| scanner := bufio.NewScanner(latestFile) | |
| found := false | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| latestLines = append(latestLines, line) | |
| if strings.TrimSpace(line) == path { | |
| found = true | |
| } | |
| } | |
| _ = latestFile.Close() | |
| if found { | |
| return nil // Already in .gitignore after re-check | |
| } | |
| } else { | |
| // If file doesn't exist, use the original lines (could be empty) | |
| latestLines = lines | |
| } | |
| // Add to .gitignore | |
| latestLines = append(latestLines, "", "# Symphony API key configuration", path) | |
| content := strings.Join(latestLines, "\n") + "\n" | |
| // Write to a temp file and atomically replace .gitignore | |
| tmpFile, err := os.CreateTemp("", "gitignore_tmp") | |
| if err != nil { | |
| return fmt.Errorf("failed to create temp file for .gitignore: %w", err) | |
| } | |
| defer func() { _ = os.Remove(tmpFile.Name()) }() | |
| if _, err := tmpFile.Write([]byte(content)); err != nil { | |
| _ = tmpFile.Close() | |
| return fmt.Errorf("failed to write to temp .gitignore: %w", err) | |
| } | |
| if err := tmpFile.Close(); err != nil { | |
| return fmt.Errorf("failed to close temp .gitignore: %w", err) | |
| } | |
| if err := os.Rename(tmpFile.Name(), gitignorePath); err != nil { | |
| return fmt.Errorf("failed to atomically update .gitignore: %w", err) | |
| } |
| if !strings.HasPrefix(key, "sk-") { | ||
| return fmt.Errorf("API key should start with 'sk-'") | ||
| } | ||
| if len(key) < 20 { |
There was a problem hiding this comment.
The API key validation is too lenient and doesn't account for newer OpenAI API key formats. As of 2023, OpenAI introduced project-based API keys that start with sk-proj- and other formats. The current validation will incorrectly reject valid API keys.
Consider updating the validation to be more flexible:
func validateAPIKey(key string) error {
if !strings.HasPrefix(key, "sk-") {
return fmt.Errorf("API key should start with 'sk-'")
}
if len(key) < 40 { // Modern OpenAI keys are typically 48+ chars
return fmt.Errorf("API key seems too short")
}
return nil
}| if len(key) < 20 { | |
| if len(key) < 40 { |
| Validate: func(input string) error { | ||
| if len(input) == 0 { | ||
| return fmt.Errorf("API key cannot be empty") | ||
| } | ||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| result, err := prompt.Run() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return strings.TrimSpace(result), nil |
There was a problem hiding this comment.
The API key value is trimmed after masking validation, but not before. If a user accidentally includes leading/trailing whitespace when pasting their API key, it will be saved with that whitespace, causing authentication failures. The validation at line 116 checks len(input) == 0 on the untrimmed input.
Consider trimming before validation:
Validate: func(input string) error {
input = strings.TrimSpace(input)
if len(input) == 0 {
return fmt.Errorf("API key cannot be empty")
}
return nil
},Or validate the trimmed result at line 128.
| return fmt.Errorf("OPENAI_API_KEY environment variable not set") | ||
| apiKey, err := getAPIKey() | ||
| if err != nil { | ||
| return fmt.Errorf("OpenAI API key not configured: %w\nTip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err) |
There was a problem hiding this comment.
The error message includes a newline character (\n) in the middle, which may not render correctly when wrapped with fmt.Errorf. Consider splitting this into separate fmt.Printf and return statements, or use a multiline string without \n in the format string.
Suggested fix:
if err != nil {
return fmt.Errorf("OpenAI API key not configured: %w. Tip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err)
}| return fmt.Errorf("OpenAI API key not configured: %w\nTip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err) | |
| return fmt.Errorf("OpenAI API key not configured: %w. Tip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err) |
| return fmt.Errorf("OPENAI_API_KEY environment variable not set") | ||
| apiKey, err := getAPIKey() | ||
| if err != nil { | ||
| return fmt.Errorf("OpenAI API key not configured: %w\nTip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err) |
There was a problem hiding this comment.
The error message includes a newline character (\n) in the middle, which may not render correctly when wrapped with fmt.Errorf. Consider splitting this into separate fmt.Printf and return statements, or use a multiline string without \n in the format string.
Suggested fix:
if err != nil {
return fmt.Errorf("OpenAI API key not configured: %w. Tip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err)
}| return fmt.Errorf("OpenAI API key not configured: %w\nTip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err) | |
| return fmt.Errorf("OpenAI API key not configured: %w. Tip: Run 'sym init' or set OPENAI_API_KEY in .sym/.env", err) |
| homeDir, _ := os.UserHomeDir() | ||
|
|
||
| // For project-specific configs, get current working directory (project root) | ||
| cwd, _ := os.Getwd() |
There was a problem hiding this comment.
Error values from os.UserHomeDir() and os.Getwd() are silently discarded. If these calls fail, the function will return an empty string for the config path, which will only be caught later. This makes debugging harder.
Consider handling these errors explicitly:
func getMCPConfigPath(app string) string {
homeDir, err := os.UserHomeDir()
if err != nil {
return "" // or log the error
}
cwd, err := os.Getwd()
if err != nil {
return "" // or log the error
}
// ... rest of function
}| homeDir, _ := os.UserHomeDir() | |
| // For project-specific configs, get current working directory (project root) | |
| cwd, _ := os.Getwd() | |
| homeDir, err := os.UserHomeDir() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Error getting user home directory: %v\n", err) | |
| return "" | |
| } | |
| // For project-specific configs, get current working directory (project root) | |
| cwd, err := os.Getwd() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Error getting current working directory: %v\n", err) | |
| return "" | |
| } |
| } | ||
|
|
||
| // Write config | ||
| if err := os.WriteFile(configPath, data, 0644); err != nil { |
There was a problem hiding this comment.
[nitpick] Potential security issue: File permissions 0644 for configuration files may be too permissive. While these files don't contain secrets directly, they control which executables get run by AI assistants with access to the user's system. Consider using more restrictive permissions (0600) to prevent other users from modifying the MCP server configuration.
Suggested fix:
if err := os.WriteFile(configPath, data, 0600); err != nil {
return fmt.Errorf("failed to write config: %w", err)
}| if err := os.WriteFile(configPath, data, 0644); err != nil { | |
| if err := os.WriteFile(configPath, data, 0600); err != nil { |
Summary
This PR enhances the
sym initcommand with automated MCP server registration and OpenAI API key configuration, improving the developer onboarding experience.What Changed
1. MCP Server Registration (
internal/cmd/mcp_register.go)~/Library/Application Support/Claude/claude_desktop_config.json(macOS)~/.cursor/mcp.json~/Library/Application Support/Code/User/settings.jsonnpx @dev-symphony/sym@latest mcp--register-mcp(register MCP only),--skip-mcp(skip MCP registration)2. OpenAI API Key Management (
internal/cmd/api_key.go).sym/.envwith 0600 permissions.sym/.envto.gitignore--setup-api-key(setup API key only),--skip-api-key(skip API key setup).sym/.envfile3. Enhanced Init Command (
internal/cmd/init.go)4. Code Quality Improvements
Usage Examples
Standard initialization with all features
Register MCP server only
sym init --register-mcp # Interactive prompt to choose: Claude Code, Cursor, or VS CodeSetup API key only
sym init --setup-api-key # Prompts for OpenAI API key with validationSkip prompts
sym init --skip-mcp --skip-api-key # Only creates roles.json and user-policy.jsonTest Plan
go vet ./...passesgolangci-lint runpasses with no errcheck violationsBreaking Changes
None. All new features are opt-in via flags.
Dependencies
github.com/manifoldco/promptuifor interactive promptsgithub.com/pkg/browserfor opening URLs