From a0dac11e421d62340093f9d6b3e73f603cf99891 Mon Sep 17 00:00:00 2001 From: Bo Date: Wed, 21 Jan 2026 22:50:03 -0500 Subject: [PATCH] refactor: reduce cyclomatic complexity in repair.go and config_values.go (#1214) - repair.go: Extract validateRepairPaths(), findAllOrphans(), printOrphansText() - config_values.go: Extract findConfigPath(), validateDurationConfig(), etc. - Target: CC < 20 for each extracted function --- cmd/bd/doctor/config_values.go | 221 ++++++++++++++++---------------- cmd/bd/repair.go | 225 ++++++++++++++++----------------- 2 files changed, 220 insertions(+), 226 deletions(-) diff --git a/cmd/bd/doctor/config_values.go b/cmd/bd/doctor/config_values.go index a3d0d579..2fbc1f93 100644 --- a/cmd/bd/doctor/config_values.go +++ b/cmd/bd/doctor/config_values.go @@ -69,68 +69,126 @@ func CheckConfigValues(repoPath string) DoctorCheck { } } -// 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") - +// findConfigPath locates config.yaml in standard locations. +func findConfigPath(repoPath string) string { 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 _, err := os.Stat(configPath); err == nil { + return configPath + } + if configDir, err := os.UserConfigDir(); err == nil { + userConfigPath := filepath.Join(configDir, "bd", "config.yaml") + if _, err := os.Stat(userConfigPath); err == nil { + return userConfigPath + } + } + if homeDir, err := os.UserHomeDir(); err == nil { + homeConfigPath := filepath.Join(homeDir, ".beads", "config.yaml") + if _, err := os.Stat(homeConfigPath); err == nil { + return homeConfigPath + } + } + return "" +} + +// validateDurationConfig validates a duration config value. +func validateDurationConfig(v *viper.Viper, key string, minDuration time.Duration) []string { + if !v.IsSet(key) { + return nil + } + durStr := v.GetString(key) + if durStr == "" { + return nil + } + d, err := time.ParseDuration(durStr) + if err != nil { + return []string{fmt.Sprintf("%s: invalid duration %q (expected format like \"30s\", \"1m\", \"500ms\")", key, durStr)} + } + if minDuration > 0 && d > 0 && d < minDuration { + return []string{fmt.Sprintf("%s: %q is too low (minimum %s)", key, durStr, minDuration)} + } + return nil +} + +// validateBooleanConfigs validates boolean config values. +func validateBooleanConfigs(v *viper.Viper, keys []string) []string { + var issues []string + for _, key := range keys { + if v.IsSet(key) { + strVal := v.GetString(key) + if strVal != "" && !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)) } } - 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 + } + return issues +} + +// validateRoutingPaths validates routing path config values. +func validateRoutingPaths(v *viper.Viper) []string { + var issues []string + for _, key := range []string{"routing.default", "routing.maintainer", "routing.contributor"} { + if v.IsSet(key) { + path := v.GetString(key) + if path != "" && path != "." { + expandedPath := expandPath(path) + if _, err := os.Stat(expandedPath); os.IsNotExist(err) { + issues = append(issues, fmt.Sprintf("%s: path %q does not exist", key, path)) } } } } + return issues +} +// validateRepoPaths validates repos.primary and repos.additional paths. +func validateRepoPaths(v *viper.Viper) []string { + var issues []string + 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)) + } + } + } + if v.IsSet("repos.additional") { + for _, path := range v.GetStringSlice("repos.additional") { + if path != "" { + expandedPath := expandPath(path) + if info, err := os.Stat(expandedPath); err == nil && !info.IsDir() { + issues = append(issues, fmt.Sprintf("repos.additional: %q is not a directory", path)) + } + } + } + } + return issues +} + +// checkYAMLConfigValues validates values in config.yaml +func checkYAMLConfigValues(repoPath string) []string { + var issues []string + + configPath := findConfigPath(repoPath) if configPath == "" { - // No config.yaml found anywhere return issues } + v := viper.New() + v.SetConfigType("yaml") 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 remote-sync-interval (should be a valid duration, min 5s) - if v.IsSet("remote-sync-interval") { - intervalStr := v.GetString("remote-sync-interval") - if intervalStr != "" { - d, err := time.ParseDuration(intervalStr) - if err != nil { - issues = append(issues, fmt.Sprintf("remote-sync-interval: invalid duration %q (expected format like \"30s\", \"1m\", \"5m\")", intervalStr)) - } else if d > 0 && d < 5*time.Second { - issues = append(issues, fmt.Sprintf("remote-sync-interval: %q is too low (minimum 5s to prevent excessive load)", intervalStr)) - } - } - } + // Validate duration configs + issues = append(issues, validateDurationConfig(v, "flush-debounce", 0)...) + issues = append(issues, validateDurationConfig(v, "remote-sync-interval", 5*time.Second)...) // Validate issue-prefix (should be alphanumeric with dashes/underscores, reasonably short) if v.IsSet("issue-prefix") { @@ -168,23 +226,7 @@ func checkYAMLConfigValues(repoPath string) []string { } // 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))) - } - } - } - } + issues = append(issues, validateRoutingPaths(v)...) // Validate actor (should be alphanumeric with common special chars if set) if v.IsSet("actor") { @@ -209,59 +251,12 @@ func checkYAMLConfigValues(repoPath string) []string { } } - // 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 boolean config values + boolKeys := []string{"json", "no-daemon", "no-auto-flush", "no-auto-import", "no-db", "auto-start-daemon", "sync.require_confirmation_on_mass_delete"} + issues = append(issues, validateBooleanConfigs(v, boolKeys)...) - // 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 - } - } - } + // Validate repos paths + issues = append(issues, validateRepoPaths(v)...) return issues } diff --git a/cmd/bd/repair.go b/cmd/bd/repair.go index a34caa8f..d7c456d9 100644 --- a/cmd/bd/repair.go +++ b/cmd/bd/repair.go @@ -68,30 +68,106 @@ func outputJSONAndExit(result repairResult, exitCode int) { os.Exit(exitCode) } -func runRepair(cmd *cobra.Command, args []string) { - // Find .beads directory +// validateRepairPaths validates the beads directory and database exist. +func validateRepairPaths() (string, error) { beadsDir := filepath.Join(repairPath, ".beads") if _, err := os.Stat(beadsDir); os.IsNotExist(err) { - if repairJSON { - outputJSONAndExit(repairResult{ - Status: "error", - Error: fmt.Sprintf(".beads directory not found at %s", beadsDir), - }, 1) - } - fmt.Fprintf(os.Stderr, "Error: .beads directory not found at %s\n", beadsDir) - os.Exit(1) + return "", fmt.Errorf(".beads directory not found at %s", beadsDir) } - - // Find database file dbPath := filepath.Join(beadsDir, "beads.db") if _, err := os.Stat(dbPath); os.IsNotExist(err) { - if repairJSON { - outputJSONAndExit(repairResult{ - Status: "error", - Error: fmt.Sprintf("database not found at %s", dbPath), - }, 1) + return "", fmt.Errorf("database not found at %s", dbPath) + } + return dbPath, nil +} + +// findAllOrphans finds all orphaned references in the database. +func findAllOrphans(db *sql.DB) (stats repairStats, orphans allOrphans, err error) { + orphans.depsIssueID, err = findOrphanedDepsIssueID(db) + if err != nil { + return stats, orphans, fmt.Errorf("checking orphaned deps (issue_id): %w", err) + } + stats.orphanedDepsIssueID = len(orphans.depsIssueID) + + orphans.depsDependsOn, err = findOrphanedDepsDependsOn(db) + if err != nil { + return stats, orphans, fmt.Errorf("checking orphaned deps (depends_on_id): %w", err) + } + stats.orphanedDepsDependsOn = len(orphans.depsDependsOn) + + orphans.labels, err = findOrphanedLabels(db) + if err != nil { + return stats, orphans, fmt.Errorf("checking orphaned labels: %w", err) + } + stats.orphanedLabels = len(orphans.labels) + + orphans.comments, err = findOrphanedComments(db) + if err != nil { + return stats, orphans, fmt.Errorf("checking orphaned comments: %w", err) + } + stats.orphanedComments = len(orphans.comments) + + orphans.events, err = findOrphanedEvents(db) + if err != nil { + return stats, orphans, fmt.Errorf("checking orphaned events: %w", err) + } + stats.orphanedEvents = len(orphans.events) + + return stats, orphans, nil +} + +// allOrphans holds all found orphaned references. +type allOrphans struct { + depsIssueID []orphanedDep + depsDependsOn []orphanedDep + labels []orphanedLabel + comments []orphanedComment + events []orphanedEvent +} + +// printOrphansText prints orphan details in text mode. +func printOrphansText(stats repairStats, orphans allOrphans) { + fmt.Printf("Found %d orphaned reference(s):\n", stats.total()) + if stats.orphanedDepsIssueID > 0 { + fmt.Printf(" • %d dependencies with missing issue_id\n", stats.orphanedDepsIssueID) + for _, dep := range orphans.depsIssueID { + fmt.Printf(" - %s → %s\n", dep.issueID, dep.dependsOnID) } - fmt.Fprintf(os.Stderr, "Error: database not found at %s\n", dbPath) + } + if stats.orphanedDepsDependsOn > 0 { + fmt.Printf(" • %d dependencies with missing depends_on_id\n", stats.orphanedDepsDependsOn) + for _, dep := range orphans.depsDependsOn { + fmt.Printf(" - %s → %s\n", dep.issueID, dep.dependsOnID) + } + } + if stats.orphanedLabels > 0 { + fmt.Printf(" • %d labels with missing issue_id\n", stats.orphanedLabels) + for _, label := range orphans.labels { + fmt.Printf(" - %s: %s\n", label.issueID, label.label) + } + } + if stats.orphanedComments > 0 { + fmt.Printf(" • %d comments with missing issue_id\n", stats.orphanedComments) + for _, comment := range orphans.comments { + fmt.Printf(" - %s (by %s)\n", comment.issueID, comment.author) + } + } + if stats.orphanedEvents > 0 { + fmt.Printf(" • %d events with missing issue_id\n", stats.orphanedEvents) + for _, event := range orphans.events { + fmt.Printf(" - %s: %s\n", event.issueID, event.eventType) + } + } + fmt.Println() +} + +func runRepair(cmd *cobra.Command, args []string) { + dbPath, err := validateRepairPaths() + if err != nil { + if repairJSON { + outputJSONAndExit(repairResult{Status: "error", Error: err.Error()}, 1) + } + fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } @@ -103,72 +179,26 @@ func runRepair(cmd *cobra.Command, args []string) { fmt.Println() } - // Open database directly, bypassing beads storage layer db, err := openRepairDB(dbPath) if err != nil { if repairJSON { - outputJSONAndExit(repairResult{ - DatabasePath: dbPath, - Status: "error", - Error: fmt.Sprintf("opening database: %v", err), - }, 1) + outputJSONAndExit(repairResult{DatabasePath: dbPath, Status: "error", Error: fmt.Sprintf("opening database: %v", err)}, 1) } fmt.Fprintf(os.Stderr, "Error opening database: %v\n", err) os.Exit(1) } defer db.Close() - // Collect repair statistics - stats := repairStats{} - - // Helper for consistent error output - exitWithError := func(msg string, err error) { + // Find all orphaned references + stats, orphans, err := findAllOrphans(db) + if err != nil { if repairJSON { - outputJSONAndExit(repairResult{ - DatabasePath: dbPath, - Status: "error", - Error: fmt.Sprintf("%s: %v", msg, err), - }, 1) + outputJSONAndExit(repairResult{DatabasePath: dbPath, Status: "error", Error: err.Error()}, 1) } - fmt.Fprintf(os.Stderr, "Error %s: %v\n", msg, err) + fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } - // 1. Find and clean orphaned dependencies (issue_id not in issues) - orphanedIssueID, err := findOrphanedDepsIssueID(db) - if err != nil { - exitWithError("checking orphaned deps (issue_id)", err) - } - stats.orphanedDepsIssueID = len(orphanedIssueID) - - // 2. Find and clean orphaned dependencies (depends_on_id not in issues) - orphanedDependsOn, err := findOrphanedDepsDependsOn(db) - if err != nil { - exitWithError("checking orphaned deps (depends_on_id)", err) - } - stats.orphanedDepsDependsOn = len(orphanedDependsOn) - - // 3. Find and clean orphaned labels - orphanedLabels, err := findOrphanedLabels(db) - if err != nil { - exitWithError("checking orphaned labels", err) - } - stats.orphanedLabels = len(orphanedLabels) - - // 4. Find and clean orphaned comments - orphanedCommentsList, err := findOrphanedComments(db) - if err != nil { - exitWithError("checking orphaned comments", err) - } - stats.orphanedComments = len(orphanedCommentsList) - - // 5. Find and clean orphaned events - orphanedEventsList, err := findOrphanedEvents(db) - if err != nil { - exitWithError("checking orphaned events", err) - } - stats.orphanedEvents = len(orphanedEventsList) - // Build JSON result structure (used for both JSON output and tracking) jsonResult := repairResult{ DatabasePath: dbPath, @@ -184,23 +214,23 @@ func runRepair(cmd *cobra.Command, args []string) { } // Build orphan details - for _, dep := range orphanedIssueID { + for _, dep := range orphans.depsIssueID { jsonResult.OrphanDetails.DependenciesIssueID = append(jsonResult.OrphanDetails.DependenciesIssueID, repairDepDetail{IssueID: dep.issueID, DependsOnID: dep.dependsOnID}) } - for _, dep := range orphanedDependsOn { + for _, dep := range orphans.depsDependsOn { jsonResult.OrphanDetails.DependenciesDependsOn = append(jsonResult.OrphanDetails.DependenciesDependsOn, repairDepDetail{IssueID: dep.issueID, DependsOnID: dep.dependsOnID}) } - for _, label := range orphanedLabels { + for _, label := range orphans.labels { jsonResult.OrphanDetails.Labels = append(jsonResult.OrphanDetails.Labels, repairLabelDetail{IssueID: label.issueID, Label: label.label}) } - for _, comment := range orphanedCommentsList { + for _, comment := range orphans.comments { jsonResult.OrphanDetails.Comments = append(jsonResult.OrphanDetails.Comments, repairCommentDetail{ID: comment.id, IssueID: comment.issueID, Author: comment.author}) } - for _, event := range orphanedEventsList { + for _, event := range orphans.events { jsonResult.OrphanDetails.Events = append(jsonResult.OrphanDetails.Events, repairEventDetail{ID: event.id, IssueID: event.issueID, EventType: event.eventType}) } @@ -217,38 +247,7 @@ func runRepair(cmd *cobra.Command, args []string) { // Print findings (text mode) if !repairJSON { - fmt.Printf("Found %d orphaned reference(s):\n", stats.total()) - if stats.orphanedDepsIssueID > 0 { - fmt.Printf(" • %d dependencies with missing issue_id\n", stats.orphanedDepsIssueID) - for _, dep := range orphanedIssueID { - fmt.Printf(" - %s → %s\n", dep.issueID, dep.dependsOnID) - } - } - if stats.orphanedDepsDependsOn > 0 { - fmt.Printf(" • %d dependencies with missing depends_on_id\n", stats.orphanedDepsDependsOn) - for _, dep := range orphanedDependsOn { - fmt.Printf(" - %s → %s\n", dep.issueID, dep.dependsOnID) - } - } - if stats.orphanedLabels > 0 { - fmt.Printf(" • %d labels with missing issue_id\n", stats.orphanedLabels) - for _, label := range orphanedLabels { - fmt.Printf(" - %s: %s\n", label.issueID, label.label) - } - } - if stats.orphanedComments > 0 { - fmt.Printf(" • %d comments with missing issue_id\n", stats.orphanedComments) - for _, comment := range orphanedCommentsList { - fmt.Printf(" - %s (by %s)\n", comment.issueID, comment.author) - } - } - if stats.orphanedEvents > 0 { - fmt.Printf(" • %d events with missing issue_id\n", stats.orphanedEvents) - for _, event := range orphanedEventsList { - fmt.Printf(" - %s: %s\n", event.issueID, event.eventType) - } - } - fmt.Println() + printOrphansText(stats, orphans) } // Handle dry-run @@ -300,7 +299,7 @@ func runRepair(cmd *cobra.Command, args []string) { var repairErr error // Delete orphaned deps (issue_id) and mark affected issues dirty - if len(orphanedIssueID) > 0 && repairErr == nil { + if len(orphans.depsIssueID) > 0 && repairErr == nil { // Note: orphanedIssueID contains deps where issue_id doesn't exist, // so we can't mark them dirty (the issue is gone). But for depends_on orphans, // the issue_id still exists and should be marked dirty. @@ -317,9 +316,9 @@ func runRepair(cmd *cobra.Command, args []string) { } // Delete orphaned deps (depends_on_id) and mark parent issues dirty - if len(orphanedDependsOn) > 0 && repairErr == nil { + if len(orphans.depsDependsOn) > 0 && repairErr == nil { // Mark parent issues as dirty for export - for _, dep := range orphanedDependsOn { + for _, dep := range orphans.depsDependsOn { _, _ = tx.Exec("INSERT OR IGNORE INTO dirty_issues (issue_id) VALUES (?)", dep.issueID) } @@ -337,7 +336,7 @@ func runRepair(cmd *cobra.Command, args []string) { } // Delete orphaned labels - if len(orphanedLabels) > 0 && repairErr == nil { + if len(orphans.labels) > 0 && repairErr == nil { // Labels reference non-existent issues, so no dirty marking needed result, err := tx.Exec(` DELETE FROM labels @@ -352,7 +351,7 @@ func runRepair(cmd *cobra.Command, args []string) { } // Delete orphaned comments - if len(orphanedCommentsList) > 0 && repairErr == nil { + if len(orphans.comments) > 0 && repairErr == nil { // Comments reference non-existent issues, so no dirty marking needed result, err := tx.Exec(` DELETE FROM comments @@ -367,7 +366,7 @@ func runRepair(cmd *cobra.Command, args []string) { } // Delete orphaned events - if len(orphanedEventsList) > 0 && repairErr == nil { + if len(orphans.events) > 0 && repairErr == nil { // Events reference non-existent issues, so no dirty marking needed result, err := tx.Exec(` DELETE FROM events