From 1cc1e6615c20b439f1a260d0b168e58e9ea0889b Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Fri, 31 Oct 2025 19:37:18 -0700 Subject: [PATCH] Code review improvements to bd validate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on oracle feedback: - Add parseChecks() helper for check normalization and validation - Supports synonyms: dupes→duplicates, git-conflicts→conflicts - Case-insensitive, whitespace-tolerant parsing - Deduplicates repeated checks while preserving order - Returns error for unknown checks (exit code 2) - Fix JSON output robustness - Serialize errors as strings, not objects - Add 'failed' boolean per check - Fix 'healthy' to include error state - Improve error handling - hasFailures() now includes check errors - Exit code 1 for any failures (issues or errors) - Exit code 2 for usage errors (invalid checks) - Optimize database access - Single SearchIssues() call shared across checks - Only fetch if needed (orphans/duplicates/pollution) - Stabilize output ordering - Print checks in deterministic order (not map iteration) - Use result.name for display labels - Better UX - Unknown checks fail fast with helpful message - Deterministic output for CI/scripting - More robust JSON for machine consumption --- cmd/bd/validate.go | 219 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 165 insertions(+), 54 deletions(-) diff --git a/cmd/bd/validate.go b/cmd/bd/validate.go index 6ca049bb..873ad7e2 100644 --- a/cmd/bd/validate.go +++ b/cmd/bd/validate.go @@ -21,10 +21,11 @@ var validateCmd = &cobra.Command{ - Git merge conflicts in JSONL Example: - bd validate # Run all checks - bd validate --fix-all # Auto-fix all issues - bd validate --checks=orphans,dupes # Run specific checks - bd validate --json # Output in JSON format`, + bd validate # Run all checks + bd validate --fix-all # Auto-fix all issues + bd validate --checks=orphans,dupes # Run specific checks + bd validate --checks=conflicts # Check for git conflicts + bd validate --json # Output in JSON format`, Run: func(cmd *cobra.Command, _ []string) { // Check daemon mode - not supported yet (uses direct storage access) if daemonClient != nil { @@ -35,49 +36,119 @@ Example: fixAll, _ := cmd.Flags().GetBool("fix-all") checksFlag, _ := cmd.Flags().GetString("checks") + jsonOut, _ := cmd.Flags().GetBool("json") ctx := context.Background() - // Determine which checks to run - var checks []string - if checksFlag == "" { - checks = []string{"orphans", "duplicates", "pollution"} - } else { - checks = strings.Split(checksFlag, ",") + // Parse and normalize checks + checks, err := parseChecks(checksFlag) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + fmt.Fprintf(os.Stderr, "Valid checks: orphans, duplicates, pollution, conflicts\n") + os.Exit(2) + } + + // Fetch all issues once for checks that need them + var allIssues []*types.Issue + needsIssues := false + for _, check := range checks { + if check == "orphans" || check == "duplicates" || check == "pollution" { + needsIssues = true + break + } + } + if needsIssues { + allIssues, err = store.SearchIssues(ctx, "", types.IssueFilter{}) + if err != nil { + fmt.Fprintf(os.Stderr, "Error fetching issues: %v\n", err) + os.Exit(1) + } } results := validationResults{ - checks: make(map[string]checkResult), + checks: make(map[string]checkResult), + checkOrder: checks, } // Run each check for _, check := range checks { switch check { case "orphans": - results.checks["orphans"] = validateOrphanedDeps(ctx, fixAll) - case "duplicates", "dupes": - results.checks["duplicates"] = validateDuplicates(ctx, fixAll) + results.checks["orphans"] = validateOrphanedDeps(ctx, allIssues, fixAll) + case "duplicates": + results.checks["duplicates"] = validateDuplicates(ctx, allIssues, fixAll) case "pollution": - results.checks["pollution"] = validatePollution(ctx, fixAll) - default: - fmt.Fprintf(os.Stderr, "Unknown check: %s\n", check) + results.checks["pollution"] = validatePollution(ctx, allIssues, fixAll) + case "conflicts": + results.checks["conflicts"] = validateGitConflicts(ctx, fixAll) } } // Output results - if jsonOutput { + if jsonOut { outputJSON(results.toJSON()) } else { results.print(fixAll) } - // Exit with error code if issues found - if results.hasIssues() { + // Exit with error code if issues found or errors occurred + if results.hasFailures() { os.Exit(1) } }, } +// parseChecks normalizes and validates check names +func parseChecks(checksFlag string) ([]string, error) { + defaultChecks := []string{"orphans", "duplicates", "pollution", "conflicts"} + + if checksFlag == "" { + return defaultChecks, nil + } + + // Map of synonyms to canonical names + synonyms := map[string]string{ + "dupes": "duplicates", + "git-conflicts": "conflicts", + } + + var result []string + seen := make(map[string]bool) + + parts := strings.Split(checksFlag, ",") + for _, part := range parts { + check := strings.ToLower(strings.TrimSpace(part)) + if check == "" { + continue + } + + // Map synonyms + if canonical, ok := synonyms[check]; ok { + check = canonical + } + + // Validate + valid := false + for _, validCheck := range defaultChecks { + if check == validCheck { + valid = true + break + } + } + if !valid { + return nil, fmt.Errorf("unknown check: %s", part) + } + + // Deduplicate + if !seen[check] { + seen[check] = true + result = append(result, check) + } + } + + return result, nil +} + type checkResult struct { name string issueCount int @@ -87,11 +158,15 @@ type checkResult struct { } type validationResults struct { - checks map[string]checkResult + checks map[string]checkResult + checkOrder []string } -func (r *validationResults) hasIssues() bool { +func (r *validationResults) hasFailures() bool { for _, result := range r.checks { + if result.err != nil { + return true + } if result.issueCount > 0 && result.fixedCount < result.issueCount { return true } @@ -106,12 +181,20 @@ func (r *validationResults) toJSON() map[string]interface{} { totalIssues := 0 totalFixed := 0 + hasErrors := false for name, result := range r.checks { + var errorStr interface{} + if result.err != nil { + errorStr = result.err.Error() + hasErrors = true + } + output["checks"].(map[string]interface{})[name] = map[string]interface{}{ "issue_count": result.issueCount, "fixed_count": result.fixedCount, - "error": result.err, + "error": errorStr, + "failed": result.err != nil, "suggestions": result.suggestions, } totalIssues += result.issueCount @@ -120,7 +203,7 @@ func (r *validationResults) toJSON() map[string]interface{} { output["total_issues"] = totalIssues output["total_fixed"] = totalFixed - output["healthy"] = totalIssues == 0 || totalIssues == totalFixed + output["healthy"] = !hasErrors && (totalIssues == 0 || totalIssues == totalFixed) return output } @@ -136,24 +219,26 @@ func (r *validationResults) print(fixAll bool) { totalIssues := 0 totalFixed := 0 - for name, result := range r.checks { + // Print in deterministic order + for _, name := range r.checkOrder { + result := r.checks[name] prefix := "✓" colorFunc := green if result.err != nil { prefix = "✗" colorFunc = red - fmt.Printf("%s %s: ERROR - %v\n", colorFunc(prefix), name, result.err) + fmt.Printf("%s %s: ERROR - %v\n", colorFunc(prefix), result.name, result.err) } else if result.issueCount > 0 { prefix = "⚠" colorFunc = yellow if result.fixedCount > 0 { - fmt.Printf("%s %s: %d found, %d fixed\n", colorFunc(prefix), name, result.issueCount, result.fixedCount) + fmt.Printf("%s %s: %d found, %d fixed\n", colorFunc(prefix), result.name, result.issueCount, result.fixedCount) } else { - fmt.Printf("%s %s: %d found\n", colorFunc(prefix), name, result.issueCount) + fmt.Printf("%s %s: %d found\n", colorFunc(prefix), result.name, result.issueCount) } } else { - fmt.Printf("%s %s: OK\n", colorFunc(prefix), name) + fmt.Printf("%s %s: OK\n", colorFunc(prefix), result.name) } totalIssues += result.issueCount @@ -184,16 +269,9 @@ func (r *validationResults) print(fixAll bool) { } } -func validateOrphanedDeps(ctx context.Context, fix bool) checkResult { +func validateOrphanedDeps(ctx context.Context, allIssues []*types.Issue, fix bool) checkResult { result := checkResult{name: "orphaned dependencies"} - // Get all issues - allIssues, err := store.SearchIssues(ctx, "", types.IssueFilter{}) - if err != nil { - result.err = err - return result - } - // Build ID existence map existingIDs := make(map[string]bool) for _, issue := range allIssues { @@ -248,16 +326,9 @@ func validateOrphanedDeps(ctx context.Context, fix bool) checkResult { return result } -func validateDuplicates(ctx context.Context, fix bool) checkResult { +func validateDuplicates(ctx context.Context, allIssues []*types.Issue, fix bool) checkResult { result := checkResult{name: "duplicates"} - // Get all issues - allIssues, err := store.SearchIssues(ctx, "", types.IssueFilter{}) - if err != nil { - result.err = err - return result - } - // Find duplicates duplicateGroups := findDuplicateGroups(allIssues) @@ -279,16 +350,9 @@ func validateDuplicates(ctx context.Context, fix bool) checkResult { return result } -func validatePollution(ctx context.Context, fix bool) checkResult { +func validatePollution(ctx context.Context, allIssues []*types.Issue, fix bool) checkResult { result := checkResult{name: "test pollution"} - // Get all issues - allIssues, err := store.SearchIssues(ctx, "", types.IssueFilter{}) - if err != nil { - result.err = err - return result - } - // Detect pollution polluted := detectTestPollution(allIssues) result.issueCount = len(polluted) @@ -305,8 +369,55 @@ func validatePollution(ctx context.Context, fix bool) checkResult { return result } +func validateGitConflicts(ctx context.Context, fix bool) checkResult { + result := checkResult{name: "git conflicts"} + + // Check JSONL file for conflict markers + jsonlPath := findJSONLPath() + data, err := os.ReadFile(jsonlPath) + if err != nil { + if os.IsNotExist(err) { + // No JSONL file = no conflicts + return result + } + result.err = fmt.Errorf("failed to read JSONL: %w", err) + return result + } + + // Look for git conflict markers + lines := strings.Split(string(data), "\n") + var conflictLines []int + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "<<<<<<< ") || + trimmed == "=======" || + strings.HasPrefix(trimmed, ">>>>>>> ") { + conflictLines = append(conflictLines, i+1) + } + } + + if len(conflictLines) > 0 { + result.issueCount = 1 // One conflict situation + result.suggestions = append(result.suggestions, + fmt.Sprintf("Resolve git conflict in %s (markers at lines: %v)", jsonlPath, conflictLines)) + if !fix { + result.suggestions = append(result.suggestions, + fmt.Sprintf("Then run 'bd import -i %s' to reload issues", jsonlPath)) + } + } + + // Can't auto-fix git conflicts + if fix && result.issueCount > 0 { + result.suggestions = append(result.suggestions, + "Git conflicts cannot be auto-fixed. Resolve manually in your editor or run 'bd export' to regenerate JSONL") + } + + return result +} + func init() { validateCmd.Flags().Bool("fix-all", false, "Auto-fix all fixable issues") - validateCmd.Flags().String("checks", "", "Comma-separated list of checks (orphans,duplicates,pollution)") + validateCmd.Flags().String("checks", "", "Comma-separated list of checks (orphans,duplicates,pollution,conflicts)") + validateCmd.Flags().Bool("json", false, "Output in JSON format") rootCmd.AddCommand(validateCmd) }