refactor: consolidate safeSSABuild into shared internal/ssaguard package

.uf/dewey/learnings/consolidate-ssa-guard-20260831T112125-yvonne-devlin.md

@@ -0,0 +1,10 @@
+---
+tag: consolidate-ssa-guard
+author: yvonne-devlin
+category: gotcha
+created_at: 2026-08-31T11:21:25Z
+identity: consolidate-ssa-guard-20260831T112125-yvonne-devlin
+tier: draft
+---
+
+When deduplicating tests during a DRY consolidation, deleting a whole *_test.go file can be the correct Zero-Waste outcome: internal/quality/pairing_test.go held ONLY the 3 duplicated TestSafeSSABuild_* tests plus a stale SC-001 comment block with no test functions. After removing the duplicated triad it would not compile (unused testing/quality imports), so deleting the entire file was cleaner than leaving a stub. Behavioral coverage was preserved because the 3 canonical tests moved to internal/ssaguard/ssaguard_test.go. Reviewers accept a net test-count decrease (-3 here: 6 duplicates -> 3 shared) as long as the acceptance criterion is framed as 'behavioral coverage is neutral-to-positive' NOT 'net test count does not decrease' — the latter wording is a self-contradicting acceptance criterion that a spec reviewer (divisor-testing) will flag HIGH. Also satisfy Constitution IV by stating the coverage strategy explicitly (unit only, 100% branch coverage, enumerate the branches) in proposal/design/tasks.

.uf/dewey/learnings/ssaguard-20260831T112119-yvonne-devlin.md

@@ -0,0 +1,10 @@
+---
+tag: ssaguard
+author: yvonne-devlin
+category: pattern
+created_at: 2026-08-31T11:21:19Z
+identity: ssaguard-20260831T112119-yvonne-devlin
+tier: draft
+---
+
+Consolidating a duplicated recover()-guard helper (safeSSABuild) into a shared package (internal/ssaguard) is safe and coverage-neutral when the guard has ZERO external dependencies — it takes a func() and returns any. This reverses spec-021 R3's 'keep packages dependency-light' rationale, which only applied when a helper might drag in dependencies. Key design decisions that made review pass 5/5: (1) Name the package `ssaguard` NOT `ssautil` to avoid shadowing golang.org/x/tools/go/ssa/ssautil already imported at both call sites. (2) Export the function (SafeSSABuild) so both callers' export_test.go shims are eliminated — under internal/ this adds no API surface concern. (3) Keep log.Warn/log.Debug at the CALLER recovery site, do NOT move logging into the guard — the guard has no visibility into the package being built (loses pkg.PkgPath diagnostic context) and importing a logger would violate the stdlib-only constraint. (4) Document the ssa.BuildSerially caller precondition in GoDoc as documentation-only (NOT runtime validation) — mode flags are set by callers before ssautil.AllPackages, outside the guard's scope; recover() is goroutine-scoped so omitting BuildSerially causes silent panic-escape/process crash (spec 033).

AGENTS.md

@@ -324,6 +324,7 @@ internal/
   aireport/            AI-powered CI quality report pipeline (gaze report)
   protocol/            JSON-RPC 2.0 client for external analyzer communication
   adapter/             External analyzer provider adapters (protocol → crap interfaces)
+  ssaguard/            Shared SSA panic-recovery guard (BuildSerially precondition)
   provider/
     goprovider/        Go-specific provider implementations (gocyclo, go test, SSA)
     mockprovider/      Mock providers for unit testing the scoring core
@@ -485,6 +486,7 @@ Formatters: gofmt, goimports.
 
 ## Recent Changes
 
+- consolidate-ssa-guard: Consolidated the byte-identical `safeSSABuild` recover-guard (previously duplicated in `internal/analysis/mutation.go` and `internal/quality/pairing.go`) into a new shared stdlib-only package `internal/ssaguard` with exported `SafeSSABuild(buildFn func()) (panicVal any)`. Reverses spec-021 R3 (which chose duplication to keep packages dependency-light) — justified because the guard has zero external dependencies. GoDoc documents the `ssa.BuildSerially` caller precondition and the goroutine-scoped `recover()` rationale (spec 033); the guard does NOT validate build mode at runtime. Both callers (`BuildSSA`, `BuildTestSSA`) now call `ssaguard.SafeSSABuild(prog.Build)` and retain their `log.Warn`/`log.Debug` recovery-site calls (logging not moved into the guard, preserving pkg-path context). Removed duplicated `TestSafeSSABuild_*` triads from `mutation_test.go`, deleted `internal/quality/pairing_test.go` (held only the duplicated triad plus a stale no-test comment block), and removed the `SafeSSABuild` shim from both `export_test.go` files. Three canonical tests live in `internal/ssaguard/ssaguard_test.go` (100% branch coverage). Net test count -3, behavioral coverage neutral. Closes #238.
 - decompose-high-complexity-functions: Decomposed 5 high-complexity functions to reduce cyclomatic complexity (CC) and CRAPload (issue #200, continuation of #166). `isPointerArgStore` (CC 13→3): removed structurally unreachable branches — `tracesToParam` already walks FieldAddr/IndexAddr/UnOp chains. `detectASTReceiverMutations` (CC 24→11): extracted 3 per-node-type handlers (`handleReceiverAssignStmt`, `handleReceiverIncDecStmt`, `handleReceiverCallExpr`) from `ast.Inspect` closure. `runCrap` (CC 19→14): extracted `resolveBaselineAndCompare`, `writeCrapOutputAndSummary`, `evaluateCrapGates` preserving D7 gate ordering (baseline before threshold). `runQuality` (CC 32→14): extracted `loadQualityConfig`, `setupQualityDeps`, `writeQualityReport`, `handleQualityEmptyResults`, plus previously extracted `runQualityPerPackage` and `writeQualityEmptyResults`. `writeOneResult` (CC 32→4): extracted `buildEffectsTable`, `writeTierSummary`, plus previously extracted `writeEffectRows` and `writeVerboseSignals`. Added ~35 new unit tests across 3 packages using synthetic AST, DI structs, and `testdata/src/` fixtures. No behavioral changes — all existing tests pass without modification.
 - adapter-call-unmarshal-helper: Extracted a generic `callAndUnmarshal[T any](ctx context.Context, client *protocol.Client, method string, params any) (T, error)` helper (`internal/adapter/call.go`, unexported) that centralizes the repeated JSON-RPC `Call → transport-error check → protocol-error check → json.Unmarshal[T]` pattern. Migrated 3 hard-error batch call sites: `ExternalComplexityProvider.Analyze` (`complexity.go`, `callAndUnmarshal[protocol.ComplexityResult]`), `ExternalLineCoverageProvider.Coverage` (`coverage.go`, `[protocol.CoverageResult]`), and `ExternalSideEffectAnalyzer.loadBatch` (`sideeffect.go`, `[protocol.AnalyzeResult]`). Because the method constants are literally `"complexity"`/`"coverage"`/`"analyze"`, the helper's D2 error templates (`"%s protocol call: %w"`, `"%s protocol error: %s (code %d)"`, `"parsing %s result: %w"`) reproduce the exact legacy error strings. Error-chain contract (design D6): transport and unmarshal errors wrap with `%w` (`errors.Is`/`errors.As` preserved); protocol errors format with `%s` (structured JSON-RPC error object, not a Go error chain). `Session.Initialize` (`session.go`) intentionally retains its inline per-branch error handling (design D2 sanctioned option) because the collapsed generic helper cannot reproduce its three distinct legacy strings (`"initialize handshake"`/`"initialize error"`/`"parsing initialize result"`); it also keeps `s.client.Close()` cleanup on each error branch. Explicitly excluded from migration: `CallStream`/streaming side-effect path, `fetchTestMappings` `p.warn()` graceful-degradation path (design D3), and `internal/protocol/` (transport layer stays usage-agnostic). Added table-driven `TestCallAndUnmarshal` (`call_test.go`, `package adapter` internal) with 5 subtests (success, transport error, protocol error, unmarshal failure, generic instantiation across a second result type) driven through the fake analyzer binary; the internal test builds its own fake-analyzer copy lazily via `sync.Once` because only one `TestMain` is allowed per test binary (the existing one lives in `package adapter_test`). Split from #201; closes #237.
 - quality-empty-results-gate: Fixed silent exit-0 bug (#103) when `gaze quality` encounters Ginkgo/BDD suites or packages where no test functions can be resolved to targets. Added `SkippedTests int` and `SkippedTestNames []string` fields to `taxonomy.PackageSummary` (`internal/taxonomy/types.go`). `quality.Assess` (`internal/quality/quality.go`) now counts and names skipped test functions at the `len(targets) == 0` continue site. `runQuality` (`cmd/gaze/main.go`) now prints structured stdout summary when `allReports` is empty (total test count, skipped names truncated at 20, `--target` hint), returns error when `--min-contract-coverage` or `--max-over-specification` thresholds are set (quality gate failure), and produces valid JSON for `--format=json`. Added `writeSkippedTests` section to `quality.WriteText` (`internal/quality/report.go`) with 20-name truncation. Updated `mergeSummaries` to aggregate skipped test data. Updated JSON Schema (`internal/report/schema.go`). Propagated through report pipeline: `runQualityForPackage` now returns 3 values, `qualityStepResult.SkippedTests`, `ReportSummary.SkippedTests`, `compactSummary.SkippedTests`, `compactPackageSummary.SkippedTests`/`SkippedTestNames`. Added `testdata/src/bddstyle/` fixture. 13 new tests across 3 packages. Closes #103.

docs/concepts/analysis-pipeline.md

@@ -142,7 +142,7 @@ The pipeline is designed to produce useful results even when parts fail:
 
 | Failure | Impact | Mitigation |
 |---|---|---|
-| SSA build panics | No mutation detection via SSA | `safeSSABuild` recovers the panic; AST fallback detects common mutation patterns |
+| SSA build panics | No mutation detection via SSA | `ssaguard.SafeSSABuild` recovers the panic; AST fallback detects common mutation patterns |
 | SSA build returns nil | Same as panic | AST fallback activates automatically |
 | Type info unavailable | Reduced precision for global detection, import resolution | Fallback to AST name matching (may produce false positives) |
 | Package load errors | No analysis for that package | Error returned to caller; other packages unaffected |

internal/analysis/export_test.go

@@ -29,11 +29,6 @@ func BaseTypeName(expr ast.Expr) string {
 	return baseTypeName(expr)
 }
 
-// SafeSSABuild is exported for testing. See safeSSABuild.
-func SafeSSABuild(buildFn func()) any {
-	return safeSSABuild(buildFn)
-}
-
 // ExprRootIdent is exported for testing. See exprRootIdent.
 func ExprRootIdent(expr ast.Expr) *ast.Ident {
 	return exprRootIdent(expr)

internal/analysis/mutation.go

@@ -12,21 +12,10 @@ import (
 	"golang.org/x/tools/go/ssa"
 	"golang.org/x/tools/go/ssa/ssautil"
 
+	"github.com/unbound-force/gaze/internal/ssaguard"
 	"github.com/unbound-force/gaze/internal/taxonomy"
 )
 
-// safeSSABuild calls buildFn and recovers from any panic it produces.
-// Returns the recovered panic value, or nil if buildFn completed
-// without panicking. This isolates the recover() pattern so it can
-// be tested independently of the SSA builder.
-func safeSSABuild(buildFn func()) (panicVal any) {
-	defer func() {
-		panicVal = recover()
-	}()
-	buildFn()
-	return nil
-}
-
 // BuildSSA constructs the SSA representation for a loaded package.
 // The result is reusable across multiple function analyses within
 // the same package, avoiding the cost of rebuilding SSA per function.
@@ -46,7 +35,7 @@ func BuildSSA(pkg *packages.Package) (ssaPkg *ssa.Package) {
 		ssa.InstantiateGenerics|ssa.BuildSerially,
 	)
 
-	if r := safeSSABuild(prog.Build); r != nil {
+	if r := ssaguard.SafeSSABuild(prog.Build); r != nil {
 		log.Warn("SSA build skipped: internal panic recovered", "pkg", pkg.PkgPath)
 		log.Debug("SSA panic value", "pkg", pkg.PkgPath, "panic", r)
 		return nil

internal/analysis/mutation_test.go

@@ -1,7 +1,6 @@
 package analysis_test
 
 import (
-	"errors"
 	"go/ast"
 	"go/parser"
 	"go/token"
@@ -15,62 +14,16 @@ import (
 	"golang.org/x/tools/go/ssa"
 )
 
-// ---------------------------------------------------------------------------
-// safeSSABuild tests
-// ---------------------------------------------------------------------------
-
-// TestSafeSSABuild_NoPanic verifies that safeSSABuild returns nil
-// when the build function completes without panicking.
-func TestSafeSSABuild_NoPanic(t *testing.T) {
-	result := analysis.SafeSSABuild(func() {
-		// no panic
-	})
-	if result != nil {
-		t.Errorf("safeSSABuild returned %v, want nil for non-panicking function", result)
-	}
-}
-
-// TestSafeSSABuild_PanicString verifies that safeSSABuild recovers
-// a panic with a string value and returns it.
-func TestSafeSSABuild_PanicString(t *testing.T) {
-	result := analysis.SafeSSABuild(func() {
-		panic("test panic message")
-	})
-	s, ok := result.(string)
-	if !ok {
-		t.Fatalf("safeSSABuild returned %T, want string", result)
-	}
-	if s != "test panic message" {
-		t.Errorf("safeSSABuild returned %q, want %q", s, "test panic message")
-	}
-}
-
-// TestSafeSSABuild_PanicError verifies that safeSSABuild recovers
-// a panic with an error value and returns it.
-func TestSafeSSABuild_PanicError(t *testing.T) {
-	errPanic := errors.New("SSA builder error")
-	result := analysis.SafeSSABuild(func() {
-		panic(errPanic)
-	})
-	e, ok := result.(error)
-	if !ok {
-		t.Fatalf("safeSSABuild returned %T, want error", result)
-	}
-	if e != errPanic {
-		t.Errorf("safeSSABuild returned error %v, want %v", e, errPanic)
-	}
-}
-
 // ---------------------------------------------------------------------------
 // SC-001 / SC-002: panic recovery contract tests
 //
 // Note: BuildSSA's panic recovery cannot be tested end-to-end because
 // prog.Build() is a concrete method on *ssa.Program that cannot be
 // mocked or injected. The recovery pattern is verified through the
-// safeSSABuild helper tests above (which exercise the identical
-// defer/recover logic). BuildSSA's logging behavior is verified by
-// code inspection — the log.Warn/log.Debug calls are co-located with the
-// safeSSABuild call in the same if-block.
+// ssaguard.SafeSSABuild tests in internal/ssaguard/ssaguard_test.go
+// (which exercise the identical defer/recover logic). BuildSSA's logging
+// behavior is verified by code inspection — the log.Warn/log.Debug calls
+// are co-located with the ssaguard.SafeSSABuild call in the same if-block.
 // ---------------------------------------------------------------------------
 
 // TestSC001_BuildSSANoPanicReturnsPackage verifies that BuildSSA

internal/quality/export_test.go

@@ -3,11 +3,6 @@ package quality
 // ResolveExprRoot exports resolveExprRoot for testing.
 var ResolveExprRoot = resolveExprRoot
 
-// SafeSSABuild is exported for testing. See safeSSABuild.
-func SafeSSABuild(buildFn func()) any {
-	return safeSSABuild(buildFn)
-}
-
 // MapAssertionsToEffectsWithStderr exports mapAssertionsToEffectsImpl
 // for testing AI mapper with stderr capture.
 var MapAssertionsToEffectsWithStderr = mapAssertionsToEffectsImpl

internal/quality/pairing.go

@@ -11,24 +11,9 @@ import (
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/go/ssa"
 	"golang.org/x/tools/go/ssa/ssautil"
-)
 
-// safeSSABuild calls buildFn and recovers from any panic it produces.
-// Returns the recovered panic value, or nil if buildFn completed
-// without panicking. This isolates the recover() pattern so it can
-// be tested independently of the SSA builder.
-//
-// Duplicated from internal/analysis/mutation.go because Go's package
-// system does not allow sharing unexported symbols across internal
-// packages. A shared package was rejected to keep both packages
-// dependency-light — see specs/021-ssa-panic-recovery/research.md R3.
-func safeSSABuild(buildFn func()) (panicVal any) {
-	defer func() {
-		panicVal = recover()
-	}()
-	buildFn()
-	return nil
-}
+	"github.com/unbound-force/gaze/internal/ssaguard"
+)
 
 // TestFunc represents a test function found in a test package.
 type TestFunc struct {
@@ -132,7 +117,7 @@ func BuildTestSSA(pkg *packages.Package) (program *ssa.Program, ssaPkg *ssa.Pack
 		ssa.InstantiateGenerics|ssa.BuildSerially,
 	)
 
-	if r := safeSSABuild(prog.Build); r != nil {
+	if r := ssaguard.SafeSSABuild(prog.Build); r != nil {
 		log.Warn("SSA build skipped: internal panic recovered", "pkg", pkg.PkgPath)
 		log.Debug("SSA panic value", "pkg", pkg.PkgPath, "panic", r)
 		return nil, nil, fmt.Errorf("SSA build panicked for package %s: internal panic recovered", pkg.PkgPath)

internal/ssaguard/ssaguard.go

@@ -0,0 +1,34 @@
+// Package ssaguard provides a shared panic-recovery guard for SSA
+// construction. SSA builds via golang.org/x/tools can panic on certain
+// upstream bugs (e.g., generic type substitution under Go 1.25); this
+// package isolates the recover() pattern so callers degrade gracefully
+// instead of crashing.
+package ssaguard
+
+// SafeSSABuild calls buildFn and recovers from any panic it produces.
+// It returns the recovered panic value, or nil if buildFn completed
+// without panicking. Isolating the recover() pattern here lets it be
+// tested independently of the SSA builder and shared by every SSA
+// build site.
+//
+// Caller precondition — ssa.BuildSerially: callers MUST construct the
+// SSA program with the ssa.BuildSerially mode flag (alongside
+// ssa.InstantiateGenerics) before invoking SafeSSABuild(prog.Build).
+// Go's recover() is goroutine-scoped and cannot catch panics raised in
+// child goroutines. Without ssa.BuildSerially, prog.Build() spawns a
+// child goroutine per package and any panic there escapes this guard,
+// crashing the process. BuildSerially forces all construction onto the
+// calling goroutine so the deferred recover() below can catch it. See
+// specs/033-ssa-goroutine-panic for the invariant.
+//
+// SafeSSABuild does NOT validate the build mode at runtime: the mode
+// flags are set by callers before ssautil.AllPackages, which is outside
+// this guard's scope. The precondition is documented and enforced by
+// convention, not by a runtime check.
+func SafeSSABuild(buildFn func()) (panicVal any) {
+	defer func() {
+		panicVal = recover()
+	}()
+	buildFn()
+	return nil
+}