Code review improvements to bd validate
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
This commit is contained in:
@@ -21,10 +21,11 @@ var validateCmd = &cobra.Command{
|
|||||||
- Git merge conflicts in JSONL
|
- Git merge conflicts in JSONL
|
||||||
|
|
||||||
Example:
|
Example:
|
||||||
bd validate # Run all checks
|
bd validate # Run all checks
|
||||||
bd validate --fix-all # Auto-fix all issues
|
bd validate --fix-all # Auto-fix all issues
|
||||||
bd validate --checks=orphans,dupes # Run specific checks
|
bd validate --checks=orphans,dupes # Run specific checks
|
||||||
bd validate --json # Output in JSON format`,
|
bd validate --checks=conflicts # Check for git conflicts
|
||||||
|
bd validate --json # Output in JSON format`,
|
||||||
Run: func(cmd *cobra.Command, _ []string) {
|
Run: func(cmd *cobra.Command, _ []string) {
|
||||||
// Check daemon mode - not supported yet (uses direct storage access)
|
// Check daemon mode - not supported yet (uses direct storage access)
|
||||||
if daemonClient != nil {
|
if daemonClient != nil {
|
||||||
@@ -35,49 +36,119 @@ Example:
|
|||||||
|
|
||||||
fixAll, _ := cmd.Flags().GetBool("fix-all")
|
fixAll, _ := cmd.Flags().GetBool("fix-all")
|
||||||
checksFlag, _ := cmd.Flags().GetString("checks")
|
checksFlag, _ := cmd.Flags().GetString("checks")
|
||||||
|
jsonOut, _ := cmd.Flags().GetBool("json")
|
||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
|
|
||||||
// Determine which checks to run
|
// Parse and normalize checks
|
||||||
var checks []string
|
checks, err := parseChecks(checksFlag)
|
||||||
if checksFlag == "" {
|
if err != nil {
|
||||||
checks = []string{"orphans", "duplicates", "pollution"}
|
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||||
} else {
|
fmt.Fprintf(os.Stderr, "Valid checks: orphans, duplicates, pollution, conflicts\n")
|
||||||
checks = strings.Split(checksFlag, ",")
|
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{
|
results := validationResults{
|
||||||
checks: make(map[string]checkResult),
|
checks: make(map[string]checkResult),
|
||||||
|
checkOrder: checks,
|
||||||
}
|
}
|
||||||
|
|
||||||
// Run each check
|
// Run each check
|
||||||
for _, check := range checks {
|
for _, check := range checks {
|
||||||
switch check {
|
switch check {
|
||||||
case "orphans":
|
case "orphans":
|
||||||
results.checks["orphans"] = validateOrphanedDeps(ctx, fixAll)
|
results.checks["orphans"] = validateOrphanedDeps(ctx, allIssues, fixAll)
|
||||||
case "duplicates", "dupes":
|
case "duplicates":
|
||||||
results.checks["duplicates"] = validateDuplicates(ctx, fixAll)
|
results.checks["duplicates"] = validateDuplicates(ctx, allIssues, fixAll)
|
||||||
case "pollution":
|
case "pollution":
|
||||||
results.checks["pollution"] = validatePollution(ctx, fixAll)
|
results.checks["pollution"] = validatePollution(ctx, allIssues, fixAll)
|
||||||
default:
|
case "conflicts":
|
||||||
fmt.Fprintf(os.Stderr, "Unknown check: %s\n", check)
|
results.checks["conflicts"] = validateGitConflicts(ctx, fixAll)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Output results
|
// Output results
|
||||||
if jsonOutput {
|
if jsonOut {
|
||||||
outputJSON(results.toJSON())
|
outputJSON(results.toJSON())
|
||||||
} else {
|
} else {
|
||||||
results.print(fixAll)
|
results.print(fixAll)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Exit with error code if issues found
|
// Exit with error code if issues found or errors occurred
|
||||||
if results.hasIssues() {
|
if results.hasFailures() {
|
||||||
os.Exit(1)
|
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 {
|
type checkResult struct {
|
||||||
name string
|
name string
|
||||||
issueCount int
|
issueCount int
|
||||||
@@ -87,11 +158,15 @@ type checkResult struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
type validationResults 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 {
|
for _, result := range r.checks {
|
||||||
|
if result.err != nil {
|
||||||
|
return true
|
||||||
|
}
|
||||||
if result.issueCount > 0 && result.fixedCount < result.issueCount {
|
if result.issueCount > 0 && result.fixedCount < result.issueCount {
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
@@ -106,12 +181,20 @@ func (r *validationResults) toJSON() map[string]interface{} {
|
|||||||
|
|
||||||
totalIssues := 0
|
totalIssues := 0
|
||||||
totalFixed := 0
|
totalFixed := 0
|
||||||
|
hasErrors := false
|
||||||
|
|
||||||
for name, result := range r.checks {
|
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{}{
|
output["checks"].(map[string]interface{})[name] = map[string]interface{}{
|
||||||
"issue_count": result.issueCount,
|
"issue_count": result.issueCount,
|
||||||
"fixed_count": result.fixedCount,
|
"fixed_count": result.fixedCount,
|
||||||
"error": result.err,
|
"error": errorStr,
|
||||||
|
"failed": result.err != nil,
|
||||||
"suggestions": result.suggestions,
|
"suggestions": result.suggestions,
|
||||||
}
|
}
|
||||||
totalIssues += result.issueCount
|
totalIssues += result.issueCount
|
||||||
@@ -120,7 +203,7 @@ func (r *validationResults) toJSON() map[string]interface{} {
|
|||||||
|
|
||||||
output["total_issues"] = totalIssues
|
output["total_issues"] = totalIssues
|
||||||
output["total_fixed"] = totalFixed
|
output["total_fixed"] = totalFixed
|
||||||
output["healthy"] = totalIssues == 0 || totalIssues == totalFixed
|
output["healthy"] = !hasErrors && (totalIssues == 0 || totalIssues == totalFixed)
|
||||||
|
|
||||||
return output
|
return output
|
||||||
}
|
}
|
||||||
@@ -136,24 +219,26 @@ func (r *validationResults) print(fixAll bool) {
|
|||||||
totalIssues := 0
|
totalIssues := 0
|
||||||
totalFixed := 0
|
totalFixed := 0
|
||||||
|
|
||||||
for name, result := range r.checks {
|
// Print in deterministic order
|
||||||
|
for _, name := range r.checkOrder {
|
||||||
|
result := r.checks[name]
|
||||||
prefix := "✓"
|
prefix := "✓"
|
||||||
colorFunc := green
|
colorFunc := green
|
||||||
|
|
||||||
if result.err != nil {
|
if result.err != nil {
|
||||||
prefix = "✗"
|
prefix = "✗"
|
||||||
colorFunc = red
|
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 {
|
} else if result.issueCount > 0 {
|
||||||
prefix = "⚠"
|
prefix = "⚠"
|
||||||
colorFunc = yellow
|
colorFunc = yellow
|
||||||
if result.fixedCount > 0 {
|
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 {
|
} 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 {
|
} else {
|
||||||
fmt.Printf("%s %s: OK\n", colorFunc(prefix), name)
|
fmt.Printf("%s %s: OK\n", colorFunc(prefix), result.name)
|
||||||
}
|
}
|
||||||
|
|
||||||
totalIssues += result.issueCount
|
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"}
|
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
|
// Build ID existence map
|
||||||
existingIDs := make(map[string]bool)
|
existingIDs := make(map[string]bool)
|
||||||
for _, issue := range allIssues {
|
for _, issue := range allIssues {
|
||||||
@@ -248,16 +326,9 @@ func validateOrphanedDeps(ctx context.Context, fix bool) checkResult {
|
|||||||
return result
|
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"}
|
result := checkResult{name: "duplicates"}
|
||||||
|
|
||||||
// Get all issues
|
|
||||||
allIssues, err := store.SearchIssues(ctx, "", types.IssueFilter{})
|
|
||||||
if err != nil {
|
|
||||||
result.err = err
|
|
||||||
return result
|
|
||||||
}
|
|
||||||
|
|
||||||
// Find duplicates
|
// Find duplicates
|
||||||
duplicateGroups := findDuplicateGroups(allIssues)
|
duplicateGroups := findDuplicateGroups(allIssues)
|
||||||
|
|
||||||
@@ -279,16 +350,9 @@ func validateDuplicates(ctx context.Context, fix bool) checkResult {
|
|||||||
return result
|
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"}
|
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
|
// Detect pollution
|
||||||
polluted := detectTestPollution(allIssues)
|
polluted := detectTestPollution(allIssues)
|
||||||
result.issueCount = len(polluted)
|
result.issueCount = len(polluted)
|
||||||
@@ -305,8 +369,55 @@ func validatePollution(ctx context.Context, fix bool) checkResult {
|
|||||||
return result
|
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() {
|
func init() {
|
||||||
validateCmd.Flags().Bool("fix-all", false, "Auto-fix all fixable issues")
|
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)
|
rootCmd.AddCommand(validateCmd)
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user