diff --git a/.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161910-yvonne-devlin.md b/.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161910-yvonne-devlin.md new file mode 100644 index 0000000..79f62c1 --- /dev/null +++ b/.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. diff --git a/.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161917-yvonne-devlin.md b/.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161917-yvonne-devlin.md new file mode 100644 index 0000000..0846d17 --- /dev/null +++ b/.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). diff --git a/.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161923-yvonne-devlin.md b/.uf/dewey/learnings/crapload-decompose-pr2a-20260708T161923-yvonne-devlin.md new file mode 100644 index 0000000..0d821d7 --- /dev/null +++ b/.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. diff --git a/internal/aireport/runner_steps.go b/internal/aireport/runner_steps.go index 4bed837..2d6034c 100644 --- a/internal/aireport/runner_steps.go +++ b/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) -} diff --git a/internal/aireport/runner_steps_test.go b/internal/aireport/runner_steps_test.go index 7c9be05..6987fba 100644 --- a/internal/aireport/runner_steps_test.go +++ b/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") - } -} diff --git a/internal/config/config.go b/internal/config/config.go index 4f59cbe..cb8cf3c 100644 --- a/internal/config/config.go +++ b/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. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6818d62..e5f55bf 100644 --- a/internal/config/config_test.go +++ b/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 { diff --git a/internal/provider/goprovider/contract.go b/internal/provider/goprovider/contract.go index 1db6130..7a1cc38 100644 --- a/internal/provider/goprovider/contract.go +++ b/internal/provider/goprovider/contract.go @@ -7,7 +7,6 @@ package goprovider import ( "fmt" "io" - "path/filepath" "strings" "golang.org/x/tools/go/packages" @@ -110,109 +109,147 @@ func BuildContractCoverageFunc( } // Load config once for all packages. - gazeConfig := loadGazeConfigBestEffort(moduleDir) + gazeConfig := config.LoadFromDir(moduleDir) - // Build coverage map: "shortPkg:qualifiedName" -> coverage info. - coverageMap := make(map[string]crap.ContractCoverageInfo) - // effectsSet tracks functions that have >0 detected side effects, + // Build effects set: functions with >0 detected side effects, // regardless of whether they have test coverage. Used to - // distinguish "no_test_coverage" from "no_effects_detected" when - // a function is absent from the coverage map. - effectsSet := make(map[string]bool) - var degradedPkgs []string + // distinguish "no_test_coverage" from "no_effects_detected" + // when a function is absent from the coverage map. + effectsSet := buildEffectsSet(pkgPaths, nil) + + // Build coverage map via quality pipeline. + ccDeps := contractCoverageDeps{ + aiMapperFn: firstAIMapper(aiMapperFn), + } + coverageMap, degradedPkgs := buildCoverageMap(pkgPaths, moduleDir, gazeConfig, stderr, ccDeps) + + if len(coverageMap) == 0 && len(effectsSet) == 0 { + return nil, degradedPkgs + } + + _, _ = fmt.Fprintf(stderr, "quality pipeline complete: %d functions with coverage\n", len(coverageMap)) + + return func(pkg, function string) (crap.ContractCoverageInfo, bool) { + key := pkg + ":" + function + info, ok := coverageMap[key] + if ok { + return info, true + } + // Function not in coverage map — distinguish between + // "has effects but no test" and "no effects detected". + // Return ok=false so the CRAP pipeline excludes these from + // GazeCRAP calculations (no test = no coverage data, not + // 0% coverage). The Reason is informational for display. + if effectsSet[key] { + return crap.ContractCoverageInfo{Reason: "no_test_coverage"}, false + } + return crap.ContractCoverageInfo{Reason: "no_effects_detected"}, false + }, degradedPkgs +} +// buildEffectsSet runs side effect analysis on each package path and +// returns a set of "shortPkg:qualifiedName" keys for functions that +// have at least one detected side effect. Analysis errors for +// individual packages are silently skipped (best-effort). +func buildEffectsSet( + pkgPaths []string, + loadAndAnalyzeFn func(string, analysis.Options) ([]taxonomy.AnalysisResult, error), +) map[string]bool { + if loadAndAnalyzeFn == nil { + loadAndAnalyzeFn = analysis.LoadAndAnalyze + } + effectsSet := make(map[string]bool) for _, pkgPath := range pkgPaths { - // Build the effects set from analysis results before the - // quality pipeline runs. This captures functions with - // effects even when loadTestPackage fails (no tests). analysisOpts := analysis.Options{ IncludeUnexported: loader.IsMainPkg(pkgPath), } - analysisResults, analysisErr := analysis.LoadAndAnalyze(pkgPath, analysisOpts) - if analysisErr == nil { - for _, result := range analysisResults { - if len(result.SideEffects) > 0 { - shortPkg := extractShortPkgName(result.Target.Package) - key := shortPkg + ":" + result.Target.QualifiedName() - effectsSet[key] = true + results, err := loadAndAnalyzeFn(pkgPath, analysisOpts) + if err != nil { + continue + } + for _, result := range results { + if len(result.SideEffects) > 0 { + shortPkg := extractShortPkgName(result.Target.Package) + key := shortPkg + ":" + result.Target.QualifiedName() + effectsSet[key] = true + } + } + } + return effectsSet +} + +// computeCoverageReason constructs a ContractCoverageInfo from a +// quality report. It sets the Percentage from contract coverage data +// and computes the Reason field when no contractual effects exist. +func computeCoverageReason(report taxonomy.QualityReport) crap.ContractCoverageInfo { + info := crap.ContractCoverageInfo{ + Percentage: report.ContractCoverage.Percentage, + } + if report.ContractCoverage.TotalContractual == 0 { + minConf, maxConf := 100, 0 + effectCount := 0 + for _, e := range report.AmbiguousEffects { + if e.Classification != nil { + effectCount++ + if e.Classification.Confidence < minConf { + minConf = e.Classification.Confidence + } + if e.Classification.Confidence > maxConf { + maxConf = e.Classification.Confidence } } } + if effectCount > 0 { + info.Reason = "all_effects_ambiguous" + info.MinConfidence = minConf + info.MaxConfidence = maxConf + } else { + info.Reason = "no_effects_detected" + } + } + return info +} + +// buildCoverageMap runs the quality pipeline on each package path and +// returns a coverage map and list of SSA-degraded package paths. The +// map keys are "shortPkg:qualifiedName" and values are +// ContractCoverageInfo computed via computeCoverageReason. When +// multiple reports exist for the same function, the entry with the +// higher Percentage wins. Reports with empty TargetFunction.Function +// (degraded) are skipped. +func buildCoverageMap( + pkgPaths []string, + moduleDir string, + gazeConfig *config.GazeConfig, + stderr io.Writer, + deps ...contractCoverageDeps, +) (map[string]crap.ContractCoverageInfo, []string) { + coverageMap := make(map[string]crap.ContractCoverageInfo) + var degradedPkgs []string + for _, pkgPath := range pkgPaths { var ccDeps contractCoverageDeps - if len(aiMapperFn) > 0 && aiMapperFn[0] != nil { - ccDeps.aiMapperFn = aiMapperFn[0] + if len(deps) > 0 { + ccDeps = deps[0] } reports, degradedPkg := analyzePackageCoverage(pkgPath, moduleDir, gazeConfig, stderr, ccDeps) if degradedPkg != "" { degradedPkgs = append(degradedPkgs, degradedPkg) } for _, report := range reports { - // Skip degraded reports — they have zero-valued - // TargetFunction and would create phantom entries - // with empty-string keys in the coverage map. if report.TargetFunction.Function == "" { continue } shortPkg := extractShortPkgName(report.TargetFunction.Package) key := shortPkg + ":" + report.TargetFunction.QualifiedName() - info := crap.ContractCoverageInfo{ - Percentage: report.ContractCoverage.Percentage, - } - - // Compute coverage reason from classification data. - if report.ContractCoverage.TotalContractual == 0 { - minConf, maxConf := 100, 0 - effectCount := 0 - for _, e := range report.AmbiguousEffects { - if e.Classification != nil { - effectCount++ - if e.Classification.Confidence < minConf { - minConf = e.Classification.Confidence - } - if e.Classification.Confidence > maxConf { - maxConf = e.Classification.Confidence - } - } - } - if effectCount > 0 { - info.Reason = "all_effects_ambiguous" - info.MinConfidence = minConf - info.MaxConfidence = maxConf - } else { - info.Reason = "no_effects_detected" - } - } - + info := computeCoverageReason(report) if existing, ok := coverageMap[key]; !ok || info.Percentage > existing.Percentage { coverageMap[key] = info } } } - - if len(coverageMap) == 0 && len(effectsSet) == 0 { - return nil, degradedPkgs - } - - _, _ = fmt.Fprintf(stderr, "quality pipeline complete: %d functions with coverage\n", len(coverageMap)) - - return func(pkg, function string) (crap.ContractCoverageInfo, bool) { - key := pkg + ":" + function - info, ok := coverageMap[key] - if ok { - return info, true - } - // Function not in coverage map — distinguish between - // "has effects but no test" and "no effects detected". - // Return ok=false so the CRAP pipeline excludes these from - // GazeCRAP calculations (no test = no coverage data, not - // 0% coverage). The Reason is informational for display. - if effectsSet[key] { - return crap.ContractCoverageInfo{Reason: "no_test_coverage"}, false - } - return crap.ContractCoverageInfo{Reason: "no_effects_detected"}, false - }, degradedPkgs + return coverageMap, degradedPkgs } // contractCoverageDeps holds injectable dependencies for @@ -253,7 +290,7 @@ func analyzePackageCoverage( d.classifyResults = classifyResults } if d.loadTestPkg == nil { - d.loadTestPkg = loadTestPackage + d.loadTestPkg = LoadTestPackage } if d.assess == nil { d.assess = quality.Assess @@ -333,9 +370,9 @@ func classifyResults( return classify.Classify(results, clOpts) } -// loadTestPackage loads a Go package with test files for quality +// LoadTestPackage loads a Go package with test files for quality // assessment. -func loadTestPackage(pkgPath string) (*packages.Package, error) { +func LoadTestPackage(pkgPath string) (*packages.Package, error) { cfg := &packages.Config{ Mode: packages.NeedName | packages.NeedFiles | @@ -384,15 +421,13 @@ func loadTestPackage(pkgPath string) (*packages.Package, error) { return nil, fmt.Errorf("no test files found for %q", pkgPath) } -// 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() +// firstAIMapper extracts the first non-nil AIMapperFunc from a +// variadic slice, returning nil if none is provided. +func firstAIMapper(fns []quality.AIMapperFunc) quality.AIMapperFunc { + if len(fns) > 0 && fns[0] != nil { + return fns[0] } - return cfg + return nil } // extractShortPkgName returns the short package name from a full diff --git a/internal/provider/goprovider/contract_internal_test.go b/internal/provider/goprovider/contract_internal_test.go index 36afd81..4a9e27a 100644 --- a/internal/provider/goprovider/contract_internal_test.go +++ b/internal/provider/goprovider/contract_internal_test.go @@ -318,11 +318,296 @@ func TestAnalyzePackageCoverage_DI_SSADegraded(t *testing.T) { } // --------------------------------------------------------------------------- -// loadTestPackage tests (Task 1.3) +// computeCoverageReason tests (Task 2.1) +// --------------------------------------------------------------------------- + +func TestComputeCoverageReason_WithContractualEffects(t *testing.T) { + report := taxonomy.QualityReport{ + ContractCoverage: taxonomy.ContractCoverage{ + Percentage: 75.0, + TotalContractual: 3, + }, + } + info := computeCoverageReason(report) + if info.Percentage != 75.0 { + t.Errorf("Percentage = %.1f, want 75.0", info.Percentage) + } + if info.Reason != "" { + t.Errorf("Reason = %q, want empty string", info.Reason) + } +} + +func TestComputeCoverageReason_AllAmbiguous(t *testing.T) { + report := taxonomy.QualityReport{ + ContractCoverage: taxonomy.ContractCoverage{ + TotalContractual: 0, + }, + AmbiguousEffects: []taxonomy.SideEffect{ + {Classification: &taxonomy.Classification{Confidence: 78}}, + {Classification: &taxonomy.Classification{Confidence: 79}}, + }, + } + info := computeCoverageReason(report) + if info.Reason != "all_effects_ambiguous" { + t.Errorf("Reason = %q, want %q", info.Reason, "all_effects_ambiguous") + } + if info.MinConfidence != 78 { + t.Errorf("MinConfidence = %d, want 78", info.MinConfidence) + } + if info.MaxConfidence != 79 { + t.Errorf("MaxConfidence = %d, want 79", info.MaxConfidence) + } +} + +func TestComputeCoverageReason_NoEffects(t *testing.T) { + report := taxonomy.QualityReport{ + ContractCoverage: taxonomy.ContractCoverage{ + TotalContractual: 0, + }, + AmbiguousEffects: []taxonomy.SideEffect{}, + } + info := computeCoverageReason(report) + if info.Reason != "no_effects_detected" { + t.Errorf("Reason = %q, want %q", info.Reason, "no_effects_detected") + } +} + +func TestComputeCoverageReason_NilClassification(t *testing.T) { + report := taxonomy.QualityReport{ + ContractCoverage: taxonomy.ContractCoverage{ + TotalContractual: 0, + }, + AmbiguousEffects: []taxonomy.SideEffect{ + {Classification: nil}, + {Classification: nil}, + }, + } + info := computeCoverageReason(report) + // Nil classifications are skipped; with no valid entries, + // effectCount == 0, so reason should be "no_effects_detected". + if info.Reason != "no_effects_detected" { + t.Errorf("Reason = %q, want %q", info.Reason, "no_effects_detected") + } + if info.MinConfidence != 0 { + t.Errorf("MinConfidence = %d, want 0", info.MinConfidence) + } + if info.MaxConfidence != 0 { + t.Errorf("MaxConfidence = %d, want 0", info.MaxConfidence) + } +} + +// --------------------------------------------------------------------------- +// buildEffectsSet tests (Task 2.2) +// --------------------------------------------------------------------------- + +func TestBuildEffectsSet_WithEffects(t *testing.T) { + fakeFn := func(_ string, _ analysis.Options) ([]taxonomy.AnalysisResult, error) { + return []taxonomy.AnalysisResult{syntheticResult()}, nil + } + got := buildEffectsSet([]string{"example.com/pkg"}, fakeFn) + if !got["pkg:DoWork"] { + t.Errorf("expected key %q in effects set, got %v", "pkg:DoWork", got) + } +} + +func TestBuildEffectsSet_NoEffects(t *testing.T) { + fakeFn := func(_ string, _ analysis.Options) ([]taxonomy.AnalysisResult, error) { + return []taxonomy.AnalysisResult{ + { + Target: taxonomy.FunctionTarget{ + Package: "example.com/pkg", + Function: "NoOp", + }, + SideEffects: []taxonomy.SideEffect{}, // no effects + }, + }, nil + } + got := buildEffectsSet([]string{"example.com/pkg"}, fakeFn) + if len(got) != 0 { + t.Errorf("expected empty effects set, got %v", got) + } +} + +func TestBuildEffectsSet_AnalysisError(t *testing.T) { + fakeFn := func(_ string, _ analysis.Options) ([]taxonomy.AnalysisResult, error) { + return nil, errors.New("analysis failed") + } + got := buildEffectsSet([]string{"example.com/broken"}, fakeFn) + if len(got) != 0 { + t.Errorf("expected empty effects set on error, got %v", got) + } +} + +func TestBuildEffectsSet_EmptyPaths(t *testing.T) { + fakeFn := func(_ string, _ analysis.Options) ([]taxonomy.AnalysisResult, error) { + t.Fatal("loadAndAnalyze should not be called with empty paths") + return nil, nil + } + got := buildEffectsSet([]string{}, fakeFn) + if len(got) != 0 { + t.Errorf("expected empty effects set for empty paths, got %v", got) + } +} + +// --------------------------------------------------------------------------- +// buildCoverageMap tests (Task 2.3) +// --------------------------------------------------------------------------- + +func TestBuildCoverageMap_Success(t *testing.T) { + var stderr bytes.Buffer + deps := successDeps() + coverageMap, degradedPkgs := buildCoverageMap( + []string{"example.com/pkg"}, ".", config.DefaultConfig(), &stderr, deps, + ) + if len(coverageMap) != 1 { + t.Fatalf("expected 1 entry in coverage map, got %d", len(coverageMap)) + } + info, ok := coverageMap["pkg:DoWork"] + if !ok { + t.Fatal("expected key \"pkg:DoWork\" in coverage map") + } + if info.Percentage != 75.0 { + t.Errorf("Percentage = %.1f, want 75.0", info.Percentage) + } + if len(degradedPkgs) != 0 { + t.Errorf("expected no degraded packages, got %v", degradedPkgs) + } +} + +func TestBuildCoverageMap_DegradedReport(t *testing.T) { + deps := successDeps() + deps.assess = func(_ []taxonomy.AnalysisResult, _ *packages.Package, _ quality.Options) ([]taxonomy.QualityReport, *taxonomy.PackageSummary, error) { + return []taxonomy.QualityReport{ + { + TestFunction: "TestDoWork", + TargetFunction: taxonomy.FunctionTarget{ + Package: "example.com/pkg", + Function: "", // degraded — empty function name + }, + ContractCoverage: taxonomy.ContractCoverage{ + Percentage: 50.0, + }, + }, + }, &taxonomy.PackageSummary{TotalTests: 1}, nil + } + var stderr bytes.Buffer + coverageMap, _ := buildCoverageMap( + []string{"example.com/pkg"}, ".", config.DefaultConfig(), &stderr, deps, + ) + if len(coverageMap) != 0 { + t.Errorf("expected empty coverage map for degraded report, got %d entries", len(coverageMap)) + } +} + +func TestBuildCoverageMap_SSADegradation(t *testing.T) { + deps := successDeps() + deps.assess = func(_ []taxonomy.AnalysisResult, _ *packages.Package, _ quality.Options) ([]taxonomy.QualityReport, *taxonomy.PackageSummary, error) { + reports := []taxonomy.QualityReport{ + { + TestFunction: "TestDoWork", + TargetFunction: taxonomy.FunctionTarget{ + Package: "example.com/pkg", + Function: "DoWork", + }, + }, + } + summary := &taxonomy.PackageSummary{ + TotalTests: 1, + SSADegraded: true, + } + return reports, summary, nil + } + var stderr bytes.Buffer + _, degradedPkgs := buildCoverageMap( + []string{"example.com/pkg"}, ".", config.DefaultConfig(), &stderr, deps, + ) + if len(degradedPkgs) != 1 || degradedPkgs[0] != "example.com/pkg" { + t.Errorf("expected degraded packages [\"example.com/pkg\"], got %v", degradedPkgs) + } +} + +func TestBuildCoverageMap_HigherCoverageWins(t *testing.T) { + // Helper that builds deps returning two reports for the same function + // in the given percentage order. + makeDeps := func(first, second float64) contractCoverageDeps { + deps := successDeps() + deps.assess = func(_ []taxonomy.AnalysisResult, _ *packages.Package, _ quality.Options) ([]taxonomy.QualityReport, *taxonomy.PackageSummary, error) { + return []taxonomy.QualityReport{ + { + TestFunction: "TestDoWork_A", + TargetFunction: taxonomy.FunctionTarget{ + Package: "example.com/pkg", + Function: "DoWork", + }, + ContractCoverage: taxonomy.ContractCoverage{ + Percentage: first, + TotalContractual: 2, + }, + }, + { + TestFunction: "TestDoWork_B", + TargetFunction: taxonomy.FunctionTarget{ + Package: "example.com/pkg", + Function: "DoWork", + }, + ContractCoverage: taxonomy.ContractCoverage{ + Percentage: second, + TotalContractual: 2, + }, + }, + }, &taxonomy.PackageSummary{TotalTests: 2}, nil + } + return deps + } + + // Order 1: 50 then 80 → 80 wins. + var stderr bytes.Buffer + coverageMap, _ := buildCoverageMap( + []string{"example.com/pkg"}, ".", config.DefaultConfig(), &stderr, + makeDeps(50.0, 80.0), + ) + info, ok := coverageMap["pkg:DoWork"] + if !ok { + t.Fatal("expected key \"pkg:DoWork\" in coverage map") + } + if info.Percentage != 80.0 { + t.Errorf("order 50→80: Percentage = %.1f, want 80.0", info.Percentage) + } + + // Order 2: 80 then 50 → 80 still wins. + stderr.Reset() + coverageMap, _ = buildCoverageMap( + []string{"example.com/pkg"}, ".", config.DefaultConfig(), &stderr, + makeDeps(80.0, 50.0), + ) + info, ok = coverageMap["pkg:DoWork"] + if !ok { + t.Fatal("expected key \"pkg:DoWork\" in coverage map") + } + if info.Percentage != 80.0 { + t.Errorf("order 80→50: Percentage = %.1f, want 80.0", info.Percentage) + } +} + +func TestBuildCoverageMap_EmptyPaths(t *testing.T) { + var stderr bytes.Buffer + coverageMap, degradedPkgs := buildCoverageMap( + []string{}, ".", config.DefaultConfig(), &stderr, + ) + if len(coverageMap) != 0 { + t.Errorf("expected empty coverage map for empty paths, got %d entries", len(coverageMap)) + } + if len(degradedPkgs) != 0 { + t.Errorf("expected no degraded packages, got %v", degradedPkgs) + } +} + +// --------------------------------------------------------------------------- +// LoadTestPackage tests (Task 1.3) // --------------------------------------------------------------------------- func TestLoadTestPackage_WithTests(t *testing.T) { - pkg, err := loadTestPackage("github.com/unbound-force/gaze/internal/quality/testdata/src/welltested") + pkg, err := LoadTestPackage("github.com/unbound-force/gaze/internal/quality/testdata/src/welltested") if err != nil { t.Fatalf("expected no error for package with tests, got: %v", err) } @@ -332,7 +617,7 @@ func TestLoadTestPackage_WithTests(t *testing.T) { } func TestLoadTestPackage_WithoutTests(t *testing.T) { - _, err := loadTestPackage("github.com/unbound-force/gaze/internal/analysis/testdata/src/returns") + _, err := LoadTestPackage("github.com/unbound-force/gaze/internal/analysis/testdata/src/returns") if err == nil { t.Fatal("expected error for package without test files") } @@ -342,7 +627,7 @@ func TestLoadTestPackage_WithoutTests(t *testing.T) { } func TestLoadTestPackage_NonExistent(t *testing.T) { - _, err := loadTestPackage("github.com/nonexistent/does-not-exist") + _, err := LoadTestPackage("github.com/nonexistent/does-not-exist") if err == nil { t.Fatal("expected error for non-existent package") } diff --git a/openspec/changes/crapload-decompose-pr2a/.openspec.yaml b/openspec/changes/crapload-decompose-pr2a/.openspec.yaml new file mode 100644 index 0000000..6140c2d --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2a/.openspec.yaml @@ -0,0 +1,2 @@ +schema: unbound-force +created: 2026-07-08 diff --git a/openspec/changes/crapload-decompose-pr2a/design.md b/openspec/changes/crapload-decompose-pr2a/design.md new file mode 100644 index 0000000..2759d16 --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2a/design.md @@ -0,0 +1,166 @@ +## Context + +`BuildContractCoverageFunc` (complexity 22, CRAP 506) in +`internal/provider/goprovider/contract.go` is the worst CRAPload offender. +It interleaves three concerns: effects discovery, coverage map construction +with reason computation, and closure assembly. Two utility functions +(`loadTestPackage`, `loadGazeConfigBestEffort`) are duplicated across +`goprovider` and `aireport`. + +Phase 1 (PRs #183, #187) reduced CRAPload from 38 to 30 via DI and tests +on simpler functions. This phase targets the worst remaining function and +eliminates cross-package duplication. + +### Current structure (contract.go lines 96–216) + +``` +BuildContractCoverageFunc(patterns, moduleDir, stderr, aiMapperFn) + ├─ ResolvePackagePaths(patterns, moduleDir) + ├─ loadGazeConfigBestEffort(moduleDir) ← DUPLICATED in aireport + ├─ for each pkgPath: + │ ├─ analysis.LoadAndAnalyze(pkgPath) ← effects discovery + │ │ └─ populate effectsSet[key] = true + │ ├─ analyzePackageCoverage(pkgPath, ...) ← quality pipeline + │ └─ for each report: ← coverage map build + │ ├─ skip degraded (empty Function) + │ ├─ compute ContractCoverageInfo ← reason computation + │ │ └─ ambiguous confidence range loop + │ └─ coverageMap[key] = info + ├─ early return if empty + └─ return closure(pkg, function): + ├─ check coverageMap + ├─ check effectsSet → "no_test_coverage" + └─ default → "no_effects_detected" +``` + +## Goals / Non-Goals + +### Goals +- Reduce `BuildContractCoverageFunc` complexity from 22 to ~6–8 +- Make each extracted concern independently testable with synthetic data +- Eliminate `loadTestPackageForQuality` (fixes missing `pkg.Errors` bug) +- Eliminate duplicate `loadGazeConfigBestEffort` across two packages +- All new tests run without `testing.Short()` guard (CRAPload-visible) + +### Non-Goals +- Unifying the `classifyResults` signature divergence between + `contractCoverageDeps` and `qualityPipelineDeps` (acknowledged design + decision D2 from PR #183 — out of scope) +- Decomposing `analyzePackageCoverage` further (already has DI + 9 tests) +- Changing the `GoContractCoverageProvider` interface or `Build()` method +- Reducing complexity of other Phase 2 targets (`matchContainerUnwrap`, + `detectASTReceiverMutations`, etc.) + +## Decisions + +### D1: Extract 3 helpers from `BuildContractCoverageFunc` + +**Decision**: Decompose into `buildEffectsSet`, `buildCoverageMap`, and +`computeCoverageReason`. + +**Rationale**: Each corresponds to a distinct concern: +1. `buildEffectsSet` — data acquisition (side effect analysis per package) +2. `buildCoverageMap` — data transformation (quality reports → coverage info) +3. `computeCoverageReason` — value construction (report → CoverageInfo) + +`computeCoverageReason` is a pure function (no I/O, no package loading). The +other two need DI for testability but are logically self-contained. + +**Alternatives rejected**: +- *2 helpers only* (merge reason computation into buildCoverageMap): Keeps + complexity higher in `buildCoverageMap` and makes the reason logic harder + to test in isolation. +- *Full DI struct on BuildContractCoverageFunc*: Over-engineered — the + function already delegates to `analyzePackageCoverage` which has its own + DI. Adding another DI layer creates indirection without proportional + benefit. + +### D2: Export `LoadTestPackage` from goprovider + +**Decision**: Export the existing `loadTestPackage` as +`goprovider.LoadTestPackage` and have `aireport` import it. + +**Rationale**: The goprovider version has `pkg.Errors` validation that the +aireport version lacks. `goprovider` already imports `quality` (for +`HasTestSyntax`), so no new dependency edges are introduced. The function +signature is unchanged: `func(string) (*packages.Package, error)`. + +**Alternatives rejected**: +- *Move to `loader` package*: Would create circular dependency + (`loader` → `quality` for `HasTestSyntax`). +- *New `testloader` package*: Unnecessary package proliferation for a single + function. +- *Callback parameter for HasTestSyntax*: Over-abstracted — `HasTestSyntax` + is a stable API unlikely to change. + +### D3: Add `config.LoadFromDir` to the config package + +**Decision**: Add `config.LoadFromDir(moduleDir string) *GazeConfig` that +constructs the `.gaze.yaml` path, calls `config.Load`, and returns +`DefaultConfig()` on any error. Replace both private copies. + +**Rationale**: `config.Load` already handles the missing-file case (returns +`DefaultConfig()` when file doesn't exist). `LoadFromDir` adds the +directory→path join and the error-swallowing behavior. This belongs in the +config package because it's config-loading logic, not package-loading logic. + +**Signature**: +```go +// LoadFromDir loads the GazeConfig from the given module directory, +// looking for .gaze.yaml. Returns DefaultConfig() on any error. +func LoadFromDir(moduleDir string) *GazeConfig +``` + +### D4: DI approach for `buildEffectsSet` and `buildCoverageMap` + +**Decision**: Use the existing `contractCoverageDeps` struct. The +`buildEffectsSet` helper accepts a `loadAndAnalyze` function parameter +directly (it only needs one dependency). `buildCoverageMap` accepts the +full `contractCoverageDeps` (it delegates to `analyzePackageCoverage` +which already uses it). + +**Rationale**: Follows the established nil-means-default DI pattern from +Phase 1a (PR #183). No new DI struct needed — reuse existing one. + +### D5: `computeCoverageReason` — pure function, no DI + +**Decision**: `computeCoverageReason` takes a `taxonomy.QualityReport` and +returns `crap.ContractCoverageInfo`. No DI needed. + +**Rationale**: The function only reads struct fields and computes derived +values. No I/O, no package loading, no external dependencies. Test directly +with struct literals. + +## Risks / Trade-offs + +### Risk: aireport now imports goprovider + +The `loadTestPackage` dedup creates a new import edge: +`internal/aireport` → `internal/provider/goprovider`. Currently aireport +already imports goprovider (line 18: `runner_steps.go`), so this is an +existing edge, not a new one. **No new coupling introduced.** + +### Risk: Error message changes in test assertions + +The `loadTestPackageForQuality` error messages differ slightly from +`LoadTestPackage` (e.g., `"no test package found"` vs `"no test files +found"`). Tests in `runner_steps_test.go` that assert on specific error +strings will need updating. **Low risk — mechanical change.** + +### Trade-off: Extracted helpers are package-internal + +The 3 new helpers (`buildEffectsSet`, `buildCoverageMap`, +`computeCoverageReason`) remain unexported in the `goprovider` package. +They exist for decomposition and testability, not for external +consumption. This keeps the public API surface unchanged. + +### Trade-off: `buildEffectsSet` duplicates the analysis call + +`BuildContractCoverageFunc` calls `analysis.LoadAndAnalyze` twice per +package: once in `buildEffectsSet` and once inside +`analyzePackageCoverage` (via `buildCoverageMap`). This duplication +exists in the current code and is intentional — the effects set must be +built even when `loadTestPackage` fails (no tests), while +`analyzePackageCoverage` short-circuits on test-load failure. Merging +these would couple the two concerns. The performance cost is negligible +(SSA build is cached by `go/packages`). diff --git a/openspec/changes/crapload-decompose-pr2a/proposal.md b/openspec/changes/crapload-decompose-pr2a/proposal.md new file mode 100644 index 0000000..8766ffc --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2a/proposal.md @@ -0,0 +1,116 @@ +## Why + +`BuildContractCoverageFunc` in `internal/provider/goprovider/contract.go` has +complexity 22 and CRAP 506 (0% line coverage) — the single worst CRAPload +offender in the codebase. Its complexity comes from interleaving three concerns +in one function: effects discovery, coverage map construction with reason +computation, and the returned closure's fallback logic. + +Additionally, two utility functions are duplicated across `goprovider` and +`aireport`: +- `loadTestPackage` / `loadTestPackageForQuality` — near-identical test package + loaders (the aireport copy is missing `pkg.Errors` validation, a latent bug) +- `loadGazeConfigBestEffort` — identical config-load-with-fallback in both + packages + +This is Phase 2a of issue #166 (CRAPload fragility reduction). Phases 1a (#183) +and 1b (#187) reduced CRAPload from 38 to 30. This phase addresses the worst +remaining offender and eliminates cross-package duplication introduced during +the provider-interfaces refactoring. + +## What Changes + +### Decomposition + +Extract three pure/near-pure helpers from `BuildContractCoverageFunc`: + +1. **`buildEffectsSet`** — per-package analysis loop that populates the + effects tracking set (currently lines 124–140) +2. **`buildCoverageMap`** — per-package quality pipeline + report-to-coverage + conversion (currently lines 142–192) +3. **`computeCoverageReason`** — per-report `ContractCoverageInfo` construction + including ambiguous-effects confidence range (currently lines 160–190) + +After extraction, `BuildContractCoverageFunc` becomes a thin orchestrator +(~30 lines): resolve paths → build effects set → build coverage map → +construct closure. + +### Deduplication + +1. Export `loadTestPackage` as `goprovider.LoadTestPackage` and replace + `loadTestPackageForQuality` in aireport with a reference to the exported + function. Fixes the missing `pkg.Errors` validation in the aireport copy. +2. Add `config.LoadFromDir(moduleDir string) *GazeConfig` to the config + package. Replace both private `loadGazeConfigBestEffort` copies. + +## Capabilities + +### New Capabilities +- `config.LoadFromDir`: Best-effort config loading by directory path (joins + `.gaze.yaml`, falls back to `DefaultConfig()` on any error) +- `goprovider.LoadTestPackage`: Exported test package loader with `pkg.Errors` + validation (previously unexported) + +### Modified Capabilities +- `BuildContractCoverageFunc`: Identical behavior, reduced complexity (22 → ~6–8) + via helper extraction. No signature or return value changes. + +### Removed Capabilities +- `loadTestPackageForQuality` (aireport, unexported): Replaced by + `goprovider.LoadTestPackage` +- `loadGazeConfigBestEffort` (goprovider + aireport, both unexported): Replaced + by `config.LoadFromDir` + +## Impact + +- **Files modified**: `internal/provider/goprovider/contract.go`, + `internal/aireport/runner_steps.go`, `internal/config/config.go` +- **Test files modified**: `contract_internal_test.go`, + `runner_steps_test.go`, `config_test.go` +- **No API surface changes**: All modified functions are internal (unexported + or package-internal). `GoContractCoverageProvider.Build()` and + `BuildContractCoverageFunc` retain identical signatures and behavior. +- **No behavior changes**: Pure refactoring + dedup. All existing tests must + pass without modification (error message strings may change for dedup). +- **CRAPload impact**: `BuildContractCoverageFunc` CRAP drops from 506 to + <30 (helpers get their own test coverage). CRAPload expected: 30 → ~29. + +## Constitution Alignment + +Assessed against the Gaze project constitution (v1.3.0). + +### I. Accuracy + +**Assessment**: PASS + +No analysis logic changes. Side effect detection, classification, and +contract coverage computation are unchanged. The three extracted helpers +preserve identical data flow. Existing regression tests verify accuracy +is maintained. + +### II. Minimal Assumptions + +**Assessment**: N/A + +This change does not alter assumptions about host projects, test frameworks, +or coding styles. The `loadTestPackage` dedup fixes a missing `pkg.Errors` +check — strictly an improvement in assumption enforcement. + +### III. Actionable Output + +**Assessment**: N/A + +No changes to output formats, report content, or metric computation. The +`ContractCoverageInfo` struct and its `Reason`/confidence fields are +constructed identically by the extracted `computeCoverageReason` helper. + +### IV. Testability + +**Assessment**: PASS + +This change directly improves testability. Three extracted helpers are +pure or near-pure functions testable with synthetic data (no `go/packages` +loading required). The `testing.Short()` constraint is respected — all new +tests run without `-short` guard so they contribute to CRAPload coverage. +Coverage strategy: unit tests on each helper with synthetic inputs covering +all branches. diff --git a/openspec/changes/crapload-decompose-pr2a/specs/decompose-dedup.md b/openspec/changes/crapload-decompose-pr2a/specs/decompose-dedup.md new file mode 100644 index 0000000..fb59167 --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2a/specs/decompose-dedup.md @@ -0,0 +1,204 @@ +## ADDED Requirements + +### Requirement: computeCoverageReason helper + +`computeCoverageReason` MUST be a pure function that takes a single +`taxonomy.QualityReport` and returns a `crap.ContractCoverageInfo` with +the correct `Percentage`, `Reason`, `MinConfidence`, and `MaxConfidence` +fields. + +The function MUST produce identical output to the inline logic it replaces +(lines 160–190 of `BuildContractCoverageFunc`). + +#### Scenario: Report with contractual effects + +- **GIVEN** a `QualityReport` with `ContractCoverage.TotalContractual > 0` + and `Percentage = 75.0` +- **WHEN** `computeCoverageReason` is called +- **THEN** the returned `ContractCoverageInfo` SHALL have `Percentage = 75.0` + and `Reason = ""` (empty — no special reason needed) + +#### Scenario: Report with all ambiguous effects + +- **GIVEN** a `QualityReport` with `ContractCoverage.TotalContractual == 0` + and `AmbiguousEffects` containing 2 entries with confidences 78 and 79 +- **WHEN** `computeCoverageReason` is called +- **THEN** the returned `ContractCoverageInfo` SHALL have + `Reason = "all_effects_ambiguous"`, `MinConfidence = 78`, + `MaxConfidence = 79` + +#### Scenario: Report with no effects detected + +- **GIVEN** a `QualityReport` with `ContractCoverage.TotalContractual == 0` + and empty `AmbiguousEffects` +- **WHEN** `computeCoverageReason` is called +- **THEN** the returned `ContractCoverageInfo` SHALL have + `Reason = "no_effects_detected"` + +#### Scenario: AmbiguousEffects with nil Classification + +- **GIVEN** a `QualityReport` with `ContractCoverage.TotalContractual == 0` + and `AmbiguousEffects` containing entries where `Classification` is nil +- **WHEN** `computeCoverageReason` is called +- **THEN** entries with nil `Classification` MUST be skipped in confidence + range computation + +### Requirement: buildEffectsSet helper + +`buildEffectsSet` MUST iterate over package paths, run side effect analysis +on each, and return a `map[string]bool` of `"shortPkg:qualifiedName"` keys +for functions with one or more detected side effects. + +The function MUST accept a `loadAndAnalyze` function parameter for +dependency injection. When nil, it MUST default to +`analysis.LoadAndAnalyze`. + +Analysis errors for individual packages MUST be silently skipped (best-effort +semantics, matching current behavior). + +#### Scenario: Package with effects + +- **GIVEN** a `loadAndAnalyze` function that returns results with side effects +- **WHEN** `buildEffectsSet` is called +- **THEN** the returned map SHALL contain the key `"pkg:DoWork"` for a + function `DoWork` in package `example.com/pkg` (key format: + `"shortPkg:qualifiedName"`) + +#### Scenario: Package with analysis error + +- **GIVEN** a `loadAndAnalyze` function that returns an error +- **WHEN** `buildEffectsSet` is called +- **THEN** the package MUST be silently skipped and the returned map SHALL + not contain entries for that package + +#### Scenario: Empty package paths + +- **GIVEN** an empty `pkgPaths` slice +- **WHEN** `buildEffectsSet` is called +- **THEN** the returned map SHALL be empty (not nil) + +### Requirement: buildCoverageMap helper + +`buildCoverageMap` MUST iterate over package paths, run the quality pipeline +via `analyzePackageCoverage` on each, convert reports to +`ContractCoverageInfo` entries using `computeCoverageReason`, and return +the coverage map and degraded packages list. + +The function MUST skip reports with empty `TargetFunction.Function` (degraded +reports) to prevent phantom entries. + +When multiple reports exist for the same function, the entry with the higher +`Percentage` MUST win. + +#### Scenario: Single package with coverage data + +- **GIVEN** a package path where `analyzePackageCoverage` returns reports +- **WHEN** `buildCoverageMap` is called +- **THEN** the returned map SHALL contain entries keyed by + `"shortPkg:qualifiedName"` with correct coverage info + +#### Scenario: Degraded report skipping + +- **GIVEN** a report with `TargetFunction.Function == ""` +- **WHEN** `buildCoverageMap` processes the report +- **THEN** the report MUST be skipped and not added to the coverage map + +#### Scenario: SSA degradation tracking + +- **GIVEN** a package where `analyzePackageCoverage` returns a non-empty + degraded package path +- **WHEN** `buildCoverageMap` is called +- **THEN** the degraded package path SHALL be included in the returned + `degradedPkgs` slice + +#### Scenario: Higher-coverage wins + +- **GIVEN** two reports for the same function, encountered in either order + (50.0 then 80.0, or 80.0 then 50.0) +- **WHEN** `buildCoverageMap` processes both +- **THEN** the coverage map entry SHALL have `Percentage = 80.0` regardless + of encounter order + +### Requirement: config.LoadFromDir + +`config.LoadFromDir` MUST accept a module directory path, construct the +`.gaze.yaml` path via `filepath.Join(moduleDir, ".gaze.yaml")`, call +`config.Load`, and return the result. On any error from `config.Load`, +it MUST return `DefaultConfig()`. + +#### Scenario: Valid directory with .gaze.yaml + +- **GIVEN** a directory containing a valid `.gaze.yaml` file +- **WHEN** `LoadFromDir` is called with that directory +- **THEN** the returned config SHALL reflect the values from the file + +#### Scenario: Directory without .gaze.yaml + +- **GIVEN** a directory with no `.gaze.yaml` file +- **WHEN** `LoadFromDir` is called +- **THEN** it SHALL return `DefaultConfig()` (no error) + +#### Scenario: Directory with invalid .gaze.yaml + +- **GIVEN** a directory with a malformed `.gaze.yaml` file +- **WHEN** `LoadFromDir` is called +- **THEN** it SHALL return `DefaultConfig()` (error swallowed) + +### Requirement: LoadTestPackage export + +`goprovider.LoadTestPackage` MUST be the exported form of the existing +`loadTestPackage` function. It MUST retain the `pkg.Errors` validation +loop that checks for package-level errors before selecting a test package. + +#### Scenario: Package with test syntax + +- **GIVEN** a valid Go package path with test files +- **WHEN** `LoadTestPackage` is called +- **THEN** it SHALL return the package that has test syntax + +#### Scenario: Package with errors + +- **GIVEN** a Go package path where loaded packages have `Errors` +- **WHEN** `LoadTestPackage` is called +- **THEN** it SHALL return an error containing the package error messages + +## MODIFIED Requirements + +### Requirement: BuildContractCoverageFunc complexity + +`BuildContractCoverageFunc` MUST delegate to `buildEffectsSet`, +`buildCoverageMap`, and `computeCoverageReason` instead of inlining +their logic. The function's cyclomatic complexity MUST be 8 or less. + +Previously: All logic was inline in a single 120-line function with +complexity 22. + +### Requirement: aireport test package loading + +`qualityPipelineDeps.loadTestPkg` default MUST resolve to +`goprovider.LoadTestPackage` instead of the local +`loadTestPackageForQuality`. + +Previously: Default was `loadTestPackageForQuality` (local copy without +`pkg.Errors` validation). + +### Requirement: aireport config loading + +`qualityPipelineDeps.loadConfig` default MUST resolve to +`config.LoadFromDir` instead of the local `loadGazeConfigBestEffort`. + +Previously: Default was local `loadGazeConfigBestEffort` (duplicate of +goprovider's copy). + +## REMOVED Requirements + +### Requirement: loadTestPackageForQuality (aireport) + +Removed — replaced by `goprovider.LoadTestPackage`. The aireport copy +lacked `pkg.Errors` validation, making it strictly inferior to the +goprovider version. + +### Requirement: loadGazeConfigBestEffort (both packages) + +Removed — replaced by `config.LoadFromDir`. Two identical copies +consolidated into the config package where the logic belongs. diff --git a/openspec/changes/crapload-decompose-pr2a/tasks.md b/openspec/changes/crapload-decompose-pr2a/tasks.md new file mode 100644 index 0000000..ecb4df0 --- /dev/null +++ b/openspec/changes/crapload-decompose-pr2a/tasks.md @@ -0,0 +1,169 @@ + + +## 1. Independent dedup targets + +These tasks touch different packages and have no inter-dependencies. + +- [x] 1.1 [P] Add `config.LoadFromDir` + tests + - Add `LoadFromDir(moduleDir string) *GazeConfig` to + `internal/config/config.go` + - Add `path/filepath` to the import block + - Construct path via `filepath.Join(moduleDir, ".gaze.yaml")`, call + `config.Load`, return `DefaultConfig()` on any error + - GoDoc comment MUST document best-effort semantics: errors are + silently swallowed because callers use config as an optimization + hint, not a hard requirement + - Add 3 tests to `internal/config/config_test.go`: + - `TestLoadFromDir_ValidConfig` — temp dir with valid `.gaze.yaml` + - `TestLoadFromDir_MissingFile` — empty temp dir → `DefaultConfig()` + - `TestLoadFromDir_InvalidYAML` — temp dir with malformed file → + `DefaultConfig()` + - **Files**: `internal/config/config.go`, `internal/config/config_test.go` + +- [x] 1.2 [P] Export `LoadTestPackage` from goprovider + - Rename `loadTestPackage` to `LoadTestPackage` in + `internal/provider/goprovider/contract.go` + - Update the internal caller at line 256 (`d.loadTestPkg = loadTestPackage` + → `d.loadTestPkg = LoadTestPackage`) + - Update test references in `contract_internal_test.go` (3 tests: + `TestLoadTestPackage_*`) to use `LoadTestPackage` + - Ensure GoDoc comment starts with `LoadTestPackage` (per CS-004 MUST) + - No new tests needed — existing 3 tests cover all branches + - **Files**: `internal/provider/goprovider/contract.go`, + `internal/provider/goprovider/contract_internal_test.go` + +## 2. Decompose `BuildContractCoverageFunc` + +These tasks modify the same file (`contract.go` / `contract_internal_test.go`) +and MUST run sequentially. + +- [x] 2.1 Extract `computeCoverageReason` helper + tests + - Extract lines 160–190 from `BuildContractCoverageFunc` into + `computeCoverageReason(report taxonomy.QualityReport) crap.ContractCoverageInfo` + - Pure function: reads `ContractCoverage.TotalContractual`, + `AmbiguousEffects`, `ContractCoverage.Percentage` + - Replace inline logic in `BuildContractCoverageFunc` with call to + `computeCoverageReason(report)` + - Add 4 tests to `contract_internal_test.go`: + - `TestComputeCoverageReason_WithContractualEffects` — Percentage + passthrough, empty Reason + - `TestComputeCoverageReason_AllAmbiguous` — Reason + `"all_effects_ambiguous"`, correct min/max confidence + - `TestComputeCoverageReason_NoEffects` — Reason + `"no_effects_detected"` + - `TestComputeCoverageReason_NilClassification` — entries with nil + Classification skipped + - Tests MUST NOT use `testing.Short()` guard + - **Files**: `internal/provider/goprovider/contract.go`, + `internal/provider/goprovider/contract_internal_test.go` + +- [x] 2.2 Extract `buildEffectsSet` helper + tests + - Extract lines 124–140 from `BuildContractCoverageFunc` into + `buildEffectsSet(pkgPaths []string, loadAndAnalyzeFn func(string, analysis.Options) ([]taxonomy.AnalysisResult, error)) map[string]bool` + - When `loadAndAnalyzeFn` is nil, default to `analysis.LoadAndAnalyze` + - Replace inline logic in `BuildContractCoverageFunc` with + `effectsSet := buildEffectsSet(pkgPaths, nil)` + - Add 4 tests to `contract_internal_test.go`: + - `TestBuildEffectsSet_WithEffects` — synthetic results with side + effects → keys present + - `TestBuildEffectsSet_NoEffects` — results with empty SideEffects → + empty map + - `TestBuildEffectsSet_AnalysisError` — loadAndAnalyze returns error → + package silently skipped + - `TestBuildEffectsSet_EmptyPaths` — empty pkgPaths → empty map + - Tests MUST NOT use `testing.Short()` guard + - **Files**: `internal/provider/goprovider/contract.go`, + `internal/provider/goprovider/contract_internal_test.go` + +- [x] 2.3 Extract `buildCoverageMap` helper + tests + - Extract lines 142–192 (the per-package quality pipeline loop + + report processing) from `BuildContractCoverageFunc` into + `buildCoverageMap(pkgPaths []string, moduleDir string, gazeConfig *config.GazeConfig, stderr io.Writer, deps ...contractCoverageDeps) (map[string]crap.ContractCoverageInfo, []string)` + - Must call `analyzePackageCoverage` per package and + `computeCoverageReason` per report (uses helpers from 2.1) + - Must skip reports with empty `TargetFunction.Function` + - Must track degraded packages + - Must keep highest-coverage entry when duplicates exist + - Replace inline logic in `BuildContractCoverageFunc` with + `coverageMap, degradedPkgs := buildCoverageMap(pkgPaths, moduleDir, gazeConfig, stderr, ccDeps)` + - Add 5 tests to `contract_internal_test.go`: + - `TestBuildCoverageMap_Success` — synthetic deps → map populated + - `TestBuildCoverageMap_DegradedReport` — empty Function field → skipped + - `TestBuildCoverageMap_SSADegradation` — degraded pkg path returned + - `TestBuildCoverageMap_HigherCoverageWins` — two reports, higher pct wins + - `TestBuildCoverageMap_EmptyPaths` — empty pkgPaths → empty map + - Tests MUST NOT use `testing.Short()` guard + - **Files**: `internal/provider/goprovider/contract.go`, + `internal/provider/goprovider/contract_internal_test.go` + +## 3. Wire dedup replacements + +These tasks modify different files and can run in parallel, but depend +on Groups 1 and 2 being complete. + +- [x] 3.1 [P] Replace `loadGazeConfigBestEffort` in goprovider with `config.LoadFromDir` + - Replace call at `contract.go` line 113: + `gazeConfig := loadGazeConfigBestEffort(moduleDir)` → + `gazeConfig := config.LoadFromDir(moduleDir)` + - Delete `loadGazeConfigBestEffort` function (lines 387–396) + - Verify no other references remain in `contract.go` + - **Files**: `internal/provider/goprovider/contract.go` + +- [x] 3.2 [P] Replace `loadGazeConfigBestEffort` and `loadTestPackageForQuality` in aireport + - Replace default in `resolveQualityDeps`: + `d.loadTestPkg = loadTestPackageForQuality` → + `d.loadTestPkg = goprovider.LoadTestPackage` + - Replace default in `resolveQualityDeps`: + `d.loadConfig = loadGazeConfigBestEffort` → + `d.loadConfig = config.LoadFromDir` + - Replace direct call at line 297: + `cfg := loadGazeConfigBestEffort(moduleDir)` → + `cfg := config.LoadFromDir(moduleDir)` + - Delete `loadGazeConfigBestEffort` function (lines 354–363) + - Delete `loadTestPackageForQuality` function (lines 365–392) + - Update `runner_steps_test.go`: + - Remove `TestLoadTestPackageForQuality_*` tests (3 tests — coverage + now provided by goprovider's `TestLoadTestPackage_*`) + - Delete `TestLoadGazeConfigBestEffort_AlwaysNonNil` — the function + it tests is being removed; config loading is now tested by + `TestLoadFromDir_*` tests in `config_test.go` + - Update any error string assertions that changed due to + `LoadTestPackage`'s different error messages + - **Files**: `internal/aireport/runner_steps.go`, + `internal/aireport/runner_steps_test.go` + +## 4. Verification + +- [x] 4.1 Build and test + - Run `go build ./...` — MUST pass + - Run `go test -race -count=1 -short ./...` — MUST pass + - Run `golangci-lint run` — MUST pass with 0 issues + - Verify no unused imports after function deletions + - **Verify**: `BuildContractCoverageFunc` complexity ≤ 8 (check via + `gocyclo -over 7 internal/provider/goprovider/contract.go`) + - **Verify**: GoDoc comments on `LoadFromDir` and `LoadTestPackage` + are complete and start with the function name (CS-004) + - **Verify**: Net test delta — ~12 new tests added, ~4 removed + +- [x] 4.2 Constitution alignment verification + - **Accuracy (I)**: Confirm existing integration tests + (`TestBuildContractCoverageFunc_WelltestedPackage`) still pass — + coverage data is computed identically + - **Testability (IV)**: Confirm all new helper tests run without + `testing.Short()` guard — they contribute to CRAPload + - **Minimal Assumptions (II)**: N/A — no analysis behavior changed + - **Actionable Output (III)**: N/A — no output format changed + + + +