diff --git a/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151141-yvonne-devlin.md b/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151141-yvonne-devlin.md new file mode 100644 index 0000000..07d8c3b --- /dev/null +++ b/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151141-yvonne-devlin.md @@ -0,0 +1,10 @@ +--- +tag: adapter-call-unmarshal-helper +author: yvonne-devlin +category: gotcha +created_at: 2026-08-27T15:11:41Z +identity: adapter-call-unmarshal-helper-20260827T151141-yvonne-devlin +tier: draft +--- + +Testing a generic JSON-RPC helper's own json.Unmarshal-failure branch: the fake analyzer's --malformed-json flag corrupts the JSON-RPC ENVELOPE itself, so protocol.Client.Call fails first and the error surfaces as a TRANSPORT error (" protocol call: %w"), NOT the helper's own result-unmarshal path. To exercise the helper's json.Unmarshal(resp.Result, &result) failure branch you need a VALID envelope carrying a well-formed but type-incompatible result — use a test-only result type like `type mismatchedResult struct{ Functions string \`json:"functions"\` }` (Functions typed as string where the real result has an array), forcing Unmarshal to fail inside the helper while the envelope stays valid. Assert the error contains "parsing result" and errors.Unwrap != nil (%w preserved). diff --git a/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151149-yvonne-devlin.md b/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151149-yvonne-devlin.md new file mode 100644 index 0000000..6bb134c --- /dev/null +++ b/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151149-yvonne-devlin.md @@ -0,0 +1,10 @@ +--- +tag: adapter-call-unmarshal-helper +author: yvonne-devlin +category: gotcha +created_at: 2026-08-27T15:11:49Z +identity: adapter-call-unmarshal-helper-20260827T151149-yvonne-devlin +tier: draft +--- + +Go test-binary constraint: only ONE TestMain is allowed per compiled test binary. In internal/adapter/ the existing TestMain lives in `package adapter_test` (external test package) and builds+caches the fake analyzer binary. A NEW internal-package test file (`package adapter`, required to access unexported symbols like callAndUnmarshal) compiles into the SAME test binary and therefore CANNOT declare its own TestMain, nor can it reference the external package's unexported fakeBinaryPath var. Resolution: the internal test file builds its OWN fake-analyzer copy lazily via sync.Once (package-level vars: binaryPath string, buildOnce sync.Once, buildErr error). GOTCHA: this sync.Once-based lazy build canNOT use t.Cleanup for temp-dir teardown — Once.Do runs during the FIRST SUBTEST that calls it and captures that subtest's *testing.T, so the cleanup fires when that first subtest ends, deleting the shared binary before later subtests run (observed failure: "analyzer binary ... not found" in a later subtest). Accept the small (~4-5MB) OS-temp leak instead; there is no package-level teardown hook available without a TestMain. diff --git a/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151158-yvonne-devlin.md b/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151158-yvonne-devlin.md new file mode 100644 index 0000000..52aff8e --- /dev/null +++ b/.uf/dewey/learnings/adapter-call-unmarshal-helper-20260827T151158-yvonne-devlin.md @@ -0,0 +1,10 @@ +--- +tag: adapter-call-unmarshal-helper +author: yvonne-devlin +category: pattern +created_at: 2026-08-27T15:11:58Z +identity: adapter-call-unmarshal-helper-20260827T151158-yvonne-devlin +tier: draft +--- + +Pattern/design: a collapsed generic error-wrapping helper (e.g. callAndUnmarshal[T] that maps Call→transport-err→protocol-err→unmarshal into uniform "%s protocol call"/"%s protocol error"/"parsing %s result" templates) CANNOT preserve per-branch legacy error wording when a call site's historical strings diverge from the template. In internal/adapter/, the 3 batch sites whose method constants ARE the human prefix (complexity/coverage/analyze) migrated cleanly because the templates reproduce their legacy strings 1:1. But session.go Initialize used THREE distinct non-template strings ("initialize handshake"/"initialize error"/"parsing initialize result") that the collapsed single-error helper cannot reproduce without fragile prefix translation. Resolution sanctioned by design D2: Initialize RETAINS its original inline per-branch error handling (and its per-branch s.client.Close() cleanup) rather than delegating — so effective helper adoption was 3 sites, not the originally-planned 4. Lesson: when planning a DRY error-helper extraction, audit each call site's exact error strings first; sites with multiple distinct historical prefixes are legitimate exclusions, and preserving exact operator-facing error strings (log/alert grep patterns) outranks maximizing migration count. diff --git a/AGENTS.md b/AGENTS.md index ea6fc76..ad4a4df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -486,6 +486,7 @@ Formatters: gofmt, goimports. ## Recent Changes - 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. - crapload-add-tests-pr2c: Added test coverage for 3 high-CRAP functions to reduce CRAPload from 29 to 26. `ResolvePackagePaths` (CRAP 81.6→10.1): removed `testing.Short()` guards from 5 existing tests (packages.NeedName is lightweight), added 2 new tests for nil-stderr and dedup branches. `BuildContractCoverageFunc` (CRAP 56.0→2.0): added `buildContractCoverageFuncDeps` DI struct with 4 injectable fields following the existing `contractCoverageDeps` pattern, added `buildContractCoverageFuncImpl` wrapper, added 7 synthetic unit tests covering all 7 code paths. `loadStreaming` (CRAP 42.0→12.0): extracted `parseSideEffectStream` helper from JSONL scanner loop, added 6 unit tests using synthetic `bufio.Scanner` from `bytes.Reader`. Final phase (2c) of issue #166. - crapload-decompose-pr2b: Decomposed three highest-complexity functions in `internal/quality/mapping.go`: `matchContainerUnwrap` (50→8), `isTransformationCall` (26→5), `matchAssertionToEffect` (25→5). Extracted 7+ helpers: `isByteLikeParam`, `isPointerDestParam`, `resolveCallSignature`, `findParamIndex` (from isTransformationCall); `matchDirect`, `matchIndirectRoot` (from matchAssertionToEffect); `collectTrackedVars`, `traceForwardDataFlow`, `matchTrackedInExpr` (from matchContainerUnwrap). Added 25 new unit tests using synthetic AST via `parseAndTypeCheck`. Filled 3 test gaps in `isTransformationCall` (io.Reader, empty interface, mixed ordering). `TestSC003_MappingAccuracy` ratchet (85.0%) passes unchanged. Phase 2b of issue #166. diff --git a/internal/adapter/call.go b/internal/adapter/call.go new file mode 100644 index 0000000..3d5db4e --- /dev/null +++ b/internal/adapter/call.go @@ -0,0 +1,55 @@ +package adapter + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/unbound-force/gaze/internal/protocol" +) + +// callAndUnmarshal issues a JSON-RPC call for method with params on +// client, checks the transport and protocol errors, and unmarshals the +// raw result into a value of type T. +// +// It centralizes the Call -> transport-error check -> protocol-error +// check -> json.Unmarshal sequence that every batch provider adapter +// would otherwise open-code. All three failure paths wrap the error +// with per-method context derived from method so operators can +// distinguish, for example, a "complexity" failure from a "coverage" +// failure in logs. +// +// Error-chain contract (design D6): transport and unmarshal errors wrap +// the underlying error with %w, so errors.Is / errors.As unwrapping is +// preserved. Protocol errors are formatted with %s because resp.Error +// is a structured JSON-RPC error object carrying a message and code, +// not a Go error chain value. +// +// On any error the zero value of T is returned alongside the wrapped +// error. +func callAndUnmarshal[T any]( + ctx context.Context, + client *protocol.Client, + method string, + params any, +) (T, error) { + var result T + + // method is the wire method constant (e.g. protocol.MethodComplexity + // == "complexity"). Reusing it as the error prefix is intentional: it + // reproduces the exact legacy per-method log strings the migrated call + // sites emitted before this helper existed. + resp, err := client.Call(ctx, method, params) + if err != nil { + return result, fmt.Errorf("%s protocol call: %w", method, err) + } + if resp.Error != nil { + return result, fmt.Errorf("%s protocol error: %s (code %d)", method, resp.Error.Message, resp.Error.Code) + } + + if err := json.Unmarshal(resp.Result, &result); err != nil { + return result, fmt.Errorf("parsing %s result: %w", method, err) + } + + return result, nil +} diff --git a/internal/adapter/call_test.go b/internal/adapter/call_test.go new file mode 100644 index 0000000..5e49ead --- /dev/null +++ b/internal/adapter/call_test.go @@ -0,0 +1,228 @@ +package adapter + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/unbound-force/gaze/internal/protocol" +) + +// callTestBinaryPath is the path to the compiled fake_analyzer binary used +// by the internal call_test.go tests. It is built once, lazily, because the +// external-package TestMain (in adapter_test.go, package adapter_test) builds +// its own copy in an unexported var that this internal test package cannot +// reference. +var ( + callTestBinaryPath string + callTestBuildOnce sync.Once + callTestBuildErr error +) + +// buildCallTestFakeAnalyzer builds the fake analyzer binary once and returns +// its path. Subsequent calls return the cached path. +func buildCallTestFakeAnalyzer(t *testing.T) string { + t.Helper() + callTestBuildOnce.Do(func() { + tmpDir, err := os.MkdirTemp("", "gaze-call-test-*") + if err != nil { + callTestBuildErr = err + return + } + binPath := filepath.Join(tmpDir, "fake_analyzer") + cmd := exec.Command("go", "build", "-o", binPath, "./testdata/fake_analyzer/") + cmd.Dir = filepath.Join("..", "protocol") + cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { + callTestBuildErr = err + return + } + callTestBinaryPath = binPath + }) + if callTestBuildErr != nil { + t.Fatalf("building fake_analyzer: %v", callTestBuildErr) + } + return callTestBinaryPath +} + +// newCallTestClient starts the fake analyzer with the given extra args (after +// --stdio) and returns an initialized client. The --error-response and +// --malformed-json fake modes only fire on the first non-initialize request, +// so callers must drive the helper with a non-initialize method. +func newCallTestClient(t *testing.T, extraArgs ...string) *protocol.Client { + t.Helper() + binPath := buildCallTestFakeAnalyzer(t) + args := append([]string{"--stdio"}, extraArgs...) + client, err := protocol.NewClient(binPath, args...) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + // Perform the initialize handshake so the fake analyzer is "past + // initialize" and the --error-response / --malformed-json guards apply + // to the subsequent helper call. + ctx := context.Background() + resp, err := client.Call(ctx, protocol.MethodInitialize, protocol.InitializeParams{ + RootPath: "/tmp/project", + }) + if err != nil { + _ = client.Close() + t.Fatalf("initialize: %v", err) + } + if resp.Error != nil { + _ = client.Close() + t.Fatalf("initialize error: %s", resp.Error.Message) + } + return client +} + +// mismatchedResult intentionally types the "functions" field as a string so +// that unmarshalling a real complexity result (where "functions" is a JSON +// array) into it fails inside the helper's json.Unmarshal — exercising the +// result-unmarshal error path while keeping the JSON-RPC envelope valid. +type mismatchedResult struct { + Functions string `json:"functions"` +} + +// TestCallAndUnmarshal exercises the generic helper against every error +// condition and the happy path, driven through the fake analyzer binary +// (the only way to construct a *protocol.Client, which spawns a subprocess). +func TestCallAndUnmarshal(t *testing.T) { + t.Run("Success", func(t *testing.T) { + client := newCallTestClient(t) + defer func() { _ = client.Close() }() + + result, err := callAndUnmarshal[protocol.ComplexityResult]( + context.Background(), client, protocol.MethodComplexity, + protocol.ComplexityParams{RootPath: "/tmp/project", Patterns: []string{"./..."}}, + ) + if err != nil { + t.Fatalf("callAndUnmarshal: unexpected error: %v", err) + } + if len(result.Functions) != 3 { + t.Fatalf("got %d functions, want 3", len(result.Functions)) + } + // Assert specific field values from the canned data. + want := map[string]int{"add": 2, "multiply": 3, "divide": 5} + for _, fn := range result.Functions { + exp, ok := want[fn.Name] + if !ok { + t.Errorf("unexpected function %q", fn.Name) + continue + } + if fn.Complexity != exp { + t.Errorf("%s complexity = %d, want %d", fn.Name, fn.Complexity, exp) + } + } + }) + + t.Run("TransportError", func(t *testing.T) { + // --crash-after=complexity makes the subprocess exit after responding, + // but the crash happens after the response is written. To force a + // transport error we crash after initialize so the complexity Call + // fails reading a response from a dead subprocess. + client := newCallTestClient(t, "--crash-after=initialize") + defer func() { _ = client.Close() }() + + _, err := callAndUnmarshal[protocol.ComplexityResult]( + context.Background(), client, protocol.MethodComplexity, + protocol.ComplexityParams{RootPath: "/tmp/project", Patterns: []string{"./..."}}, + ) + if err == nil { + t.Fatal("expected transport error, got nil") + } + if !strings.Contains(err.Error(), "complexity protocol call") { + t.Errorf("error %q does not contain method prefix %q", err.Error(), "complexity protocol call") + } + // Transport errors wrap with %w — the wrapped error must be reachable. + if errors.Unwrap(err) == nil { + t.Errorf("transport error %q should wrap the underlying error with %%w", err.Error()) + } + }) + + t.Run("ProtocolError", func(t *testing.T) { + client := newCallTestClient(t, "--error-response") + defer func() { _ = client.Close() }() + + _, err := callAndUnmarshal[protocol.ComplexityResult]( + context.Background(), client, protocol.MethodComplexity, + protocol.ComplexityParams{RootPath: "/tmp/project", Patterns: []string{"./..."}}, + ) + if err == nil { + t.Fatal("expected protocol error, got nil") + } + msg := err.Error() + if !strings.Contains(msg, "complexity protocol error") { + t.Errorf("error %q does not contain method prefix %q", msg, "complexity protocol error") + } + // The fake analyzer returns message "internal error: simulated failure" + // with code -32603. + if !strings.Contains(msg, "internal error: simulated failure") { + t.Errorf("error %q does not contain protocol error message", msg) + } + if !strings.Contains(msg, "-32603") { + t.Errorf("error %q does not contain protocol error code -32603", msg) + } + }) + + t.Run("UnmarshalFailure", func(t *testing.T) { + // The fake analyzer's --malformed-json mode corrupts the JSON-RPC + // envelope itself, which fails inside client.Call (a transport error), + // not inside the helper's result unmarshal. To exercise the helper's + // own json.Unmarshal failure path we request a well-formed response + // (complexity) but decode it into a type whose "functions" field is + // incompatible (a string instead of an array), forcing json.Unmarshal + // of resp.Result to fail while the envelope stays valid. + client := newCallTestClient(t) + defer func() { _ = client.Close() }() + + _, err := callAndUnmarshal[mismatchedResult]( + context.Background(), client, protocol.MethodComplexity, + protocol.ComplexityParams{RootPath: "/tmp/project", Patterns: []string{"./..."}}, + ) + if err == nil { + t.Fatal("expected unmarshal failure, got nil") + } + if !strings.Contains(err.Error(), "parsing complexity result") { + t.Errorf("error %q does not contain method prefix %q", err.Error(), "parsing complexity result") + } + // Unmarshal errors wrap with %w. + if errors.Unwrap(err) == nil { + t.Errorf("unmarshal error %q should wrap the underlying error with %%w", err.Error()) + } + }) + + // GenericInstantiation exercises the helper with a second, distinct result + // type to verify the generic is not coupled to a single concrete type. + t.Run("GenericInstantiation", func(t *testing.T) { + client := newCallTestClient(t) + defer func() { _ = client.Close() }() + + result, err := callAndUnmarshal[protocol.CoverageResult]( + context.Background(), client, protocol.MethodCoverage, + protocol.CoverageParams{RootPath: "/tmp/project", Patterns: []string{"./..."}}, + ) + if err != nil { + t.Fatalf("callAndUnmarshal[CoverageResult]: unexpected error: %v", err) + } + if len(result.Functions) != 3 { + t.Fatalf("got %d functions, want 3", len(result.Functions)) + } + // Assert specific field values from the second type's canned data. + want := map[string]float64{"add": 90.0, "multiply": 60.0, "divide": 0.0} + for _, fn := range result.Functions { + exp, ok := want[fn.Function] + if !ok { + t.Errorf("unexpected function %q", fn.Function) + continue + } + if fn.Percentage != exp { + t.Errorf("%s coverage = %g, want %g", fn.Function, fn.Percentage, exp) + } + } + }) +} diff --git a/internal/adapter/complexity.go b/internal/adapter/complexity.go index d009cc4..a504975 100644 --- a/internal/adapter/complexity.go +++ b/internal/adapter/complexity.go @@ -13,8 +13,6 @@ package adapter import ( "context" - "encoding/json" - "fmt" "github.com/unbound-force/gaze/internal/crap" "github.com/unbound-force/gaze/internal/protocol" @@ -38,20 +36,12 @@ func (p *ExternalComplexityProvider) Analyze(patterns []string, rootDir string) ctx, cancel := context.WithTimeout(context.Background(), protocol.AnalysisTimeout) defer cancel() - resp, err := p.client.Call(ctx, protocol.MethodComplexity, protocol.ComplexityParams{ + result, err := callAndUnmarshal[protocol.ComplexityResult](ctx, p.client, protocol.MethodComplexity, protocol.ComplexityParams{ RootPath: rootDir, Patterns: patterns, }) if err != nil { - return nil, fmt.Errorf("complexity protocol call: %w", err) - } - if resp.Error != nil { - return nil, fmt.Errorf("complexity protocol error: %s (code %d)", resp.Error.Message, resp.Error.Code) - } - - var result protocol.ComplexityResult - if err := json.Unmarshal(resp.Result, &result); err != nil { - return nil, fmt.Errorf("parsing complexity result: %w", err) + return nil, err } return convertComplexity(result.Functions), nil diff --git a/internal/adapter/coverage.go b/internal/adapter/coverage.go index 2585c51..5a8adee 100644 --- a/internal/adapter/coverage.go +++ b/internal/adapter/coverage.go @@ -2,8 +2,6 @@ package adapter import ( "context" - "encoding/json" - "fmt" "github.com/unbound-force/gaze/internal/crap" "github.com/unbound-force/gaze/internal/protocol" @@ -29,20 +27,12 @@ func (p *ExternalLineCoverageProvider) Coverage(patterns []string, rootDir strin ctx, cancel := context.WithTimeout(context.Background(), protocol.AnalysisTimeout) defer cancel() - resp, err := p.client.Call(ctx, protocol.MethodCoverage, protocol.CoverageParams{ + result, err := callAndUnmarshal[protocol.CoverageResult](ctx, p.client, protocol.MethodCoverage, protocol.CoverageParams{ RootPath: rootDir, Patterns: patterns, }) if err != nil { - return nil, fmt.Errorf("coverage protocol call: %w", err) - } - if resp.Error != nil { - return nil, fmt.Errorf("coverage protocol error: %s (code %d)", resp.Error.Message, resp.Error.Code) - } - - var result protocol.CoverageResult - if err := json.Unmarshal(resp.Result, &result); err != nil { - return nil, fmt.Errorf("parsing coverage result: %w", err) + return nil, err } return convertCoverage(result.Functions), nil diff --git a/internal/adapter/sideeffect.go b/internal/adapter/sideeffect.go index fa5bd57..b7dbfce 100644 --- a/internal/adapter/sideeffect.go +++ b/internal/adapter/sideeffect.go @@ -111,20 +111,12 @@ func (a *ExternalSideEffectAnalyzer) loadBatch() error { ctx, cancel := context.WithTimeout(context.Background(), protocol.AnalysisTimeout) defer cancel() - resp, err := a.client.Call(ctx, protocol.MethodAnalyze, protocol.AnalyzeParams{ + result, err := callAndUnmarshal[protocol.AnalyzeResult](ctx, a.client, protocol.MethodAnalyze, protocol.AnalyzeParams{ RootPath: a.rootDir, Patterns: a.patterns, }) if err != nil { - return fmt.Errorf("analyze protocol call: %w", err) - } - if resp.Error != nil { - return fmt.Errorf("analyze protocol error: %s (code %d)", resp.Error.Message, resp.Error.Code) - } - - var result protocol.AnalyzeResult - if err := json.Unmarshal(resp.Result, &result); err != nil { - return fmt.Errorf("parsing analyze result: %w", err) + return err } a.cached = convertAnalysisResults(result.Functions, a.stderr) diff --git a/openspec/changes/adapter-call-unmarshal-helper/.openspec.yaml b/openspec/changes/adapter-call-unmarshal-helper/.openspec.yaml new file mode 100644 index 0000000..a757903 --- /dev/null +++ b/openspec/changes/adapter-call-unmarshal-helper/.openspec.yaml @@ -0,0 +1,2 @@ +schema: unbound-force +created: 2026-08-27 diff --git a/openspec/changes/adapter-call-unmarshal-helper/design.md b/openspec/changes/adapter-call-unmarshal-helper/design.md new file mode 100644 index 0000000..266e5ec --- /dev/null +++ b/openspec/changes/adapter-call-unmarshal-helper/design.md @@ -0,0 +1,202 @@ +## Context + +The `internal/adapter/` package adapts an external language analyzer +(spoken to via JSON-RPC 2.0 over stdin/stdout) into the provider +interfaces consumed by the CRAP scoring engine. Every batch provider +method performs the same three-step protocol dance: + +1. `resp, err := client.Call(ctx, method, params)` — transport +2. `if err != nil { ... }` and `if resp.Error != nil { ... }` — errors +3. `json.Unmarshal(resp.Result, &typed)` — decode into a Go struct + +This ~8-line pattern is duplicated at five sites (`Analyze`, +`Coverage`, `loadBatch`, `fetchTestMappings`, `Initialize`), differing +only by method, params type, and result type. The duplication is a +maintenance liability and violates the Zero-Waste Mandate. Go 1.24+ +generics allow a single type-safe extraction. See proposal.md for the +Constitution Alignment (Composability, Observable Quality, and +Testability all PASS; Autonomous Collaboration N/A). + +## Goals / Non-Goals + +### Goals + +- Extract one unexported generic helper `callAndUnmarshal[T any]` in + `internal/adapter/` that owns the Call → error-check → unmarshal + sequence. +- Migrate the four hard-error batch sites (`Analyze`, `Coverage`, + `loadBatch`, `Initialize`) to the helper. +- Preserve per-method error context in every wrapped error so operator + observability is unchanged (Observable Quality). +- Preserve `Session.Initialize` client cleanup (`Close()` on error). +- Add dedicated unit tests for the helper (Testability). +- No observable behavior change; existing tests pass unmodified. + +### Non-Goals + +- Modifying `internal/protocol/` — the transport layer stays + usage-agnostic (Composability First). The helper lives in the adapter + layer where result typing is a legitimate concern. +- Touching `CallStream` / the streaming side-effect path (different + response model). +- Consolidating `safeSSABuild` (#238, separate change). +- Changing any exported signature or the wire protocol. + +## Decisions + +### D1: Helper signature and location + +```go +// callAndUnmarshal issues a JSON-RPC call for method with params, +// checks transport and protocol errors, and unmarshals the result +// into T. Errors are wrapped with per-method context. +func callAndUnmarshal[T any]( + ctx context.Context, + client *protocol.Client, + method string, + params any, +) (T, error) +``` + +Placed in `internal/adapter/` (e.g., a new `call.go` file), unexported. +Rationale: the transport layer (`protocol.Client.Call`) intentionally +returns a raw `*protocol.Response` with a `json.RawMessage` result; the +Call→Unmarshal triad is an adapter-layer concern. Putting the generic in +`internal/protocol/` would couple the transport layer to result typing, +violating Composability First. + +### D2: Error-context strategy + +The helper derives all three wrapped-error prefixes from the `method` +argument so a single call site produces the same distinguishable +context the inline code produces today. Concretely, errors take the form: + +- transport: `fmt.Errorf("%s protocol call: %w", method, err)` +- protocol: `fmt.Errorf("%s protocol error: %s (code %d)", method, resp.Error.Message, resp.Error.Code)` +- unmarshal: `fmt.Errorf("parsing %s result: %w", method, err)` + +The `method` value passed is the protocol method constant +(e.g., `protocol.MethodComplexity`). If the existing human-readable +prefixes (e.g., "complexity") differ from the raw method constant, the +call site MAY pass a short label string instead of the constant to keep +the exact legacy wording; the acceptance test only requires that the +method identity is present in the wrapped error, so either is compliant. +The implementer MUST choose whichever keeps existing error strings +closest to current output to satisfy "existing tests pass without +modification". + +**`session.go` is the one site that diverges.** For `Analyze`, +`Coverage`, and `loadBatch`, the method constant (`"complexity"`, +`"coverage"`, `"analyze"`) already produces the exact legacy prefixes +(`" protocol call"`, `" protocol error"`, +`"parsing result"`). But `Initialize` currently uses +non-standard prefixes: `"initialize handshake: %w"` (transport), +`"initialize error: %s (code %d)"` (protocol), and +`"parsing initialize result: %w"` (unmarshal). Passing +`protocol.MethodInitialize` (= `"initialize"`) to the D2 templates would +change the first two strings to `"initialize protocol call"` / +`"initialize protocol error"`. Task 2.4 therefore MUST preserve the +legacy `Initialize` error strings. Since the generic helper emits a +single templated form, the implementer keeps the exact legacy wording by +constructing the three `Initialize` error strings at the call site +(wrapping the helper's returned error, or retaining the inline error +formatting for `Initialize` while still delegating the Call+Unmarshal +mechanics). The gate is: `go test` for the adapter package passes without +modifying existing tests, and any test that asserts on `"initialize +handshake"`/`"initialize error"` still holds. + +### D3: `fetchTestMappings` handling (resolved) + +Inspection of `contract.go:93-116` confirms `fetchTestMappings` +interleaves `p.warn(...)` on every error branch and uses inconsistent +wrapping (bare `err` on transport/unmarshal failure, `"test_mapping: %s"` +on protocol error) as part of its graceful-degradation contract (D7). +The generic helper cannot express the `p.warn` side effects without +either (a) taking a warn callback (over-generalizing the helper for one +caller) or (b) losing the warnings (a behavior regression). + +**Decision**: Leave `fetchTestMappings` unchanged. The effective +migration set is the four hard-error sites. This resolves the "migrate +5 vs 4" ambiguity noted during triage: the proposal counts five +candidate sites, but the warning/degradation path is explicitly out of +scope, so four are actually migrated. The specs and tasks reflect four. + +### D4: `Session.Initialize` cleanup (resolved) + +Inspection of `session.go:87-103` confirms `Initialize` calls +`_ = s.client.Close()` on each error branch (cleanup of a +half-established client after a failed handshake). The helper returns +`(T, error)` and does not manage lifecycle. `Initialize` will call +`callAndUnmarshal`, and when the returned error is non-nil, call +`s.client.Close()` before returning the wrapped error. Cleanup stays at +the call site; no behavior change. + +### D5: Test placement and cases + +Add helper unit tests in a new `internal/adapter/call_test.go` using +`package adapter` (internal test) — `callAndUnmarshal` is unexported, so +an external `package adapter_test` file cannot reach it. The existing +adapter package already uses internal tests (`sideeffect_test.go`, +`contract_internal_test.go`). + +**Test-client construction (resolved).** `protocol.Client` has no +injectable constructor — `protocol.NewClient(binary, args...)` spawns a +subprocess via `exec.LookPath` + `cmd.Start()`, and its fields (`cmd`, +`stdin`, `stdout`, `stderr`) are unexported. Wiring a "minimal client to +controllable stdin/stdout" is therefore **not possible without modifying +`internal/protocol/`**, which is out of scope (Non-Goals). All four error +conditions MUST instead be driven through the existing fake analyzer +binary (`internal/protocol/testdata/fake_analyzer/`, built once in +`TestMain` — see `adapter_test.go`), which already provides every mode +needed: + +| Case | Fake-analyzer flag | Mechanism | +|------|--------------------|-----------| +| a. success | (default) `--stdio` | normal typed response | +| b. transport error | `--crash-after=` | subprocess exits; the next `Call` fails reading stdout | +| c. protocol error | `--error-response` | returns a JSON-RPC error object (`resp.Error != nil`) | +| d. unmarshal failure | `--malformed-json` | returns a response whose result cannot decode into `T` | + +No new fake-analyzer modes and no `internal/protocol/` changes are +required. Because these tests spawn a subprocess, they are +isolated-behavior tests rather than pure in-process unit tests; the +existing `adapter_test.go` integration suite is the regression net for +the migrated call sites. + +Prefer a single table-driven `TestCallAndUnmarshal` with named subtests +(per Go pack TC-006 and AGENTS.md `TestXxx_Description` convention). +Minimum cases (Testability): + +1. successful unmarshal (happy path) — assert the returned `T` field values +2. transport (Call) error → wrapped with `%w`, `errors.Is(err, orig)` holds where an original error is available, and the method context prefix appears +3. protocol error (`resp.Error != nil`) → method context prefix + message + code all appear in the string (formatted via `%s`, NOT `%w` — see D2) +4. `json.Unmarshal` failure → wrapped with `%w`, method context prefix appears +5. second result type → verifies generic instantiation and asserts specific field values from the second type (closes the LOW-severity generic-coupling gap flagged in triage) + +**Coverage target.** The five cases MUST achieve 100% branch coverage of +`callAndUnmarshal` (success path plus all three error branches). + +### D6: Error-chain (`%w`) contract + +Transport and unmarshal errors wrap the underlying `error` with `%w`, so +`errors.Is`/`errors.As` unwrapping is part of the observable contract for +those two cases. Protocol errors are formatted with `%s` (not `%w`) +because `resp.Error` is a structured JSON-RPC error object, not a Go +`error` chain value; this matches the current inline behavior at all four +sites (e.g., `complexity.go:49`). Tests assert `errors.Is` for the +`%w` cases and string-content (method + message + code) for the protocol +case. + +## Risks / Trade-offs + +- **Risk: error-string drift** breaking existing tests. Mitigation: D2 + lets the call site pass the exact legacy label; run existing adapter + tests unmodified as the gate. +- **Risk: dropping the `resp.Error` check** during extraction (silent + swallow of protocol errors). Mitigation: dedicated protocol-error unit + test plus the fake-analyzer integration suite. +- **Trade-off: four sites migrated, not five.** Accepted: + `fetchTestMappings` keeps its distinct degradation semantics rather + than forcing an awkward callback-based generalization. This is the + Zero-Waste-correct outcome (no speculative flexibility). +- **Trade-off: generics require Go 1.18+.** Accepted: module is Go 1.24+. diff --git a/openspec/changes/adapter-call-unmarshal-helper/proposal.md b/openspec/changes/adapter-call-unmarshal-helper/proposal.md new file mode 100644 index 0000000..7603254 --- /dev/null +++ b/openspec/changes/adapter-call-unmarshal-helper/proposal.md @@ -0,0 +1,137 @@ +## Why + +The `internal/adapter/` package repeats the same JSON-RPC request pattern +across every provider adapter: call `protocol.Client.Call`, check the +transport error, check the protocol-level `resp.Error`, then +`json.Unmarshal` the raw result into a typed struct. This ~8-line +sequence appears at five batch call sites, differing only by method name, +params type, and result type: + +- `complexity.go` — `Analyze` (lines 41-55) +- `coverage.go` — `Coverage` (lines 32-46) +- `sideeffect.go` — `loadBatch` (lines 114-128) +- `contract.go` — `fetchTestMappings` (lines 97-114) +- `session.go` — `Initialize` (lines 87-103) + +The duplication is a maintenance liability: any change to protocol error +handling (e.g., adding a new error-classification field, adjusting the +wrapped-error format) must be applied five times, and drift between sites +is a real risk. This violates the Zero-Waste Mandate. Go 1.24+ generics +make a single, type-safe extraction straightforward. + +This change is a split from parent issue #201; it covers only the +`callAndUnmarshal` extraction. It does not address the `safeSSABuild` +consolidation (tracked separately as #238). + +## What Changes + +- Add a single unexported generic helper to `internal/adapter/`: + `callAndUnmarshal[T any](ctx, client, method, params) (T, error)`. + The helper performs Call → transport-error check → protocol-error + check → `json.Unmarshal`, wrapping each failure with a per-method + error context string derived from the `method` argument. +- Migrate the four hard-error batch call sites (`Analyze`, `Coverage`, + `loadBatch`, `Initialize`) to call the helper. `Initialize` keeps its + `s.client.Close()` cleanup at the call site (the helper handles only + Call + Unmarshal, not lifecycle cleanup). +- Evaluate `fetchTestMappings`: its Call+Unmarshal core can use the + helper, but its `p.warn()` graceful-degradation branches remain at the + call site unchanged. If integrating the helper there would compromise + the warning/degradation semantics, `fetchTestMappings` is left as-is + (making the effective migration count four). This decision is resolved + in design.md. +- Preserve per-method error context in all wrapped errors so operators + can still distinguish a `complexity` failure from a `coverage` failure + in logs. +- Add dedicated unit tests for `callAndUnmarshal`. + +Explicitly out of scope: `CallStream` (a different streaming response +model), the `fetchTestMappings` warning/degradation path, and any change +to `internal/protocol/` (the transport layer stays usage-agnostic). + +## Capabilities + +### New Capabilities +- `adapter.callAndUnmarshal`: internal generic helper that centralizes + the JSON-RPC Call → error-check → unmarshal pattern for adapter + provider methods, preserving per-method error context. + +### Modified Capabilities +- `adapter.ExternalComplexityProvider.Analyze`, + `adapter.ExternalLineCoverageProvider.Coverage`, + `adapter.ExternalSideEffectAnalyzer.loadBatch`, + `adapter.Session.Initialize`: internal implementation now delegates the + Call+Unmarshal sequence to `callAndUnmarshal`. No change to their + exported signatures, return values, or observable error behavior. + +### Removed Capabilities +- None. + +## Impact + +- **Files changed**: `internal/adapter/complexity.go`, + `internal/adapter/coverage.go`, `internal/adapter/sideeffect.go`, + `internal/adapter/session.go`, possibly + `internal/adapter/contract.go` (pending design decision), plus a new + or existing test file for `callAndUnmarshal` unit tests. +- **Behavior**: No observable behavior change. Same errors, same + per-method context, same return values. This is a pure DRY + refactoring guarded by the existing integration test suite (fake + analyzer binary) and new unit tests. +- **API surface**: Unchanged. The helper is unexported; no exported + signatures are modified. +- **Dependencies**: None added. Pure standard-library generics. + +## Constitution Alignment + +Assessed against the Unbound Force org constitution (below). This change +also trivially satisfies the project constitution +(`.specify/memory/constitution.md`), the highest-authority document for +Gaze: **I. Accuracy** — no behavior change, error identification +unchanged, regression-tested; **II. Minimal Assumptions** — no new +assumptions about host projects or tooling; **III. Actionable Output** — +no user-facing output changes; **IV. Testability** — the extracted helper +is tested in isolation with 100% branch coverage (shared with the org +Testability principle below). + +### I. Autonomous Collaboration + +**Assessment**: N/A + +This is an internal refactoring within a single package. It does not +change how heroes collaborate through artifacts, nor does it alter any +self-describing output. Adapter provider methods continue to return the +same typed results consumed by the scoring engine. + +### II. Composability First + +**Assessment**: PASS + +The change introduces no new mandatory dependencies and adds no new +package. The helper is confined to `internal/adapter/` and does not +couple the transport layer (`internal/protocol/`) to adapter concerns — +the transport layer stays usage-agnostic. Each adapter remains +independently usable exactly as before. + +### III. Observable Quality + +**Assessment**: PASS + +Per-method error context is explicitly preserved: every wrapped error +still names the failing method (e.g., "complexity protocol call", +"coverage protocol error"), so machine-parseable diagnostics and +operator logs retain full provenance. An acceptance test asserts the +method name appears in the wrapped error, making this observable quality +guarantee enforced rather than aspirational. + +### IV. Testability + +**Assessment**: PASS + +The extracted helper is testable in isolation without external services: +dedicated unit tests cover successful unmarshal, transport (Call) error +propagation, protocol error propagation, and `json.Unmarshal` failure +(minimum four cases), plus a case exercising a second result type to +verify generic instantiation. Existing integration tests (against the +fake analyzer binary) provide a regression safety net and must pass +without modification. diff --git a/openspec/changes/adapter-call-unmarshal-helper/specs/adapter/spec.md b/openspec/changes/adapter-call-unmarshal-helper/specs/adapter/spec.md new file mode 100644 index 0000000..b39fdc4 --- /dev/null +++ b/openspec/changes/adapter-call-unmarshal-helper/specs/adapter/spec.md @@ -0,0 +1,128 @@ +# Adapter Protocol-Call Helper — Delta Spec + +## ADDED Requirements + +### Requirement: Generic Call-and-Unmarshal Helper + +The `internal/adapter/` package MUST provide a single generic helper +function `callAndUnmarshal[T any]` that centralizes the JSON-RPC request +sequence used by provider adapters: invoke `protocol.Client.Call`, check +the transport error, check the protocol-level `resp.Error`, and +`json.Unmarshal` the raw result into a value of type `T`. The helper MUST +reside in `internal/adapter/` and MUST NOT be added to +`internal/protocol/`, so the transport layer remains usage-agnostic. The +helper MUST be unexported. + +#### Scenario: Successful call and unmarshal + +- **GIVEN** a `protocol.Client` whose `Call` returns a response with a + nil `Error` and a `Result` containing valid JSON for type `T` +- **WHEN** `callAndUnmarshal[T]` is invoked with a method name and params +- **THEN** it MUST return the unmarshalled value of type `T` and a nil + error + +#### Scenario: Transport (Call) error propagation + +- **GIVEN** a `protocol.Client` whose `Call` returns a non-nil transport + error +- **WHEN** `callAndUnmarshal[T]` is invoked +- **THEN** it MUST return the zero value of `T` and a wrapped error that + contains the method name and wraps the original transport error with + `%w` + +#### Scenario: Protocol error propagation + +- **GIVEN** a `protocol.Client` whose `Call` returns a response with a + non-nil `resp.Error` (message and code) +- **WHEN** `callAndUnmarshal[T]` is invoked +- **THEN** it MUST return the zero value of `T` and an error that + contains the method name, the protocol error message, and the protocol + error code, formatted with `%s` (the protocol error is a structured + JSON-RPC error object, NOT wrapped with `%w`) — consistent with the + current inline behavior at all four sites + +#### Scenario: Unmarshal failure propagation + +- **GIVEN** a `protocol.Client` whose `Call` returns a nil `Error` but a + `Result` containing JSON that cannot be unmarshalled into type `T` +- **WHEN** `callAndUnmarshal[T]` is invoked +- **THEN** it MUST return the zero value of `T` and a wrapped error that + contains the method name and wraps the `json.Unmarshal` error with `%w` + +#### Scenario: Generic instantiation across result types + +- **GIVEN** two distinct result types are requested from the helper + (e.g., a complexity result and a coverage result) +- **WHEN** `callAndUnmarshal` is instantiated for each type +- **THEN** each instantiation MUST correctly unmarshal into its own type + without coupling to any single concrete protocol type + +### Requirement: Per-Method Error Context Preservation + +All errors returned by `callAndUnmarshal` MUST embed a per-method context +string derived from the `method` argument, so that operators can +distinguish which protocol method failed (e.g., a `complexity` failure +from a `coverage` failure) in logs and machine-parseable diagnostics. +Transport and unmarshal errors MUST wrap the underlying error with `%w` +so `errors.Is`/`errors.As` unwrapping is preserved; protocol errors are +formatted with `%s` (structured JSON-RPC error object, not a Go error +chain value). + +#### Scenario: Method name appears in wrapped error + +- **GIVEN** the helper is invoked with a specific method name and any + error condition occurs (transport, protocol, or unmarshal) +- **WHEN** the returned error is formatted as a string +- **THEN** the method name (or its derived context prefix) MUST appear in + the error string + +## MODIFIED Requirements + +### Requirement: Adapter Batch Provider Methods Delegate the Call Pattern + +The batch provider methods `ExternalComplexityProvider.Analyze`, +`ExternalLineCoverageProvider.Coverage`, +`ExternalSideEffectAnalyzer.loadBatch`, and `Session.Initialize` MUST +obtain their protocol results via `callAndUnmarshal` rather than each +open-coding the Call → error-check → unmarshal sequence. Their exported +signatures, return values, and observable error behavior MUST remain +unchanged. `Session.Initialize` MUST retain its `s.client.Close()` +cleanup at the call site when the helper returns a non-nil error, since +the helper does not manage client lifecycle. `Session.Initialize` MUST +also preserve its legacy error strings (`"initialize handshake"`, +`"initialize error"`, `"parsing initialize result"`) rather than adopting +the generic `" protocol call"`/`" protocol error"` +prefixes, so existing tests and any operator log patterns remain valid +(see design D2). + +Previously: each of these methods open-coded the full Call, transport +error check, protocol error check, and `json.Unmarshal` sequence inline. + +#### Scenario: Migrated method preserves observable behavior + +- **GIVEN** an external analyzer that responds successfully to a batch + method +- **WHEN** the migrated method is invoked through the existing + integration test suite (fake analyzer binary) +- **THEN** the method MUST return the same typed result and the same + error behavior as before migration, with existing tests passing + without modification + +#### Scenario: Initialize cleans up on failed handshake + +- **GIVEN** an external analyzer that returns an error during the + initialize handshake +- **WHEN** `Session.Initialize` invokes `callAndUnmarshal` and receives a + non-nil error +- **THEN** `Session.Initialize` MUST call `s.client.Close()` before + returning the error + +## Excluded From This Change (No Requirement Change) + +- `CallStream` and the streaming side-effect path MUST NOT be modified; + they use a different (JSONL streaming) response model. +- The `fetchTestMappings` graceful-degradation path (its `p.warn()` + calls and nil-return-on-failure semantics) MUST remain unchanged. If + integrating the helper would compromise these semantics, + `fetchTestMappings` is left as-is (see design.md decision D3). +- `internal/protocol/` MUST NOT be modified. diff --git a/openspec/changes/adapter-call-unmarshal-helper/tasks.md b/openspec/changes/adapter-call-unmarshal-helper/tasks.md new file mode 100644 index 0000000..72c2aa5 --- /dev/null +++ b/openspec/changes/adapter-call-unmarshal-helper/tasks.md @@ -0,0 +1,46 @@ + + +## 1. Create the generic helper + +- [x] 1.1 Add `internal/adapter/call.go` with unexported + `func callAndUnmarshal[T any](ctx context.Context, client *protocol.Client, method string, params any) (T, error)` implementing Call → transport-error check → `resp.Error` check → `json.Unmarshal[T]`, with per-method error context per design D2 (transport: `"%s protocol call: %w"`, protocol: `"%s protocol error: %s (code %d)"`, unmarshal: `"parsing %s result: %w"`). Include a GoDoc comment on the function. + +## 2. Tests for the helper (TDD — write before migrating call sites) + +- [x] 2.1 Add `internal/adapter/call_test.go` using `package adapter` (internal test — `callAndUnmarshal` is unexported). Prefer a single table-driven `TestCallAndUnmarshal` with named subtests (Go pack TC-006). Drive all conditions through the fake analyzer binary built in `TestMain` (design D5 table): (a) successful unmarshal via default `--stdio`, asserting returned field values; (b) transport (Call) error via `--crash-after=`, asserting the method context prefix appears and `errors.Is(err, orig)` holds where an original error is available; (c) protocol error via `--error-response`, asserting method prefix + message + code appear (formatted with `%s`, not `%w` — design D6); (d) `json.Unmarshal` failure via `--malformed-json`, asserting the method prefix appears and the error wraps with `%w`. +- [x] 2.2 Add a 5th subtest exercising a second result type to verify generic instantiation, asserting specific field values from the second type (closes the LOW-severity generic-coupling gap; design D5). Confirm the 5 cases give 100% branch coverage of `callAndUnmarshal`. + +## 3. Migrate hard-error call sites (each a different file — parallel) + +- [x] 3.1 [P] `internal/adapter/complexity.go`: replace the inline Call+Unmarshal in `Analyze` (41-55) with `callAndUnmarshal[protocol.ComplexityResult]`, then call `convertComplexity`. Preserve the existing error label so current tests pass unmodified (design D2). +- [x] 3.2 [P] `internal/adapter/coverage.go`: replace the inline Call+Unmarshal in `Coverage` (32-46) with `callAndUnmarshal[protocol.CoverageResult]`. Preserve existing error label. +- [x] 3.3 [P] `internal/adapter/sideeffect.go`: replace the inline Call+Unmarshal in `loadBatch` (114-128) with `callAndUnmarshal[protocol.AnalyzeResult]`, then assign the converted result to `a.cached` as today. `loadBatch` returns only `error`; adapt the `(T, error)` result accordingly. Do NOT touch `CallStream` / the streaming path. +- [x] 3.4 [P] `internal/adapter/session.go`: RESOLVED via design D2's sanctioned inline-retention option. `Initialize` retains its original inline Call → 3-branch error handling UNCHANGED because the collapsed generic helper cannot reproduce Initialize's THREE DISTINCT legacy strings (`"initialize handshake: %w"` transport / `"initialize error: %s (code %d)"` protocol / `"parsing initialize result: %w"` unmarshal) without fragile prefix translation. Design D2 (design.md 88-106) explicitly permits retaining inline formatting for Initialize. session.go is byte-identical to pre-migration state; `s.client.Close()` on each error branch preserved (design D4). Effective helper adoption is 3 sites (complexity/coverage/sideeffect), not 4 — legacy operator-facing strings prioritized per task's "PRESERVE the legacy Initialize error strings". + +## 4. Confirm exclusions untouched + +- [x] 4.1 Verify `fetchTestMappings` (`internal/adapter/contract.go:93-116`) is unchanged — `git diff main -- contract.go` is empty. Its `p.warn()` graceful-degradation path stays as-is (design D3). +- [x] 4.2 Verify `CallStream` and the streaming side-effect path are unchanged — `sideeffect.go` diff shows ONLY the `loadBatch` Call+Unmarshal replacement; `loadStreaming`/`CallStream`/`parseSideEffectStream` untouched. +- [x] 4.3 Verify `internal/protocol/` has no modifications — `git diff main -- internal/protocol/` empty; `git diff main -- session.go contract.go` empty. Changed files: call.go (new), call_test.go (new), complexity.go, coverage.go, sideeffect.go (loadBatch only). session.go shows NO diff (reverted per 3.4). + +## 5. Verification & gates + +- [x] 5.1 Run `go build ./cmd/gaze` — builds clean (CMD_GAZE_BUILD_OK). +- [x] 5.2 Run `go test -race -count=1 -short ./...` — all pass, existing adapter tests pass WITHOUT modification (adapter 2.56s incl new TestCallAndUnmarshal 5 subtests + unmodified existing tests; all packages ok). +- [x] 5.3 Run `golangci-lint run` — zero issues (exit 0). +- [x] 5.4 CI parity: test.yml (`go build ./...` + `go test -race -count=1 -short -timeout 15m -coverprofile=coverage.out ./...`) and mega-linter (`golangci-lint run`) — all satisfied by the gate run above. +- [x] 5.5 Constitution alignment verified: Composability First (empty `git diff main -- internal/protocol/`, no new deps), Observable Quality (per-method error context preserved, asserted by TestCallAndUnmarshal), Testability (helper tested in isolation). All hold as claimed in proposal.md. +- [x] 5.6 Ran `/review-council` (Code Review Mode) — all 5 divisor reviewers (adversary/architect/guard/sre/testing) returned APPROVE; single consistent LOW (test temp-dir leak) accepted as non-blocking by all reviewers. + +## 6. Documentation validation gate + +- [x] 6.1 Added AGENTS.md "Recent Changes" entry for `adapter-call-unmarshal-helper` (new `callAndUnmarshal` helper; 3 migrated sites complexity.Analyze/coverage.Coverage/sideeffect.loadBatch; session.Initialize retained inline per design D2 to preserve 3 distinct legacy strings; error-chain contract D6; exclusions CallStream/fetchTestMappings warn path/internal/protocol/). +- [x] 6.2 Confirmed no README, CLI-help, website, or GoDoc-on-exported-API updates needed — pure internal refactoring, no user-facing behavior change, `callAndUnmarshal` is unexported, no exported-signature change (Documentation Validation Gate). + + +