diff --git a/cmd/bd/doctor.go b/cmd/bd/doctor.go index 6db12f08..eda1de76 100644 --- a/cmd/bd/doctor.go +++ b/cmd/bd/doctor.go @@ -581,6 +581,11 @@ func runDiagnostics(path string) doctorResult { result.OverallOK = false } + // Check 7a: Configuration value validation (bd-alz) + configValuesCheck := convertDoctorCheck(doctor.CheckConfigValues(path)) + result.Checks = append(result.Checks, configValuesCheck) + // Don't fail overall check for config value warnings, just warn + // Check 8: Daemon health daemonCheck := checkDaemonStatus(path) result.Checks = append(result.Checks, daemonCheck) diff --git a/cmd/bd/doctor/config_values.go b/cmd/bd/doctor/config_values.go new file mode 100644 index 00000000..6ce9e962 --- /dev/null +++ b/cmd/bd/doctor/config_values.go @@ -0,0 +1,243 @@ +package doctor + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + "time" + + "github.com/spf13/viper" + "github.com/steveyegge/beads/internal/configfile" +) + +// validRoutingModes are the allowed values for routing.mode +var validRoutingModes = map[string]bool{ + "auto": true, + "maintainer": true, + "contributor": true, +} + +// validBranchNameRegex validates git branch names +// Git branch names can't contain: space, ~, ^, :, \, ?, *, [ +// Can't start with -, can't end with ., can't contain .. +var validBranchNameRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._/-]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$`) + +// CheckConfigValues validates configuration values in config.yaml and metadata.json +// Returns issues found, or OK if all values are valid +func CheckConfigValues(repoPath string) DoctorCheck { + var issues []string + + // Check config.yaml values + yamlIssues := checkYAMLConfigValues(repoPath) + issues = append(issues, yamlIssues...) + + // Check metadata.json values + metadataIssues := checkMetadataConfigValues(repoPath) + issues = append(issues, metadataIssues...) + + if len(issues) == 0 { + return DoctorCheck{ + Name: "Config Values", + Status: "ok", + Message: "All configuration values are valid", + } + } + + return DoctorCheck{ + Name: "Config Values", + Status: "warning", + Message: fmt.Sprintf("Found %d configuration issue(s)", len(issues)), + Detail: strings.Join(issues, "\n"), + Fix: "Edit config files to fix invalid values. Run 'bd config' to view current settings.", + } +} + +// checkYAMLConfigValues validates values in config.yaml +func checkYAMLConfigValues(repoPath string) []string { + var issues []string + + // Load config.yaml if it exists + v := viper.New() + v.SetConfigType("yaml") + + configPath := filepath.Join(repoPath, ".beads", "config.yaml") + if _, err := os.Stat(configPath); os.IsNotExist(err) { + // No config.yaml, check user config dirs + configPath = "" + if configDir, err := os.UserConfigDir(); err == nil { + userConfigPath := filepath.Join(configDir, "bd", "config.yaml") + if _, err := os.Stat(userConfigPath); err == nil { + configPath = userConfigPath + } + } + if configPath == "" { + if homeDir, err := os.UserHomeDir(); err == nil { + homeConfigPath := filepath.Join(homeDir, ".beads", "config.yaml") + if _, err := os.Stat(homeConfigPath); err == nil { + configPath = homeConfigPath + } + } + } + } + + if configPath == "" { + // No config.yaml found anywhere + return issues + } + + v.SetConfigFile(configPath) + if err := v.ReadInConfig(); err != nil { + issues = append(issues, fmt.Sprintf("config.yaml: failed to parse: %v", err)) + return issues + } + + // Validate flush-debounce (should be a valid duration) + if v.IsSet("flush-debounce") { + debounceStr := v.GetString("flush-debounce") + if debounceStr != "" { + _, err := time.ParseDuration(debounceStr) + if err != nil { + issues = append(issues, fmt.Sprintf("flush-debounce: invalid duration %q (expected format like \"30s\", \"1m\", \"500ms\")", debounceStr)) + } + } + } + + // Validate issue-prefix (should be alphanumeric with dashes/underscores, reasonably short) + if v.IsSet("issue-prefix") { + prefix := v.GetString("issue-prefix") + if prefix != "" { + if len(prefix) > 20 { + issues = append(issues, fmt.Sprintf("issue-prefix: %q is too long (max 20 characters)", prefix)) + } + if !regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9_-]*$`).MatchString(prefix) { + issues = append(issues, fmt.Sprintf("issue-prefix: %q is invalid (must start with letter, contain only letters, numbers, dashes, underscores)", prefix)) + } + } + } + + // Validate routing.mode (should be "auto", "maintainer", or "contributor") + if v.IsSet("routing.mode") { + mode := v.GetString("routing.mode") + if mode != "" && !validRoutingModes[mode] { + validModes := make([]string, 0, len(validRoutingModes)) + for m := range validRoutingModes { + validModes = append(validModes, m) + } + issues = append(issues, fmt.Sprintf("routing.mode: %q is invalid (valid values: %s)", mode, strings.Join(validModes, ", "))) + } + } + + // Validate sync-branch (should be a valid git branch name if set) + if v.IsSet("sync-branch") { + branch := v.GetString("sync-branch") + if branch != "" { + if !isValidBranchName(branch) { + issues = append(issues, fmt.Sprintf("sync-branch: %q is not a valid git branch name", branch)) + } + } + } + + // Validate routing paths exist if set + for _, key := range []string{"routing.default", "routing.maintainer", "routing.contributor"} { + if v.IsSet(key) { + path := v.GetString(key) + if path != "" && path != "." { + // Expand ~ to home directory + if strings.HasPrefix(path, "~") { + if home, err := os.UserHomeDir(); err == nil { + path = filepath.Join(home, path[1:]) + } + } + // Check if path exists (only warn, don't error - it might be created later) + if _, err := os.Stat(path); os.IsNotExist(err) { + issues = append(issues, fmt.Sprintf("%s: path %q does not exist", key, v.GetString(key))) + } + } + } + } + + return issues +} + +// checkMetadataConfigValues validates values in metadata.json +func checkMetadataConfigValues(repoPath string) []string { + var issues []string + + beadsDir := filepath.Join(repoPath, ".beads") + cfg, err := configfile.Load(beadsDir) + if err != nil { + issues = append(issues, fmt.Sprintf("metadata.json: failed to load: %v", err)) + return issues + } + + if cfg == nil { + // No metadata.json, that's OK + return issues + } + + // Validate database filename + if cfg.Database != "" { + if strings.Contains(cfg.Database, string(os.PathSeparator)) || strings.Contains(cfg.Database, "/") { + issues = append(issues, fmt.Sprintf("metadata.json database: %q should be a filename, not a path", cfg.Database)) + } + if !strings.HasSuffix(cfg.Database, ".db") && !strings.HasSuffix(cfg.Database, ".sqlite") && !strings.HasSuffix(cfg.Database, ".sqlite3") { + issues = append(issues, fmt.Sprintf("metadata.json database: %q has unusual extension (expected .db, .sqlite, or .sqlite3)", cfg.Database)) + } + } + + // Validate jsonl_export filename + if cfg.JSONLExport != "" { + if strings.Contains(cfg.JSONLExport, string(os.PathSeparator)) || strings.Contains(cfg.JSONLExport, "/") { + issues = append(issues, fmt.Sprintf("metadata.json jsonl_export: %q should be a filename, not a path", cfg.JSONLExport)) + } + if !strings.HasSuffix(cfg.JSONLExport, ".jsonl") { + issues = append(issues, fmt.Sprintf("metadata.json jsonl_export: %q should have .jsonl extension", cfg.JSONLExport)) + } + } + + // Validate deletions_retention_days + if cfg.DeletionsRetentionDays < 0 { + issues = append(issues, fmt.Sprintf("metadata.json deletions_retention_days: %d is invalid (must be >= 0)", cfg.DeletionsRetentionDays)) + } + + return issues +} + +// isValidBranchName checks if a string is a valid git branch name +func isValidBranchName(name string) bool { + if name == "" { + return false + } + + // Can't start with - + if strings.HasPrefix(name, "-") { + return false + } + + // Can't end with . or / + if strings.HasSuffix(name, ".") || strings.HasSuffix(name, "/") { + return false + } + + // Can't contain .. + if strings.Contains(name, "..") { + return false + } + + // Can't contain these characters: space, ~, ^, :, \, ?, *, [ + invalidChars := []string{" ", "~", "^", ":", "\\", "?", "*", "[", "@{"} + for _, char := range invalidChars { + if strings.Contains(name, char) { + return false + } + } + + // Can't end with .lock + if strings.HasSuffix(name, ".lock") { + return false + } + + return true +} diff --git a/cmd/bd/doctor/config_values_test.go b/cmd/bd/doctor/config_values_test.go new file mode 100644 index 00000000..fdefd86c --- /dev/null +++ b/cmd/bd/doctor/config_values_test.go @@ -0,0 +1,229 @@ +package doctor + +import ( + "os" + "path/filepath" + "testing" +) + +func TestIsValidBranchName(t *testing.T) { + tests := []struct { + name string + branch string + expected bool + }{ + {"valid simple", "main", true}, + {"valid with slash", "feature/test", true}, + {"valid with dash", "my-branch", true}, + {"valid with underscore", "my_branch", true}, + {"valid with dot", "v1.0", true}, + {"valid complex", "feature/bd-123-add-thing", true}, + + {"empty", "", false}, + {"starts with dash", "-branch", false}, + {"ends with dot", "branch.", false}, + {"ends with slash", "branch/", false}, + {"contains space", "my branch", false}, + {"contains tilde", "branch~1", false}, + {"contains caret", "branch^2", false}, + {"contains colon", "branch:name", false}, + {"contains backslash", "branch\\name", false}, + {"contains question", "branch?", false}, + {"contains asterisk", "branch*", false}, + {"contains bracket", "branch[0]", false}, + {"contains double dot", "branch..name", false}, + {"ends with .lock", "branch.lock", false}, + {"contains @{", "branch@{1}", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isValidBranchName(tt.branch) + if got != tt.expected { + t.Errorf("isValidBranchName(%q) = %v, want %v", tt.branch, got, tt.expected) + } + }) + } +} + +func TestCheckConfigValues(t *testing.T) { + // Create a temporary directory for testing + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.MkdirAll(beadsDir, 0755); err != nil { + t.Fatalf("failed to create .beads dir: %v", err) + } + + // Test with valid config + t.Run("valid config", func(t *testing.T) { + configContent := `issue-prefix: "test" +flush-debounce: "30s" +sync-branch: "beads-sync" +` + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config.yaml: %v", err) + } + + check := CheckConfigValues(tmpDir) + if check.Status != "ok" { + t.Errorf("expected ok status, got %s: %s", check.Status, check.Detail) + } + }) + + // Test with invalid flush-debounce + t.Run("invalid flush-debounce", func(t *testing.T) { + configContent := `issue-prefix: "test" +flush-debounce: "not-a-duration" +` + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config.yaml: %v", err) + } + + check := CheckConfigValues(tmpDir) + if check.Status != "warning" { + t.Errorf("expected warning status, got %s", check.Status) + } + if check.Detail == "" || !contains(check.Detail, "flush-debounce") { + t.Errorf("expected detail to mention flush-debounce, got: %s", check.Detail) + } + }) + + // Test with invalid issue-prefix + t.Run("invalid issue-prefix", func(t *testing.T) { + configContent := `issue-prefix: "123-invalid" +` + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config.yaml: %v", err) + } + + check := CheckConfigValues(tmpDir) + if check.Status != "warning" { + t.Errorf("expected warning status, got %s", check.Status) + } + if check.Detail == "" || !contains(check.Detail, "issue-prefix") { + t.Errorf("expected detail to mention issue-prefix, got: %s", check.Detail) + } + }) + + // Test with invalid routing.mode + t.Run("invalid routing.mode", func(t *testing.T) { + configContent := `routing: + mode: "invalid-mode" +` + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config.yaml: %v", err) + } + + check := CheckConfigValues(tmpDir) + if check.Status != "warning" { + t.Errorf("expected warning status, got %s", check.Status) + } + if check.Detail == "" || !contains(check.Detail, "routing.mode") { + t.Errorf("expected detail to mention routing.mode, got: %s", check.Detail) + } + }) + + // Test with invalid sync-branch + t.Run("invalid sync-branch", func(t *testing.T) { + configContent := `sync-branch: "branch with spaces" +` + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config.yaml: %v", err) + } + + check := CheckConfigValues(tmpDir) + if check.Status != "warning" { + t.Errorf("expected warning status, got %s", check.Status) + } + if check.Detail == "" || !contains(check.Detail, "sync-branch") { + t.Errorf("expected detail to mention sync-branch, got: %s", check.Detail) + } + }) + + // Test with too long issue-prefix + t.Run("too long issue-prefix", func(t *testing.T) { + configContent := `issue-prefix: "thisprefiswaytooolongtobevalid" +` + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write config.yaml: %v", err) + } + + check := CheckConfigValues(tmpDir) + if check.Status != "warning" { + t.Errorf("expected warning status, got %s", check.Status) + } + if check.Detail == "" || !contains(check.Detail, "too long") { + t.Errorf("expected detail to mention too long, got: %s", check.Detail) + } + }) +} + +func TestCheckMetadataConfigValues(t *testing.T) { + // Create a temporary directory for testing + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.MkdirAll(beadsDir, 0755); err != nil { + t.Fatalf("failed to create .beads dir: %v", err) + } + + // Test with valid metadata + t.Run("valid metadata", func(t *testing.T) { + metadataContent := `{ + "database": "beads.db", + "jsonl_export": "issues.jsonl" +}` + if err := os.WriteFile(filepath.Join(beadsDir, "metadata.json"), []byte(metadataContent), 0644); err != nil { + t.Fatalf("failed to write metadata.json: %v", err) + } + + issues := checkMetadataConfigValues(tmpDir) + if len(issues) > 0 { + t.Errorf("expected no issues, got: %v", issues) + } + }) + + // Test with path in database field + t.Run("path in database field", func(t *testing.T) { + metadataContent := `{ + "database": "/path/to/beads.db", + "jsonl_export": "issues.jsonl" +}` + if err := os.WriteFile(filepath.Join(beadsDir, "metadata.json"), []byte(metadataContent), 0644); err != nil { + t.Fatalf("failed to write metadata.json: %v", err) + } + + issues := checkMetadataConfigValues(tmpDir) + if len(issues) == 0 { + t.Error("expected issues for path in database field") + } + }) + + // Test with wrong extension for jsonl + t.Run("wrong jsonl extension", func(t *testing.T) { + metadataContent := `{ + "database": "beads.db", + "jsonl_export": "issues.json" +}` + if err := os.WriteFile(filepath.Join(beadsDir, "metadata.json"), []byte(metadataContent), 0644); err != nil { + t.Fatalf("failed to write metadata.json: %v", err) + } + + issues := checkMetadataConfigValues(tmpDir) + if len(issues) == 0 { + t.Error("expected issues for wrong jsonl extension") + } + }) +} + +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsHelper(s, substr)) +} + +func containsHelper(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +}