diff --git a/README.md b/README.md index 5b166e5a..d6ebfdde 100644 --- a/README.md +++ b/README.md @@ -211,6 +211,51 @@ See **[Configuration Specification](https://github.com/github/gh-aw/blob/main/do - **Passthrough**: Set value to empty string (`""`) to pass through from host - **Expansion**: Use `${VAR_NAME}` syntax for dynamic substitution (fails if undefined) +### Configuration Validation and Error Handling + +MCP Gateway provides detailed error messages and validation to help catch configuration issues early: + +#### Parse Errors with Precise Location + +When there's a syntax error in your TOML configuration, the gateway reports the exact line and column: + +```bash +$ awmg --config config.toml +Error: failed to parse TOML at line 2, column 6: expected '.' or '=', but got '3' instead +``` + +This helps you quickly identify and fix syntax issues. + +#### Unknown Key Detection (Typo Detection) + +The gateway detects and warns about unknown configuration keys, helping catch typos and deprecated options: + +```toml +[gateway] +prot = 3000 # Typo: should be 'port' +startup_timout = 30 # Typo: should be 'startup_timeout' +``` + +When you run the gateway with these typos, you'll see warnings in the log file: + +``` +[2026-02-07T17:46:51Z] [WARN] [config] Unknown configuration key 'gateway.prot' - check for typos or deprecated options +[2026-02-07T17:46:51Z] [WARN] [config] Unknown configuration key 'gateway.startup_timout' - check for typos or deprecated options +``` + +The gateway will use default values for unrecognized keys, so it will still start, but the warnings help you identify and fix configuration issues. + +#### Memory-Efficient Parsing + +The gateway uses streaming parsing for configuration files, making it efficient even with large configuration files containing many servers. + +#### Best Practices + +1. **Check logs for warnings**: After starting the gateway, check the log file for any warnings about unknown keys +2. **Use precise error messages**: When you see a parse error, the line and column numbers point exactly to the problem +3. **Validate configuration**: Test your configuration changes by running the gateway and checking for warnings +4. **Keep configuration clean**: Remove any deprecated or unused configuration options + ## Usage ``` diff --git a/config.example.toml b/config.example.toml index 895ed1fb..9cce5475 100644 --- a/config.example.toml +++ b/config.example.toml @@ -139,8 +139,14 @@ args = [ # # Configuration Validation: # - Required fields: command, args (for stdio servers) -# - Unknown keys will generate warnings in logs -# - Parse errors will show line numbers for easy debugging +# - Unknown keys will generate warnings in logs (helps catch typos!) +# - Parse errors show exact line and column numbers for easy debugging +# - Example: "failed to parse TOML at line 2, column 6: expected '=' ..." +# +# Common Typos Detected: +# - prot → port +# - startup_timout → startup_timeout +# - tool_timout → tool_timeout # # For more information, see: # - MCP Protocol: https://github.com/modelcontextprotocol diff --git a/internal/config/config_core.go b/internal/config/config_core.go index c2d1bfbb..9416167c 100644 --- a/internal/config/config_core.go +++ b/internal/config/config_core.go @@ -9,6 +9,7 @@ import ( "os" "github.com/BurntSushi/toml" + "github.com/github/gh-aw-mcpg/internal/logger" ) // Core constants for configuration defaults @@ -91,20 +92,48 @@ type ServerConfig struct { // LoadFromFile loads configuration from a TOML file. func LoadFromFile(path string) (*Config, error) { logConfig.Printf("Loading configuration from file: %s", path) - data, err := os.ReadFile(path) + + // Open file for streaming + file, err := os.Open(path) if err != nil { - return nil, fmt.Errorf("failed to read config file: %w", err) + return nil, fmt.Errorf("failed to open config file: %w", err) } + defer file.Close() - logConfig.Printf("Read %d bytes from config file", len(data)) - + // Use streaming decoder for better memory efficiency var cfg Config - if _, err := toml.Decode(string(data), &cfg); err != nil { + decoder := toml.NewDecoder(file) + md, err := decoder.Decode(&cfg) + if err != nil { + // Extract position information from ParseError for better error messages + // Note: We use Position.Line, Position.Col, and Message separately to provide + // a consistent, precise error format. perr.Error() includes line info but not + // column, so we construct our own message with both for better UX. + // Try pointer type first (for compatibility) + if perr, ok := err.(*toml.ParseError); ok { + return nil, fmt.Errorf("failed to parse TOML at line %d, column %d: %s", + perr.Position.Line, perr.Position.Col, perr.Message) + } + // Try value type (used by toml.Decode) + if perr, ok := err.(toml.ParseError); ok { + return nil, fmt.Errorf("failed to parse TOML at line %d, column %d: %s", + perr.Position.Line, perr.Position.Col, perr.Message) + } return nil, fmt.Errorf("failed to parse TOML: %w", err) } logConfig.Printf("Parsed TOML config with %d servers", len(cfg.Servers)) + // Detect and warn about unknown configuration keys (typos, deprecated options) + undecoded := md.Undecoded() + if len(undecoded) > 0 { + for _, key := range undecoded { + // Log to both debug logger and file logger for visibility + logConfig.Printf("WARNING: Unknown configuration key '%s' - check for typos or deprecated options", key) + logger.LogWarn("config", "Unknown configuration key '%s' - check for typos or deprecated options", key) + } + } + // Validate required fields if len(cfg.Servers) == 0 { return nil, fmt.Errorf("no servers defined in configuration") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 194e2f5f..a726f2cf 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1096,3 +1096,150 @@ args = ["run", "--rm", "-i", "mcp/memory"] _, ok = cfg.Servers["memory"] assert.True(t, ok, "Server 'memory' not found") } + +// TestLoadFromFile_ParseErrorWithColumnNumber tests that parse errors include column information +func TestLoadFromFile_ParseErrorWithColumnNumber(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "config.toml") + + // Invalid TOML: missing equals sign + tomlContent := `[gateway] +port 3000 +` + + err := os.WriteFile(tmpFile, []byte(tomlContent), 0644) + require.NoError(t, err, "Failed to write temp TOML file") + + cfg, err := LoadFromFile(tmpFile) + require.Error(t, err, "Expected error for invalid TOML") + assert.Nil(t, cfg, "Config should be nil on error") + + // Error should contain line and column information from our improved error format + errMsg := err.Error() + assert.Contains(t, errMsg, "line", "Error should mention line number") + // Our improved format includes "column" explicitly when ParseError is detected + assert.True(t, strings.Contains(errMsg, "column") || strings.Contains(errMsg, "line 2"), + "Error should mention column or line position, got: %s", errMsg) +} + +// TestLoadFromFile_UnknownKeysInGateway tests detection of unknown keys in gateway section +func TestLoadFromFile_UnknownKeysInGateway(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "config.toml") + + // TOML with typo in gateway field: "prot" instead of "port" + tomlContent := ` +[gateway] +prot = 3000 +api_key = "test-key" + +[servers.test] +command = "docker" +args = ["run", "--rm", "-i", "test/container:latest"] +` + + err := os.WriteFile(tmpFile, []byte(tomlContent), 0644) + require.NoError(t, err, "Failed to write temp TOML file") + + // Enable debug logging to capture warning about unknown key + SetDebug(true) + defer SetDebug(false) + + // Should still load successfully, but warning will be logged + cfg, err := LoadFromFile(tmpFile) + require.NoError(t, err, "LoadFromFile() should succeed even with unknown keys") + require.NotNil(t, cfg, "Config should not be nil") + + // Port should be default since "prot" was not recognized + assert.Equal(t, DefaultPort, cfg.Gateway.Port, "Port should be default since 'prot' is unknown") + assert.Equal(t, "test-key", cfg.Gateway.APIKey, "API key should be set correctly") +} + +// TestLoadFromFile_MultipleUnknownKeys tests detection of multiple typos +func TestLoadFromFile_MultipleUnknownKeys(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "config.toml") + + // TOML with multiple typos + tomlContent := ` +[gateway] +port = 8080 +startup_timout = 30 +tool_timout = 60 + +[servers.test] +command = "docker" +args = ["run", "--rm", "-i", "test/container:latest"] +typ = "stdio" +` + + err := os.WriteFile(tmpFile, []byte(tomlContent), 0644) + require.NoError(t, err, "Failed to write temp TOML file") + + // Enable debug logging to capture warnings + SetDebug(true) + defer SetDebug(false) + + // Should still load successfully + cfg, err := LoadFromFile(tmpFile) + require.NoError(t, err, "LoadFromFile() should succeed even with multiple unknown keys") + require.NotNil(t, cfg, "Config should not be nil") + + // Correctly spelled fields should work + assert.Equal(t, 8080, cfg.Gateway.Port, "Port should be set correctly") + // Misspelled fields should use defaults + assert.Equal(t, DefaultStartupTimeout, cfg.Gateway.StartupTimeout, "StartupTimeout should be default") + assert.Equal(t, DefaultToolTimeout, cfg.Gateway.ToolTimeout, "ToolTimeout should be default") +} + +// TestLoadFromFile_StreamingLargeFile tests that streaming decoder works efficiently +func TestLoadFromFile_StreamingLargeFile(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "large-config.toml") + + // Create a TOML file with many servers + var tomlContent strings.Builder + tomlContent.WriteString("[gateway]\nport = 3000\n\n") + + for i := 1; i <= 100; i++ { + tomlContent.WriteString(fmt.Sprintf("[servers.server%d]\n", i)) + tomlContent.WriteString("command = \"docker\"\n") + tomlContent.WriteString(fmt.Sprintf("args = [\"run\", \"--rm\", \"-i\", \"test/server%d:latest\"]\n\n", i)) + } + + err := os.WriteFile(tmpFile, []byte(tomlContent.String()), 0644) + require.NoError(t, err, "Failed to write temp TOML file") + + // Should load successfully using streaming decoder + cfg, err := LoadFromFile(tmpFile) + require.NoError(t, err, "LoadFromFile() should handle large files") + require.NotNil(t, cfg, "Config should not be nil") + assert.Len(t, cfg.Servers, 100, "Expected 100 servers") +} + +// TestLoadFromFile_InvalidTOMLDuplicateKey tests handling of duplicate keys +func TestLoadFromFile_InvalidTOMLDuplicateKey(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "config.toml") + + // TOML 1.1+ should detect duplicate keys (available in v1.6.0) + tomlContent := ` +[gateway] +port = 3000 +port = 8080 + +[servers.test] +command = "docker" +args = ["run", "--rm", "-i", "test/container:latest"] +` + + err := os.WriteFile(tmpFile, []byte(tomlContent), 0644) + require.NoError(t, err, "Failed to write temp TOML file") + + cfg, err := LoadFromFile(tmpFile) + require.Error(t, err, "Expected error for duplicate key") + assert.Nil(t, cfg, "Config should be nil on error") + + // Error should mention the duplicate key + assert.Contains(t, err.Error(), "line", "Error should mention line number") +}