feat: wire --analyzer to gaze quality for external test_mapping - #242
feat: wire --analyzer to gaze quality for external test_mapping#242jflowers wants to merge 5 commits into
Conversation
Repository settings are now managed centrally via safe-settings in the unbound-force/.github admin repo. Closes unbound-force/.github#30
…CRAP + contract coverage) Add BuildQualityFromMappings in internal/adapter/quality.go that converts []protocol.AssertionMappingData + []taxonomy.AnalysisResult into []taxonomy.QualityReport + *taxonomy.PackageSummary, reusing quality.ComputeContractCoverage for metric computation. Add FetchTestMappings standalone function for direct test_mapping protocol calls. Add computeOverSpecification for incidental-effect assertion counting. Add Session.Client() accessor. Expose SideEffects on Providers struct. In cmd/gaze/main.go: replace --analyzer rejection block with runQualityWithExternalAnalyzer following runCrapWithExternalAnalyzer pattern. Add handleQualityNoTestMapping and handleQualityTestMappingError for graceful degradation. Unhide --analyzer/--language flags. Flag validation: --target and --ai-mapper rejected with --analyzer (Go-specific SSA/AST features). 14 new tests across internal/adapter/ and cmd/gaze/. Closes unbound-force#229
- Fix FetchTestMappings GoDoc to accurately describe error propagation - Remove double-logging by eliminating stderr param from FetchTestMappings - Add happy-path integration test for quality with external analyzer - Add unit tests for FetchTestMappings (success + protocol error) - Add tests for graceful degradation handlers (no test_mapping, error) - Add buildQualitySummary truncation test with 6+ test functions - Add findSideEffectID direct test with 4 table-driven cases - Add computeOverSpecification edge case for empty SideEffectID - Update README with --language example and flag incompatibility note Closes unbound-force#229 Assisted-by: claude-opus Generated with AI assistance (claude-opus)
yvonnedevlinrh
left a comment
yvonnedevlinrh
left a comment
There was a problem hiding this comment.
Review: PR #242 — feat: wire --analyzer to gaze quality for external test_mapping
Thanks for this — the core feature is well-structured, spec-backed, and CI-green, reusing existing session/adapter/coverage infrastructure cleanly. Two review passes (general + deep correctness) surfaced findings below. Verdict: REQUEST CHANGES, driven by one HIGH correctness bug and one HIGH scope issue.
Findings summary
| # | Severity | Category | Location | Finding | Suggested resolution |
|---|---|---|---|---|---|
| 1 | HIGH | Correctness (Constitution I: Accuracy) | internal/adapter/quality.go:60-119 |
BuildQualityFromMappings keys the effect set off only the first mapping's target (testMappings[0]). When one test asserts on 2+ distinct target functions, contract-coverage/over-specification metrics are silently wrong and the second target is dropped from the report. Untested. |
Sub-group each test's mappings by target (one report per test,target), or union effects across all referenced targets; add a multi-target test. |
| 2 | MEDIUM | Spec drift (Constitution III: Actionable Output) | cmd/gaze/main.go:1244, :1264 |
Degraded handlers emit bare &taxonomy.PackageSummary{}. Spec (specs/quality-external-analyzer.md) mandates a reason of "test_mapping_unavailable" / "test_mapping_error", but PackageSummary has no reason field, so neither string appears in text or JSON. Observable degradation (stderr warning + zero coverage + exit code) is correct and tested. |
Add a reason field to PackageSummary (and schema), or amend the spec scenarios to drop it. |
| 3 | MEDIUM | Test coverage gap | internal/adapter/quality_internal_test.go:14-448 |
Every TestBuildQualityFromMappings subtest uses a single target per test, so the multi-target path (finding #1) is never exercised — which is why the bug shipped CI-green. |
Add a subtest with one test function mapping to two distinct targets; assert both targets' effects count. |
| 4 | LOW | Scope noise | .uf/dewey/learnings/*.md (×4) |
Four draft learning notes (author jay-flowers) bundled into a feature PR — harmless/additive but unrelated; different author suggests a separate workflow. |
Optional: strip from this PR. |
| 5 | LOW | Process (AGENTS.md Website Documentation Gate) | — | New user-facing CLI capability (gaze quality --analyzer) requires a tracking issue in unbound-force/website; #229 calls out website#227/#165 as inaccurate. No website issue referenced. |
Create the website tracking issue before merge. |
Positives
- Security: no findings — no new
exec/shell construction;--analyzer/--languagereach subprocess via pre-existinginitExternalSession/Discover;FetchTestMappingsuses JSON-RPC over stdin with a boundedcontext.WithTimeout(protocol.AnalysisTimeout);moduleDirfromos.Getwd(); errors wrapped without secret exposure. - Caller impact: none —
runQualitysignature unchanged; external path is a pure early-return branch gated onanalyzerFlag != "".Providers/Sessionadditions are additive. - Branch reachability: all new branches (flag rejections, both degradation handlers under both threshold conditions, happy path) are reachable and tested.
- Constitution: I Accuracy — now questioned by #1; II Minimal Assumptions PASS (Go-native path untouched, opt-in); III Actionable Output PARTIAL (#3); IV Testability PASS (pure functions, table-driven tests, GoDoc present).
Blocking items before merge
| for tk, testMappings := range mappingsByTest { | ||
| // Determine target function(s) from mappings — use the first | ||
| // target as the primary (most common in practice: 1 test → 1 target). | ||
| targetKey := funcKey{ |
There was a problem hiding this comment.
[HIGH] Multi-target data loss. targetKey is derived from testMappings[0] only, but protocol.AssertionMappingData carries per-mapping TargetFunction/TargetPackage. If one test asserts on 2+ distinct targets, effects (line 67) covers only the first target — so ComputeContractCoverage (line 83) undercounts the denominator, silently drops assertions covering the second target, and the report's TargetFunction names only the first. This produces a false coverage metric (Constitution I: Accuracy).
Consider sub-grouping by target (one report per test,target) or unioning effects across all targets referenced in the group:
// e.g. collect every distinct target the group references
targets := map[funcKey]struct{}{}
for _, m := range testMappings {
targets[funcKey{pkg: m.TargetPackage, function: m.TargetFunction}] = struct{}{}
}| // BuildQualityFromMappings tests (table-driven) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestBuildQualityFromMappings(t *testing.T) { |
There was a problem hiding this comment.
[MEDIUM] Test gap. Every subtest here uses a single target per test, so the multi-target path in BuildQualityFromMappings is never exercised — this is why the accuracy bug above passes CI. Please add a subtest where one test function has mappings to two distinct targets and assert both targets' contractual effects are counted.
| "contract coverage and over-specification metrics are unavailable\n", | ||
| providers.AnalyzerName) | ||
|
|
||
| summary := &taxonomy.PackageSummary{} |
There was a problem hiding this comment.
[MEDIUM] Spec drift: missing reason. The spec (specs/quality-external-analyzer.md) requires the degraded report to carry reason: "test_mapping_unavailable", but this emits a bare &taxonomy.PackageSummary{} and PackageSummary has no reason field — so the string never appears in text or JSON. Same applies to handleQualityTestMappingError (line 1264, expects "test_mapping_error"). Either add a reason field to PackageSummary (+schema) or amend the spec.
3 participants
Summary
Wires
--analyzerand--languageflags togaze qualityfor externaltest_mappingsupport, lifting the D12 deferral from #95.Changes
New:
internal/adapter/quality.goBuildQualityFromMappings— converts[]protocol.AssertionMappingData+[]taxonomy.AnalysisResultinto[]taxonomy.QualityReport+*taxonomy.PackageSummary, reusingquality.ComputeContractCoveragefor metric computationFetchTestMappings— standalone function for directtest_mappingprotocol callscomputeOverSpecification— counts assertions targeting incidental side effectsbuildQualitySummary— aggregates reports into PackageSummaryModified:
internal/adapter/session.goSideEffects *ExternalSideEffectAnalyzerfield toProvidersstructSession.Client()accessor for direct protocol callsModified:
cmd/gaze/main.go--analyzerrejection block withrunQualityWithExternalAnalyzer(followsrunCrapWithExternalAnalyzerpattern)handleQualityNoTestMappingandhandleQualityTestMappingErrorfor graceful degradation--analyzer/--languageflags innewQualityCmd--targetand--ai-mapperrejected with--analyzer(Go-specific SSA/AST features)Tests
internal/adapter/quality_internal_test.go(BuildQualityFromMappings + computeOverSpecification)cmd/gaze/external_analyzer_test.go(CLI flag validation)Documentation
gaze quality --analyzerusage exampleVerification
go test -race -count=1 -short ./...— all passgolangci-lint run— 0 issuesgaze quality --helpshows--analyzerand--languageflagsSpec Artifacts
openspec/changes/quality-external-analyzer/— proposal, design, specs, tasksCloses #229