feat(doctor): add configuration value validation (bd-alz)
Add comprehensive validation for config values in bd doctor: YAML config (config.yaml) validations: - actor: alphanumeric with dashes, underscores, dots, @ - db: valid database extension (.db, .sqlite, .sqlite3) - Boolean flags: json, no-daemon, no-auto-flush, no-auto-import, no-db, auto-start-daemon validate as true/false/yes/no/1/0/on/off - sync.require_confirmation_on_mass_delete: boolean validation - repos.primary: must be a directory if path exists - repos.additional: paths must be directories if they exist Database config validations: - status.custom: validates custom status names are lowercase alphanumeric with underscores, checks for conflicts with built-in statuses (open, in_progress, blocked, closed) - sync.branch (legacy): validates as git branch name Includes tests for all new validation functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Steve Yegge
@@ -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 == "" {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user