From ed687d62aef13468b040e66a0c4ad97b6ee80de7 Mon Sep 17 00:00:00 2001 From: Mauro Date: Mon, 27 Apr 2026 03:45:52 +0200 Subject: [PATCH] fix(config): show precise malformed config diagnostics (#2415) * fix(config): show precise malformed config diagnostics * fix lint * fix test --- pkg/config/config.go | 48 ++- pkg/config/config_test.go | 83 ++++- pkg/config/diagnostics.go | 441 ++++++++++++++++++++++++ pkg/config/migration.go | 74 +++- pkg/config/security.go | 2 +- pkg/config/security_integration_test.go | 5 +- 6 files changed, 619 insertions(+), 34 deletions(-) create mode 100644 pkg/config/diagnostics.go diff --git a/pkg/config/config.go b/pkg/config/config.go index 305c3a5c0..16497b4ac 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -996,7 +996,9 @@ func LoadConfig(path string) (*Config, error) { Version int `json:"version"` } if e := json.Unmarshal(data, &versionInfo); e != nil { - return nil, fmt.Errorf("failed to detect config version: %w", e) + e = wrapJSONError(data, e, "config.json") + logger.ErrorCF("config", formatDiagnosticLogMessage("Malformed config file", e), map[string]any{"path": path}) + return nil, e } if len(data) <= 10 { logger.Warn(fmt.Sprintf("content is [%s]", string(data))) @@ -1011,10 +1013,23 @@ func LoadConfig(path string) (*Config, error) { "config migrate start", map[string]any{"from": versionInfo.Version, "to": CurrentVersion}, ) + if err = validateLegacyConfigDiagnostics(data); err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) + return nil, err + } var m map[string]any m, err = loadConfigMap(path) if err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) return nil, err } @@ -1056,10 +1071,23 @@ func LoadConfig(path string) (*Config, error) { "config migrate start", map[string]any{"from": versionInfo.Version, "to": CurrentVersion}, ) + if err = validateLegacyConfigDiagnostics(data); err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) + return nil, err + } var m map[string]any m, err = loadConfigMap(path) if err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) return nil, err } @@ -1101,9 +1129,22 @@ func LoadConfig(path string) (*Config, error) { "config migrate start", map[string]any{"from": versionInfo.Version, "to": CurrentVersion}, ) + if err = validateLegacyConfigDiagnostics(data); err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) + return nil, err + } var m map[string]any m, err = loadConfigMap(path) if err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) return nil, err } migrateErr := migrateV2ToV3(m) @@ -1138,6 +1179,11 @@ func LoadConfig(path string) (*Config, error) { // Current version cfg, err = loadConfig(data) if err != nil { + logger.ErrorCF( + "config", + formatDiagnosticLogMessage("Failed to load config", err), + map[string]any{"path": path}, + ) return nil, err } // Load security configuration diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 2be1bcc67..d455572eb 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -847,6 +847,72 @@ func TestLoadConfig_WebPreferNativeCanBeDisabled(t *testing.T) { } } +func TestLoadConfig_SyntaxErrorReportsLineAndColumn(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + raw := "{\n \"version\": 2,\n \"tools\": {\n \"web\": {\n \"enabled\": true,,\n \"format\": \"markdown\"\n }\n }\n}\n" + if err := os.WriteFile(configPath, []byte(raw), 0o600); err != nil { + t.Fatalf("WriteFile() error: %v", err) + } + + _, err := LoadConfig(configPath) + if err == nil { + t.Fatal("expected syntax error, got nil") + } + if !strings.Contains(err.Error(), "syntax error at line 5, column 23") { + t.Fatalf("expected line/column diagnostic, got %q", err.Error()) + } + if !strings.Contains(err.Error(), "\"enabled\": true,,") { + t.Fatalf("expected source snippet in diagnostic, got %q", err.Error()) + } + if !strings.Contains(err.Error(), "^") { + t.Fatalf("expected caret marker in diagnostic, got %q", err.Error()) + } +} + +func TestLoadConfig_TypeErrorReportsFieldPath(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + raw := "{\n \"version\": 2,\n \"tools\": {\n \"web\": {\n \"fetch_limit_bytes\": \"oops\"\n }\n }\n}\n" + if err := os.WriteFile(configPath, []byte(raw), 0o600); err != nil { + t.Fatalf("WriteFile() error: %v", err) + } + + _, err := LoadConfig(configPath) + if err == nil { + t.Fatal("expected type error, got nil") + } + if !strings.Contains(err.Error(), "type error at line 5, column 33") { + t.Fatalf("expected line/column diagnostic, got %q", err.Error()) + } + if !strings.Contains(err.Error(), "fetch_limit_bytes") { + t.Fatalf("expected field name in diagnostic, got %q", err.Error()) + } + if !strings.Contains(err.Error(), "\"fetch_limit_bytes\": \"oops\"") { + t.Fatalf("expected source snippet in diagnostic, got %q", err.Error()) + } + if !strings.Contains(err.Error(), "^") { + t.Fatalf("expected caret marker in diagnostic, got %q", err.Error()) + } +} + +func TestLoadConfig_UnknownFieldsReportsExactPaths(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + raw := "{\n \"version\": 2,\n \"tools\": {\n \"weeb\": {\n \"enabled\": true\n },\n \"web\": {\n \"fatch_limit_bytes\": 123\n }\n }\n}\n" + if err := os.WriteFile(configPath, []byte(raw), 0o600); err != nil { + t.Fatalf("WriteFile() error: %v", err) + } + + _, err := LoadConfig(configPath) + if err == nil { + t.Fatal("expected unknown field error, got nil") + } + if !strings.Contains(err.Error(), "tools.weeb") || !strings.Contains(err.Error(), "tools.web.fatch_limit_bytes") { + t.Fatalf("expected exact unknown field paths, got %q", err.Error()) + } +} + func TestDefaultConfig_ExecAllowRemoteEnabled(t *testing.T) { cfg := DefaultConfig() if !cfg.Tools.Exec.AllowRemote { @@ -1355,25 +1421,12 @@ func TestLoadConfig_TelegramPlaceholderTextAcceptsSingleString(t *testing.T) { } // TestLoadConfig_WarnsForPlaintextAPIKey verifies that LoadConfig resolves a plaintext -// api_key into memory but does NOT rewrite the config file. File writes are the sole +// api_keys entry into memory but does NOT rewrite the config file. File writes are the sole // responsibility of SaveConfig. func TestLoadConfig_WarnsForPlaintextAPIKey(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "config.json") - const original = `{"version":1,"model_list":[{"model_name":"test","model":"openai/gpt-4","api_key":"sk-plaintext"}]}` - if err := os.WriteFile(cfgPath, []byte(original), 0o600); err != nil { - t.Fatalf("setup: %v", err) - } - secPath := filepath.Join(dir, SecurityConfigFile) - const securityConfig = ` -model_list: - test:0: - api_keys: - - "sk-plaintext" -` - if err := os.WriteFile(secPath, []byte(securityConfig), 0o600); err != nil { - t.Fatalf("setup: %v", err) - } + const original = `{"version":2,"model_list":[{"model_name":"test","model":"openai/gpt-4","api_keys":["sk-plaintext"]}]}` if err := os.WriteFile(cfgPath, []byte(original), 0o600); err != nil { t.Fatalf("setup: %v", err) } diff --git a/pkg/config/diagnostics.go b/pkg/config/diagnostics.go new file mode 100644 index 000000000..bbc59c03b --- /dev/null +++ b/pkg/config/diagnostics.go @@ -0,0 +1,441 @@ +package config + +import ( + "encoding/json" + "fmt" + "os" + "reflect" + "sort" + "strings" + "unicode/utf8" + + "golang.org/x/term" +) + +func decodeJSONWithDiagnostics(data []byte, target any, label string) error { + var raw any + if err := json.Unmarshal(data, &raw); err != nil { + return wrapJSONError(data, err, label) + } + + unknownFields := collectUnknownJSONFields(raw, reflect.TypeOf(target), "") + if len(unknownFields) > 0 { + sort.Strings(unknownFields) + return fmt.Errorf( + "%s contains unknown field(s): %s", + label, + strings.Join(unknownFields, ", "), + ) + } + + if err := json.Unmarshal(data, target); err != nil { + return wrapJSONError(data, err, label) + } + return nil +} + +func DiagnosticSummary(err error) string { + if err == nil { + return "" + } + summary, _ := splitDiagnosticError(err.Error()) + return stripANSISequences(summary) +} + +func formatDiagnosticLogMessage(prefix string, err error) string { + if err == nil { + return prefix + } + + summary, preview := splitDiagnosticError(err.Error()) + summary = stripANSISequences(summary) + if preview == "" { + if summary == "" { + return prefix + } + return prefix + ": " + summary + } + if summary == "" { + return prefix + "\n" + preview + } + return prefix + ": " + summary + "\n" + preview +} + +func wrapJSONError(data []byte, err error, label string) error { + switch e := err.(type) { + case *json.SyntaxError: + line, column := lineAndColumnForOffset(data, e.Offset) + preview := diagnosticPreviewForOffset(data, e.Offset) + if preview != "" { + return fmt.Errorf( + "%s syntax error at line %d, column %d: %w\n%s", + label, + line, + column, + err, + preview, + ) + } + return fmt.Errorf("%s syntax error at line %d, column %d: %w", label, line, column, err) + case *json.UnmarshalTypeError: + line, column := lineAndColumnForOffset(data, e.Offset) + preview := diagnosticPreviewForOffset(data, e.Offset) + field := strings.TrimSpace(e.Field) + if field != "" { + if preview != "" { + return fmt.Errorf( + "%s type error at line %d, column %d for field %q: expected %s but got %s\n%s", + label, + line, + column, + field, + e.Type.String(), + e.Value, + preview, + ) + } + return fmt.Errorf( + "%s type error at line %d, column %d for field %q: expected %s but got %s", + label, + line, + column, + field, + e.Type.String(), + e.Value, + ) + } + if preview != "" { + return fmt.Errorf( + "%s type error at line %d, column %d: expected %s but got %s\n%s", + label, + line, + column, + e.Type.String(), + e.Value, + preview, + ) + } + return fmt.Errorf( + "%s type error at line %d, column %d: expected %s but got %s", + label, + line, + column, + e.Type.String(), + e.Value, + ) + default: + return fmt.Errorf("failed to parse %s: %w", label, err) + } +} + +func splitDiagnosticError(message string) (string, string) { + if idx := strings.IndexByte(message, '\n'); idx >= 0 { + return message[:idx], message[idx+1:] + } + return message, "" +} + +func stripANSISequences(s string) string { + if s == "" { + return "" + } + + var b strings.Builder + b.Grow(len(s)) + + for i := 0; i < len(s); i++ { + if s[i] != 0x1b { + b.WriteByte(s[i]) + continue + } + if i+1 >= len(s) || s[i+1] != '[' { + continue + } + i += 2 + for i < len(s) { + c := s[i] + if c >= '@' && c <= '~' { + break + } + i++ + } + } + + return b.String() +} + +func diagnosticPreviewForOffset(data []byte, offset int64) string { + if len(data) == 0 { + return "" + } + + start, end := lineBoundsForOffset(data, offset) + if start >= end { + return "" + } + + lineNumber, column := lineAndColumnForOffset(data, offset) + line := strings.TrimRight(string(data[start:end]), "\r\n") + if strings.TrimSpace(line) == "" { + return "" + } + + trimmedLine, trimOffset := trimDiagnosticLine(line, column) + if trimmedLine == "" { + return "" + } + + prefix := fmt.Sprintf("%4d | ", lineNumber) + caretColumn := column - trimOffset + if caretColumn < 1 { + caretColumn = 1 + } + + if diagnosticsUseColor() { + linePrefix := "\x1b[2m" + prefix + "\x1b[0m" + caretPrefix := "\x1b[2m" + strings.Repeat(" ", len(fmt.Sprintf("%4d", lineNumber))) + " | " + "\x1b[0m" + highlighted := highlightDiagnosticColumn(trimmedLine, caretColumn) + caretPad := strings.Repeat(" ", maxRuneCount(trimmedLine, caretColumn-1)) + return fmt.Sprintf( + " %s%s\n %s%s\x1b[1;31m^\x1b[0m", + linePrefix, + highlighted, + caretPrefix, + caretPad, + ) + } + + caretPrefix := strings.Repeat(" ", len(prefix)) + caretPad := strings.Repeat(" ", maxRuneCount(trimmedLine, caretColumn-1)) + return fmt.Sprintf( + " %s%s\n %s%s^", + prefix, + trimmedLine, + caretPrefix, + caretPad, + ) +} + +func lineAndColumnForOffset(data []byte, offset int64) (int, int) { + if offset <= 0 { + return 1, 1 + } + if offset > int64(len(data)) { + offset = int64(len(data)) + } + + line := 1 + column := 1 + for i := int64(0); i < offset-1; i++ { + if data[i] == '\n' { + line++ + column = 1 + continue + } + column++ + } + return line, column +} + +func lineBoundsForOffset(data []byte, offset int64) (int, int) { + if len(data) == 0 { + return 0, 0 + } + + if offset <= 0 { + offset = 1 + } + if offset > int64(len(data)) { + offset = int64(len(data)) + } + + index := int(offset - 1) + if index < 0 { + index = 0 + } + if index >= len(data) { + index = len(data) - 1 + } + + start := index + for start > 0 && data[start-1] != '\n' { + start-- + } + + end := index + for end < len(data) && data[end] != '\n' { + end++ + } + + return start, end +} + +func trimDiagnosticLine(line string, column int) (string, int) { + runes := []rune(line) + if len(runes) == 0 { + return "", 0 + } + + if len(runes) <= 160 { + return line, 0 + } + + const contextBefore = 60 + const maxWidth = 160 + + start := column - 1 - contextBefore + if start < 0 { + start = 0 + } + if start > len(runes)-maxWidth { + start = len(runes) - maxWidth + } + if start < 0 { + start = 0 + } + + end := start + maxWidth + if end > len(runes) { + end = len(runes) + } + + trimmed := string(runes[start:end]) + trimOffset := start + + if start > 0 { + trimmed = "..." + trimmed + trimOffset -= 3 + } + if end < len(runes) { + trimmed += "..." + } + + return trimmed, trimOffset +} + +func diagnosticsUseColor() bool { + return term.IsTerminal(int(os.Stdout.Fd())) +} + +func highlightDiagnosticColumn(line string, column int) string { + runes := []rune(line) + if column < 1 || column > len(runes) { + return line + } + + index := column - 1 + return string(runes[:index]) + "\x1b[31m" + string(runes[index]) + "\x1b[0m" + string(runes[index+1:]) +} + +func maxRuneCount(s string, count int) int { + if count <= 0 { + return 0 + } + runes := []rune(s) + if count > len(runes) { + count = len(runes) + } + return utf8.RuneCountInString(string(runes[:count])) +} + +func collectUnknownJSONFields(raw any, targetType reflect.Type, path string) []string { + targetType = derefType(targetType) + if targetType == nil { + return nil + } + + switch targetType.Kind() { + case reflect.Struct: + obj, ok := raw.(map[string]any) + if !ok { + return nil + } + fieldMap := jsonFieldTypeMap(targetType) + var issues []string + for key, value := range obj { + fieldType, exists := fieldMap[key] + fieldPath := appendJSONPath(path, key) + if !exists { + issues = append(issues, fieldPath) + continue + } + issues = append(issues, collectUnknownJSONFields(value, fieldType, fieldPath)...) + } + return issues + case reflect.Slice, reflect.Array: + items, ok := raw.([]any) + if !ok { + return nil + } + var issues []string + elemType := targetType.Elem() + for i, item := range items { + itemPath := fmt.Sprintf("%s[%d]", path, i) + issues = append(issues, collectUnknownJSONFields(item, elemType, itemPath)...) + } + return issues + case reflect.Map: + obj, ok := raw.(map[string]any) + if !ok { + return nil + } + var issues []string + elemType := targetType.Elem() + for key, value := range obj { + fieldPath := appendJSONPath(path, key) + issues = append(issues, collectUnknownJSONFields(value, elemType, fieldPath)...) + } + return issues + default: + return nil + } +} + +func jsonFieldTypeMap(t reflect.Type) map[string]reflect.Type { + result := make(map[string]reflect.Type) + populateJSONFieldTypeMap(result, derefType(t)) + return result +} + +func populateJSONFieldTypeMap(result map[string]reflect.Type, t reflect.Type) { + if t == nil || t.Kind() != reflect.Struct { + return + } + + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + if !field.IsExported() { + continue + } + + tag := field.Tag.Get("json") + name := strings.Split(tag, ",")[0] + if name == "-" { + continue + } + + if field.Anonymous && name == "" { + populateJSONFieldTypeMap(result, derefType(field.Type)) + continue + } + + if name == "" { + name = field.Name + } + result[name] = field.Type + } +} + +func derefType(t reflect.Type) reflect.Type { + for t != nil && t.Kind() == reflect.Pointer { + t = t.Elem() + } + return t +} + +func appendJSONPath(path, segment string) string { + if path == "" { + return segment + } + return path + "." + segment +} diff --git a/pkg/config/migration.go b/pkg/config/migration.go index 4fe2148b2..96914819e 100644 --- a/pkg/config/migration.go +++ b/pkg/config/migration.go @@ -27,6 +27,59 @@ func buildModelWithProtocol(protocol, model string) string { return protocol + "/" + model } +type legacyDiagnosticConfig struct { + Version int `json:"version"` + Isolation IsolationConfig `json:"isolation,omitempty"` + Agents legacyDiagnosticAgents `json:"agents,omitempty"` + Session SessionConfig `json:"session,omitempty"` + Channels map[string]any `json:"channels,omitempty"` + ChannelList ChannelsConfig `json:"channel_list,omitempty"` + ModelList []map[string]any `json:"model_list,omitempty"` + Gateway GatewayConfig `json:"gateway,omitempty"` + Hooks HooksConfig `json:"hooks,omitempty"` + Tools ToolsConfig `json:"tools,omitempty"` + Heartbeat HeartbeatConfig `json:"heartbeat,omitempty"` + Devices DevicesConfig `json:"devices,omitempty"` + Voice VoiceConfig `json:"voice,omitempty"` + Bindings json.RawMessage `json:"bindings,omitempty"` + Providers json.RawMessage `json:"providers,omitempty"` +} + +type legacyDiagnosticAgents struct { + Defaults legacyDiagnosticAgentDefaults `json:"defaults,omitempty"` + List []AgentConfig `json:"list,omitempty"` + Dispatch *DispatchConfig `json:"dispatch,omitempty"` +} + +type legacyDiagnosticAgentDefaults struct { + AgentDefaults + LegacyModel string `json:"model,omitempty"` +} + +func validateLegacyConfigDiagnostics(data []byte) error { + var cfg legacyDiagnosticConfig + return decodeJSONWithDiagnostics(data, &cfg, "config.json") +} + +func migrateLegacyAgentDefaultsModel(m map[string]any) { + agents, ok := m["agents"].(map[string]any) + if !ok { + return + } + defaults, ok := agents["defaults"].(map[string]any) + if !ok { + return + } + model, hasModel := defaults["model"] + if !hasModel { + return + } + if _, hasModelName := defaults["model_name"]; !hasModelName { + defaults["model_name"] = model + } + delete(defaults, "model") +} + // loadConfigV1 loads a version 1 config (current schema) func loadConfig(data []byte) (*Config, error) { cfg := DefaultConfig() @@ -38,14 +91,14 @@ func loadConfig(data []byte) (*Config, error) { // index position. We only reset cfg.ModelList when the user actually provides // entries; when count is 0 we keep DefaultConfig's built-in list as fallback. var tmp Config - if err := json.Unmarshal(data, &tmp); err != nil { + if err := decodeJSONWithDiagnostics(data, &tmp, "config.json"); err != nil { return nil, err } if len(tmp.ModelList) > 0 { cfg.ModelList = nil } - if err := json.Unmarshal(data, cfg); err != nil { + if err := decodeJSONWithDiagnostics(data, cfg, "config.json"); err != nil { return nil, err } return cfg, nil @@ -96,17 +149,7 @@ func migrateV0ToV1(m map[string]any) error { return fmt.Errorf("migrateV0ToV1: expected version 0, got %v", m["version"]) } - // Migrate agents.defaults.model → agents.defaults.model_name - if agents, ok := m["agents"].(map[string]any); ok { - if defaults, ok := agents["defaults"].(map[string]any); ok { - if model, hasModel := defaults["model"]; hasModel { - if _, hasModelName := defaults["model_name"]; !hasModelName { - defaults["model_name"] = model - } - delete(defaults, "model") - } - } - } + migrateLegacyAgentDefaultsModel(m) // Migrate legacy providers to model_list if no model_list exists if _, hasModelList := m["model_list"]; !hasModelList { @@ -275,6 +318,9 @@ func migrateV2ToV3(m map[string]any) error { return fmt.Errorf("migrateV2ToV3: expected version 2, got %v", m["version"]) } + migrateLegacyAgentDefaultsModel(m) + delete(m, "bindings") + // Rename channels → channel_list if channels, ok := m["channels"]; ok { delete(m, "channels") @@ -334,7 +380,7 @@ func loadConfigMap(path string) (map[string]any, error) { return nil, fmt.Errorf("failed to read config: %w", err) } if err = json.Unmarshal(data, &m1); err != nil { - return nil, fmt.Errorf("failed to parse config: %w", err) + return nil, wrapJSONError(data, err, "config.json") } secPath := securityPath(path) data, err = os.ReadFile(secPath) diff --git a/pkg/config/security.go b/pkg/config/security.go index c5d3bf507..9f0d1339c 100644 --- a/pkg/config/security.go +++ b/pkg/config/security.go @@ -75,7 +75,7 @@ func loadSecurityConfig(cfg *Config, securityPath string) error { // Unmarshal non-channel fields from security.yml // This will resolve encrypted values for model_list, tools, etc. if err := yaml.Unmarshal(data, cfg); err != nil { - return fmt.Errorf("failed to parse security config: %w", err) + return fmt.Errorf("failed to parse security config %s: %w", securityPath, err) } if err := applyLegacySkillsSecurityConfig(cfg, data); err != nil { return fmt.Errorf("failed to parse legacy skills security config: %w", err) diff --git a/pkg/config/security_integration_test.go b/pkg/config/security_integration_test.go index 5fe7b6b97..8fc2f167c 100644 --- a/pkg/config/security_integration_test.go +++ b/pkg/config/security_integration_test.go @@ -43,11 +43,10 @@ func TestSecurityConfigIntegration(t *testing.T) { t.Run("Full workflow with security references", func(t *testing.T) { tmpDir := t.TempDir() - // Create config.json with direct security values (not ref: references) - // These values should take precedence over .security.yml + // Create config.json with direct security values using the current schema. configPath := filepath.Join(tmpDir, "config.json") configContent := `{ - "version": 1, + "version": 2, "model_list": [ { "model_name": "test-model",