Code cleanup and refactoring (openshift-kni#14)

* Code cleanup and refactoring

Signed-off-by: Mario Vazquez <mavazque@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>

* Added CodeRabbit suggestions

Signed-off-by: Mario Vazquez <mavazque@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>

* Adds a section to agents.md on design decisions

Signed-off-by: Mario Vazquez <mavazque@redhat.com>

---------

Signed-off-by: Mario Vazquez <mavazque@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: mvazquezc

AGENTS.md

@@ -983,3 +983,20 @@ Before submitting new rules:
 - [ ] `supporting_doc` is included when relevant documentation exists
 - [ ] Rules are tested with real or realistic diff data
 - [ ] No duplicate rules (check existing rules first)
+
+## Conscious Design Choices
+
+This section documents intentional design decisions that may be flagged by linters or code review tools. These are not defects -- they are deliberate choices with documented rationale.
+
+### Ignoring `fmt.Fprint*` Errors in Report Generators
+
+The text and reporting generators (`pkg/report/text.go`, `pkg/report/reporting.go`) call `fmt.Fprint`, `fmt.Fprintf`, and `fmt.Fprintln` without checking their return values. This is intentional.
+
+**Why this is acceptable:**
+
+- Ignoring `fmt.Fprint*` return values when writing CLI output to an `io.Writer` is idiomatic Go. The standard library tools (`go test`, `go vet`, `go fmt`) follow this same pattern, and neither `go vet` nor `golangci-lint` flag it.
+- There are ~90 such calls in `text.go` and ~67 in `reporting.go`. Wrapping all of them with error-capturing wrappers (`writeErr` field, short-circuit checks, `g.write`/`g.writef`/`g.writeln` methods) would add significant structural complexity to what are currently clean, readable print functions.
+- In the CLI context, a write failure to stdout causes `SIGPIPE`, terminating the process. A returned error would never reach a caller that could act on it.
+- The HTML generator uses `template.Execute` which handles write errors internally, but that is a fundamentally different pattern (single write call vs. many imperative prints). Comparing them is not meaningful.
+
+**When to revisit:** If a future use case requires writing reports over a network connection or other unreliable transport where partial failure must be detected and handled, this decision can be revisited with a clear justification for the added complexity.

pkg/analyzer/analyzer.go

@@ -51,11 +51,14 @@ func NewFromBytes(rulesData []byte, version string) (*Analyzer, error) {
 // The format parameter determines output type: "text" or "html".
 // The mode parameter determines output mode: "simple" or "reporting".
 func (a *Analyzer) Analyze(w io.Writer, validationReport types.ValidationReport, format, mode string) error {
-	if mode == "reporting" {
+	switch mode {
+	case "reporting":
 		return a.generateReportingOutput(w, validationReport)
+	case "simple":
+		return a.generateFormattedOutput(w, validationReport, format)
+	default:
+		return fmt.Errorf("unsupported output mode: %s (valid: simple, reporting)", mode)
 	}
-
-	return a.generateFormattedOutput(w, validationReport, format)
 }
 
 // generateReportingOutput generates output in reporting mode.

pkg/parser/diff.go

@@ -8,18 +8,6 @@ import (
 	"github.com/openshift-kni/rds-analyzer/pkg/types"
 )
 
-// ANSI color codes for terminal output formatting.
-const (
-	ColorReset  = "\033[0m"
-	ColorRed    = "\033[31m"
-	ColorGreen  = "\033[32m"
-	ColorYellow = "\033[33m"
-	ColorBlue   = "\033[34m"
-	ColorCyan   = "\033[36m"
-	ColorBold   = "\033[1m"
-	ColorDim    = "\033[2m"
-)
-
 // diffLineInfo holds parsed line info during diff processing.
 type diffLineInfo struct {
 	content  string // Line content without diff marker

pkg/report/html.go

@@ -1,6 +1,7 @@
 package report
 
 import (
+	"html"
 	"html/template"
 	"io"
 	"path/filepath"
@@ -16,10 +17,7 @@ import (
 // escapeHTML escapes characters that could break HTML structure.
 // Returns template.HTML to prevent double-escaping by the template engine.
 func escapeHTML(s string) template.HTML {
-	s = strings.ReplaceAll(s, "&", "&")
-	s = strings.ReplaceAll(s, "<", "&lt;")
-	s = strings.ReplaceAll(s, ">", "&gt;")
-	return template.HTML(s)
+	return template.HTML(html.EscapeString(s))
 }
 
 // HTMLReport contains all data needed to render the HTML report.
@@ -310,10 +308,7 @@ func (g *HTMLGenerator) processDiffs(diffs []types.Diff, stats *ImpactStats) ([]
 	for _, d := range diffs {
 		// Handle empty diffs - add minimal DiffCheck for count rules only.
 		if d.DiffOutput == "" {
-			allDiffChecks = append(allDiffChecks, types.DiffCheck{
-				CRName:           d.CRName,
-				TemplateFileName: filepath.Base(d.CorrelatedTemplate),
-			})
+			allDiffChecks = append(allDiffChecks, minimalDiffCheck(d))
 			continue
 		}
 
@@ -454,6 +449,9 @@ func getMatchingRulesHTML(line, diffType string, ruleResult rules.EvaluationResu
 	for _, condResult := range ruleResult.Conditions {
 		if condResult.ConditionType == diffType && condResult.Matched {
 			trimmedMatched := strings.TrimSpace(condResult.MatchedText)
+			if trimmedMatched == "" {
+				continue
+			}
 			if strings.Contains(trimmedLine, trimmedMatched) || strings.Contains(trimmedMatched, trimmedLine) {
 				if !seen[condResult.RuleID] {
 					seen[condResult.RuleID] = true

pkg/report/html_test.go

@@ -270,7 +270,7 @@ func TestEscapeHTML(t *testing.T) {
 		{
 			name:     "multiple special characters",
 			input:    "<script>alert('xss')</script>",
-			expected: "&lt;script&gt;alert('xss')&lt;/script&gt;",
+			expected: "&lt;script&gt;alert(&#39;xss&#39;)&lt;/script&gt;",
 		},
 		{
 			name:     "yaml content",

pkg/report/reporting.go

@@ -240,10 +240,7 @@ func (g *ReportingGenerator) evaluateAllDiffs(diffs []types.Diff) ([]reportingDi
 	for _, d := range diffs {
 		// Handle empty diffs - add minimal DiffCheck for count rules only.
 		if d.DiffOutput == "" {
-			allDiffChecks = append(allDiffChecks, types.DiffCheck{
-				CRName:           d.CRName,
-				TemplateFileName: filepath.Base(d.CorrelatedTemplate),
-			})
+			allDiffChecks = append(allDiffChecks, minimalDiffCheck(d))
 			continue
 		}
 
@@ -256,7 +253,7 @@ func (g *ReportingGenerator) evaluateAllDiffs(diffs []types.Diff) ([]reportingDi
 		allDiffChecks = append(allDiffChecks, diffCheck)
 
 		ruleResult := g.ruleEngine.Evaluate(diffCheck)
-		hasNeedsReview := g.hasUnmatchedLines(diffCheck, ruleResult)
+		hasNeedsReview := hasUnmatchedLines(diffCheck, ruleResult)
 		finalImpact := determineImpact(ruleResult, hasNeedsReview)
 
 		// Skip NotADeviation diffs for display purposes only.
@@ -280,44 +277,9 @@ func (g *ReportingGenerator) evaluateAllDiffs(diffs []types.Diff) ([]reportingDi
 	return results, allDiffChecks
 }
 
-// hasUnmatchedLines checks if any diff lines are not matched by rules.
-func (g *ReportingGenerator) hasUnmatchedLines(diffCheck types.DiffCheck, ruleResult rules.EvaluationResult) bool {
-	for _, line := range diffCheck.ExpectedNotFound {
-		if len(g.getMatchingRuleIDs(line, "ExpectedNotFound", ruleResult)) == 0 {
-			return true
-		}
-	}
-	for _, line := range diffCheck.FoundNotExpected {
-		if len(g.getMatchingRuleIDs(line, "FoundNotExpected", ruleResult)) == 0 {
-			return true
-		}
-	}
-	for _, line := range diffCheck.FoundValue {
-		if len(g.getMatchingRuleIDs(line, "ExpectedFound", ruleResult)) == 0 {
-			return true
-		}
-	}
-	return false
-}
-
 // getMatchingRuleIDs returns rule IDs that matched a specific line.
 func (g *ReportingGenerator) getMatchingRuleIDs(line, diffType string, ruleResult rules.EvaluationResult) []string {
-	trimmedLine := strings.TrimSpace(line)
-	var ruleIDs []string
-	seen := make(map[string]bool)
-
-	for _, condResult := range ruleResult.Conditions {
-		if condResult.ConditionType == diffType && condResult.Matched {
-			trimmedMatched := strings.TrimSpace(condResult.MatchedText)
-			if strings.Contains(trimmedLine, trimmedMatched) || strings.Contains(trimmedMatched, trimmedLine) {
-				if !seen[condResult.RuleID] {
-					seen[condResult.RuleID] = true
-					ruleIDs = append(ruleIDs, condResult.RuleID)
-				}
-			}
-		}
-	}
-	return ruleIDs
+	return matchingRuleIDs(line, diffType, ruleResult)
 }
 
 // extractImpactingRules returns only the rules with Impacting impact.
@@ -360,41 +322,29 @@ func (g *ReportingGenerator) extractUnresolvedLines(diffCheck types.DiffCheck, r
 
 	for _, line := range diffCheck.ExpectedNotFound {
 		if len(g.getMatchingRuleIDs(line, "ExpectedNotFound", ruleResult)) == 0 {
-			unresolved.expectedNotFound = append(unresolved.expectedNotFound, g.stripANSI(line))
+			unresolved.expectedNotFound = append(unresolved.expectedNotFound, stripANSI(line))
 		}
 	}
 
 	for _, line := range diffCheck.FoundNotExpected {
 		if len(g.getMatchingRuleIDs(line, "FoundNotExpected", ruleResult)) == 0 {
-			unresolved.foundNotExpected = append(unresolved.foundNotExpected, g.stripANSI(line))
+			unresolved.foundNotExpected = append(unresolved.foundNotExpected, stripANSI(line))
 		}
 	}
 
 	// For ExpectedFound (value differences), include both expected and found if unmatched.
 	for i, line := range diffCheck.FoundValue {
 		if len(g.getMatchingRuleIDs(line, "ExpectedFound", ruleResult)) == 0 {
-			unresolved.foundValue = append(unresolved.foundValue, g.stripANSI(line))
+			unresolved.foundValue = append(unresolved.foundValue, stripANSI(line))
 			if i < len(diffCheck.ExpectedValue) {
-				unresolved.expectedValue = append(unresolved.expectedValue, g.stripANSI(diffCheck.ExpectedValue[i]))
+				unresolved.expectedValue = append(unresolved.expectedValue, stripANSI(diffCheck.ExpectedValue[i]))
 			}
 		}
 	}
 
 	return unresolved
 }
 
-// stripANSI removes ANSI color codes from a string.
-func (g *ReportingGenerator) stripANSI(s string) string {
-	s = strings.ReplaceAll(s, parser.ColorReset, "")
-	s = strings.ReplaceAll(s, parser.ColorRed, "")
-	s = strings.ReplaceAll(s, parser.ColorGreen, "")
-	s = strings.ReplaceAll(s, parser.ColorYellow, "")
-	s = strings.ReplaceAll(s, parser.ColorBlue, "")
-	s = strings.ReplaceAll(s, parser.ColorCyan, "")
-	s = strings.ReplaceAll(s, parser.ColorBold, "")
-	return s
-}
-
 // printIndentedLines prints lines preserving their relative YAML indentation.
 func (g *ReportingGenerator) printIndentedLines(lines []string, baseIndent string) {
 	if len(lines) == 0 {
@@ -480,15 +430,6 @@ func (g *ReportingGenerator) printContextualDiffView(diffLines []types.DiffLine,
 	}
 }
 
-// extractDiffChecks extracts DiffCheck objects from results.
-func (g *ReportingGenerator) extractDiffChecks(results []reportingDiffResult) []types.DiffCheck {
-	checks := make([]types.DiffCheck, len(results))
-	for i, r := range results {
-		checks[i] = r.diffCheck
-	}
-	return checks
-}
-
 // hasImpactingMissingCRs checks if there are any impacting missing CRs.
 func (g *ReportingGenerator) hasImpactingMissingCRs(results map[string]rules.MissingCRResult) bool {
 	for _, r := range results {

pkg/report/reporting_test.go

@@ -393,8 +393,6 @@ func TestReportingGenerator_TemplateSeparator(t *testing.T) {
 }
 
 func TestReportingGenerator_StripANSI(t *testing.T) {
-	generator := &ReportingGenerator{}
-
 	tests := []struct {
 		input    string
 		expected string
@@ -406,7 +404,7 @@ func TestReportingGenerator_StripANSI(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		result := generator.stripANSI(tt.input)
+		result := stripANSI(tt.input)
 		if result != tt.expected {
 			t.Errorf("stripANSI(%q) = %q, want %q", tt.input, result, tt.expected)
 		}
@@ -560,95 +558,6 @@ func TestReportingGenerator_PrintCountRuleViolations_Empty(t *testing.T) {
 	}
 }
 
-func TestReportingGenerator_ExtractDiffChecks(t *testing.T) {
-	rulesFile := createTestRulesFile(t)
-	engine, err := rules.NewEngine(rulesFile)
-	if err != nil {
-		t.Fatalf("Failed to create engine: %v", err)
-	}
-
-	generator := NewReportingGenerator(engine)
-
-	// Create test data with reportingDiffResult
-	results := []reportingDiffResult{
-		{
-			diffCheck: types.DiffCheck{
-				CRName:           "v1_ConfigMap_ns1_config1",
-				TemplateFileName: "Config1.yaml",
-				ExpectedNotFound: []string{"key1: value1"},
-			},
-			finalImpact: "Impacting",
-		},
-		{
-			diffCheck: types.DiffCheck{
-				CRName:           "v1_ConfigMap_ns2_config2",
-				TemplateFileName: "Config2.yaml",
-				FoundNotExpected: []string{"extra: field"},
-			},
-			finalImpact: "NotImpacting",
-		},
-		{
-			diffCheck: types.DiffCheck{
-				CRName:           "v1_Secret_ns3_secret1",
-				TemplateFileName: "Secret1.yaml",
-				ExpectedValue:    []string{"password: old"},
-				FoundValue:       []string{"password: new"},
-			},
-			finalImpact: "NeedsReview",
-		},
-	}
-
-	checks := generator.extractDiffChecks(results)
-
-	// Verify correct number of checks extracted
-	if len(checks) != 3 {
-		t.Fatalf("expected 3 checks, got %d", len(checks))
-	}
-
-	// Verify first check
-	if checks[0].CRName != "v1_ConfigMap_ns1_config1" {
-		t.Errorf("check[0].CRName = %q, want %q", checks[0].CRName, "v1_ConfigMap_ns1_config1")
-	}
-	if checks[0].TemplateFileName != "Config1.yaml" {
-		t.Errorf("check[0].TemplateFileName = %q, want %q", checks[0].TemplateFileName, "Config1.yaml")
-	}
-	if len(checks[0].ExpectedNotFound) != 1 {
-		t.Errorf("check[0].ExpectedNotFound length = %d, want 1", len(checks[0].ExpectedNotFound))
-	}
-
-	// Verify second check
-	if checks[1].CRName != "v1_ConfigMap_ns2_config2" {
-		t.Errorf("check[1].CRName = %q, want %q", checks[1].CRName, "v1_ConfigMap_ns2_config2")
-	}
-	if len(checks[1].FoundNotExpected) != 1 {
-		t.Errorf("check[1].FoundNotExpected length = %d, want 1", len(checks[1].FoundNotExpected))
-	}
-
-	// Verify third check
-	if checks[2].CRName != "v1_Secret_ns3_secret1" {
-		t.Errorf("check[2].CRName = %q, want %q", checks[2].CRName, "v1_Secret_ns3_secret1")
-	}
-	if len(checks[2].ExpectedValue) != 1 || len(checks[2].FoundValue) != 1 {
-		t.Errorf("check[2] should have 1 ExpectedValue and 1 FoundValue")
-	}
-}
-
-func TestReportingGenerator_ExtractDiffChecks_Empty(t *testing.T) {
-	rulesFile := createTestRulesFile(t)
-	engine, err := rules.NewEngine(rulesFile)
-	if err != nil {
-		t.Fatalf("Failed to create engine: %v", err)
-	}
-
-	generator := NewReportingGenerator(engine)
-
-	checks := generator.extractDiffChecks([]reportingDiffResult{})
-
-	if len(checks) != 0 {
-		t.Errorf("expected 0 checks for empty input, got %d", len(checks))
-	}
-}
-
 func TestReportingGenerator_ExtractResolvedRules(t *testing.T) {
 	rulesFile := createTestRulesFile(t)
 	engine, err := rules.NewEngine(rulesFile)