Address code review feedback: extract magic numbers and add optimizations

- Extract hardcoded values as named constants for better maintainability
- Add early return for empty invalidProps parameter
- Optimize similarity scoring with early exit for poor matches
- All tests pass and functionality remains intact

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Author: Copilot

.github/workflows/ci-doctor.lock.yml

@@ -91,7 +91,7 @@ var ErrNoArtifacts = errors.New("no artifacts found for this run")
 // fetchJobStatuses gets job information for a workflow run and counts failed jobs
 func fetchJobStatuses(runID int64, verbose bool) (int, error) {
 	args := []string{"api", fmt.Sprintf("repos/{owner}/{repo}/actions/runs/%d/jobs", runID), "--jq", ".jobs[] | {name: .name, status: .status, conclusion: .conclusion}"}
-	
+
 	if verbose {
 		fmt.Println(console.FormatVerboseMessage(fmt.Sprintf("Fetching job statuses for run %d", runID)))
 	}
@@ -113,15 +113,15 @@ func fetchJobStatuses(runID int64, verbose bool) (int, error) {
 		if strings.TrimSpace(line) == "" {
 			continue
 		}
-		
+
 		var job JobInfo
 		if err := json.Unmarshal([]byte(line), &job); err != nil {
 			if verbose {
 				fmt.Println(console.FormatVerboseMessage(fmt.Sprintf("Failed to parse job info: %s", line)))
 			}
 			continue
 		}
-		
+
 		// Count jobs with failure conclusions as errors
 		if job.Conclusion == "failure" || job.Conclusion == "cancelled" || job.Conclusion == "timed_out" {
 			failedJobs++
@@ -130,7 +130,7 @@ func fetchJobStatuses(runID int64, verbose bool) (int, error) {
 			}
 		}
 	}
-	
+
 	return failedJobs, nil
 }
 

.github/workflows/dev.lock.yml

@@ -212,7 +212,7 @@ func validateWithSchemaAndLocation(frontmatter map[string]any, schemaJSON, conte
 
 				// Rewrite "additional properties not allowed" errors to be more friendly
 				message := rewriteAdditionalPropertiesError(primaryPath.Message)
-				
+
 				// Add schema-based suggestions
 				suggestions := generateSchemaBasedSuggestions(schemaJSON, primaryPath.Message, primaryPath.Path)
 				if suggestions != "" {
@@ -240,7 +240,7 @@ func validateWithSchemaAndLocation(frontmatter map[string]any, schemaJSON, conte
 
 		// Rewrite "additional properties not allowed" errors to be more friendly
 		message := rewriteAdditionalPropertiesError(errorMsg)
-		
+
 		// Add schema-based suggestions for fallback case
 		suggestions := generateSchemaBasedSuggestions(schemaJSON, errorMsg, "")
 		if suggestions != "" {
@@ -393,6 +393,14 @@ func findFrontmatterBounds(lines []string) (startIdx int, endIdx int, frontmatte
 	return startIdx, endIdx, frontmatterContent
 }
 
+// Constants for suggestion limits and field generation
+const (
+	maxClosestMatches = 3  // Maximum number of closest matches to find
+	maxSuggestions    = 5  // Maximum number of suggestions to show
+	maxAcceptedFields = 10 // Maximum number of accepted fields to display
+	maxExampleFields  = 3  // Maximum number of fields to include in example JSON
+)
+
 // generateSchemaBasedSuggestions generates helpful suggestions based on the schema and error type
 func generateSchemaBasedSuggestions(schemaJSON, errorMessage, jsonPath string) string {
 	// Parse the schema to extract information for suggestions
@@ -405,7 +413,7 @@ func generateSchemaBasedSuggestions(schemaJSON, errorMessage, jsonPath string) s
 	if strings.Contains(strings.ToLower(errorMessage), "additional propert") && strings.Contains(strings.ToLower(errorMessage), "not allowed") {
 		invalidProps := extractAdditionalPropertyNames(errorMessage)
 		acceptedFields := extractAcceptedFieldsFromSchema(schemaDoc, jsonPath)
-		
+
 		if len(acceptedFields) > 0 {
 			return generateFieldSuggestions(invalidProps, acceptedFields)
 		}
@@ -510,12 +518,12 @@ func resolveSchemaWithOneOf(schema map[string]any) map[string]any {
 
 // generateFieldSuggestions creates a helpful suggestion message for invalid field names
 func generateFieldSuggestions(invalidProps, acceptedFields []string) string {
-	if len(acceptedFields) == 0 {
+	if len(acceptedFields) == 0 || len(invalidProps) == 0 {
 		return ""
 	}
 
 	var suggestion strings.Builder
-	
+
 	if len(invalidProps) == 1 {
 		suggestion.WriteString("Did you mean one of: ")
 	} else {
@@ -525,26 +533,26 @@ func generateFieldSuggestions(invalidProps, acceptedFields []string) string {
 	// Find closest matches using simple string distance
 	var suggestions []string
 	for _, invalidProp := range invalidProps {
-		closest := findClosestMatches(invalidProp, acceptedFields, 3)
+		closest := findClosestMatches(invalidProp, acceptedFields, maxClosestMatches)
 		suggestions = append(suggestions, closest...)
 	}
 
 	// If we have specific suggestions, show them first
 	if len(suggestions) > 0 {
 		// Remove duplicates
 		uniqueSuggestions := removeDuplicates(suggestions)
-		if len(uniqueSuggestions) <= 5 {
+		if len(uniqueSuggestions) <= maxSuggestions {
 			suggestion.WriteString(strings.Join(uniqueSuggestions, ", "))
 		} else {
-			suggestion.WriteString(strings.Join(uniqueSuggestions[:5], ", "))
+			suggestion.WriteString(strings.Join(uniqueSuggestions[:maxSuggestions], ", "))
 			suggestion.WriteString(", ...")
 		}
 	} else {
 		// Show all accepted fields if no close matches
-		if len(acceptedFields) <= 10 {
+		if len(acceptedFields) <= maxAcceptedFields {
 			suggestion.WriteString(strings.Join(acceptedFields, ", "))
 		} else {
-			suggestion.WriteString(strings.Join(acceptedFields[:10], ", "))
+			suggestion.WriteString(strings.Join(acceptedFields[:maxAcceptedFields], ", "))
 			suggestion.WriteString(", ...")
 		}
 	}
@@ -565,7 +573,7 @@ func findClosestMatches(target string, candidates []string, maxResults int) []st
 	for _, candidate := range candidates {
 		candidateLower := strings.ToLower(candidate)
 		score := calculateSimilarityScore(targetLower, candidateLower)
-		
+
 		// Only include if there's some similarity
 		if score > 0 {
 			matches = append(matches, match{value: candidate, score: score})
@@ -588,32 +596,41 @@ func findClosestMatches(target string, candidates []string, maxResults int) []st
 
 // calculateSimilarityScore calculates a simple similarity score between two strings
 func calculateSimilarityScore(a, b string) int {
+	// Early exit for obviously poor matches (length difference > 2x shorter string length)
+	minLen := len(a)
+	if len(b) < minLen {
+		minLen = len(b)
+	}
+	lengthDiff := abs(len(a) - len(b))
+	if lengthDiff > minLen*2 && minLen > 0 {
+		return 0
+	}
+
 	// Simple heuristics for string similarity
 	score := 0
-	
+
 	// Bonus for substring matches
 	if strings.Contains(b, a) || strings.Contains(a, b) {
 		score += 10
 	}
-	
+
 	// Bonus for common prefixes
 	commonPrefix := 0
 	for i := 0; i < len(a) && i < len(b) && a[i] == b[i]; i++ {
 		commonPrefix++
 	}
 	score += commonPrefix * 2
-	
+
 	// Bonus for common suffixes
 	commonSuffix := 0
 	for i := 0; i < len(a) && i < len(b) && a[len(a)-1-i] == b[len(b)-1-i]; i++ {
 		commonSuffix++
 	}
 	score += commonSuffix * 2
-	
+
 	// Penalty for length difference
-	lengthDiff := abs(len(a) - len(b))
 	score -= lengthDiff
-	
+
 	return score
 }
 
@@ -694,11 +711,10 @@ func generateExampleFromSchema(schema map[string]any) any {
 
 			// Add a few example properties (prioritize required ones)
 			count := 0
-			maxFields := 3
 
 			// First, add required fields
 			for propName, propSchema := range properties {
-				if requiredFields[propName] && count < maxFields {
+				if requiredFields[propName] && count < maxExampleFields {
 					if propSchemaMap, ok := propSchema.(map[string]any); ok {
 						result[propName] = generateExampleFromSchema(propSchemaMap)
 						count++
@@ -708,7 +724,7 @@ func generateExampleFromSchema(schema map[string]any) any {
 
 			// Then add some optional fields if we have room
 			for propName, propSchema := range properties {
-				if !requiredFields[propName] && count < maxFields {
+				if !requiredFields[propName] && count < maxExampleFields {
 					if propSchemaMap, ok := propSchema.(map[string]any); ok {
 						result[propName] = generateExampleFromSchema(propSchemaMap)
 						count++
@@ -726,14 +742,14 @@ func generateExampleFromSchema(schema map[string]any) any {
 func removeDuplicates(strings []string) []string {
 	seen := make(map[string]bool)
 	var result []string
-	
+
 	for _, str := range strings {
 		if !seen[str] {
 			seen[str] = true
 			result = append(result, str)
 		}
 	}
-	
+
 	return result
 }
 

.github/workflows/tidy.lock.yml

@@ -317,4 +317,4 @@ func TestGenerateExampleJSONForPath(t *testing.T) {
 			}
 		})
 	}
-}
+}

pkg/cli/logs.go

@@ -264,25 +264,25 @@ type ErrorWarningCounts struct {
 // This is more accurate than simple string matching and uses the same logic as validate_errors.cjs
 func CountErrorsAndWarningsWithPatterns(logContent string, patterns []ErrorPattern) ErrorWarningCounts {
 	counts := ErrorWarningCounts{}
-	
+
 	if len(patterns) == 0 {
 		return counts
 	}
-	
+
 	lines := strings.Split(logContent, "\n")
-	
+
 	for _, pattern := range patterns {
 		regex, err := regexp.Compile(pattern.Pattern)
 		if err != nil {
 			// Skip invalid patterns
 			continue
 		}
-		
+
 		for _, line := range lines {
 			matches := regex.FindAllStringSubmatch(line, -1)
 			for _, match := range matches {
 				level := extractLevelFromMatch(match, pattern)
-				
+
 				if strings.ToLower(level) == "error" {
 					counts.ErrorCount++
 				} else if strings.ToLower(level) == "warning" || strings.ToLower(level) == "warn" {
@@ -291,7 +291,7 @@ func CountErrorsAndWarningsWithPatterns(logContent string, patterns []ErrorPatte
 			}
 		}
 	}
-	
+
 	return counts
 }
 
@@ -301,26 +301,26 @@ func extractLevelFromMatch(match []string, pattern ErrorPattern) string {
 	if pattern.LevelGroup > 0 && pattern.LevelGroup < len(match) && match[pattern.LevelGroup] != "" {
 		levelText := strings.ToLower(match[pattern.LevelGroup])
 		// Normalize common error/warning keywords
-		if strings.Contains(levelText, "err") || strings.Contains(levelText, "error") || 
-		   strings.Contains(levelText, "fail") || strings.Contains(levelText, "fatal") {
+		if strings.Contains(levelText, "err") || strings.Contains(levelText, "error") ||
+			strings.Contains(levelText, "fail") || strings.Contains(levelText, "fatal") {
 			return "error"
 		} else if strings.Contains(levelText, "warn") || strings.Contains(levelText, "warning") {
 			return "warning"
 		}
 		// Return the original level text if it doesn't match common patterns
 		return match[pattern.LevelGroup]
 	}
-	
+
 	// Try to infer level from the full match content
 	if len(match) > 0 {
 		fullMatch := strings.ToLower(match[0])
 		if strings.Contains(fullMatch, "error") || strings.Contains(fullMatch, "err") ||
-		   strings.Contains(fullMatch, "fail") || strings.Contains(fullMatch, "fatal") {
+			strings.Contains(fullMatch, "fail") || strings.Contains(fullMatch, "fatal") {
 			return "error"
 		} else if strings.Contains(fullMatch, "warn") || strings.Contains(fullMatch, "warning") {
 			return "warning"
 		}
 	}
-	
+
 	return "unknown"
 }

pkg/parser/schema.go

@@ -116,7 +116,7 @@ Another warning message`,
 				},
 			},
 			expectedErrors: 2, // npm ERR! + random error
-			expectedWarns:  2,  // npm WARN + warning message
+			expectedWarns:  2, // npm WARN + warning message
 		},
 		{
 			name: "invalid regex pattern",
@@ -150,11 +150,11 @@ warning: be careful`,
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			counts := CountErrorsAndWarningsWithPatterns(tt.logContent, tt.patterns)
-			
+
 			if counts.ErrorCount != tt.expectedErrors {
 				t.Errorf("Expected %d errors, got %d", tt.expectedErrors, counts.ErrorCount)
 			}
-			
+
 			if counts.WarningCount != tt.expectedWarns {
 				t.Errorf("Expected %d warnings, got %d", tt.expectedWarns, counts.WarningCount)
 			}
@@ -215,4 +215,4 @@ func TestExtractLevelFromMatch(t *testing.T) {
 			}
 		})
 	}
-}
+}