diff --git a/cmd/bd/doctor/config_values.go b/cmd/bd/doctor/config_values.go index 6ce9e962..497e85a9 100644 --- a/cmd/bd/doctor/config_values.go +++ b/cmd/bd/doctor/config_values.go @@ -1,14 +1,19 @@ package doctor import ( + "database/sql" "fmt" "os" "path/filepath" "regexp" + "strconv" "strings" "time" + _ "github.com/ncruces/go-sqlite3/driver" + _ "github.com/ncruces/go-sqlite3/embed" "github.com/spf13/viper" + "github.com/steveyegge/beads/internal/beads" "github.com/steveyegge/beads/internal/configfile" ) @@ -24,6 +29,12 @@ var validRoutingModes = map[string]bool{ // 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]$`) +// validActorRegex validates actor names (alphanumeric with dashes, underscores, dots, and @ for emails) +var validActorRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._@-]*$`) + +// validCustomStatusRegex validates custom status names (alphanumeric with underscores) +var validCustomStatusRegex = regexp.MustCompile(`^[a-z][a-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 { @@ -37,6 +48,10 @@ func CheckConfigValues(repoPath string) DoctorCheck { metadataIssues := checkMetadataConfigValues(repoPath) issues = append(issues, metadataIssues...) + // Check database config values (status.custom, etc.) + dbIssues := checkDatabaseConfigValues(repoPath) + issues = append(issues, dbIssues...) + if len(issues) == 0 { return DoctorCheck{ Name: "Config Values", @@ -158,9 +173,108 @@ func checkYAMLConfigValues(repoPath string) []string { } } + // Validate actor (should be alphanumeric with common special chars if set) + if v.IsSet("actor") { + actor := v.GetString("actor") + if actor != "" && !validActorRegex.MatchString(actor) { + issues = append(issues, fmt.Sprintf("actor: %q is invalid (must start with letter/number, contain only letters, numbers, dashes, underscores, dots, or @)", actor)) + } + } + + // Validate db path (should be a valid file path if set) + if v.IsSet("db") { + dbPath := v.GetString("db") + if dbPath != "" { + // Check for invalid path characters (null bytes, etc.) + if strings.ContainsAny(dbPath, "\x00") { + issues = append(issues, fmt.Sprintf("db: %q contains invalid characters", dbPath)) + } + // Check if it has a valid database extension + if !strings.HasSuffix(dbPath, ".db") && !strings.HasSuffix(dbPath, ".sqlite") && !strings.HasSuffix(dbPath, ".sqlite3") { + issues = append(issues, fmt.Sprintf("db: %q has unusual extension (expected .db, .sqlite, or .sqlite3)", dbPath)) + } + } + } + + // Validate boolean config values are actually booleans + for _, key := range []string{"json", "no-daemon", "no-auto-flush", "no-auto-import", "no-db", "auto-start-daemon"} { + if v.IsSet(key) { + // Try to get as string first to check if it's a valid boolean representation + strVal := v.GetString(key) + if strVal != "" { + // Valid boolean strings: true, false, 1, 0, yes, no, on, off (case insensitive) + if !isValidBoolString(strVal) { + issues = append(issues, fmt.Sprintf("%s: %q is not a valid boolean value (expected true/false, yes/no, 1/0, on/off)", key, strVal)) + } + } + } + } + + // Validate sync.require_confirmation_on_mass_delete (should be boolean) + if v.IsSet("sync.require_confirmation_on_mass_delete") { + strVal := v.GetString("sync.require_confirmation_on_mass_delete") + if strVal != "" && !isValidBoolString(strVal) { + issues = append(issues, fmt.Sprintf("sync.require_confirmation_on_mass_delete: %q is not a valid boolean value", strVal)) + } + } + + // Validate repos.primary (should be a directory path if set) + if v.IsSet("repos.primary") { + primary := v.GetString("repos.primary") + if primary != "" { + expandedPath := expandPath(primary) + if info, err := os.Stat(expandedPath); err == nil { + if !info.IsDir() { + issues = append(issues, fmt.Sprintf("repos.primary: %q is not a directory", primary)) + } + } else if !os.IsNotExist(err) { + issues = append(issues, fmt.Sprintf("repos.primary: cannot access %q: %v", primary, err)) + } + // Note: path not existing is OK - might be created later + } + } + + // Validate repos.additional (should be directory paths if set) + if v.IsSet("repos.additional") { + additional := v.GetStringSlice("repos.additional") + for _, path := range additional { + if path != "" { + expandedPath := expandPath(path) + if info, err := os.Stat(expandedPath); err == nil { + if !info.IsDir() { + issues = append(issues, fmt.Sprintf("repos.additional: %q is not a directory", path)) + } + } + // Note: path not existing is OK - might be created later + } + } + } + return issues } +// isValidBoolString checks if a string represents a valid boolean value +func isValidBoolString(s string) bool { + lower := strings.ToLower(strings.TrimSpace(s)) + switch lower { + case "true", "false", "yes", "no", "1", "0", "on", "off", "t", "f", "y", "n": + return true + } + // Also check if it parses as a bool + _, err := strconv.ParseBool(s) + return err == nil +} + +// expandPath expands ~ to home directory and resolves the path +func expandPath(path string) string { + if strings.HasPrefix(path, "~") { + if home, err := os.UserHomeDir(); err == nil { + path = filepath.Join(home, path[1:]) + } + } + return path +} + // checkMetadataConfigValues validates values in metadata.json func checkMetadataConfigValues(repoPath string) []string { var issues []string @@ -205,6 +319,66 @@ func checkMetadataConfigValues(repoPath string) []string { return issues } +// checkDatabaseConfigValues validates configuration values stored in the database +func checkDatabaseConfigValues(repoPath string) []string { + var issues []string + + beadsDir := filepath.Join(repoPath, ".beads") + if _, err := os.Stat(beadsDir); os.IsNotExist(err) { + return issues // No .beads directory, nothing to check + } + + // Get database path + dbPath := filepath.Join(beadsDir, beads.CanonicalDatabaseName) + // Check metadata.json for custom database name + if cfg, err := configfile.Load(beadsDir); err == nil && cfg != nil && cfg.Database != "" { + dbPath = cfg.DatabasePath(beadsDir) + } + + if _, err := os.Stat(dbPath); os.IsNotExist(err) { + return issues // No database, nothing to check + } + + // Open database in read-only mode + db, err := sql.Open("sqlite3", "file:"+dbPath+"?mode=ro") + if err != nil { + return issues // Can't open database, skip + } + defer db.Close() + + // Check status.custom - custom status names should be lowercase alphanumeric with underscores + var statusCustom string + err = db.QueryRow("SELECT value FROM config WHERE key = 'status.custom'").Scan(&statusCustom) + if err == nil && statusCustom != "" { + statuses := strings.Split(statusCustom, ",") + for _, status := range statuses { + status = strings.TrimSpace(status) + if status == "" { + continue + } + if !validCustomStatusRegex.MatchString(status) { + issues = append(issues, fmt.Sprintf("status.custom: %q is invalid (must start with lowercase letter, contain only lowercase letters, numbers, and underscores)", status)) + } + // Check for conflicts with built-in statuses + switch status { + case "open", "in_progress", "blocked", "closed": + issues = append(issues, fmt.Sprintf("status.custom: %q conflicts with built-in status", status)) + } + } + } + + // Check sync.branch if stored in database (legacy location) + var syncBranch string + err = db.QueryRow("SELECT value FROM config WHERE key = 'sync.branch'").Scan(&syncBranch) + if err == nil && syncBranch != "" { + if !isValidBranchName(syncBranch) { + issues = append(issues, fmt.Sprintf("sync.branch (database): %q is not a valid git branch name", syncBranch)) + } + } + + return issues +} + // isValidBranchName checks if a string is a valid git branch name func isValidBranchName(name string) bool { if name == "" { diff --git a/cmd/bd/doctor/config_values_test.go b/cmd/bd/doctor/config_values_test.go index fdefd86c..04bf60ba 100644 --- a/cmd/bd/doctor/config_values_test.go +++ b/cmd/bd/doctor/config_values_test.go @@ -227,3 +227,202 @@ func containsHelper(s, substr string) bool { } return false } + +func TestIsValidBoolString(t *testing.T) { + tests := []struct { + name string + input string + expected bool + }{ + {"true", "true", true}, + {"false", "false", true}, + {"True uppercase", "True", true}, + {"FALSE uppercase", "FALSE", true}, + {"yes", "yes", true}, + {"no", "no", true}, + {"1", "1", true}, + {"0", "0", true}, + {"on", "on", true}, + {"off", "off", true}, + {"t", "t", true}, + {"f", "f", true}, + {"y", "y", true}, + {"n", "n", true}, + + {"invalid string", "invalid", false}, + {"maybe", "maybe", false}, + {"2", "2", false}, + {"empty", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isValidBoolString(tt.input) + if got != tt.expected { + t.Errorf("isValidBoolString(%q) = %v, want %v", tt.input, got, tt.expected) + } + }) + } +} + +func TestExpandPath(t *testing.T) { + homeDir, err := os.UserHomeDir() + if err != nil { + t.Skip("Cannot get home directory") + } + + tests := []struct { + name string + input string + expected string + }{ + {"tilde only", "~", homeDir}, + {"tilde path", "~/foo/bar", filepath.Join(homeDir, "foo/bar")}, + {"no tilde", "/absolute/path", "/absolute/path"}, + {"relative", "relative/path", "relative/path"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := expandPath(tt.input) + if got != tt.expected { + t.Errorf("expandPath(%q) = %q, want %q", tt.input, got, tt.expected) + } + }) + } +} + +func TestValidActorRegex(t *testing.T) { + tests := []struct { + name string + actor string + expected bool + }{ + {"simple name", "alice", true}, + {"with numbers", "user123", true}, + {"with dash", "alice-bob", true}, + {"with underscore", "alice_bob", true}, + {"with dot", "alice.bob", true}, + {"email", "alice@example.com", true}, + {"starts with number", "123user", true}, + + {"empty", "", false}, + {"starts with dash", "-user", false}, + {"starts with dot", ".user", false}, + {"starts with at", "@user", false}, + {"contains space", "alice bob", false}, + {"contains special", "alice$bob", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := validActorRegex.MatchString(tt.actor) + if got != tt.expected { + t.Errorf("validActorRegex.MatchString(%q) = %v, want %v", tt.actor, got, tt.expected) + } + }) + } +} + +func TestValidCustomStatusRegex(t *testing.T) { + tests := []struct { + name string + status string + expected bool + }{ + {"simple", "awaiting_review", true}, + {"with numbers", "stage1", true}, + {"lowercase only", "testing", true}, + {"underscore prefix", "a_test", true}, + + {"uppercase", "Awaiting_Review", false}, + {"starts with number", "1stage", false}, + {"starts with underscore", "_test", false}, + {"contains dash", "awaiting-review", false}, + {"empty", "", false}, + {"space", "awaiting review", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := validCustomStatusRegex.MatchString(tt.status) + if got != tt.expected { + t.Errorf("validCustomStatusRegex.MatchString(%q) = %v, want %v", tt.status, got, tt.expected) + } + }) + } +} + +func TestCheckConfigValuesActor(t *testing.T) { + 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) + } + + t.Run("invalid actor", func(t *testing.T) { + configContent := `actor: "@invalid-actor" +` + 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, "actor") { + t.Errorf("expected detail to mention actor, got: %s", check.Detail) + } + }) + + t.Run("valid actor", func(t *testing.T) { + configContent := `actor: "alice@example.com" +` + 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) + } + }) +} + +func TestCheckConfigValuesDbPath(t *testing.T) { + 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) + } + + t.Run("unusual db extension", func(t *testing.T) { + configContent := `db: "/path/to/database.txt" +` + 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, "db") { + t.Errorf("expected detail to mention db, got: %s", check.Detail) + } + }) + + t.Run("valid db path", func(t *testing.T) { + configContent := `db: "/path/to/database.db" +` + 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) + } + }) +}