refactor: decompose BuildContractCoverageFunc + dedup utilities

.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161910-yvonne-devlin.md

@@ -0,0 +1,10 @@
+---
+tag: crapload-decompose-pr2a
+author: yvonne-devlin
+category: pattern
+created_at: 2026-07-08T16:19:10Z
+identity: crapload-decompose-pr2a-20260708T161910-yvonne-devlin
+tier: draft
+---
+
+When decomposing a high-complexity Go function (CRAP 506, complexity 22) like BuildContractCoverageFunc, the most effective strategy is identifying distinct concerns interleaved in the same loop: effects discovery, coverage map construction, and reason computation were three independent concerns sharing a single for-loop iteration. Extracting each as a separate function with its own DI parameter reduced complexity from 22 to 7 in one PR. The key enabling pattern was that each helper could accept a function parameter (loadAndAnalyzeFn) or the existing contractCoverageDeps struct, allowing tests to inject synthetic data without loading real Go packages. This avoids the testing.Short() guard problem — tests contribute to CRAPload because they don't need the -short guard since they use synthetic data instead of calling packages.Load.

.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161917-yvonne-devlin.md

@@ -0,0 +1,10 @@
+---
+tag: crapload-decompose-pr2a
+author: yvonne-devlin
+category: pattern
+created_at: 2026-07-08T16:19:17Z
+identity: crapload-decompose-pr2a-20260708T161917-yvonne-devlin
+tier: draft
+---
+
+When deduplicating functions across packages (loadTestPackage in goprovider vs loadTestPackageForQuality in aireport), always compare the implementations line by line before choosing which to keep. In this case the goprovider version had pkg.Errors validation that the aireport version lacked — a latent bug. The aireport version would silently return packages with compilation errors, potentially producing incorrect quality data. By consolidating to the more robust copy and exporting it (goprovider.LoadTestPackage), we fixed a bug as a side effect of dedup. Similarly, the two copies of loadGazeConfigBestEffort were consolidated into config.LoadFromDir — the natural package for config-loading logic. When deciding where deduped functions should live, prefer the package that already has the supporting imports (goprovider already imported quality for HasTestSyntax) and avoid creating circular dependencies (loader couldn't import quality).

.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161923-yvonne-devlin.md

@@ -0,0 +1,10 @@
+---
+tag: crapload-decompose-pr2a
+author: yvonne-devlin
+category: gotcha
+created_at: 2026-07-08T16:19:23Z
+identity: crapload-decompose-pr2a-20260708T161923-yvonne-devlin
+tier: draft
+---
+
+The spec review council's most consistently raised finding across 5 reviewers for the crapload-decompose-pr2a change was the ambiguity in task 3.2's test cleanup instruction: "Update TestLoadGazeConfigBestEffort to reference config.LoadFromDir (or delete if the test only tested the local copy)." Four of five reviewers flagged the "or delete" phrasing as ambiguous. The fix was straightforward — change it to a definitive "Delete TestLoadGazeConfigBestEffort_AlwaysNonNil" since the function being tested is being removed. Lesson: task specs should be definitive, not conditional, especially for deletion/cleanup steps. When a function is being deleted, tests for it should always be deleted too (unless they test behavior preserved by the replacement), and the task should say so explicitly.

internal/aireport/runner_steps.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path/filepath"
 
 	"golang.org/x/tools/go/packages"
 
@@ -67,7 +66,7 @@ func resolveQualityDeps(deps []qualityPipelineDeps) qualityPipelineDeps {
 		d.classifyResults = runClassifyResults
 	}
 	if d.loadTestPkg == nil {
-		d.loadTestPkg = loadTestPackageForQuality
+		d.loadTestPkg = goprovider.LoadTestPackage
 	}
 	if d.assess == nil {
 		d.assess = quality.Assess
@@ -76,7 +75,7 @@ func resolveQualityDeps(deps []qualityPipelineDeps) qualityPipelineDeps {
 		d.resolveModulePkgs = resolveModulePackages
 	}
 	if d.loadConfig == nil {
-		d.loadConfig = loadGazeConfigBestEffort
+		d.loadConfig = config.LoadFromDir
 	}
 	return d
 }
@@ -301,7 +300,7 @@ func runClassifyStep(patterns []string, moduleDir string, deps ...qualityPipelin
 
 // runDocscanStep runs the documentation scanner and returns the JSON output.
 func runDocscanStep(moduleDir string) (json.RawMessage, error) {
-	cfg := loadGazeConfigBestEffort(moduleDir)
+	cfg := config.LoadFromDir(moduleDir)
 	scanOpts := docscan.ScanOptions{Config: cfg}
 
 	docs, err := docscan.Scan(moduleDir, scanOpts)
@@ -357,43 +356,3 @@ func resolveModulePackages(moduleDir string) []*packages.Package {
 	}
 	return modResult.Packages
 }
-
-// loadGazeConfigBestEffort loads the GazeConfig from the module root,
-// falling back to the default config on any error.
-func loadGazeConfigBestEffort(moduleDir string) *config.GazeConfig {
-	cfgPath := filepath.Join(moduleDir, ".gaze.yaml")
-	cfg, err := config.Load(cfgPath)
-	if err != nil {
-		return config.DefaultConfig()
-	}
-	return cfg
-}
-
-// loadTestPackageForQuality loads a Go package with test files included.
-func loadTestPackageForQuality(pkgPath string) (*packages.Package, error) {
-	cfg := &packages.Config{
-		Mode: packages.NeedName |
-			packages.NeedFiles |
-			packages.NeedCompiledGoFiles |
-			packages.NeedImports |
-			packages.NeedDeps |
-			packages.NeedTypes |
-			packages.NeedSyntax |
-			packages.NeedTypesInfo |
-			packages.NeedTypesSizes,
-		Tests: true,
-	}
-	pkgs, err := packages.Load(cfg, pkgPath)
-	if err != nil {
-		return nil, fmt.Errorf("loading test package: %w", err)
-	}
-	if len(pkgs) == 0 {
-		return nil, fmt.Errorf("no packages found for %q", pkgPath)
-	}
-	for _, pkg := range pkgs {
-		if quality.HasTestSyntax(pkg) {
-			return pkg, nil
-		}
-	}
-	return nil, fmt.Errorf("no test package found for %q", pkgPath)
-}

internal/aireport/runner_steps_test.go

@@ -16,15 +16,6 @@ import (
 	"github.com/unbound-force/gaze/internal/taxonomy"
 )
 
-// TestLoadGazeConfigBestEffort_AlwaysNonNil verifies that the function always
-// returns a non-nil config, even in a directory with no .gaze.yaml.
-func TestLoadGazeConfigBestEffort_AlwaysNonNil(t *testing.T) {
-	cfg := loadGazeConfigBestEffort(".")
-	if cfg == nil {
-		t.Error("expected non-nil GazeConfig from loadGazeConfigBestEffort")
-	}
-}
-
 // TestRunCRAPStep_RealPackage verifies that runCRAPStep successfully runs on
 // a real package and returns a non-nil JSON payload.
 // Guarded by testing.Short() — spawns the Go analysis pipeline.
@@ -542,34 +533,3 @@ func TestRunClassifyStep_DI_ClassifyErrorSkip(t *testing.T) {
 		t.Errorf("expected Contractual=1 (from good only), got %d", result.Contractual)
 	}
 }
-
-// ---------------------------------------------------------------------------
-// Task 2.5: Unit tests for loadTestPackageForQuality
-// ---------------------------------------------------------------------------
-
-func TestLoadTestPackageForQuality_WithTests(t *testing.T) {
-	pkg, err := loadTestPackageForQuality("github.com/unbound-force/gaze/internal/quality/testdata/src/welltested")
-	if err != nil {
-		t.Fatalf("expected success for package with tests, got error: %v", err)
-	}
-	if pkg == nil {
-		t.Fatal("expected non-nil package")
-	}
-}
-
-func TestLoadTestPackageForQuality_WithoutTests(t *testing.T) {
-	_, err := loadTestPackageForQuality("github.com/unbound-force/gaze/internal/analysis/testdata/src/returns")
-	if err == nil {
-		t.Fatal("expected error for package without tests")
-	}
-	if !strings.Contains(err.Error(), "no test package found") {
-		t.Errorf("expected 'no test package found' in error, got: %v", err)
-	}
-}
-
-func TestLoadTestPackageForQuality_NonExistent(t *testing.T) {
-	_, err := loadTestPackageForQuality("github.com/nonexistent/does-not-exist")
-	if err == nil {
-		t.Fatal("expected error for non-existent package")
-	}
-}

internal/config/config.go

@@ -5,6 +5,7 @@ package config
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"time"
 
 	"gopkg.in/yaml.v3"
@@ -142,6 +143,20 @@ func DefaultConfig() *GazeConfig {
 	}
 }
 
+// LoadFromDir loads the GazeConfig from the given module directory by
+// looking for .gaze.yaml. This is a best-effort loader: if the config
+// file is missing, malformed, or fails validation, it silently returns
+// DefaultConfig(). Use this when callers treat config as an
+// optimization hint rather than a hard requirement.
+func LoadFromDir(moduleDir string) *GazeConfig {
+	cfgPath := filepath.Join(moduleDir, ".gaze.yaml")
+	cfg, err := Load(cfgPath)
+	if err != nil {
+		return DefaultConfig()
+	}
+	return cfg
+}
+
 // Load reads a .gaze.yaml configuration file from the given path.
 // If the file does not exist, it returns DefaultConfig without error.
 // If the file exists but is invalid, it returns an error.

internal/config/config_test.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -350,6 +351,73 @@ func TestSC007_ConfigValidation_NegativeGazeCRAPThreshold(t *testing.T) {
 	}
 }
 
+// --- LoadFromDir tests ---
+
+func TestLoadFromDir_ValidConfig(t *testing.T) {
+	dir := t.TempDir()
+	yamlContent := []byte(`classification:
+  thresholds:
+    contractual: 90
+    incidental: 40
+`)
+	if err := os.WriteFile(filepath.Join(dir, ".gaze.yaml"), yamlContent, 0o644); err != nil {
+		t.Fatalf("writing .gaze.yaml: %v", err)
+	}
+
+	cfg := LoadFromDir(dir)
+	if cfg == nil {
+		t.Fatal("LoadFromDir returned nil")
+	}
+	if cfg.Classification.Thresholds.Contractual != 90 {
+		t.Errorf("contractual = %d, want 90",
+			cfg.Classification.Thresholds.Contractual)
+	}
+	if cfg.Classification.Thresholds.Incidental != 40 {
+		t.Errorf("incidental = %d, want 40",
+			cfg.Classification.Thresholds.Incidental)
+	}
+}
+
+func TestLoadFromDir_MissingFile(t *testing.T) {
+	dir := t.TempDir()
+
+	cfg := LoadFromDir(dir)
+	if cfg == nil {
+		t.Fatal("LoadFromDir returned nil for missing file")
+	}
+	// Should return defaults when no .gaze.yaml exists.
+	if cfg.Classification.Thresholds.Contractual != 80 {
+		t.Errorf("contractual = %d, want default 80",
+			cfg.Classification.Thresholds.Contractual)
+	}
+	if cfg.Classification.Thresholds.Incidental != 50 {
+		t.Errorf("incidental = %d, want default 50",
+			cfg.Classification.Thresholds.Incidental)
+	}
+}
+
+func TestLoadFromDir_InvalidYAML(t *testing.T) {
+	dir := t.TempDir()
+	badContent := []byte(`{{{not valid yaml at all!!!`)
+	if err := os.WriteFile(filepath.Join(dir, ".gaze.yaml"), badContent, 0o644); err != nil {
+		t.Fatalf("writing .gaze.yaml: %v", err)
+	}
+
+	cfg := LoadFromDir(dir)
+	if cfg == nil {
+		t.Fatal("LoadFromDir returned nil for invalid YAML")
+	}
+	// Should fall back to defaults when YAML is malformed.
+	if cfg.Classification.Thresholds.Contractual != 80 {
+		t.Errorf("contractual = %d, want default 80",
+			cfg.Classification.Thresholds.Contractual)
+	}
+	if cfg.Classification.Thresholds.Incidental != 50 {
+		t.Errorf("incidental = %d, want default 50",
+			cfg.Classification.Thresholds.Incidental)
+	}
+}
+
 func TestLoad_ZeroIncidental(t *testing.T) {
 	_, err := Load(filepath.Join("testdata", "zero-incidental.yaml"))
 	if err == nil {