Merge pull request #173 from SimplyLiz/fix/review-feedback

feat: Address 5 items from external technical review

Author: SimplyLiz

internal/query/review.go

@@ -165,13 +165,24 @@ type GeneratedFileInfo struct {
 // DefaultReviewPolicy returns sensible defaults.
 func DefaultReviewPolicy() *ReviewPolicy {
 	return &ReviewPolicy{
-		BlockBreakingChanges:  true,
-		BlockSecrets:          true,
-		FailOnLevel:           "error",
-		HoldTheLine:           true,
-		SplitThreshold:        50,
-		GeneratedPatterns:     []string{"*.generated.*", "*.pb.go", "*.pb.cc", "parser.tab.c", "lex.yy.c"},
-		GeneratedMarkers:      []string{"DO NOT EDIT", "Generated by", "AUTO-GENERATED", "This file is generated"},
+		BlockBreakingChanges: true,
+		BlockSecrets:         true,
+		FailOnLevel:          "error",
+		HoldTheLine:          true,
+		SplitThreshold:       50,
+		GeneratedPatterns: []string{
+			"*.generated.*", "*.pb.go", "*.pb.cc", "*.pb.h",
+			"parser.tab.c", "lex.yy.c",
+			"*.swagger.json", "*.openapi.json",
+			"*_generated.go", "*_gen.go",
+			"*.min.js", "*.min.css",
+		},
+		GeneratedMarkers: []string{
+			"DO NOT EDIT", "Generated by", "AUTO-GENERATED", "This file is generated",
+			"Code generated", "Automatically generated",
+			"eslint-disable", "swagger-codegen", "openapi-generator",
+			"@generated", "protoc-gen", "graphql-codegen",
+		},
 		CriticalSeverity:      "error",
 		DeadCodeMinConfidence: 0.8,
 		TestGapMinLines:       5,
@@ -790,10 +801,6 @@ func (e *Engine) checkBreakingChanges(ctx context.Context, opts ReviewPROptions)
 	}
 
 	var findings []ReviewFinding
-	breakingCount := 0
-	if resp.Summary != nil {
-		breakingCount = resp.Summary.BreakingChanges
-	}
 
 	for _, change := range resp.Changes {
 		if change.Severity == "breaking" || change.Severity == "error" {
@@ -808,6 +815,11 @@ func (e *Engine) checkBreakingChanges(ctx context.Context, opts ReviewPROptions)
 		}
 	}
 
+	// Filter out rename pairs — a removed + added symbol in the same file
+	// with the same kind is likely a rename, not a breaking change.
+	findings = filterRenamePairs(findings)
+	breakingCount := len(findings)
+
 	status := "pass"
 	severity := "error"
 	summary := "No breaking API changes"
@@ -825,6 +837,43 @@ func (e *Engine) checkBreakingChanges(ctx context.Context, opts ReviewPROptions)
 	}, findings
 }
 
+// filterRenamePairs removes findings that are likely renames rather than
+// breaking changes. A rename produces "removed X" + "added Y" in the same
+// file with the same kind — not a real API break.
+func filterRenamePairs(findings []ReviewFinding) []ReviewFinding {
+	// Group by file
+	byFile := make(map[string][]ReviewFinding)
+	for _, f := range findings {
+		byFile[f.File] = append(byFile[f.File], f)
+	}
+
+	var filtered []ReviewFinding
+	for _, fileFindings := range byFile {
+		// Count removed and added per kind
+		removedByKind := make(map[string]int)
+		addedByKind := make(map[string]int)
+		for _, f := range fileFindings {
+			if strings.Contains(f.Message, "removed") || strings.Contains(f.Message, "Removed") {
+				removedByKind[f.RuleID]++
+			} else if strings.Contains(f.Message, "added") || strings.Contains(f.Message, "Added") || strings.Contains(f.Message, "new") {
+				addedByKind[f.RuleID]++
+			}
+		}
+
+		for _, f := range fileFindings {
+			kind := f.RuleID
+			isRemoved := strings.Contains(f.Message, "removed") || strings.Contains(f.Message, "Removed")
+			// If there's a matching add for this remove in the same file+kind, skip it
+			if isRemoved && addedByKind[kind] > 0 {
+				addedByKind[kind]--
+				continue // Likely a rename
+			}
+			filtered = append(filtered, f)
+		}
+	}
+	return filtered
+}
+
 func (e *Engine) checkSecrets(ctx context.Context, files []string) (ReviewCheck, []ReviewFinding) {
 	start := time.Now()
 

internal/query/review_coupling.go

@@ -3,13 +3,27 @@ package query
 import (
 	"context"
 	"fmt"
+	"os/exec"
 	"strings"
 	"time"
 
 	"github.com/SimplyLiz/CodeMCP/internal/backends/git"
 	"github.com/SimplyLiz/CodeMCP/internal/coupling"
 )
 
+const maxCouplingAge = 180 * 24 * time.Hour
+
+// fileLastModified returns the last modification date of a file according to git.
+func (e *Engine) fileLastModified(ctx context.Context, file string) time.Time {
+	cmd := exec.CommandContext(ctx, "git", "-C", e.repoRoot, "log", "-1", "--format=%aI", "--", file)
+	out, err := cmd.Output()
+	if err != nil {
+		return time.Time{}
+	}
+	t, _ := time.Parse(time.RFC3339, strings.TrimSpace(string(out)))
+	return t
+}
+
 // CouplingGap represents a missing co-changed file.
 type CouplingGap struct {
 	ChangedFile  string  `json:"changedFile"`
@@ -76,10 +90,18 @@ func (e *Engine) checkCouplingGaps(ctx context.Context, changedFiles []string, d
 				missing = corr.File
 			}
 			if corr.Correlation >= minCorrelation && !changedSet[missing] && !isCouplingNoiseFile(missing) {
+				// Skip stale couplings — if the coupled file hasn't been
+				// modified in the last 180 days, the co-change relationship
+				// is historical noise (e.g., test written once alongside source).
+				lastMod := e.fileLastModified(ctx, missing)
+				if !lastMod.IsZero() && time.Since(lastMod) > maxCouplingAge {
+					continue
+				}
 				gaps = append(gaps, CouplingGap{
 					ChangedFile:  file,
 					MissingFile:  missing,
 					CoChangeRate: corr.Correlation,
+					LastCoChange: lastMod.Format(time.RFC3339),
 				})
 			}
 		}

internal/query/review_health.go

@@ -51,8 +51,8 @@ type CodeHealthReport struct {
 // Coverage was removed because no coverage data source is available yet.
 // When coverage is added, reduce churn and cyclomatic by 0.05 each.
 const (
-	weightCyclomatic = 0.25
-	weightCognitive  = 0.15
+	weightCyclomatic = 0.15
+	weightCognitive  = 0.25
 	weightFileSize   = 0.10
 	weightChurn      = 0.15
 	weightCoupling   = 0.10

internal/query/review_split.go

@@ -43,6 +43,10 @@ func (e *Engine) suggestPRSplit(ctx context.Context, diffStats []git.DiffStats,
 		statsMap[ds.FilePath] = ds
 	}
 
+	// For very large PRs, skip coupling analysis (O(n) git calls)
+	// and rely on module-based clustering only
+	skipCoupling := len(diffStats) > 200
+
 	// Build adjacency graph: files are connected if they share a module
 	// or have high coupling correlation
 	adj := make(map[string]map[string]bool)
@@ -70,7 +74,9 @@ func (e *Engine) suggestPRSplit(ctx context.Context, diffStats []git.DiffStats,
 	}
 
 	// Connect files with high coupling
-	e.addCouplingEdges(ctx, files, adj)
+	if !skipCoupling {
+		e.addCouplingEdges(ctx, files, adj)
+	}
 
 	// Find connected components using BFS
 	visited := make(map[string]bool)
@@ -84,6 +90,19 @@ func (e *Engine) suggestPRSplit(ctx context.Context, diffStats []git.DiffStats,
 		components = append(components, component)
 	}
 
+	const maxClusters = 20
+	if len(components) > maxClusters {
+		// Merge smallest clusters into an "other" bucket
+		sort.Slice(components, func(i, j int) bool {
+			return len(components[i]) > len(components[j])
+		})
+		var other []string
+		for i := maxClusters - 1; i < len(components); i++ {
+			other = append(other, components[i]...)
+		}
+		components = append(components[:maxClusters-1], other)
+	}
+
 	if len(components) <= 1 {
 		return &PRSplitSuggestion{
 			ShouldSplit: false,